Now `MultiTextMapPropagator` allows to select extractors and injectors separately #3366

slinkydeveloper · 2021-07-02T09:37:29Z

Fix #3364

This PR includes:

A patch for MultiTextMapPropagator to select extractors and injectors separately, also rename the class to CompositeTextMapPropagator
The builder CompositeTextMapPropagatorBuilder
Unit test

Signed-off-by: Francesco Guardiani [email protected]

Signed-off-by: Francesco Guardiani <[email protected]>

linux-foundation-easycla · 2021-07-02T09:37:33Z

The committers are authorized under a signed CLA.

✅ Francesco Guardiani (4267202)

jkwatson · 2021-07-02T16:26:54Z

@slinkydeveloper this proposed change includes additions to stable public APIs. Can you run ./gradlew build and include the changes in doc/apidiffs in this PR so we can examine and document what public API changes you are proposing? Thanks!

...xt/src/main/java/io/opentelemetry/context/propagation/CompositeTextMapPropagatorBuilder.java

context/src/main/java/io/opentelemetry/context/propagation/MultiTextMapPropagator.java

Signed-off-by: Francesco Guardiani <[email protected]>

slinkydeveloper · 2021-07-05T08:18:37Z

Pushed the doc changes as well

anuraaga

Thanks!

jkwatson · 2021-07-08T15:37:26Z

context/src/main/java/io/opentelemetry/context/propagation/CompositeTextMapPropagator.java

+    this.injectors = new TextMapPropagator[injectors.size()];
+    injectors.toArray(this.injectors);
+
+    this.allFields = Collections.unmodifiableList(getAllFields(this.injectors));


is this right? The fields are only from the injectors? I think this might need to be the union of the extractors and injectors, rather than just the injectors.

That's not what I understood from the fields() javadoc, it rather seems to me that method is used only when writing the context back to the wire, and not while extracting it...

I think the javadoc provides that as a usage (to clear out headers if the header implementation is re-used), but I think fields() could also be used by extraction in some cases. I'm honestly not 100% sure what the behavior should be in this case when injection doesn't necessarily match extraction.

I created a spec issue for this, since I don't think I know what the answer should be: open-telemetry/opentelemetry-specification#1809

I second @jkwatson with this - I know at least one case in instrumentation where fields are considered during extraction (see io.opentelemetry.instrumentation.awslambda.v1_0.TracingRequestStreamHandler)

Please comment on the spec issue. :)

Weighting issues - sub-optimal performance vs actual bug I'd say that keeping all fields still wins. But yeah, spec issue is the proper place to discuss this.

@kubawach please provide a permalink to your example of TracingRequestStreamHandler, I couldn't find it.

@yurishkuro here you go: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/aws-lambda-1.0/library/src/main/java/io/opentelemetry/instrumentation/awslambda/v1_0/ApiGatewayProxyRequest.java

@kubawach thanks. If I am reading this correctly, I wouldn't classify this usage as "on the extract path", and I probably would've implemented that check differently in the first place, the use of fields() to determine "noHttpPropagationNeeded" seems a bit round-about. Here's the exact usage link, for reference:

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/1b5df6d78a6877dab38f573b44d0358927d1ab3d/instrumentation/aws-lambda-1.0/library/src/main/java/io/opentelemetry/instrumentation/awslambda/v1_0/ApiGatewayProxyRequest.java#L27

codecov · 2021-07-09T01:11:22Z

Codecov Report

Merging #3366 (27b4eda) into main (fffc831) will increase coverage by 90.96%.
The diff coverage is 96.96%.

@@             Coverage Diff             @@
##             main    #3366       +/-   ##
===========================================
+ Coverage        0   90.96%   +90.96%     
- Complexity      0     3172     +3172     
===========================================
  Files           0      364      +364     
  Lines           0     9794     +9794     
  Branches        0      983      +983     
===========================================
+ Hits            0     8909     +8909     
- Misses          0      585      +585     
- Partials        0      300      +300

