Adding new optional post_injection_hook to RequestsInstrumentor
#3657
Conversation
The `tracestate` and `baggage` headers may contain sensitive information that is valuable to propagate to trusted systems but could pose concerns when sent to untrusted systems. Currently there is no way to support instrumenting these spans while ensuring these, or other, headers are conditionally propagated. This new option allows callers to define a callable to mutate the post-injection headers based on each request.
moved change to correct unreleased spot
forgot to remove the old line
removed unnecessary white space change
| @@ -300,6 +329,9 @@ def get_or_create_headers(): | |||
| headers = get_or_create_headers() | |||
| inject(headers) | |||
|
|
|||
| if callable(post_injection_hook): | |||
| post_injection_hook(headers, request) | |||
There was a problem hiding this comment.
Would it work for your use case to -- instead of passing in a general-purpose post-injection hook -- pass in an allowlist and inject or not based on that? Seems like the allowlist use case would be common enough to justify baking in explicit support (even at the api/spec level).
There was a problem hiding this comment.
I considered that but I think users will differ in what and when to propagate these headers. For example, this user wants to avoid all headers for one host while only baggage for external domains.
I suspect it will be difficult to have a truly one-size fits all. I could even see a case where you base the decision on values in the baggage itself.
edit: also i noticed the request/response hooks started as separate single purpose options that were changed to hooks and I didn't want to repeat that
|
If it helps, there have been a few discussions about this in opentelemetry-specification. I had originally requested something like this in open-telemetry/opentelemetry-specification#3799, but the consensus was to focus on baggage, which is in open-telemetry/opentelemetry-specification#3957. But since then there seemed to be some consensus around doing this in instrumentations, like this PR does: open-telemetry/opentelemetry-specification#1633 (comment) FWIW, this style of hook (or the allowlist pmcollins suggested) would solve my original issue, so I hope some version of this moves forward. |
Thank you so much for these links. I wondered how I could be the first person to consider the security implications of auto instrumentation sending baggage to 3rd parties. I could definitely see this being a broader need of the contrib package. This PR was really just a simple solution to get value as quickly as possible. |
Description
The
tracestateandbaggageheaders may contain sensitive information that is valuable to propagate to trusted systems but could pose concerns when sent to untrusted systems. Currently there is no way to support instrumenting these spans while ensuring these, or other, headers are conditionally propagated. This new option allows callers to define a callable to mutate the post-injection headers based on each request.Type of change
How Has This Been Tested?
tox -e py312-test-instrumentation-requestsDoes This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.