-
Notifications
You must be signed in to change notification settings - Fork 1
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
NH-51264 Remove custom naming baggage at propagator.inject #184
Conversation
) -> str: | ||
"""Removes values used for custom naming from baggage header created | ||
by upstream W3CBaggagePropagator propagator, if present""" | ||
baggage_entries: list[str] = split(_DELIMITER_PATTERN, baggage_header) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This split is the same as how W3CBaggagePropgator.extract does it: https://github.com/open-telemetry/opentelemetry-python/blob/b9f31e91a8263bc8effc9abce889d438927d47ec/opentelemetry-api/src/opentelemetry/baggage/propagation/__init__.py#L66
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the other split
don't have to be this way of course. Open to suggestions if any.
baggage_kvs = {} | ||
for entry in baggage_entries: | ||
try: | ||
e_name, e_value = entry.split("=", 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also the same as how W3CBaggagePropagator does it: https://github.com/open-telemetry/opentelemetry-python/blob/b9f31e91a8263bc8effc9abce889d438927d47ec/opentelemetry-api/src/opentelemetry/baggage/propagation/__init__.py#L85
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Though I wonder if it'd be simpler if we used a single baggage key?
Could do! I'll do it in a separate PR. |
APM Python's custom propagator now removes any
sw-current-trace-id
orsw-current-entry-span-id
KVs from context baggage at injection, if present. These may or may not have been set whenset_transaction_name
was previously called.This relies on Otel Python W3CBaggagePropagator being used first in the composite. This is to leverage extraction of incoming
baggage
headers and check that they're correctly formatted, then to pre-set thebaggage
header for injection. Custom propagator removes the twosw
KVs from thebaggage
header already set by W3CBaggagePropagator. Therefore, there is an added rule here for specifyingOTEL_PROPAGATORS
and customer docs will have to be updated.Here's what happens now when testbed Django A receives a request
curl -v http://0.0.0.0:8002/home_a/ -H "baggage:my-foo=some-baggage"
resulting in a request to Django B:OTEL_PROPAGATORS
Baggage
request header to Django Btracecontext,baggage,solarwinds_propagator
)'Baggage': 'my-foo=some-baggage'
tracecontext,solarwinds_propagator
baggage
is optionaltracecontext,baggage,solarwinds_propagator
'Baggage': 'my-foo=some-baggage'
tracecontext,solarwinds_propagator,baggage
baggage
is aftersolarwinds_propagator
Before this PR for the first scenario, the request header would have been, for example:
'Baggage': 'my-foo=some-baggage,sw-current-trace-id=211437738876350461983321418219135041650,sw-current-entry-span-id=14984741770333256707'
Please let me know if any questions or suggestions! 😺