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.

Sign up for GitHub

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

Now MultiTextMapPropagator allows to select extractors and injectors separately #3366

Closed
wants to merge 5 commits into from
Closed

Now MultiTextMapPropagator allows to select extractors and injectors separately #3366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4267202
Fix #3364
slinkydeveloper Jul 2, 2021
3e706c5
Fix checkstyle lint
slinkydeveloper Jul 5, 2021
5b15144
Resolve PR comments
slinkydeveloper Jul 5, 2021
fe81221
More suggestions
slinkydeveloper Jul 5, 2021
27b4eda
Add method change doc
slinkydeveloper Jul 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions ...t/propagation/MultiTextMapPropagator.java → ...opagation/CompositeTextMapPropagator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,31 @@
import java.util.Set;
import javax.annotation.Nullable;

final class MultiTextMapPropagator implements TextMapPropagator {
private final TextMapPropagator[] textPropagators;
final class CompositeTextMapPropagator implements TextMapPropagator {
private final TextMapPropagator[] extractors;
private final TextMapPropagator[] injectors;
private final Collection<String> allFields;

MultiTextMapPropagator(TextMapPropagator... textPropagators) {
CompositeTextMapPropagator(TextMapPropagator... textPropagators) {
this(Arrays.asList(textPropagators));
}

MultiTextMapPropagator(List<TextMapPropagator> textPropagators) {
this.textPropagators = new TextMapPropagator[textPropagators.size()];
textPropagators.toArray(this.textPropagators);
this.allFields = Collections.unmodifiableList(getAllFields(this.textPropagators));
CompositeTextMapPropagator(List<TextMapPropagator> textPropagators) {
this.extractors = new TextMapPropagator[textPropagators.size()];
textPropagators.toArray(this.extractors);
this.injectors = this.extractors; // No need to copy
this.allFields = Collections.unmodifiableList(getAllFields(this.injectors));
}

CompositeTextMapPropagator(
List<TextMapPropagator> extractors, List<TextMapPropagator> injectors) {
this.extractors = new TextMapPropagator[extractors.size()];
extractors.toArray(this.extractors);

this.injectors = new TextMapPropagator[injectors.size()];
injectors.toArray(this.injectors);

this.allFields = Collections.unmodifiableList(getAllFields(this.injectors));
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Please comment on the spec issue. :)

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

@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

}

@Override
Expand All @@ -48,7 +61,7 @@ public <C> void inject(Context context, @Nullable C carrier, TextMapSetter<C> se
if (context == null || setter == null) {
return;
}
for (TextMapPropagator textPropagator : textPropagators) {
for (TextMapPropagator textPropagator : injectors) {
textPropagator.inject(context, carrier, setter);
}
}
Expand All @@ -61,7 +74,7 @@ public <C> Context extract(Context context, @Nullable C carrier, TextMapGetter<C
if (getter == null) {
return context;
}
for (TextMapPropagator textPropagator : textPropagators) {
for (TextMapPropagator textPropagator : extractors) {
context = textPropagator.extract(context, carrier, getter);
}
return context;
Expand Down
65 changes: 65 additions & 0 deletions ...src/main/java/io/opentelemetry/context/propagation/CompositeTextMapPropagatorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.context.propagation;

import java.util.ArrayList;
import java.util.List;
import java.util.Objects;

/**
* A builder for configuring a {@link TextMapPropagator} specifying which propagators should be used
* for extracting the context and which should be used for injecting the context.
*/
public final class CompositeTextMapPropagatorBuilder {

private final List<TextMapPropagator> extractors;
private final List<TextMapPropagator> injectors;

/**
* Package protected to disallow direct initialization.
*
* @see TextMapPropagator#builder()
*/
CompositeTextMapPropagatorBuilder() {
this.extractors = new ArrayList<>();
this.injectors = new ArrayList<>();
}

/** Adds a {@link TextMapPropagator} to be used only when extracting context. */
public CompositeTextMapPropagatorBuilder addExtractor(TextMapPropagator propagator) {
Objects.requireNonNull(propagator, "propagator");
this.extractors.add(propagator);
slinkydeveloper marked this conversation as resolved.
Show resolved Hide resolved
return this;
}

/** Adds a {@link TextMapPropagator} to be used only when injecting context. */
public CompositeTextMapPropagatorBuilder addInjector(TextMapPropagator propagator) {
Objects.requireNonNull(propagator, "propagator");
this.injectors.add(propagator);
return this;
}

/** Adds a {@link TextMapPropagator} to be used both when extracting and injecting context. */
public CompositeTextMapPropagatorBuilder addPropagator(TextMapPropagator propagator) {
Objects.requireNonNull(propagator, "propagator");
this.injectors.add(propagator);
this.extractors.add(propagator);
return this;
}

/**
* Returns the built {@link TextMapPropagator}.
*
* @see CompositeTextMapPropagatorBuilder
*/
public TextMapPropagator build() {
if (this.injectors.isEmpty() && this.extractors.isEmpty()) {
return NoopTextMapPropagator.getInstance();
}

return new CompositeTextMapPropagator(this.extractors, this.injectors);
}
}
10 changes: 9 additions & 1 deletion context/src/main/java/io/opentelemetry/context/propagation/TextMapPropagator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,14 @@
@ThreadSafe
public interface TextMapPropagator {

/**
* Returns a composite text map propagator builder that allows to specify specific propagators as
* extractors and specific propagators as injectors.
*/
static CompositeTextMapPropagatorBuilder builder() {
return new CompositeTextMapPropagatorBuilder();
}

/**
* Returns a {@link TextMapPropagator} which simply delegates injection and extraction to the
* provided propagators.
Expand Down Expand Up @@ -72,7 +80,7 @@ static TextMapPropagator composite(Iterable<TextMapPropagator> propagators) {
if (propagatorsList.size() == 1) {
return propagatorsList.get(0);
}
return new MultiTextMapPropagator(propagatorsList);
return new CompositeTextMapPropagator(propagatorsList);
}

/** Returns a {@link TextMapPropagator} which does no injection or extraction. */
Expand Down
78 changes: 64 additions & 14 deletions ...opagation/MultiTextMapPropagatorTest.java → ...ation/CompositeTextMapPropagatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

Expand All @@ -27,7 +28,7 @@
import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
class MultiTextMapPropagatorTest {
class CompositeTextMapPropagatorTest {

private static final ContextKey<String> KEY = ContextKey.named("key");

Expand All @@ -50,17 +51,17 @@ public String get(Map<String, String> carrier, String key) {
};

@Test
void addPropagator_null() {
void new_composite_with_null_list() {
assertThrows(
NullPointerException.class,
() -> new MultiTextMapPropagator((List<TextMapPropagator>) null));
() -> new CompositeTextMapPropagator((List<TextMapPropagator>) null));
}

@Test
void fields() {
when(propagator1.fields()).thenReturn(Arrays.asList("foo", "bar"));
when(propagator2.fields()).thenReturn(Arrays.asList("hello", "world"));
TextMapPropagator prop = new MultiTextMapPropagator(propagator1, propagator2);
TextMapPropagator prop = new CompositeTextMapPropagator(propagator1, propagator2);

Collection<String> fields = prop.fields();
assertThat(fields).containsExactly("foo", "bar", "hello", "world");
Expand All @@ -70,7 +71,7 @@ void fields() {
void fields_duplicates() {
when(propagator1.fields()).thenReturn(Arrays.asList("foo", "bar", "foo"));
when(propagator2.fields()).thenReturn(Arrays.asList("hello", "world", "world", "bar"));
TextMapPropagator prop = new MultiTextMapPropagator(propagator1, propagator2);
TextMapPropagator prop = new CompositeTextMapPropagator(propagator1, propagator2);

Collection<String> fields = prop.fields();
assertThat(fields).containsExactly("foo", "bar", "hello", "world");
Expand All @@ -80,7 +81,7 @@ void fields_duplicates() {
void fields_readOnly() {
when(propagator1.fields()).thenReturn(Arrays.asList("rubber", "baby"));
when(propagator2.fields()).thenReturn(Arrays.asList("buggy", "bumpers"));
TextMapPropagator prop = new MultiTextMapPropagator(propagator1, propagator2);
TextMapPropagator prop = new CompositeTextMapPropagator(propagator1, propagator2);
Collection<String> fields = prop.fields();
assertThrows(UnsupportedOperationException.class, () -> fields.add("hi"));
}
Expand All @@ -91,7 +92,7 @@ void inject_allDelegated() {
Context context = mock(Context.class);
TextMapSetter<Map<String, String>> setter = Map::put;

TextMapPropagator prop = new MultiTextMapPropagator(propagator1, propagator2, propagator3);
TextMapPropagator prop = new CompositeTextMapPropagator(propagator1, propagator2, propagator3);
prop.inject(context, carrier, setter);
verify(propagator1).inject(context, carrier, setter);
verify(propagator2).inject(context, carrier, setter);
Expand All @@ -103,15 +104,15 @@ void extract_noPropagators() {
Map<String, String> carrier = new HashMap<>();
Context context = mock(Context.class);

TextMapPropagator prop = new MultiTextMapPropagator();
TextMapPropagator prop = new CompositeTextMapPropagator();
Context resContext = prop.extract(context, carrier, getter);
assertThat(context).isSameAs(resContext);
}

@Test
void extract_found_all() {
Map<String, String> carrier = new HashMap<>();
TextMapPropagator prop = new MultiTextMapPropagator(propagator1, propagator2, propagator3);
TextMapPropagator prop = new CompositeTextMapPropagator(propagator1, propagator2, propagator3);
Context context1 = mock(Context.class);
Context context2 = mock(Context.class);
Context context3 = mock(Context.class);
Expand All @@ -131,7 +132,7 @@ void extract_notFound() {
when(propagator1.extract(context, carrier, getter)).thenReturn(context);
when(propagator2.extract(context, carrier, getter)).thenReturn(context);

TextMapPropagator prop = new MultiTextMapPropagator(propagator1, propagator2);
TextMapPropagator prop = new CompositeTextMapPropagator(propagator1, propagator2);
Context result = prop.extract(context, carrier, getter);

assertThat(result).isSameAs(context);
Expand All @@ -140,7 +141,7 @@ void extract_notFound() {
@Test
void extract_nullContext() {
assertThat(
new MultiTextMapPropagator(propagator1, propagator2)
new CompositeTextMapPropagator(propagator1, propagator2)
.extract(null, Collections.emptyMap(), getter))
.isSameAs(Context.root());
}
Expand All @@ -149,23 +150,72 @@ void extract_nullContext() {
void extract_nullGetter() {
Context context = Context.current().with(KEY, "treasure");
assertThat(
new MultiTextMapPropagator(propagator1, propagator2)
new CompositeTextMapPropagator(propagator1, propagator2)
.extract(context, Collections.emptyMap(), null))
.isSameAs(context);
}

@Test
void inject_nullContext() {
Map<String, String> carrier = new LinkedHashMap<>();
new MultiTextMapPropagator(propagator1, propagator2).inject(null, carrier, Map::put);
new CompositeTextMapPropagator(propagator1, propagator2).inject(null, carrier, Map::put);
assertThat(carrier).isEmpty();
}

@Test
void inject_nullSetter() {
Map<String, String> carrier = new LinkedHashMap<>();
Context context = Context.current().with(KEY, "treasure");
new MultiTextMapPropagator(propagator1, propagator2).inject(context, carrier, null);
new CompositeTextMapPropagator(propagator1, propagator2).inject(context, carrier, null);
assertThat(carrier).isEmpty();
}

@Test
void different_inject_and_extract() {
Map<String, String> carrier = new HashMap<>();

TextMapPropagator prop =
TextMapPropagator.builder()
.addInjector(propagator1)
.addExtractor(propagator2)
.addPropagator(propagator3)
.build();

Context context1 = mock(Context.class);
Context context2 = mock(Context.class);
Context expectedContext = mock(Context.class);

when(propagator2.extract(context1, carrier, getter)).thenReturn(context2);
when(propagator3.extract(context2, carrier, getter)).thenReturn(expectedContext);

assertThat(prop.extract(context1, carrier, getter)).isEqualTo(expectedContext);

Context context = mock(Context.class);
TextMapSetter<Map<String, String>> setter = Map::put;

prop.inject(context, carrier, setter);
verify(propagator1).inject(context, carrier, setter);
verify(propagator2, never()).inject(context, carrier, setter);
verify(propagator3).inject(context, carrier, setter);
}

@Test
void addPropagator_null() {
assertThrows(NullPointerException.class, () -> TextMapPropagator.builder().addPropagator(null));
}

@Test
void addExtractor_null() {
assertThrows(NullPointerException.class, () -> TextMapPropagator.builder().addExtractor(null));
}

@Test
void addInjector_null() {
assertThrows(NullPointerException.class, () -> TextMapPropagator.builder().addInjector(null));
}

@Test
void empty_builder_returns_noop() {
assertThat(TextMapPropagator.builder().build()).isSameAs(NoopTextMapPropagator.getInstance());
}
}
11 changes: 10 additions & 1 deletion docs/apidiffs/current_vs_latest/opentelemetry-context.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,11 @@
Comparing source compatibility of against
No changes.
+++ NEW CLASS: PUBLIC(+) FINAL(+) io.opentelemetry.context.propagation.CompositeTextMapPropagatorBuilder (not serializable)
+++ CLASS FILE FORMAT VERSION: 52.0 <- n.a.
+++ NEW SUPERCLASS: java.lang.Object
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.context.propagation.CompositeTextMapPropagatorBuilder addExtractor(io.opentelemetry.context.propagation.TextMapPropagator)
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.context.propagation.CompositeTextMapPropagatorBuilder addInjector(io.opentelemetry.context.propagation.TextMapPropagator)
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.context.propagation.CompositeTextMapPropagatorBuilder addPropagator(io.opentelemetry.context.propagation.TextMapPropagator)
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.context.propagation.TextMapPropagator build()
***! MODIFIED INTERFACE: PUBLIC ABSTRACT io.opentelemetry.context.propagation.TextMapPropagator (not serializable)
=== CLASS FILE FORMAT VERSION: 52.0 <- 52.0
+++! NEW METHOD: PUBLIC(+) STATIC(+) io.opentelemetry.context.propagation.CompositeTextMapPropagatorBuilder builder()