search

open-telemetry / opentelemetry-java-instrumentation

OpenTelemetry auto-instrumentation and instrumentation libraries for Java
https://opentelemetry.io
Apache License 2.0
1.92k stars 839 forks source link

Add support for TraceMultiPropagator #637

Closed malafeev closed 4 years ago

malafeev commented 4 years ago

opentelemtry 0.6 comes with TraceMultiPropagator

current PropagatorsInitializer supports only 'tracecontext' or 'b3'.

malafeev commented 4 years ago

please assign issue to me

malafeev commented 4 years ago

I see two options:

  1. Add tracemulti which means b3, b3single, tracecontext:

    PROPAGATORS=tracemulti

  2. Or another format:

    PROPAGATORS=tracemulti:b3,b3single,tracecontext

    It means user can specify propagators to use in multi propagator.

cc @carlosalberto

anuraaga commented 4 years ago

To make sure I understand, what's the difference between these in the proposal?

PROPAGATORS=tracemulti:b3,b3single,tracecontext

and

PROPAGATORS=b3,b3single,tracecontext

malafeev commented 4 years ago

@anuraaga as I understand @carlosalberto propose explicitly specify tracemulti keyword.

iNikem commented 4 years ago

@carlosalberto please tell us, why have you done this? :)

carlosalberto commented 4 years ago

PROPAGATORS=tracemulti:b3,b3single,tracecontext

This one extracts a single SpanContext at most.

PROPAGATORS=b3,b3single,tracecontext

This one extracts many SpanContext, if such formats are found in the carrier.

carlosalberto commented 4 years ago

Also, for the tracemulti we can add additional logic, such as extract many formats, and only inject the first (or last) format (say, TraceContext).

iNikem commented 4 years ago

PROPAGATORS=tracemulti:b3,b3single,tracecontext

This one extracts a single SpanContext at most.

PROPAGATORS=b3,b3single,tracecontext

This one extracts many SpanContext, if such formats are found in the carrier.

Wow, I think that will be confusing for end users. Do we really need such separation?

carlosalberto commented 4 years ago

The plan is to actually discourage the users from using the first case, by exposing the second case ;)

Observe that in general only a Propagator per signal should be specified, and the tracing case is a specific, given that are many formats around (and thus we may need to support a few or all of them).

iNikem commented 4 years ago

The plan is to actually discourage the users from using the first case, by exposing the second case ;)

Are you sure you got the order right here?

Extracting many SpanContext does not make sense. You can have only 1 remote parent span. Can we just have PROPAGATORS=b3,b3single,tracecontext format and specify which one wins? It seems to me as the least surprising thing to do. And we are not GA yet, so it is Ok to change behaviour.

anuraaga commented 4 years ago

Wow, I think that will be confusing for end users. Do we really need such separation?

I'm already confused, what does it mean to "extracts many SpanContext"? There can only ever be one SpanContext which corresponds to the new currentSpan. Is it just about multiple headers would overwrite each other? That doesn't seem so bad, is the concern performance?

such as extract many formats, and only inject the first (or last) format

This sounds like it would be easier to configure as two flags, EXTRACTORS and INJECTORS. Creating a magic flag with such specific behavior that a user needs to dig into docs to fully understand seems too complicated to me.

iNikem commented 4 years ago

@carlosalberto I think what you are saying here contradicts javadoc of https://github.com/open-telemetry/opentelemetry-java/blob/master/extensions/trace_propagators/src/main/java/io/opentelemetry/extensions/trace/propagation/TraceMultiPropagator.java. I mean, PROPAGATORS=b3,b3single,tracecontext format should work exactly as that javadoc says. I don't see any need for multiple formats here.

If PROPAGATORS contains one value, corresponding propagator will be used. If it contains multiple values, then TraceMultiPropagator will be used with already nicely defined semantic.

carlosalberto commented 4 years ago

Hey hey. Will clarify a few things:

By doing: 

OTEL_PROPAGATORS=jaeger,b3,tracecontext

You effectively get multiple formats support - but if the carrier contains the three formats, it means SpanContext will be extracted three times. So this is a correct and valid scenario, but it's wasting cycles.

OTEL_PROPAGATORS=multi-trace:jaeger,b3,tracecontext

In this case, we create a MultiTracePropagator with these 3 formats, and extraction will stop/return when the first SpanContext is successfully extracted.

If PROPAGATORS contains one value, corresponding propagator will be used. If it contains multiple values, then TraceMultiPropagator will be used with already nicely defined semantic.

@malafeev had already told me about this possibility. I personally don't like it so much, but if you prefer it, let's go with it ;) But you would need extra logic/fun to detect which propagator is a trace one, as propagators will be used by other signals:

OTEL_PROPAGATORS=correlationcontext,jaeger,b3,tracecontext,custompropagationsignal
iNikem commented 4 years ago

But you would need extra logic/fun to detect which propagator is a trace one, as propagators will be used by other signals

How does the original propose make this easier?

trask commented 4 years ago

Hey @carlosalberto, does it make sense for the OTEL_PROPAGATORS env var be defined at the spec level? e.g. https://github.com/open-telemetry/opentelemetry-specification/issues/572

carlosalberto commented 4 years ago

@trask Actually yes, let's work on this proposal there.

trask commented 4 years ago

I suggest we go with @iNikem's suggestion above, at least for now, so we can unblock this needed feature:

If PROPAGATORS contains one value, corresponding propagator will be used. If it contains multiple values, then TraceMultiPropagator will be used with already nicely defined semantic.

In auto-instrumentation at least, the PROPAGATORS configuration only supports a fixed set of known propagators, so I don't think we have the issue of detecting propagators for other signals, e.g. custompropagationsignal.