Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Added support for traceresponse headers #436

Merged
merged 1 commit into from
Apr 19, 2021
Merged

Added support for traceresponse headers #436

merged 1 commit into from
Apr 19, 2021

Conversation

owais
Copy link
Contributor

@owais owais commented Apr 13, 2021 •
edited

Description

Added opt-in support to return traceresponse headers for server instrumentations.

This allows users to configure their web apps to inject trace context
as headers in HTTP responses. This is useful when client side apps need
to connect their spans with the server side spans e.g, in RUM products.

Today the most practical way to do this is to use the Server-Timing
header but in near future we might use the traceresponse header as
described here: https://w3c.github.io/trace-context/#trace-context-http-response-headers-format

Added trace response propagation support for:

  • Django
  • Falcon
  • Flask
  • Pyramid
  • Tornado

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Added unit tests
  • Manual testing

Does This PR Require a Core Repo Change?

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

@owais owais requested a review from a team as a code owner April 13, 2021 13:46
@owais owais requested review from aabmass and lzchen and removed request for a team April 13, 2021 13:46
@owais owais force-pushed the traceresponse-header-propagation branch 2 times, most recently from 72589a6 to 10ea879 Compare April 13, 2021 13:48
@owais
Copy link
Contributor Author

owais commented Apr 13, 2021

I created individual PRs first but they were very repetitive and quite small. I thought it'd be better to have them bundled as a single PR. This way they can be reviewed at once by the same reviewers. It should be more efficient I think.

This was referenced Apr 13, 2021
Closed
Closed
Closed
Closed
Closed
Merged
@owais owais force-pushed the traceresponse-header-propagation branch 8 times, most recently from 4028808 to f453f21 Compare April 16, 2021 19:01
lzchen
lzchen reviewed Apr 19, 2021
View reviewed changes
...on/opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py Outdated
@@ -273,5 +279,9 @@ def process_response(
)
)

propagator = get_global_response_propagator()
if propagator:
propagator.inject(resp, setter=self.back_propagation_setter)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are able to access a class attribute like this via self?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but it can lead to bugs later. Will change.

aabmass
aabmass approved these changes Apr 19, 2021
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

lzchen
lzchen reviewed Apr 19, 2021
View reviewed changes
...tation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py Outdated
@@ -264,3 +264,11 @@ def _end_span_after_iterating(iterable, span, tracer, token):
close()
span.end()
context.detach(token)


class BackPropagationSetter:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inherit from Setter

@owais owais force-pushed the traceresponse-header-propagation branch 4 times, most recently from 4bf4ec0 to 9f33735 Compare April 19, 2021 17:49
@owais
Copy link
Contributor Author

owais commented Apr 19, 2021

After sub-classing, docs job is failing with:

Warning, treated as error:
/home/runner/work/opentelemetry-python-contrib/opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py:docstring of opentelemetry.instrumentation.wsgi.ResponsePropagationSetter:1:py:class reference target not found: opentelemetry.instrumentation.propagators.Setter
ERROR: InvocationError for command /home/runner/work/opentelemetry-python-contrib/opentelemetry-python-contrib/.tox/docs/bin/sphinx-build -E -a -W -b html -T . _build/html (exited with code 2)

The core repo GIT SHA was updated to tip of core main which contains the change. The file for which docs are being generated is obviously able to import the said class as it passes all tests and lint checks.

I tried invalidating tox cache but it isn't helping either. Looks like docs job is not installing the latest core main or is not using the locally checked out copy for some reason.

@owais owais force-pushed the traceresponse-header-propagation branch from 9f33735 to c0afe1d Compare April 19, 2021 18:00
@owais
Added opt-in support to return traceresponse headers for server instr…
9a710fb 
…umentations.

This allows users to configure their web apps to inject trace context
as headers in HTTP responses. This is useful when client side apps need
to connect their spans with the server side spans e.g, in RUM products.

Today the most practical way to do this is to use the Server-Timing
header but in near future we might use the traceresponse header as
described here: https://w3c.github.io/trace-context/#trace-context-http-response-headers-format

Added trace response propagation support for:

    Django
    Falcon
    Flask
    Pyramid
    Tornado
@owais owais force-pushed the traceresponse-header-propagation branch from c0afe1d to 9a710fb Compare April 19, 2021 18:05
@owais
Copy link
Contributor Author

owais commented Apr 19, 2021

@lzchen I'm at a loss. I'm reverting inheriting from ABC setter for now and added a TODO to unblock release for now. Will need more time to figure out why docs doesn't like it. I remember Srikanth had a similar issue last week so probably something is wrong with docs tox job that we need to look into anyway.

@lzchen
Copy link
Contributor

lzchen commented Apr 19, 2021

@owais
Sounds good to me.

@lzchen lzchen merged commit 3083690 into open-telemetry:main Apr 19, 2021
@owais owais deleted the traceresponse-header-propagation branch April 19, 2021 19:25
@phillipuniverse phillipuniverse mentioned this pull request Nov 11, 2021
Closed
phillipuniverse added a commit to phillipuniverse/opentelemetry-python-contrib that referenced this pull request Nov 28, 2021
@phillipuniverse
Add traceresponse headers for asgi apps (FastAPI, Starlette)
67ff3c6 
This asgi version is modeled after the original wsgi version in open-telemetry#436.
@phillipuniverse phillipuniverse mentioned this pull request Nov 28, 2021
Merged
6 tasks
phillipuniverse added a commit to phillipuniverse/opentelemetry-python-contrib that referenced this pull request Nov 28, 2021
@phillipuniverse
Add traceresponse headers for asgi apps (FastAPI, Starlette)
03c1f55 
This asgi version is modeled after the original wsgi version in open-telemetry#436.
phillipuniverse added a commit to phillipuniverse/opentelemetry-python-contrib that referenced this pull request Dec 21, 2021
@phillipuniverse
Add traceresponse headers for asgi apps (FastAPI, Starlette)
f2c2b72 
This asgi version is modeled after the original wsgi version in open-telemetry#436.
phillipuniverse added a commit to phillipuniverse/opentelemetry-python-contrib that referenced this pull request Dec 23, 2021
@phillipuniverse
Add traceresponse headers for asgi apps (FastAPI, Starlette)
d0d2422 
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.
owais pushed a commit that referenced this pull request Dec 25, 2021
@phillipuniverse
Add traceresponse headers for asgi apps (FastAPI, Starlette) (#817)
3de2986 
This asgi version is modeled after the original wsgi version in #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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabmass aabmass aabmass approved these changes

@lzchen lzchen lzchen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@owais @lzchen @aabmass