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

Optionally add baggage to span attributes #8023

Closed

Conversation

adamleantech
Copy link
Contributor

@adamleantech adamleantech commented Mar 10, 2023

resolves #4520

@adamleantech adamleantech marked this pull request as ready for review March 10, 2023 13:08
@adamleantech adamleantech requested a review from a team March 10, 2023 13:08
@laurit
Copy link
Contributor

laurit commented Mar 10, 2023

Did you consider using a SpanProcessor for this?

@adamleantech
Copy link
Contributor Author

@laurit good suggestion, I wasn't aware of that

@adamleantech adamleantech force-pushed the add-baggage-to-spans branch from 064765a to 6443b85 Compare March 16, 2023 11:39
@adamleantech adamleantech force-pushed the add-baggage-to-spans branch from 6443b85 to 4afc902 Compare March 16, 2023 12:04
@adamleantech
Copy link
Contributor Author

@laurit updated PR as per your suggestion. I've realised that there is probably an issue with my previous PR for logback MDC in that the "baggage." prefix should be on the key not the value

@laurit
Copy link
Contributor

laurit commented Mar 16, 2023

@adamleantech thanks for reporting, I'll fix it

@@ -20,6 +20,7 @@
@AutoService(AutoConfigurationCustomizerProvider.class)
public class AgentTracerProviderConfigurer implements AutoConfigurationCustomizerProvider {
private static final String ADD_THREAD_DETAILS = "otel.javaagent.add-thread-details";
private static final String ADD_BAGGAGE = "otel.javaagent.span.add-baggage";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would correspond with, e.g. otel.traces.exporter, otel.metrics.exporter, otel.logs.exporter

Suggested change
private static final String ADD_BAGGAGE = "otel.javaagent.span.add-baggage";
private static final String ADD_BAGGAGE = "otel.javaagent.traces.add-baggage";

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if otel.javaagent.span.add-baggage should take a list of baggage to add (or * for all), this would match otel.instrumentation.logback-appender.experimental.capture-mdc-attributes

@trask
Copy link
Member

trask commented Apr 13, 2023

I wonder if otel.javaagent.span.add-baggage should take a list of baggage to add (or * for all), this would match otel.instrumentation.logback-appender.experimental.capture-mdc-attributes

and maybe we should do the same for otel.instrumentation.logback-mdc.add-baggage?

@evantorrie
Copy link
Contributor

I'm interested in this same functionality but for use as an AutoConfigureSpi addition to a manually instrumented program (which uses the AutoConfigure SDK extension).

Is there anything which ties AgentTracerProviderConfigurer to the java agent (other than the name?).

Not for this PR obviously, but I wonder if this and AddThreadDetailsSpanProcessor could be relocated into the https://github.com/open-telemetry/opentelemetry-java repo as some sort of sdk-extension-customizer artifact?

@breedx-splk
Copy link
Contributor

breedx-splk commented Jul 10, 2023

Is there anything which ties AgentTracerProviderConfigurer to the java agent (other than the name?).

Yeah, this configurer is coupled to the agent through the AgentConfig as well...but I think there's room for that to be a separate responsibility (separate PR).

Not for this PR obviously, but I wonder if this and AddThreadDetailsSpanProcessor could be relocated into the https://github.com/open-telemetry/opentelemetry-java repo as some sort of sdk-extension-customizer artifact?

I am liking this idea. @jack-berg and @jkwatson what do you think? Is there room for add-on processors like this in the core repo, or would you prefer to see it as a contrib addition, or something else?

@jack-berg
Copy link
Member

Contrib is the appropriate place for this. If such a processor were to occur in the spec, we could promote that component the core repo.

@galbash
Copy link

galbash commented Nov 19, 2023

Has this been contributed to the contrib repo / exists on the java agent?

@trask
Copy link
Member

trask commented Nov 28, 2023

Has this been contributed to the contrib repo / exists on the java agent?

not yet

span.setAttribute(
// add prefix to key to not override existing attributes
"baggage." + key,
value.getValue()));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the lack of spec around this topic, it would be good to keep the attribute naming flexible and have the baggage namespace prefix come in through the constructor as an optional parameter. The suggested whitelisting of baggage keys to attach would provide the necessary security to prevent unwanted overrides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically append baggage entries to span attributes
8 participants