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

Tracing only supports a single context propagation type #35611

Conversation

mhalbritter
Copy link
Contributor

@mhalbritter mhalbritter commented May 24, 2023

This change adds support for multiple context propagation types (e.g. B3 and W3C). It also adds support for a new propagation type, B3_MULTI (which is B3 with multiple headers instead of the single b3 header).

There are now two new properties:

  • management.tracing.propagation.consume which controls which trace formats the application will understand on incoming requests. It defaults to all known trace formats (W3C, B3 single, B3 multi).
  • management.tracing.propagation.produce which controls which trace formats the application will add to outgoing requests. It defaults to W3C.

The management.tracing.propagation.type has been changed from PropagationType to List<PropagationType> and the default has been removed. If set, this will override both management.tracing.propagation.consume and management.tracing.propagation.produce.

The formats have an ordering, meaning that if one of the extractors matches, the one after it are no longer considered. The default ordering is:

  1. W3C
  2. B3 single
  3. B3 multi

The CompositePropagationFactory is the main piece of code for Brave which knows how to extract and inject multiple formats. It's a copy from Sleuth, somewhat refactored.

The CompositeTextMapPropagator is the main piece of code for OTel. It's somewhat more complicated than the Brave pendant as we need to support the BaggageTextMapPropagator which must run always even if one of the extractors before it has already extracted the context.

I also removed a @ConditionalOnMissingBean for type B3Propagator and W3CTraceContextPropagator. This should be fine as it's not consistent anyway. In the bagagge = true case those @ConditionalOnMissingBean are not effective but the extractors are added unconditionally in w3cTextMapPropagatorWithBaggage and b3BaggageTextMapPropagator.

Property examples

  • To only use B3 single, set management.tracing.propagation.type to B3

  • To only use B3 multi, set management.tracing.propagation.type to B3_MULTI

  • To only use W3C multi, set management.tracing.propagation.type to W3C

  • To produce B3 single and W3C and accept W3C, B3 single and multi, set management.tracing.propagation.produce to B3,W3C and management.tracing.propagation.consume to B3,B3_MULTI,W3C

See gh-35096

@mhalbritter mhalbritter added status: waiting-for-triage An issue we've not yet triaged theme: observability Issues related to observability labels May 24, 2023
@mhalbritter mhalbritter changed the title Add support for multiple context propagation types for tracing WIP: Add support for multiple context propagation types for tracing May 24, 2023
@mhalbritter mhalbritter added the status: blocked An issue that's blocked on an external project change label May 24, 2023
@philwebb philwebb added this to the 3.x milestone May 24, 2023
@mhalbritter mhalbritter force-pushed the mh/35096-add-support-for-multiple-context-propagation-types-for-tracing branch 2 times, most recently from c5ef27d to 7070de0 Compare May 25, 2023 12:04
@mhalbritter mhalbritter force-pushed the mh/35096-add-support-for-multiple-context-propagation-types-for-tracing branch 2 times, most recently from da15179 to 4dd3d42 Compare May 30, 2023 13:06
@mhalbritter mhalbritter removed the status: blocked An issue that's blocked on an external project change label May 31, 2023
@mhalbritter
Copy link
Contributor Author

It works for both Brave and Otel now, build is green too.

@mhalbritter mhalbritter changed the title WIP: Add support for multiple context propagation types for tracing Add support for multiple context propagation types for tracing May 31, 2023
@mhalbritter mhalbritter force-pushed the mh/35096-add-support-for-multiple-context-propagation-types-for-tracing branch from a3da5b1 to 532227d Compare June 5, 2023 10:03
@jonatan-ivanov jonatan-ivanov modified the milestones: 3.x, 3.0.x Jun 5, 2023
@jonatan-ivanov jonatan-ivanov added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 5, 2023
@wilkinsona wilkinsona changed the title Add support for multiple context propagation types for tracing Tracing only supports a single context propagation type Jun 6, 2023
@mhalbritter
Copy link
Contributor Author

mhalbritter commented Jun 7, 2023

When merging into 3.1.x, the test BraveAutoConfigurationTests.shouldSupportJoinedSpansIfB3UsedAndBackendSupportsIt fails. That's because it expects that span joining is supported when using B3. However, as we now extract all formats from the incoming request, CompositePropagationFactory.supportsJoin always returns false, as there is a W3CPropagation in the extractor list and it doesn't support joins. CompositePropagationFactory.supportJoin is implemented like this:

Stream.concat(injectorFactories.stream(), extractorFactories.stream()).allMatch(Factory::supportsJoin);

Is that just an unfortunate consequence because we now apply all extractors, or is the span joining feature just for injecting and the supportJoin should be changed to only work on the injectorFactories?

@jonatan-ivanov As you implemented the test / feature, maybe you have an idea?

If that's an unfortunate consequence and can't be changed, do we need a way to opt out of extracting all known formats? With that, users could configure B3 only for injecting and extracting, and span joining works again. Maybe by adding a property management.tracing.propagation.consume-types of type List<PropagationType> which defaults to PropagationType.values() (all known types)?

@mhalbritter mhalbritter added the status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team label Jun 7, 2023
@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented Jun 7, 2023

Yeah, if we consume all formats by default supportsJoin won't work, I completely missed this when we discussed this issue, I'm sorry.
I think letting the users define the consumed formats would work. I'm also thinking if we should set the right formats if supportsJoin=true. That might be a little shady side effect though.

@philwebb philwebb mentioned this pull request Jun 7, 2023
31 tasks
@mhalbritter mhalbritter force-pushed the mh/35096-add-support-for-multiple-context-propagation-types-for-tracing branch from a0e5e2a to a2796f1 Compare June 12, 2023 10:05
@mhalbritter
Copy link
Contributor Author

No worries Jonatan. I've now added properties to control which formats we produce, which we accept and the management.tracing.propagation.type controls both at the same time.

management.tracing.propagation.type no longer has a default. If set, it overrides management.tracing.propagation.consume-types and management.tracing.propagation.produce-types.

With that in place, we should have enough flexibility in our tracing setup. And users still have one property management.tracing.propagation.type if they aren't interested in configuring the low-level details.

@wilkinsona
Copy link
Member

management.tracing.propagation.consume-types and management.tracing.propagation.produce-types

Alternative names for these properties to consider:

  • management.tracing.propagation.consume and management.tracing.propagation.produce
  • management.tracing.propagation.types.consume and management.tracing.propagation.types.produce

@mhalbritter
Copy link
Contributor Author

I've settled on management.tracing.propagation.consume and management.tracing.propagation.produce. This gets rid of the dash and reads cleaner.

@mhalbritter mhalbritter removed the status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team label Jun 15, 2023
@mhalbritter mhalbritter modified the milestones: 3.0.x, 3.0.8 Jun 15, 2023
@mhalbritter
Copy link
Contributor Author

And it's a merge! Thank you all for providing input!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: observability Issues related to observability type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants