add baggage propagator to default propagators #1545

malafeev · 2020-11-03T12:48:13Z

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/PropagatorsInitializer.java

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

carlosalberto · 2020-11-03T18:56:02Z

IIRC there's a spot where, if the user specified values (b3, jaeger, etc), we put all the propagators in a TraceMultiPropagator, right? If that's the case, we need to update that as well, to prevent adding the Baggage propagator there ;)

malafeev · 2020-11-04T02:22:01Z

@carlosalberto if user specifies propagators (e.g. b3, jaeger) then Baggage propagator will not be added.
Baggage propagator added only if user didn't specify any propagator or if user specified baggage propagator.
In the case if user provided b3,baggage then Baggage propagator will be added in a TraceMultiPropagator.
Am I right that TraceMultiPropagator should never contain Baggage propagator even if user specified multiple propagators including baggage?

trask · 2020-11-04T18:12:44Z

Am I right that TraceMultiPropagator should never contain Baggage propagator even if user specified multiple propagators including baggage?

Yes, I hadn't thought about this, but it makes sense, as TraceMultiPropagator stops extracting once it extracts a span context successfully.

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

malafeev · 2020-11-05T16:02:17Z

@trask @carlosalberto fixed adding Baggage propagator to TraceMultiPropagator

malafeev · 2020-11-05T17:15:17Z

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/PropagatorsInitializer.java

@@ -50,6 +53,7 @@ public static void initializePropagators(List<String> propagators) {
      OpenTelemetry.setGlobalPropagators(
          DefaultContextPropagators.builder()
              .addTextMapPropagator(HttpTraceContext.getInstance())
+              .addTextMapPropagator(W3CBaggagePropagator.getInstance())


@carlosalberto
W3CBaggagePropagator should be first one or last one?
it doesn't matter for inject but for extract we return context from last propagator.

It does't matter for the default configuration case 😄

@carlosalberto does it matter the order we add W3CBaggagePropagator for the non-default configuration case?

@trask So Baggage propagators work just like the Trace ones (B3, TraceContext, etc), regarding the fact that, upon successful extraction, any previous value will be overridden (i.e. that last one wins).

Currently only the Jaeger propagator does Baggage extraction besides W3CBaggage, so the order may matter in such case, e.g. if the user explicitly specifies OTEL_PROPAGATORS=jaeger,baggage, then W3CBaggage will have higher priority, given the case that Baggage was propagated using both Jaeger and W3CBaggage formats.

Then again, when users specify this, they should know what they are doing, IMHO 😉 (although maybe we should add a note in OTel Java regarding this, etc).

@carlosalberto do u know what we should do if the user specifies:

OTEL_PROPAGATORS=jaeger,baggage,tracecontext

Should we add both TraceMultiPropagator(jaeger,tracecontext) and W3BaggagePropagator? And if so, do u know whether we should add TraceMultiPropagator first or last?

@malafeev Yes ;) - and put a comment in the code on why we are doing it (i.e. Jaeger handles BOTH tracing and baggage, etc).

@malafeev I think both users and developers will have a hard time understanding how otel.propagators works now :) Please document the final result both in PropagatorsInitializer.java and in README. The latter, for example, currently does very poor job explaining the difference between trace context propagators and baggage ones.

@iNikem I added javadoc, is it clear enough?

Sorry, but no :) I will add comments to javadoc. And we still have to update README

# Conflicts: # README.md

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

iNikem

This PR introduces quiet complex rules about how propagators are configured and how they impact each other. These rules are not documented neither for developers in PropagatorsInitializer nor for end-users in README.

iNikem · 2020-11-10T14:22:27Z

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/PropagatorsInitializer.java