Impacted Files	Coverage Δ
...propagation/CompositeTextMapPropagatorBuilder.java	`94.11% <94.11%> (ø)`
...ontext/propagation/CompositeTextMapPropagator.java	`100.00% <100.00%> (ø)`
...lemetry/context/propagation/TextMapPropagator.java	`100.00% <100.00%> (ø)`
...dk/metrics/export/IntervalMetricReaderBuilder.java	`92.85% <0.00%> (ø)`
.../opentelemetry/api/baggage/propagation/Parser.java	`98.18% <0.00%> (ø)`
...opentelemetry/sdk/metrics/data/MetricDataType.java	`100.00% <0.00%> (ø)`
...y/sdk/trace/samplers/TraceIdRatioBasedSampler.java	`85.71% <0.00%> (ø)`
...a/io/opentelemetry/api/baggage/ImmutableEntry.java	`100.00% <0.00%> (ø)`
...i/trace/propagation/W3CTraceContextPropagator.java	`98.24% <0.00%> (ø)`
...a/io/opentelemetry/sdk/trace/SdkTracerBuilder.java	`100.00% <0.00%> (ø)`
... and 357 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fffc831...27b4eda. Read the comment docs.

ghost

Definition of fields with separate injectors / extractors needs clarification before merging.

github-actions · 2022-06-22T20:30:42Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2022-07-06T20:30:57Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Fix open-telemetry#3364

4267202

Signed-off-by: Francesco Guardiani <[email protected]>

slinkydeveloper requested review from anuraaga, arminru, bogdandrutu, carlosalberto and jkwatson as code owners July 2, 2021 09:37

slinkydeveloper requested a review from a user July 2, 2021 09:37

slinkydeveloper requested review from Oberon00, pavolloffay, thisthat and tylerbenson as code owners July 2, 2021 09:37

anuraaga reviewed Jul 5, 2021

View reviewed changes

slinkydeveloper added 4 commits July 5, 2021 09:53

Fix checkstyle lint

3e706c5

Signed-off-by: Francesco Guardiani <[email protected]>

Resolve PR comments

5b15144

Signed-off-by: Francesco Guardiani <[email protected]>

More suggestions

fe81221

Signed-off-by: Francesco Guardiani <[email protected]>

Add method change doc

27b4eda

Signed-off-by: Francesco Guardiani <[email protected]>

slinkydeveloper requested a review from anuraaga July 5, 2021 08:18

anuraaga approved these changes Jul 5, 2021

View reviewed changes

jkwatson reviewed Jul 8, 2021

View reviewed changes

jkwatson mentioned this pull request Jul 8, 2021

It is unclear what the behavior of the fields() method/function should be on a composite TextMapPropagator open-telemetry/opentelemetry-specification#1809

Open

ghost approved these changes Jul 9, 2021

View reviewed changes

jkwatson added the blocked:spec blocked on open or unresolved spec label Jul 9, 2021

jkwatson removed the request for review from tylerbenson April 8, 2022 17:26

github-actions bot added the Stale label Jun 22, 2022

github-actions bot closed this Jul 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Now `MultiTextMapPropagator` allows to select extractors and injectors separately #3366

Now `MultiTextMapPropagator` allows to select extractors and injectors separately #3366

slinkydeveloper commented Jul 2, 2021 •

edited

Loading

linux-foundation-easycla bot commented Jul 2, 2021 •

edited

Loading

jkwatson commented Jul 2, 2021

slinkydeveloper commented Jul 5, 2021

anuraaga left a comment

jkwatson Jul 8, 2021

slinkydeveloper Jul 8, 2021

jkwatson Jul 8, 2021

jkwatson Jul 8, 2021

ghost Jul 9, 2021

jkwatson Jul 9, 2021

ghost Jul 12, 2021

yurishkuro Jul 23, 2021

ghost Jul 23, 2021

yurishkuro Jul 23, 2021

codecov bot commented Jul 9, 2021

ghost left a comment

github-actions bot commented Jun 22, 2022

github-actions bot commented Jul 6, 2022

Now MultiTextMapPropagator allows to select extractors and injectors separately #3366

Now MultiTextMapPropagator allows to select extractors and injectors separately #3366

Conversation

slinkydeveloper commented Jul 2, 2021 • edited Loading

linux-foundation-easycla bot commented Jul 2, 2021 • edited Loading

jkwatson commented Jul 2, 2021

slinkydeveloper commented Jul 5, 2021

anuraaga 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

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

codecov bot commented Jul 9, 2021

Codecov Report

ghost left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 22, 2022

github-actions bot commented Jul 6, 2022

Now `MultiTextMapPropagator` allows to select extractors and injectors separately #3366

Now `MultiTextMapPropagator` allows to select extractors and injectors separately #3366

slinkydeveloper commented Jul 2, 2021 •

edited

Loading

linux-foundation-easycla bot commented Jul 2, 2021 •

edited

Loading