Add traceresponse headers for asgi apps (FastAPI, Starlette) #817

phillipuniverse · 2021-11-28T05:12:47Z

Description

Adds a traceresponse header for ASGI apps just like the WSGI version in #436.

Fixes #803

Type of change

Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Yes. Added unit tests to assert whether or not the traceresponse header is included.

Does This PR Require a Core Repo Change?

No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added
Documentation has been updated

phillipuniverse · 2021-11-28T05:14:24Z

CHANGELOG.md

@@ -17,6 +22,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

 ## [1.7.1-0.26b1](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.7.0-0.26b0) - 2021-11-11

+### Added


This wasn't included in the last release but seemed like the right format

phillipuniverse · 2021-11-28T05:15:32Z

...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py

+            headers = []
+            carrier["headers"] = headers
+
+        headers.append([key.lower().encode(), value.encode()])


I know the Access-Control-Expose-Headers usually is cased, but the docs explicitly say lowercase.

phillipuniverse · 2021-11-28T05:18:31Z

instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py

+        self.send_input({"type": "websocket.disconnect"})
+        _, socket_send, *_ = self.get_all_output()
+
+        # traceresponse header corresponds to the 2nd websocket.send span


Same question here about which span exactly should be returned.

phillipuniverse · 2021-11-28T05:18:35Z

instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py

+        self.seed_app(app)
+        self.send_default_request()
+
+        # traceresponse header corresponds to http.response.start span


This one was a bit surprising to me, I'm not so sure it makes intuitive sense at first glance. I kind of feel like the span returned in the response header should be the final "kind": trace_api.SpanKind.SERVER span.

What do you think?

Yes, response headers should represent the SERVER span.

Got it, done!

phillipuniverse · 2021-12-07T13:54:31Z

@owais anything else we need to do here?

lzchen · 2021-12-07T17:16:05Z

...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py

+            headers = []
+            carrier["headers"] = headers
+
+        headers.append([key.lower().encode(), value.encode()])


Might be a stupid question but why do we encode the key and values in the header? Is this because asgi requires them to be byte strings?

@lzchen good question, it's non-obvious. From the spec:

Keys:
type (Unicode string) – "http.response.start".
status (int) – HTTP status code.
headers (Iterable[[byte string, byte string]]) – An iterable of [name, value] two-item iterables, where name is the > header name, and value is the header value. Order must be preserved in the HTTP response. Header names must be lowercased. Optional; if missing defaults to an empty list. Pseudo headers (present in HTTP/2 and HTTP/3) must not be present.

Important parts:

headers (Iterable[[byte string, byte string]]) - spec says they should be encoded

Header names must be lowercased

phillipuniverse · 2021-12-21T12:41:36Z

@owais @lzchen I was disappointed this wasn't included in the latest 0.27b0 release. What is remaining here for me to do to get this merged?

phillipuniverse · 2021-12-22T18:31:05Z

@owais hopefully this is the last one, latest changes includes fixes for Pylint from the failure at https://github.com/open-telemetry/opentelemetry-python-contrib/runs/4598037270?check_suite_focus=true

instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/init.py:309:4: R0914: Too many local variables (16/15) (too-many-locals)

owais · 2021-12-23T08:41:46Z

...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py

