-
Notifications
You must be signed in to change notification settings - Fork 41.3k
MDC & Baggage AutoConfiguration for Micrometer Tracing with Brave and OpenTelemetry #32480
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
MDC & Baggage AutoConfiguration for Micrometer Tracing with Brave and OpenTelemetry #32480
Conversation
0bce626
to
4552de2
Compare
c549826
to
3e1424b
Compare
@ConditionalOnProperty(value = "management.tracing.propagation.type", havingValue = "W3C", | ||
matchIfMissing = true) | ||
Factory w3cPropagationNoBaggageFactory() { | ||
return new W3CPropagation(BRAVE_BAGGAGE_MANAGER, List.of()); // TODO: Use |
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.
micrometer-metrics/tracing@29feef8 is the change that we need so that there's no baggage.
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.
Correct, I mean if we leave this as it is the components for baggage (i.e. BraveBaggageManager
) will be called, but no baggage will be propagated cause there's nothing configured. With this change in tracing we are nulling anything baggage related so that will not be called at all
/** | ||
* Whether to enable correlation of the baggage context with logging contexts. | ||
*/ | ||
private boolean correlationEnabled = true; | ||
|
||
/** | ||
* List of fields that should be correlated with the logging context. That means | ||
* that these fields would end up as key-value pairs in e.g. MDC. | ||
*/ | ||
private List<String> correlationFields = new ArrayList<>(); |
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.
It looks as if there could be a correlation
group with enabled
and fields
properties.
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.
Yeah, that's a good idea!
@@ -53,4 +74,81 @@ public void setProbability(float probability) { | |||
|
|||
} | |||
|
|||
public static class Baggage { |
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 class could have an enabled
property, removing the need for some additional metadata.
}, | ||
{ | ||
"name": "management.tracing.sampling.probability", | ||
"defaultValue": "0.10" |
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.
Does the annotation processor not pick up this default automatically?
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.
Intellij told me that it doesn't 🤷
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.
It appears to be wrong (https://docs.spring.io/spring-boot/docs/3.0.0-M5/reference/html/application-properties.html#application-properties.actuator.management.tracing.sampling.probability) so this can be reverted.
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.
Please check default values are produced by building on the command-line.
3e1424b
to
1b21cb2
Compare
1b21cb2
to
e8730ac
Compare
Rationale
In order to provide the users with the basic tracing observability story we need the following, additional features
Conditionals
prerequisites
superseeds #32214