Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

django: Fix carrier usage on ASGI requests #767

Merged
merged 2 commits into from
Oct 25, 2021

Conversation

adamantike
Copy link
Contributor

@adamantike adamantike commented Oct 22, 2021

Description

For ASGI requests, we must use request.scope instead of request.META. This is because ASGIGetter retrieves the headers key from the carrier [0], which is only present in request.scope.

[0] https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py#L133

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update

How Has This Been Tested?

  • Tested Django ASGI application along with the aws propagator and requests including header: X-Amzn-Trace-Id: Root=1-5759e988-bd862e3fe1be46a994272793; Parent=53995c3f42cd8ad8; Sampled=1

Does This PR Require a Core Repo Change?

  • No.

Checklist:

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

@adamantike adamantike requested a review from a team October 22, 2021 00:17
@adamantike adamantike force-pushed the django-fix-carrier-extract branch from 742101b to 157c260 Compare October 22, 2021 00:18
@owais
Copy link
Contributor

owais commented Oct 22, 2021

would be nice to add a small test case so we detect breakage with ASGI context extraction.

@adamantike
Copy link
Contributor Author

Happy to provide a test, but didn't find an easy way to do it, without adding a propagator as dependency, or mocking attach/extract and validating what is received there.

If you have a better test scenario in mind, I can definitely implement it and add it to this PR.

@adamantike
Copy link
Contributor Author

adamantike commented Oct 23, 2021

@owais, I was able to add a test by taking advantage of how the default propagator works, so we check the provided traceparent is retrieved from the carrier both for WSGI and ASGI.

@owais
Copy link
Contributor

owais commented Oct 23, 2021

propagator as test dep sounds fine. I think all instrumentations that propagate context should do this if they don't already.

For ASGI requests, we must use `request.scope` instead of
`request.META`. This is because `ASGIGetter` retrieves the `headers` key
from the carrier [0], which is only present in `request.scope`.

[0] https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py#L133
@adamantike adamantike force-pushed the django-fix-carrier-extract branch from 14cca2e to 22c8b1f Compare October 25, 2021 16:04
@owais owais enabled auto-merge (squash) October 25, 2021 16:11
@owais owais merged commit 4b9b6dc into open-telemetry:main Oct 25, 2021
@adamantike adamantike deleted the django-fix-carrier-extract branch October 29, 2021 18:18
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants