From f797537cc63b2b5f41f99c8581e1ad4aea45eb19 Mon Sep 17 00:00:00 2001 From: Carlos Alberto Cortez Date: Tue, 7 Mar 2023 16:08:43 -0600 Subject: [PATCH] OpenTracing Shim: Update Tracer.close() (#5151) * Update the Tracer.close() implementation. * Unobfuscate SdkTracerProvider * Use AtomicBoolean instead of volatile --------- Co-authored-by: Jack Berg --- .../opentracingshim/OpenTracingShim.java | 2 +- .../opentracingshim/TracerShim.java | 45 +++++++++-- .../opentracingshim/TracerShimTest.java | 80 +++++++++++++++---- 3 files changed, 107 insertions(+), 20 deletions(-) diff --git a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/OpenTracingShim.java b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/OpenTracingShim.java index d8e0a746d65..b6b7fb62b28 100644 --- a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/OpenTracingShim.java +++ b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/OpenTracingShim.java @@ -44,6 +44,6 @@ public static io.opentracing.Tracer createTracerShim( TracerProvider provider, TextMapPropagator textMapPropagator, TextMapPropagator httpPropagator) { - return new TracerShim(provider.get("opentracing-shim"), textMapPropagator, httpPropagator); + return new TracerShim(provider, textMapPropagator, httpPropagator); } } diff --git a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/TracerShim.java b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/TracerShim.java index 32cbc30e2af..40a4f5b0815 100644 --- a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/TracerShim.java +++ b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/TracerShim.java @@ -5,6 +5,7 @@ package io.opentelemetry.opentracingshim; +import io.opentelemetry.api.trace.TracerProvider; import io.opentelemetry.context.propagation.TextMapPropagator; import io.opentracing.Scope; import io.opentracing.ScopeManager; @@ -14,6 +15,10 @@ import io.opentracing.propagation.Format; import io.opentracing.propagation.TextMapExtract; import io.opentracing.propagation.TextMapInject; +import java.io.Closeable; +import java.io.IOException; +import java.lang.reflect.Field; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.logging.Level; import java.util.logging.Logger; import javax.annotation.Nullable; @@ -21,16 +26,18 @@ final class TracerShim implements Tracer { private static final Logger logger = Logger.getLogger(TracerShim.class.getName()); + private final io.opentelemetry.api.trace.TracerProvider provider; private final io.opentelemetry.api.trace.Tracer tracer; private final ScopeManager scopeManagerShim; private final Propagation propagation; - private volatile boolean isClosed; + private final AtomicBoolean isShutdown = new AtomicBoolean(); TracerShim( - io.opentelemetry.api.trace.Tracer tracer, + io.opentelemetry.api.trace.TracerProvider provider, TextMapPropagator textMapPropagator, TextMapPropagator httpPropagator) { - this.tracer = tracer; + this.provider = provider; + this.tracer = provider.get("opentracing-shim"); this.propagation = new Propagation(textMapPropagator, httpPropagator); this.scopeManagerShim = new ScopeManagerShim(); } @@ -62,7 +69,7 @@ public Scope activateSpan(Span span) { @Override public SpanBuilder buildSpan(String operationName) { - if (isClosed) { + if (isShutdown.get()) { return new NoopSpanBuilderShim(operationName); } @@ -110,6 +117,34 @@ public SpanContext extract(Format format, C carrier) { @Override public void close() { - isClosed = true; + if (!isShutdown.compareAndSet(false, true)) { + return; + } + + TracerProvider provider = maybeUnobfuscate(this.provider); + if (provider instanceof Closeable) { + try { + ((Closeable) provider).close(); + } catch (RuntimeException | IOException e) { + logger.log(Level.INFO, "Exception caught while closing TracerProvider.", e); + } + } + } + + private static TracerProvider maybeUnobfuscate(TracerProvider tracerProvider) { + if (!tracerProvider.getClass().getSimpleName().equals("ObfuscatedTracerProvider")) { + return tracerProvider; + } + try { + Field delegateField = tracerProvider.getClass().getDeclaredField("delegate"); + delegateField.setAccessible(true); + Object delegate = delegateField.get(tracerProvider); + if (delegate instanceof TracerProvider) { + return (TracerProvider) delegate; + } + } catch (NoSuchFieldException | IllegalAccessException e) { + logger.log(Level.INFO, "Error trying to unobfuscate SdkTracerProvider", e); + } + return tracerProvider; } } diff --git a/opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/TracerShimTest.java b/opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/TracerShimTest.java index fafdbca8d87..d15d7d98d6b 100644 --- a/opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/TracerShimTest.java +++ b/opentracing-shim/src/test/java/io/opentelemetry/opentracingshim/TracerShimTest.java @@ -6,16 +6,23 @@ package io.opentelemetry.opentracingshim; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import io.opentelemetry.api.GlobalOpenTelemetry; +import io.opentelemetry.api.OpenTelemetry; import io.opentelemetry.api.baggage.propagation.W3CBaggagePropagator; -import io.opentelemetry.api.trace.Tracer; +import io.opentelemetry.api.trace.TracerProvider; import io.opentelemetry.context.Context; import io.opentelemetry.context.propagation.TextMapPropagator; +import io.opentelemetry.sdk.OpenTelemetrySdk; import io.opentelemetry.sdk.testing.junit5.OpenTelemetryExtension; +import io.opentelemetry.sdk.trace.SdkTracerProvider; import io.opentracing.Scope; import io.opentracing.Span; import io.opentracing.SpanContext; +import io.opentracing.Tracer; import io.opentracing.propagation.Format; import io.opentracing.propagation.TextMapAdapter; import io.opentracing.tag.BooleanTag; @@ -26,6 +33,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.Map; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -40,15 +48,22 @@ class TracerShimTest { @RegisterExtension static OpenTelemetryExtension otelTesting = OpenTelemetryExtension.create(); TracerShim tracerShim; - Tracer tracer; + TracerProvider provider; @BeforeEach void setUp() { - tracer = otelTesting.getOpenTelemetry().getTracer("opentracingshim"); + GlobalOpenTelemetry.resetForTest(); + GlobalOpenTelemetry.set(otelTesting.getOpenTelemetry()); + + provider = otelTesting.getOpenTelemetry().getTracerProvider(); TextMapPropagator propagator = otelTesting.getOpenTelemetry().getPropagators().getTextMapPropagator(); - ; - tracerShim = new TracerShim(tracer, propagator, propagator); + tracerShim = new TracerShim(provider, propagator, propagator); + } + + @AfterEach + void cleanup() { + GlobalOpenTelemetry.resetForTest(); } @Test @@ -278,8 +293,8 @@ void inject_textMap() { Map map = new HashMap<>(); CustomTextMapPropagator textMapPropagator = new CustomTextMapPropagator(); CustomTextMapPropagator httpHeadersPropagator = new CustomTextMapPropagator(); - tracerShim = new TracerShim(tracer, textMapPropagator, httpHeadersPropagator); - io.opentelemetry.api.trace.Span span = tracer.spanBuilder("span").startSpan(); + tracerShim = new TracerShim(provider, textMapPropagator, httpHeadersPropagator); + io.opentelemetry.api.trace.Span span = tracerShim.tracer().spanBuilder("span").startSpan(); SpanContext context = new SpanShim(span).context(); tracerShim.inject(context, Format.Builtin.TEXT_MAP, new TextMapAdapter(map)); @@ -292,8 +307,8 @@ void inject_httpHeaders() { Map map = new HashMap<>(); CustomTextMapPropagator textMapPropagator = new CustomTextMapPropagator(); CustomTextMapPropagator httpHeadersPropagator = new CustomTextMapPropagator(); - tracerShim = new TracerShim(tracer, textMapPropagator, httpHeadersPropagator); - io.opentelemetry.api.trace.Span span = tracer.spanBuilder("span").startSpan(); + tracerShim = new TracerShim(provider, textMapPropagator, httpHeadersPropagator); + io.opentelemetry.api.trace.Span span = tracerShim.tracer().spanBuilder("span").startSpan(); SpanContext context = new SpanShim(span).context(); tracerShim.inject(context, Format.Builtin.HTTP_HEADERS, new TextMapAdapter(map)); @@ -306,7 +321,7 @@ void extract_textMap() { Map map = new HashMap<>(); CustomTextMapPropagator textMapPropagator = new CustomTextMapPropagator(); CustomTextMapPropagator httpHeadersPropagator = new CustomTextMapPropagator(); - tracerShim = new TracerShim(tracer, textMapPropagator, httpHeadersPropagator); + tracerShim = new TracerShim(provider, textMapPropagator, httpHeadersPropagator); tracerShim.extract(Format.Builtin.TEXT_MAP, new TextMapAdapter(map)); assertThat(textMapPropagator.isExtracted()).isTrue(); @@ -318,7 +333,7 @@ void extract_httpHeaders() { Map map = new HashMap<>(); CustomTextMapPropagator textMapPropagator = new CustomTextMapPropagator(); CustomTextMapPropagator httpHeadersPropagator = new CustomTextMapPropagator(); - tracerShim = new TracerShim(tracer, textMapPropagator, httpHeadersPropagator); + tracerShim = new TracerShim(provider, textMapPropagator, httpHeadersPropagator); tracerShim.extract(Format.Builtin.HTTP_HEADERS, new TextMapAdapter(map)); assertThat(textMapPropagator.isExtracted()).isFalse(); @@ -328,7 +343,7 @@ void extract_httpHeaders() { @Test void extract_onlyBaggage() { W3CBaggagePropagator propagator = W3CBaggagePropagator.getInstance(); - tracerShim = new TracerShim(tracer, propagator, TextMapPropagator.noop()); + tracerShim = new TracerShim(provider, propagator, TextMapPropagator.noop()); io.opentelemetry.api.baggage.Baggage baggage = io.opentelemetry.api.baggage.Baggage.builder().put("foo", "bar").build(); @@ -342,8 +357,44 @@ void extract_onlyBaggage() { } @Test - void close() { + void close_OpenTelemetrySdk() { + SdkTracerProvider sdkProvider = mock(SdkTracerProvider.class); + doThrow(new RuntimeException("testing error")).when(sdkProvider).close(); + + Tracer tracerShim = + OpenTracingShim.createTracerShim( + OpenTelemetrySdk.builder().setTracerProvider(sdkProvider).build()); + tracerShim.close(); + + verify(sdkProvider).close(); + + Span otSpan = tracerShim.buildSpan(null).start(); + io.opentelemetry.api.trace.Span span = ((SpanShim) otSpan).getSpan(); + assertThat(span.getSpanContext().isValid()).isFalse(); + } + + @Test + void close_GlobalOpenTelemetry() { + GlobalOpenTelemetry.resetForTest(); + SdkTracerProvider sdkProvider = mock(SdkTracerProvider.class); + doThrow(new RuntimeException("testing error")).when(sdkProvider).close(); + GlobalOpenTelemetry.set(OpenTelemetrySdk.builder().setTracerProvider(sdkProvider).build()); + + Tracer tracerShim = OpenTracingShim.createTracerShim(GlobalOpenTelemetry.get()); tracerShim.close(); + + verify(sdkProvider).close(); + + Span otSpan = tracerShim.buildSpan(null).start(); + io.opentelemetry.api.trace.Span span = ((SpanShim) otSpan).getSpan(); + assertThat(span.getSpanContext().isValid()).isFalse(); + } + + @Test + void close_NoopOpenTelemetry() { + Tracer tracerShim = OpenTracingShim.createTracerShim(OpenTelemetry.noop()); + tracerShim.close(); + Span otSpan = tracerShim.buildSpan(null).start(); io.opentelemetry.api.trace.Span span = ((SpanShim) otSpan).getSpan(); assertThat(span.getSpanContext().isValid()).isFalse(); @@ -390,7 +441,8 @@ void doesNotCrash() { @Test void noopDoesNotCrash() { - tracerShim.close(); + Tracer tracerShim = OpenTracingShim.createTracerShim(OpenTelemetry.noop()); + Span span = tracerShim .buildSpan("test")