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

Django trace response headers #395

Closed
wants to merge 1 commit into from

Conversation

owais
Copy link
Contributor

@owais owais commented Apr 2, 2021

Description

Added opt-in support to return trace response headers from Django.

This allows users to configure their Django 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

As a result the implementation does not use a hard-coded header and
instead let's the users pick one.

This can be done by setting the OTEL_PYTHON_TRACE_RESPONSE_HEADER to
the header name that users want to inject in HTTP responses. The option
does not have a default value and the feature is disbaled when a env var
is not set.

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

  • Tested manually
  • Added unit tests

Does This PR Require a Contrib 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 force-pushed the traceresponse-headers branch 8 times, most recently from 4db9aa9 to f7cacbc Compare April 2, 2021 16:03
@owais owais marked this pull request as ready for review April 2, 2021 16:10
@owais owais requested review from a team, codeboten and hectorhdzg and removed request for a team April 2, 2021 16:10
@aabmass
Copy link
Member

aabmass commented Apr 8, 2021

My comment from the SIG meeting:

Since W3C trace context is core to OTel and has drafted traceresponse back-propagation, this will probably make its way into the OTel Propagator spec. I'm trying to imagine what that API might look like, maybe:

  1. We keep a single global textmap propagator but add a new method e.g. injectResponse(carrier, setter). Or
  2. We add a global textmap "back-propagator" with the same signature as the existing one inject(carrier, setter)

I just want to make sure this code is easy to migrate when this happens, or maybe python can have a prototype for this feature in the spec. @owais any idea of the timeline for the traceresponse header to be stable?

@owais owais force-pushed the traceresponse-headers branch 2 times, most recently from 0e05254 to bb1bde1 Compare April 12, 2021 13:01
@owais owais changed the title Traceresponse headers Django trace response headers Apr 12, 2021
@owais owais force-pushed the traceresponse-headers branch 6 times, most recently from e3c842a to dacd59a Compare April 12, 2021 14:49
@owais
Copy link
Contributor Author

owais commented Apr 12, 2021

@aabmass Thanks. That makes sense and definitely makes everything a lot more cleaner and easier to implement in multiple instrumentations. I added an experimental back propagator in open-telemetry/opentelemetry-python#1762

I'll get back to you on the timeframe for the spec proposal.

Added opt-in support to return traceresponse headers from Django.

This allows users to configure their Django 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

As a result the implementation does not use a hard-coded header and
instead let's the users pick one.

This can be done by setting the `OTEL_PYTHON_TRACE_RESPONSE_HEADER` to
the header name that users want to inject in HTTP responses. The option
does not have a default value and the feature is disbaled when a env var
is not set.
@owais
Copy link
Contributor Author

owais commented Apr 13, 2021

Closed in favor of #436

@owais owais closed this Apr 13, 2021
@owais owais deleted the traceresponse-headers branch January 26, 2022 08:23
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.

2 participants