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.

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

Added support for traceresponse headers #436

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

owais
Copy link
Contributor

@owais owais commented Apr 13, 2021

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 review from a team, 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.

@@ -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.

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

@@ -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
…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 added a commit to phillipuniverse/opentelemetry-python-contrib that referenced this pull request Nov 28, 2021
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 Nov 28, 2021
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
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
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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants