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

Implement OpenTracing Shim specifying custom propagators #3059

Merged
merged 8 commits into from
Mar 31, 2021
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -6,7 +6,6 @@
package io.opentelemetry.opentracingshim;

import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.context.propagation.ContextPropagators;

abstract class BaseShimObject {

Expand All @@ -28,7 +27,7 @@ SpanContextShimTable spanContextTable() {
return telemetryInfo.spanContextTable();
}

ContextPropagators propagators() {
OpenTracingPropagators propagators() {
return telemetryInfo.propagators();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.opentracingshim;

import io.opentelemetry.context.propagation.TextMapPropagator;

/**
* Container for {@link io.opentracing.propagation.Format.Builtin#TEXT_MAP} and {@link
* io.opentracing.propagation.Format.Builtin#HTTP_HEADERS} format propagators.
*/
public class OpenTracingPropagators {
private final TextMapPropagator textMapPropagator;
private final TextMapPropagator httpHeadersPropagator;

OpenTracingPropagators(
TextMapPropagator textMapPropagator, TextMapPropagator httpHeadersPropagator) {
this.textMapPropagator = textMapPropagator;
this.httpHeadersPropagator = httpHeadersPropagator;
}

/**
* Returns a new builder instance for {@link OpenTracingPropagators}.
*
* @return a new builder instance for {@link OpenTracingPropagators}.
*/
public static OpenTracingPropagatorsBuilder builder() {
jkwatson marked this conversation as resolved.
Show resolved Hide resolved
return new OpenTracingPropagatorsBuilder();
}

/**
* Returns the propagator for {@link io.opentracing.propagation.Format.Builtin#TEXT_MAP} format.
*
* @return the propagator for {@link io.opentracing.propagation.Format.Builtin#TEXT_MAP} format.
*/
public TextMapPropagator textMapPropagator() {
return textMapPropagator;
}

/**
* Returns the propagator for {@link io.opentracing.propagation.Format.Builtin#HTTP_HEADERS}
* format.
*
* @return the propagator for {@link io.opentracing.propagation.Format.Builtin#HTTP_HEADERS}
* format.
*/
public TextMapPropagator httpHeadersPropagator() {
return httpHeadersPropagator;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.opentracingshim;

import io.opentelemetry.api.GlobalOpenTelemetry;
import io.opentelemetry.context.propagation.TextMapPropagator;
import java.util.Objects;

/** Builder for {@link OpenTracingPropagators}. */
public class OpenTracingPropagatorsBuilder {
jkwatson marked this conversation as resolved.
Show resolved Hide resolved

private TextMapPropagator textMapPropagator =
GlobalOpenTelemetry.getPropagators().getTextMapPropagator();
private TextMapPropagator httpHeadersPropagator =
GlobalOpenTelemetry.getPropagators().getTextMapPropagator();

/** Set propagator for {@link io.opentracing.propagation.Format.Builtin#TEXT_MAP} format. */
public OpenTracingPropagatorsBuilder setTextMap(TextMapPropagator textMapPropagator) {
Objects.requireNonNull(textMapPropagator, "textMapPropagator");
this.textMapPropagator = textMapPropagator;
return this;
}

/** Set propagator for {@link io.opentracing.propagation.Format.Builtin#HTTP_HEADERS} format. */
public OpenTracingPropagatorsBuilder setHttpHeaders(TextMapPropagator httpHeadersPropagator) {
Objects.requireNonNull(httpHeadersPropagator, "httpHeadersPropagator");
this.httpHeadersPropagator = httpHeadersPropagator;
return this;
}

/**
* Constructs a new instance of the {@link OpenTracingPropagators} based on the builder's values.
* If propagators are not set then {@code GlobalOpenTelemetry.getPropagators()} is used.
*
* @return a new Propagators instance.
*/
public OpenTracingPropagators build() {
return new OpenTracingPropagators(textMapPropagator, httpHeadersPropagator);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,34 @@ public final class OpenTracingShim {
private OpenTracingShim() {}

/**
* Creates a {@code io.opentracing.Tracer} shim out of {@code OpenTelemetry.getTracerProvider()}
* and {@code OpenTelemetry.getPropagators()}.
* Creates a {@code io.opentracing.Tracer} shim out of {@code
* GlobalOpenTelemetry.getTracerProvider()} and {@code GlobalOpenTelemetry.getPropagators()}.
*
* @return a {@code io.opentracing.Tracer}.
*/
public static io.opentracing.Tracer createTracerShim() {
return new TracerShim(
new TelemetryInfo(
getTracer(GlobalOpenTelemetry.getTracerProvider()),
GlobalOpenTelemetry.getPropagators()));
return createTracerShim(getTracer(GlobalOpenTelemetry.getTracerProvider()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any way here to actually use custom propagators. As far as I can see, we only every use the standard stuff in GlobalOpenTelemetry or the OpenTelemetry instance passed in directly. Are we missing a creational method here that will allow passing in custom OpenTracingPropagators somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

}

/**
* Creates a {@code io.opentracing.Tracer} shim using provided Tracer instance and {@code
* GlobalOpenTelemetry.getPropagators()}.
*
* @return a {@code io.opentracing.Tracer}.
*/
public static io.opentracing.Tracer createTracerShim(Tracer tracer) {
return createTracerShim(tracer, OpenTracingPropagators.builder().build());
}

/**
* Creates a {@code io.opentracing.Tracer} shim using provided Tracer instance and {@code
* OpenTracingPropagators} instance.
*
* @return a {@code io.opentracing.Tracer}.
*/
public static io.opentracing.Tracer createTracerShim(
Copy link
Contributor

Choose a reason for hiding this comment

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

3 creational methods make me think we should be using a builder. any thoughts about that? It could definitely be done as a follow-up PR.

Copy link
Contributor Author

@malafeev malafeev Mar 31, 2021

Choose a reason for hiding this comment

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

even 4.
maybe leave only two by removing createTracerShim(OpenTelemetry openTelemetry) and createTracerShim() ?
@carlosalberto ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call. Let's do that in a follow up. @malafeev would you mind creating an issue for that, so we don't forget, before merging this PR?

Copy link
Contributor Author

@malafeev malafeev Mar 31, 2021

Choose a reason for hiding this comment

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

created #3084

Tracer tracer, OpenTracingPropagators propagators) {
return new TracerShim(new TelemetryInfo(tracer, propagators));
}

/**
Expand All @@ -37,9 +55,12 @@ public static io.opentracing.Tracer createTracerShim() {
* @return a {@code io.opentracing.Tracer}.
*/
public static io.opentracing.Tracer createTracerShim(OpenTelemetry openTelemetry) {
return new TracerShim(
new TelemetryInfo(
getTracer(openTelemetry.getTracerProvider()), openTelemetry.getPropagators()));
return createTracerShim(
getTracer(openTelemetry.getTracerProvider()),
OpenTracingPropagators.builder()
.setTextMap(openTelemetry.getPropagators().getTextMapPropagator())
.setHttpHeaders(openTelemetry.getPropagators().getTextMapPropagator())
.build());
}

private static Tracer getTracer(TracerProvider tracerProvider) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import io.opentelemetry.api.baggage.Baggage;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapPropagator;
import io.opentracing.propagation.Format;
import io.opentracing.propagation.TextMapExtract;
import io.opentracing.propagation.TextMapInject;
import java.util.HashMap;
Expand All @@ -22,24 +24,21 @@ final class Propagation extends BaseShimObject {
super(telemetryInfo);
}

void injectTextMap(SpanContextShim contextShim, TextMapInject carrier) {
<C> void injectTextMap(SpanContextShim contextShim, Format<C> format, TextMapInject carrier) {
Context context = Context.current().with(Span.wrap(contextShim.getSpanContext()));
context = context.with(contextShim.getBaggage());

propagators().getTextMapPropagator().inject(context, carrier, SETTER_INSTANCE);
getPropagator(format).inject(context, carrier, SETTER_INSTANCE);
}

@Nullable
SpanContextShim extractTextMap(TextMapExtract carrier) {
<C> SpanContextShim extractTextMap(Format<C> format, TextMapExtract carrier) {
Map<String, String> carrierMap = new HashMap<>();
for (Map.Entry<String, String> entry : carrier) {
carrierMap.put(entry.getKey(), entry.getValue());
}

Context context =
propagators()
.getTextMapPropagator()
.extract(Context.current(), carrierMap, GETTER_INSTANCE);
Context context = getPropagator(format).extract(Context.current(), carrierMap, GETTER_INSTANCE);

Span span = Span.fromContext(context);
if (!span.getSpanContext().isValid()) {
Expand All @@ -49,6 +48,13 @@ SpanContextShim extractTextMap(TextMapExtract carrier) {
return new SpanContextShim(telemetryInfo, span.getSpanContext(), Baggage.fromContext(context));
}

private <C> TextMapPropagator getPropagator(Format<C> format) {
if (format == Format.Builtin.HTTP_HEADERS) {
return propagators().httpHeadersPropagator();
}
return propagators().textMapPropagator();
}

static final class TextMapSetter
implements io.opentelemetry.context.propagation.TextMapSetter<TextMapInject> {
private TextMapSetter() {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import io.opentelemetry.api.baggage.Baggage;
import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.context.propagation.ContextPropagators;

/**
* Utility class that holds a Tracer, a BaggageManager, and related objects that are core part of
Expand All @@ -17,12 +16,12 @@ final class TelemetryInfo {

private final Tracer tracer;
private final Baggage emptyBaggage;
private final ContextPropagators propagators;
private final OpenTracingPropagators openTracingPropagators;
private final SpanContextShimTable spanContextTable;

TelemetryInfo(Tracer tracer, ContextPropagators propagators) {
TelemetryInfo(Tracer tracer, OpenTracingPropagators openTracingPropagators) {
this.tracer = tracer;
this.propagators = propagators;
this.openTracingPropagators = openTracingPropagators;
this.emptyBaggage = Baggage.empty();
this.spanContextTable = new SpanContextShimTable();
}
Expand All @@ -39,7 +38,7 @@ Baggage emptyBaggage() {
return emptyBaggage;
}

ContextPropagators propagators() {
return propagators;
OpenTracingPropagators propagators() {
return openTracingPropagators;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public <C> void inject(SpanContext context, Format<C> format, C carrier) {
if (format == Format.Builtin.TEXT_MAP
|| format == Format.Builtin.TEXT_MAP_INJECT
|| format == Format.Builtin.HTTP_HEADERS) {
propagation.injectTextMap(contextShim, (TextMapInject) carrier);
propagation.injectTextMap(contextShim, format, (TextMapInject) carrier);
}
}

Expand All @@ -76,7 +76,7 @@ public <C> SpanContext extract(Format<C> format, C carrier) {
if (format == Format.Builtin.TEXT_MAP
|| format == Format.Builtin.TEXT_MAP_EXTRACT
|| format == Format.Builtin.HTTP_HEADERS) {
return propagation.extractTextMap((TextMapExtract) carrier);
return propagation.extractTextMap(format, (TextMapExtract) carrier);
}
} catch (RuntimeException e) {
logger.log(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.opentracingshim;

import io.opentelemetry.context.Context;
import io.opentelemetry.context.propagation.TextMapGetter;
import io.opentelemetry.context.propagation.TextMapPropagator;
import io.opentelemetry.context.propagation.TextMapSetter;
import java.util.Collection;
import java.util.Collections;
import javax.annotation.Nullable;

class CustomTextMapPropagator implements TextMapPropagator {
private boolean extracted;
private boolean injected;

@Override
public Collection<String> fields() {
return Collections.emptyList();
}

@Override
public <C> void inject(Context context, @Nullable C carrier, TextMapSetter<C> setter) {
injected = true;
}

@Override
public <C> Context extract(Context context, @Nullable C carrier, TextMapGetter<C> getter) {
extracted = true;
return context;
}

public boolean isExtracted() {
return extracted;
}

public boolean isInjected() {
return injected;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@

import io.opentelemetry.api.GlobalOpenTelemetry;
import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.context.propagation.ContextPropagators;
import io.opentelemetry.context.propagation.TextMapPropagator;
import io.opentelemetry.sdk.trace.SdkTracerProvider;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
Expand All @@ -34,9 +36,31 @@ void createTracerShim_fromOpenTelemetryInstance() {
OpenTelemetry openTelemetry = mock(OpenTelemetry.class);
SdkTracerProvider sdk = SdkTracerProvider.builder().build();
when(openTelemetry.getTracerProvider()).thenReturn(sdk);
when(openTelemetry.getPropagators()).thenReturn(mock(ContextPropagators.class));
ContextPropagators contextPropagators = mock(ContextPropagators.class);
when(contextPropagators.getTextMapPropagator()).thenReturn(mock(TextMapPropagator.class));
when(openTelemetry.getPropagators()).thenReturn(contextPropagators);

TracerShim tracerShim = (TracerShim) OpenTracingShim.createTracerShim(openTelemetry);
assertThat(tracerShim.tracer()).isEqualTo(sdk.get("opentracingshim"));
}

@Test
void createTracerShim_withPropagators() {
Tracer tracer = mock(Tracer.class);

TextMapPropagator textMapPropagator = new CustomTextMapPropagator();
TextMapPropagator httpHeadersPropagator = new CustomTextMapPropagator();

TracerShim tracerShim =
(TracerShim)
OpenTracingShim.createTracerShim(
tracer,
OpenTracingPropagators.builder()
.setTextMap(textMapPropagator)
.setHttpHeaders(httpHeadersPropagator)
.build());

assertThat(tracerShim.propagators().textMapPropagator()).isSameAs(textMapPropagator);
assertThat(tracerShim.propagators().httpHeadersPropagator()).isSameAs(httpHeadersPropagator);
}
}
Loading