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

Hide default context propagators implementation behind interface. #2089

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import io.opentelemetry.api.metrics.MeterProvider;
import io.opentelemetry.api.trace.TracerProvider;
import io.opentelemetry.context.propagation.ContextPropagators;
import io.opentelemetry.context.propagation.DefaultContextPropagators;
import io.opentelemetry.spi.OpenTelemetryFactory;
import io.opentelemetry.spi.metrics.MeterProviderFactory;
import io.opentelemetry.spi.trace.TracerProviderFactory;
Expand Down Expand Up @@ -118,7 +117,7 @@ public Builder toBuilder() {
}

protected static class Builder implements OpenTelemetryBuilder<Builder> {
protected ContextPropagators propagators = DefaultContextPropagators.builder().build();
protected ContextPropagators propagators = ContextPropagators.builder().build();

protected TracerProvider tracerProvider;
protected MeterProvider meterProvider;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.api.trace.TracerProvider;
import io.opentelemetry.context.propagation.ContextPropagators;
import io.opentelemetry.context.propagation.DefaultContextPropagators;
import io.opentelemetry.spi.metrics.MeterProviderFactory;
import io.opentelemetry.spi.trace.TracerProviderFactory;
import java.io.File;
Expand Down Expand Up @@ -64,7 +63,6 @@ void testDefault() {
.isEqualTo("DefaultMeterProvider");
assertThat(OpenTelemetry.getGlobalMeterProvider())
.isSameAs(OpenTelemetry.getGlobalMeterProvider());
assertThat(OpenTelemetry.getGlobalPropagators()).isInstanceOf(DefaultContextPropagators.class);
assertThat(OpenTelemetry.getGlobalPropagators()).isSameAs(OpenTelemetry.getGlobalPropagators());
}

Expand Down Expand Up @@ -156,7 +154,7 @@ void testMeterNotFound() {

@Test
void testPropagatorsSet() {
ContextPropagators propagators = DefaultContextPropagators.builder().build();
ContextPropagators propagators = ContextPropagators.builder().build();
OpenTelemetry.setGlobalPropagators(propagators);
assertThat(OpenTelemetry.getGlobalPropagators()).isEqualTo(propagators);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,28 @@
@ThreadSafe
public interface ContextPropagators {

/**
* Returns a {@link ContextPropagatorsBuilder} used to construct a new {@code ContextPropagators}
* object with the specified propagators.
*
* <p>Invocation order of {@code TextMapPropagator#inject()} and {@code
* TextMapPropagator#extract()} for registered trace propagators is undefined.
*
* <p>This is a example of a {@code ContextPropagators} object being created:
*
* <pre>{@code
* ContextPropagators propagators = DefaultContextPropagators.builder()
* .addTextMapPropagator(new HttpTraceContext())
* .addTextMapPropagator(new HttpBaggage())
* .addTextMapPropagator(new MyCustomContextPropagator())
* .build();
* }</pre>
*/
@SuppressWarnings("deprecation")
static ContextPropagatorsBuilder builder() {
return DefaultContextPropagators.builder();
}

/**
* Returns a {@link TextMapPropagator} propagator.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.context.propagation;

/**
* A builder of a default {@link ContextPropagators} which can have multiple {@link
* TextMapPropagator} added, and extraction and injection will execute every {@link
* TextMapPropagator} in order.
*/
public interface ContextPropagatorsBuilder {
Copy link
Contributor

Choose a reason for hiding this comment

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

Having an interface for a builder, which is just a list of propagators and a constructor call feels like overkill to me. Is the rationale here to make the agent more easily able to bridge the builder?

Could we ditch the builder entirely and use a pattern like in #2091 where we just have a static method on the ContextPropagators that takes the list of TextMapPropagators?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup - it came to mind after sending the PR but before also having the idea of killing ContextPropagators completely :) Will do so since now I see the binary propagation idea is probably the main reason for this interface.

/**
* Adds a {@link TextMapPropagator} propagator.
*
* <p>One propagator per concern (traces, correlations, etc) should be added if this format is
* supported.
*
* @param textMapPropagator the propagator to be added.
* @return this.
* @throws NullPointerException if {@code textMapPropagator} is {@code null}.
*/
ContextPropagatorsBuilder addTextMapPropagator(TextMapPropagator textMapPropagator);

/**
* Builds a new {@code ContextPropagators} with the specified propagators.
*
* @return the newly created {@code ContextPropagators} instance.
*/
ContextPropagators build();
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@
* synchronically upon injection and extraction.
*
* <p>The propagation fields retrieved from all registered propagators are de-duplicated.
*
* @deprecated Use {@link ContextPropagators#builder()}
*/
@Deprecated
public final class DefaultContextPropagators implements ContextPropagators {
private final TextMapPropagator textMapPropagator;

Expand All @@ -35,8 +38,10 @@ public TextMapPropagator getTextMapPropagator() {
* object.
*
* @return a {@link DefaultContextPropagators.Builder}.
* @deprecated Use {@link ContextPropagators#builder()}
*/
public static Builder builder() {
@Deprecated
public static ContextPropagatorsBuilder builder() {
return new Builder();
}

Expand All @@ -45,35 +50,15 @@ private DefaultContextPropagators(TextMapPropagator textMapPropagator) {
}

/**
* {@link Builder} is used to construct a new {@code ContextPropagators} object with the specified
* propagators.
*
* <p>Invocation order of {@code TextMapPropagator#inject()} and {@code
* TextMapPropagator#extract()} for registered trace propagators is undefined.
*
* <p>This is a example of a {@code ContextPropagators} object being created:
* A builder of {@link DefaultContextPropagators}.
*
* <pre>{@code
* ContextPropagators propagators = DefaultContextPropagators.builder()
* .addTextMapPropagator(new HttpTraceContext())
* .addTextMapPropagator(new HttpBaggage())
* .addTextMapPropagator(new MyCustomContextPropagator())
* .build();
* }</pre>
* @deprecated Use {@link ContextPropagators#builder()}
*/
public static final class Builder {
@Deprecated
public static final class Builder implements ContextPropagatorsBuilder {
List<TextMapPropagator> textPropagators = new ArrayList<>();

/**
* Adds a {@link TextMapPropagator} propagator.
*
* <p>One propagator per concern (traces, correlations, etc) should be added if this format is
* supported.
*
* @param textMapPropagator the propagator to be added.
* @return this.
* @throws NullPointerException if {@code textMapPropagator} is {@code null}.
*/
@Override
public Builder addTextMapPropagator(TextMapPropagator textMapPropagator) {
if (textMapPropagator == null) {
throw new NullPointerException("textMapPropagator");
Expand All @@ -83,11 +68,7 @@ public Builder addTextMapPropagator(TextMapPropagator textMapPropagator) {
return this;
}

/**
* Builds a new {@code ContextPropagators} with the specified propagators.
*
* @return the newly created {@code ContextPropagators} instance.
*/
@Override
public ContextPropagators build() {
if (textPropagators.isEmpty()) {
return new DefaultContextPropagators(NoopTextMapPropagator.INSTANCE);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,15 @@ class DefaultPropagatorsTest {
@Test
void addTextMapPropagatorNull() {
assertThrows(
NullPointerException.class,
() -> DefaultContextPropagators.builder().addTextMapPropagator(null));
NullPointerException.class, () -> ContextPropagators.builder().addTextMapPropagator(null));
}

@Test
void testInject() {
CustomTextMapPropagator propagator1 = new CustomTextMapPropagator("prop1");
CustomTextMapPropagator propagator2 = new CustomTextMapPropagator("prop2");
ContextPropagators propagators =
DefaultContextPropagators.builder()
ContextPropagators.builder()
.addTextMapPropagator(propagator1)
.addTextMapPropagator(propagator2)
.build();
Expand All @@ -52,7 +51,7 @@ void testExtract() {
CustomTextMapPropagator propagator2 = new CustomTextMapPropagator("prop2");
CustomTextMapPropagator propagator3 = new CustomTextMapPropagator("prop3");
ContextPropagators propagators =
DefaultContextPropagators.builder()
ContextPropagators.builder()
.addTextMapPropagator(propagator1)
.addTextMapPropagator(propagator2)
.build();
Expand All @@ -76,7 +75,7 @@ public void testDuplicatedFields() {
CustomTextMapPropagator propagator3 = new CustomTextMapPropagator("prop1");
CustomTextMapPropagator propagator4 = new CustomTextMapPropagator("prop2");
ContextPropagators propagators =
DefaultContextPropagators.builder()
ContextPropagators.builder()
.addTextMapPropagator(propagator1)
.addTextMapPropagator(propagator2)
.addTextMapPropagator(propagator3)
Expand All @@ -89,7 +88,7 @@ public void testDuplicatedFields() {

@Test
void noopPropagator() {
ContextPropagators propagators = DefaultContextPropagators.builder().build();
ContextPropagators propagators = ContextPropagators.builder().build();

Context context = Context.current();
Map<String, String> map = new HashMap<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.propagation.HttpTraceContext;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.DefaultContextPropagators;
import io.opentelemetry.context.propagation.ContextPropagators;
import io.opentelemetry.context.propagation.TextMapPropagator.Getter;
import io.opentelemetry.context.propagation.TextMapPropagator.Setter;
import java.util.ArrayList;
Expand All @@ -36,7 +36,7 @@ public class Application {
openTelemetry =
OpenTelemetry.get().toBuilder()
.setPropagators(
DefaultContextPropagators.builder()
ContextPropagators.builder()
.addTextMapPropagator(HttpTraceContext.getInstance())
.build())
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.api.trace.propagation.HttpTraceContext;
import io.opentelemetry.context.propagation.DefaultContextPropagators;
import io.opentelemetry.context.propagation.ContextPropagators;
import io.opentelemetry.sdk.OpenTelemetrySdk;
import io.opentelemetry.sdk.testing.exporter.InMemorySpanExporter;
import io.opentelemetry.sdk.trace.TracerSdkManagement;
Expand Down Expand Up @@ -56,7 +56,7 @@ public static OpenTelemetryRule create() {
OpenTelemetrySdk openTelemetry =
OpenTelemetrySdk.builder()
.setPropagators(
DefaultContextPropagators.builder()
ContextPropagators.builder()
.addTextMapPropagator(HttpTraceContext.getInstance())
.build())
.setTracerProvider(tracerProvider)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.api.trace.propagation.HttpTraceContext;
import io.opentelemetry.context.propagation.DefaultContextPropagators;
import io.opentelemetry.context.propagation.ContextPropagators;
import io.opentelemetry.sdk.OpenTelemetrySdk;
import io.opentelemetry.sdk.testing.assertj.TracesAssert;
import io.opentelemetry.sdk.testing.exporter.InMemorySpanExporter;
Expand Down Expand Up @@ -62,7 +62,7 @@ public static OpenTelemetryExtension create() {
OpenTelemetrySdk openTelemetry =
OpenTelemetrySdk.builder()
.setPropagators(
DefaultContextPropagators.builder()
ContextPropagators.builder()
.addTextMapPropagator(HttpTraceContext.getInstance())
.build())
.setTracerProvider(tracerProvider)
Expand Down