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

PROPAGATORS=b3,tracecontext has confusing behavior #425

Closed
trask opened this issue May 21, 2020 · 5 comments
Closed

PROPAGATORS=b3,tracecontext has confusing behavior #425

trask opened this issue May 21, 2020 · 5 comments
Assignees
@malafeev

Comments

@trask
Copy link
Member

trask commented May 21, 2020

If you configure:

PROPAGATORS=b3,tracecontext

then b3 header extraction does not work, because tracecontext header extraction occurs afterwards and will overwrite any context extracted from b3 headers with an invalid context.

From the javadoc for DefaultContextPropagators.Builder.addHttpTextFormat():

One propagator per concern (traces, correlations, etc) should be added

I suggest that we log a warning if user supplies two propagators for the same concern (e.g. both "b3" and "tracecontext") and only apply the first one.

Reported on gitter: https://gitter.im/open-telemetry/opentelemetry-auto-instr-java?at=5ec66b83549761730b4b882d

See also open-telemetry/opentelemetry-specification#496
@trask
Copy link
Member Author

trask commented May 21, 2020

@carlosalberto I want to check that this proposal make sense to you before I tag it help wanted

@carlosalberto
Copy link
Contributor

I suggest that we log a warning if user supplies two propagators for the same concern (e.g. both "b3" and "tracecontext") and only apply the first one.

This sounds like a reasonable option for now.

(That being said, I have heard users wanting a Propagator that could supports many formats, i.e. B3Propagator PLUS TraceContext PLUS Jaeger etc, and for this case cooperation between them so they don't overwrite their values is needed - something I'm prototyping locally, but eventually, medium term, I hope we can support that)

@trask trask added the contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome label May 22, 2020
@trask trask mentioned this issue May 25, 2020
Closed
@malafeev
Copy link
Contributor

@trask could you assign it to me?

@trask trask removed the contribution welcome Request makes sense, maintainers probably won't have time, contribution would be welcome label May 27, 2020
malafeev added a commit to malafeev/opentelemetry-auto-instr-java that referenced this issue May 28, 2020
@malafeev
open-telemetry#425 allow only one propagator per concern
46aed88 
Signed-off-by: Sergei Malafeev <[email protected]>
@malafeev malafeev mentioned this issue May 28, 2020
Merged
trask pushed a commit that referenced this issue May 28, 2020
@malafeev
#425 allow only one propagator per concern (#451)
c86a2af
@malafeev
Copy link
Contributor

fixed via #451, can be closed now.

@trask
Copy link
Member Author

trask commented May 29, 2020

👍

@trask trask closed this as completed May 29, 2020
iNikem added a commit that referenced this issue May 31, 2020
@iNikem @trask
@flands @malafeev @tylerbenson @mar-kolya @richardstartin @anuraaga
Rebase otel-java snapshot branch (#461)
72c9224 
* Define packages in exporter class loader (#409)

* Update docs about needing java 11 to build (#412)

* Update version to 0.3.0 (#413)

* Update the README (#414)

* Move build and configure to top as getting started section
* Add manual instrumentation section
* Document `@WithSpan` annotation
* Move developer specific information to CONTRIBUTING.md
* Cleanup formatting and use consistent spacing

* Update version to 0.4.0-SNAPSHOT (#415)

* Update CONTRIBUTING.md back to Java 11 (#419)

* Add Zipkin exporter support  (#411)

* #375 Add Zipkin exporter support

Signed-off-by: Sergei Malafeev <[email protected]>

* #375 use OkHttpSender for Zipkin exporter

Signed-off-by: Sergei Malafeev <[email protected]>

* #375 add Zipkin exporter to README

Signed-off-by: Sergei Malafeev <[email protected]>

Co-authored-by: Trask Stalnaker <[email protected]>

* Remove inactive maintainer (#420)

* Fix brolen anchor link (#422)

* Fix khttp instrumentation in case of absent or read-only headers map (#416)

* Add new approver (#429)

* Fix sqlNormalizerEnabled initialization (#432)

* Remove unused config (#424)

* Change names of servlet based server spans (#428)

* Add documentation describing non-obvious points of Servlet instrumentations

* Change names of servlet based server spans

* Fix java google format (#439)

* Remove deprecated usage from internal instrumentation (DataDog/dd-trace-java#1441)

* Update gradle to 6.4 (DataDog/dd-trace-java#1443)

* Migrate lettuce instrumentation away from deprecated finishSpanOnClose (DataDog/dd-trace-java#1445)

* Make Retrys consistent (DataDog/dd-trace-java#1442)

* Rename java packages for lettuce 4 and 5 to not have collisions (DataDog/dd-trace-java#1450)

* Adding an option to manually disable Kafka headers (DataDog/dd-trace-java#1448)

* Add version specific names to allow disabling only a specific version (DataDog/dd-trace-java#1456)

* Attempt to improve muzzle time by randomly ignoring versions until 10 remain (DataDog/dd-trace-java#1451)

* Save circle cache with name matching restore patterns (DataDog/dd-trace-java#1457)

* Wrap log statements using varargs to avoid object allocation (DataDog/dd-trace-java#1466)

* Grizzly-http and grizzly-client instrumentation (DataDog/dd-trace-java#1365)

* More refactoring for ScopeManager (DataDog/dd-trace-java#1467)

* Remove Java 9 and 10 from the build (DataDog/dd-trace-java#1473)

* Disable CI cache for muzzle (DataDog/dd-trace-java#1479)

* Separate out core instrumentation for AWS SDK to allow manual setup o… (#421)

* Separate out core instrumentation for AWS SDK to allow manual setup of instrumentation.

* Instrumentation core test

* Fix sporadic Elasticsearch test failures (#444)

* Fix sporadic grizzly test failure (#448)

* Fixes integration with latest version of Finatra (#450)

* #425 allow only one propagator per concern (#451)

* Updates to reflect new repo name (#454)

* Remove printlns that were accidentally committed (#459)

* Remove unnecessary version constant (#455)

Co-authored-by: Trask Stalnaker <[email protected]>
Co-authored-by: Steve Flanders <[email protected]>
Co-authored-by: Sergei Malafeev <[email protected]>
Co-authored-by: Tyler Benson <[email protected]>
Co-authored-by: Nikolay Martynov <[email protected]>
Co-authored-by: Brian Devins-Suresh <[email protected]>
Co-authored-by: Richard Startin <[email protected]>
Co-authored-by: Anuraag Agrawal <[email protected]>
@jamesylgan jamesylgan mentioned this issue Jul 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@malafeev malafeev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@trask @carlosalberto @malafeev