@@ -50,6 +53,7 @@ public static void initializePropagators(List<String> propagators) {
      OpenTelemetry.setGlobalPropagators(
          DefaultContextPropagators.builder()
              .addTextMapPropagator(HttpTraceContext.getInstance())
+              .addTextMapPropagator(W3CBaggagePropagator.getInstance())


Sorry, but no :) I will add comments to javadoc. And we still have to update README

iNikem · 2020-11-10T14:22:56Z

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/PropagatorsInitializer.java

+   * extraction next rules applied:
+   *
+   * <ul>
+   *   <li>W3CBaggagePropagator is added to DefaultContextPropagators to allow another propagator


Is it always added? Or only if baggage value is provided?

added can be ... only

iNikem · 2020-11-10T14:23:09Z

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/PropagatorsInitializer.java

+   * <ul>
+   *   <li>W3CBaggagePropagator is added to DefaultContextPropagators to allow another propagator
+   *       extract Context.
+   *   <li>JaegerPropagator is added to DefaultContextPropagators because it extracts both Context


Is it always added? Or only if jaeger value is provided?

added can be ... only

iNikem · 2020-11-10T14:28:13Z

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/PropagatorsInitializer.java

+   *   <li>JaegerPropagator is added to DefaultContextPropagators because it extracts both Context
+   *       and Baggage. Otherwise in TraceMultiPropagator it may not get a chance to extract any
+   *       existing Baggage.
+   *   <li>W3CBaggagePropagator comes after JaegerPropagator, as it can have more complex/complete


Above you say "returns first successful extracted Context", here you seem to say that later Baggage extraction can enrich the previous one. If context and baggage extractions behave differently in this regard, we have to say so explicitly.

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

malafeev · 2020-11-10T15:26:37Z

I didn't update README yet, let me finish with javadoc.
And I'm not sure that it should be in instrumentation README, maybe better place is otel-java.

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

malafeev · 2020-11-12T15:28:59Z

updated README

trask

ok, I think I understand propagators now 😅

of course, that's what I thought before too 😁

last request is that I think this behavior of OTEL_PROPAGATORS should be documented in the spec

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/PropagatorsInitializer.java

trask · 2020-11-13T04:15:02Z

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/PropagatorsInitializer.java

-      TextMapPropagator textPropagator = TEXTMAP_PROPAGATORS.get(propagatorId.trim().toLowerCase());
+    List<String> propagatorIds =
+        propagators.stream()
+            .map(propagator -> propagator.trim().toLowerCase())


I don't see mention of supporting case-insensitivity in the spec, I would suggest leaving this out?

it was from beginning I suppose, didn't want change behavior

ok, I created #1632 to track this

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/PropagatorsInitializer.java

README.md

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

malafeev · 2020-11-13T13:25:10Z

@trask I applied your suggested changes except one about case sensitivity.

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

trask · 2020-11-13T19:41:51Z

last request is that I think this behavior of OTEL_PROPAGATORS should be documented in the spec

@carlosalberto can you take this?

carlosalberto · 2020-11-15T21:20:28Z

@trask This part is not documented at all in the Spec (although there's a ticket to standardize TraceMultiPropagator, which only exists for Java at the moment, as an improvement in order to not extract SpanContext more than once), so no need to do it there for now.

However, we should, for sanity purposes, document this in TraceMultiPropagator in OTel Java, mentioning it should only contain SpanContext-only Propagators, in order to avoid unexpected, incorrect scenarios. Does that makes sense?

(We could also document in the Spec that the OTEL_PROPAGATOR values, and probably all env-var values, should be lower-case. Sounds about right?)

trask · 2020-11-15T21:49:24Z

thanks @carlosalberto

my concern with this not being defined in the spec, is that once it is defined, if it varies from our implementation, it would be a breaking change for our users

# Conflicts: # README.md

carlosalberto · 2020-11-18T15:15:46Z

Hey @trask

Sorry for the delay.

my concern with this not being defined in the spec, is that once it is defined, if it varies from our implementation, it would be a breaking change for our users

So the idea of TraceMultiPropagator is simply that we don't extract SpanContext more than once (as an optimization), and MUST NOT change the propagation semantics at all. Logically speaking, the only thing to be careful about these days, is that only trace-only Propagators are added to it.

So, when we standardize this in the Spec, semantics MUST NOT change, and we will simply offer this as a optimization.

trask · 2020-11-19T07:34:57Z

@carlosalberto thanks for that explanation, I think I understand now(!?).

@malafeev I sent a PR to your PR that I think would clarify a lot of questions/troubles that reviewers had understanding this.

Simplify the rules

malafeev · 2020-11-19T08:48:11Z

@trask I merged your PR

iNikem · 2020-11-19T09:19:02Z

@trask @malafeev @carlosalberto Can you please update me on this PR? I am still concerned that just from README nobody will understand how to configure propagators. Especially when interested in baggage as well.

malafeev · 2020-11-19T10:20:03Z

For user it's simple like OTEL_PROPAGATORS=b3,baggage. Internally it's complicated but not for end user.

iNikem · 2020-11-19T10:37:35Z

For user it's simple like OTEL_PROPAGATORS=b3,baggage. Internally it's complicated but not for end user.

So if I specify b3multi, I will not get baggage? So I have to always specify baggage if I want to propagated? What about jaeger,baggage?

iNikem · 2020-11-19T10:38:46Z

How should user know at all that some of the "Supported propagators are tracecontext, baggage, b3, b3multi, jaeger, ottracer, and xray" are trace only (which ones?), some are baggage only (which ones?) and some are both?

malafeev · 2020-11-19T11:05:22Z

I don't think these questions should be answered here. Explanation of how propagators work is the otel-java or spec job.

iNikem · 2020-11-19T12:09:52Z

I don't think these questions should be answered here. Explanation of how propagators work is the otel-java or spec job.

You may be right, I will not argue. Then we should link to that explanation from our README.

carlosalberto · 2020-11-19T16:24:06Z

@iNikem Hah! Yes, the user doesn't really need to worry about the propagators, at least through OTEL_PROPAGATORS - actual users of TraceMultiPropagator do however, as it helps to prevent double extraction of SpanContext, with the restriction that if another concern is extracted as part of it, such as baggage, it MUST NOT be included there (Jaeger is the prime example).

We should definitely document that (specially in OTel Java itself).

In other words:

Only Trace/SpanContext-only Propagators must be added to TraceMultiPropagator.
HttpTraceContext, AWSPropagator, B3 (both single and multi encoding) can be added to TraceMultiPropagator.
Baggage and Jaeger (and soon OTPropagator) must not be added to TraceMultiPropagator, as they are not Trace/SpanContext-only Propagators.

I'd say: in case of doubt, don't use TraceMultiPropagator, as it's an optimization to prevent multiple extraction of the same data.

trask · 2020-11-19T23:32:37Z

@iNikem is it ok to dismiss your request for changes, or is there anything else you'd like in this PR?

trask · 2020-11-20T04:16:11Z

Closes #1632 (does it work to put that here?)

iNikem

I have created #1716 for proper documentation.

iNikem · 2020-11-20T13:24:13Z

@iNikem is it ok to dismiss your request for changes, or is there anything else you'd like in this PR?

why do you ask when you haven't yet approved it yourself? :)

trask · 2020-11-20T17:55:34Z

why do you ask when you haven't yet approved it yourself? :)

oops 😅

add baggage propagators to default propagators

1ef59d8

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

malafeev requested review from anuraaga, iNikem, jkwatson, mateuszrzeszutek, trask and tylerbenson as code owners November 3, 2020 12:48

malafeev changed the title ~~add baggage propagators to default propagators~~ add baggage propagator to default propagators Nov 3, 2020

carlosalberto reviewed Nov 3, 2020

View reviewed changes

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/PropagatorsInitializer.java Outdated Show resolved Hide resolved

add W3CBaggagePropagator directly through addTextMapPropagator()

dcd3f1e

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

never add Baggage propagator to TraceMultiPropagator

7fd8013

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

malafeev commented Nov 5, 2020

View reviewed changes

Sergei Malafeev added 3 commits November 9, 2020 21:26

Merge branch 'master' into baggage-propagator

d00845a

# Conflicts: # README.md

leave JaegerPropagator out of the TraceMultiPropagator

f550cdd

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

add javadoc explaining rules for putting propagators

4076488

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

iNikem suggested changes Nov 10, 2020

View reviewed changes

Sergei Malafeev added 2 commits November 10, 2020 23:17

update javadoc explaining rules for putting propagators

8f4ada7

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

update javadoc explaining rules for putting propagators

c78d5af

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

update README.md

c08df32

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

malafeev requested a review from iNikem November 12, 2020 16:19

trask reviewed Nov 13, 2020

View reviewed changes

Apply suggestions from code review

b2ebc48

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

fix styling

d46fba2

Signed-off-by: Sergei Malafeev <sergei@malafeev.org>

trask mentioned this pull request Nov 13, 2020

Don't support case-insensitivity in propagator names #1632

Closed

Merge branch 'master' into baggage-propagator

a678c7e

# Conflicts: # README.md

Simplify the rules

40a00e7

Merge pull request #6 from trask/pr-baggage-propagator

3569df1

Simplify the rules

iNikem approved these changes Nov 20, 2020

View reviewed changes

trask approved these changes Nov 20, 2020

View reviewed changes

trask merged commit 4b3bbf6 into open-telemetry:master Nov 20, 2020

add baggage propagator to default propagators #1545

add baggage propagator to default propagators #1545

Conversation

malafeev commented Nov 3, 2020

carlosalberto commented Nov 3, 2020

malafeev commented Nov 4, 2020 • edited Loading

trask commented Nov 4, 2020

malafeev commented Nov 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlosalberto Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iNikem Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

iNikem left a comment

Choose a reason for hiding this comment

iNikem Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malafeev commented Nov 10, 2020

malafeev commented Nov 12, 2020

trask left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malafeev commented Nov 13, 2020

trask commented Nov 13, 2020

carlosalberto commented Nov 15, 2020

trask commented Nov 15, 2020

carlosalberto commented Nov 18, 2020

trask commented Nov 19, 2020

malafeev commented Nov 19, 2020

iNikem commented Nov 19, 2020

malafeev commented Nov 19, 2020

iNikem commented Nov 19, 2020

iNikem commented Nov 19, 2020

malafeev commented Nov 19, 2020 • edited Loading

iNikem commented Nov 19, 2020

carlosalberto commented Nov 19, 2020 • edited Loading

trask commented Nov 19, 2020

trask commented Nov 20, 2020

iNikem left a comment

Choose a reason for hiding this comment

iNikem commented Nov 20, 2020

trask commented Nov 20, 2020

malafeev commented Nov 4, 2020 •

edited

Loading

carlosalberto Nov 5, 2020 •

edited

Loading

iNikem Nov 10, 2020 •

edited

Loading

iNikem Nov 10, 2020 •

edited

Loading

malafeev commented Nov 19, 2020 •

edited

Loading

carlosalberto commented Nov 19, 2020 •

edited

Loading