+                otel_send = self._get_otel_send(
+                    span,
+                    span_name,
+                    trace.context_api.get_current(),


At line 328, we start a SERVER span explicitly and it is present in the scope bound to the variable span. I recommend renaming that variable to server_span and then using it directly instead of relying on context API to fetch current context. This would be a lot more explicit and the intention would be very obvious making future contributions easier.

Got it, great suggestion!

owais

Looks good. Suggest one last simplification before we can merge.

This asgi version is modeled after the original wsgi version in open-telemetry#436 and corresponds to the SERVER span. Also cleans up some of the existing ASGI functionality to reduce complexity and make future contributions more straightforward.

phillipuniverse · 2021-12-23T20:31:02Z

...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py

+                        context=set_span_in_context(
+                            server_span, trace.context_api.Context()
+                        ),


IMO this is a bit obtuse but I'm sure there is a good extensibility reason for it. What I really wanted to do was:

propagator.inject( message, span=server_span, setter=asgi_setter, )

But for now I will consider this yak shaved!

phillipuniverse · 2021-12-23T20:31:43Z

@owais latest updates include an update from dev and also all of my previous commits squashed together.

Thanks for reviewing!

phillipuniverse requested a review from a team November 28, 2021 05:12

phillipuniverse force-pushed the asgi-traceresponse branch from 67ff3c6 to 03c1f55 Compare November 28, 2021 05:14

phillipuniverse commented Nov 28, 2021

View reviewed changes

phillipuniverse requested review from owais and removed request for a team November 29, 2021 14:50

lzchen reviewed Dec 7, 2021

View reviewed changes

lzchen approved these changes Dec 7, 2021

View reviewed changes

phillipuniverse requested a review from lzchen December 7, 2021 22:00

owais approved these changes Dec 21, 2021

View reviewed changes

phillipuniverse force-pushed the asgi-traceresponse branch from 6c9afa6 to 18e8cab Compare December 21, 2021 19:42

phillipuniverse requested a review from owais December 22, 2021 18:28

phillipuniverse force-pushed the asgi-traceresponse branch from 4ab8ea2 to 33d98b0 Compare December 22, 2021 18:29

phillipuniverse force-pushed the asgi-traceresponse branch from 33d98b0 to de560ed Compare December 22, 2021 20:31

owais reviewed Dec 23, 2021

View reviewed changes

owais requested changes Dec 23, 2021

View reviewed changes

phillipuniverse force-pushed the asgi-traceresponse branch from de560ed to d0d2422 Compare December 23, 2021 20:29

phillipuniverse commented Dec 23, 2021

View reviewed changes

phillipuniverse requested a review from owais December 23, 2021 20:31

owais approved these changes Dec 24, 2021

View reviewed changes

owais enabled auto-merge (squash) December 25, 2021 21:34

Merge branch 'main' into asgi-traceresponse

fa2aa1b

owais merged commit 3de2986 into open-telemetry:main Dec 25, 2021

phillipuniverse deleted the asgi-traceresponse branch December 25, 2021 23:00

phillipuniverse mentioned this pull request Aug 1, 2022

AsgiSetter should override header values instead of appending #1210

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add traceresponse headers for asgi apps (FastAPI, Starlette) #817

Add traceresponse headers for asgi apps (FastAPI, Starlette) #817

phillipuniverse commented Nov 28, 2021 •

edited

Loading

phillipuniverse Nov 28, 2021

phillipuniverse Nov 28, 2021

phillipuniverse Nov 28, 2021

phillipuniverse Nov 28, 2021

owais Nov 29, 2021

phillipuniverse Nov 29, 2021

phillipuniverse commented Dec 7, 2021

lzchen Dec 7, 2021 •

edited

Loading

phillipuniverse Dec 7, 2021

phillipuniverse commented Dec 21, 2021

phillipuniverse commented Dec 22, 2021

owais Dec 23, 2021

phillipuniverse Dec 23, 2021

owais left a comment

phillipuniverse Dec 23, 2021

phillipuniverse commented Dec 23, 2021

		@@ -17,6 +22,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

		## [1.7.1-0.26b1](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.7.0-0.26b0) - 2021-11-11

		### Added

Add traceresponse headers for asgi apps (FastAPI, Starlette) #817

Add traceresponse headers for asgi apps (FastAPI, Starlette) #817

Conversation

phillipuniverse commented Nov 28, 2021 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillipuniverse commented Dec 7, 2021

lzchen Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillipuniverse commented Dec 21, 2021

phillipuniverse commented Dec 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owais left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillipuniverse commented Dec 23, 2021

phillipuniverse commented Nov 28, 2021 •

edited

Loading

lzchen Dec 7, 2021 •

edited

Loading