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

Improve documentation for baggage propagation into MDC #34977

Closed
Tracked by #35776
szpak opened this issue Apr 13, 2023 · 3 comments
Closed
Tracked by #35776

Improve documentation for baggage propagation into MDC #34977

szpak opened this issue Apr 13, 2023 · 3 comments
Assignees
Labels
theme: observability Issues related to observability type: documentation A documentation update
Milestone

Comments

@szpak
Copy link
Contributor

szpak commented Apr 13, 2023

Thanks to @marcingrzejszczak and #32480, a standard OpenTelemetry "baggage content" is automatically propagated by default (with just micrometer-tracing-bridge-otel added). However, to have it accessible via (logging) MDC, it is required to add some extra configuration (only spanId and traceId are propagated to MDC automatically):

management:
  tracing:
    baggage:
      correlation:
        fields:
          - baggage
      remote-fields:
        - baggage

It might be confusing, as one might expect to have the correlation and remote-fields available for non-default fields (while "baggage" seems to be - proposed - standard - https://www.w3.org/TR/baggage/#header-name).

If the "baggage" is enabled in tracing and if anything is provided inside why not put it also into MDC (to be clear - just to be accessible with custom custom logging pattern from MDC)?

Things to consider:

  • performance - could it be a problem? It is already passed by internally, only injecting it into MDC is to be added
  • ability to disable "baggage" field propagation - by adding some other fields or by setting the empty value/list in those 2 places in configuration?
  • backward compatibility - maybe there could be just a switch to turn the bahavior on (disabled by default for now), and eventually it would be enabled by default in some 3.x or maybe 4.0?
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 13, 2023
@marcingrzejszczak
Copy link
Contributor

marcingrzejszczak commented Apr 13, 2023

Hey Marcin :)

With OTel support we have 2 ways of supporting baggage. One, where baggage key == header and the other where all the baggage entries are there in the baggage key as mentioned in the standard (via W3cBaggagePropagator).

If the "baggage" is enabled in tracing and if anything is provided inside why not put it also into MDC (to be clear - just to be accessible with custom custom logging pattern from MDC)?

That's exactly how this should be working. We take all the baggage from all possible places (as mentioned above) and then we put it into MDC. Do you observe sth else?

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Apr 13, 2023
@szpak
Copy link
Contributor Author

szpak commented Apr 13, 2023

Thanks for your quick reply Marcin :). I needed some time to analyze those files and my configuration. In the end, I slightly misused the configuration.

I was using the W3C approach with the baggage HTTP header set to key=value, but I also set correlation.fields and remote-fields to baggage what was generating duplication. When I only set correlation.fields to key (remote-fields not set) it started to work as expected.

I've encountered that case, because for the baggage header with content baggage1=eKdxGpVclf, by default (no extra configuration with correlation/remote-fields), tracer.getAllBaggage() returns the baggage content (here {baggage1=eKdxGpVclf}), while MDC.copyOfContextMap() (SLF4J) returns only spanId and traceId (which I was trying to explain). One needs to use correlation.fields=baggage1 to put baggage1 into MDC.

In hindsight, if you know it (it might be deducted from the code examples after I re-read the Micrometer Tracing documentation), it is fine.

If the "baggage" is enabled in tracing and if anything is provided inside why not put it also into MDC (to be clear - just to be accessible with custom custom logging pattern from MDC)?

We take all the baggage from all possible places (as mentioned above) and then we put it into MDC. Do you observe sth else?

Essentially yes, but with the need to define correlation.fields for MDC, which is not needed for for baggage propagation in general. That generates some discrepancy in behavior.

Nevertheless, if it is a deliberate decision (performance?), with some documentation tweaks this issue could be closed.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 13, 2023
@marcingrzejszczak
Copy link
Contributor

Ok i guess we need to improve the docs with better examples. Brave requires to be explicit about these entries so we've decided to go with the same approach with otel

@wilkinsona wilkinsona added theme: observability Issues related to observability and removed status: feedback-provided Feedback has been provided labels May 9, 2023
@philwebb philwebb added type: documentation A documentation update and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 7, 2023
@philwebb philwebb added this to the 3.0.x milestone Jun 7, 2023
@philwebb philwebb changed the title Consider putting standard "baggage" field/header in MDC by default (if baggage is enabled in tracing) Improvide documentation for baggage propagation into MDC Jun 7, 2023
@philwebb philwebb changed the title Improvide documentation for baggage propagation into MDC Improve documentation for baggage propagation into MDC Jun 7, 2023
@philwebb philwebb mentioned this issue Jun 7, 2023
31 tasks
@mhalbritter mhalbritter self-assigned this Jun 19, 2023
@mhalbritter mhalbritter modified the milestones: 3.0.x, 3.1.x, 3.0.9 Jun 29, 2023
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: documentation A documentation update
Projects
None yet
Development

No branches or pull requests

6 participants