From 7208edc1832c6be3e8548d01289a88370027e5bb Mon Sep 17 00:00:00 2001 From: suraj kumar Date: Tue, 11 Jul 2023 17:11:40 +0530 Subject: [PATCH] Add wrapped tracer implementation (#8565) * Add wrapped tracer implementation Signed-off-by: suranjay * Add changelog entry Signed-off-by: suranjay * Add @opensearch.internal annotation Signed-off-by: suranjay * Fix test Signed-off-by: suranjay * Fix changelog entry Signed-off-by: suranjay --------- Signed-off-by: suranjay --- CHANGELOG.md | 3 + .../org/opensearch/telemetry/Telemetry.java | 2 + .../telemetry/tracing/AbstractSpan.java | 2 + .../telemetry/tracing/DefaultSpanScope.java | 4 +- .../telemetry/tracing/DefaultTracer.java | 4 +- .../opensearch/telemetry/tracing/Span.java | 4 +- .../telemetry/tracing/SpanReference.java | 4 +- .../tracing/TracerContextStorage.java | 2 + .../tracing/TracingContextPropagator.java | 2 + .../telemetry/tracing/TracingTelemetry.java | 2 + .../telemetry/tracing/noop/NoopSpanScope.java | 2 + .../telemetry/tracing/noop/NoopTracer.java | 2 + .../telemetry/tracing/NoopTracerFactory.java | 2 + ...hreadContextBasedTracerContextStorage.java | 2 + .../telemetry/tracing/TracerFactory.java | 24 +++++-- .../telemetry/tracing/WrappedTracer.java | 52 ++++++++++++++ .../telemetry/tracing/TracerFactoryTests.java | 9 +-- .../telemetry/tracing/WrappedTracerTests.java | 69 +++++++++++++++++++ 18 files changed, 175 insertions(+), 16 deletions(-) create mode 100644 server/src/main/java/org/opensearch/telemetry/tracing/WrappedTracer.java create mode 100644 server/src/test/java/org/opensearch/telemetry/tracing/WrappedTracerTests.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 5130dd00845b6..fc367be22c1fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Pass localNode info to all plugins on node start ([#7919](https://github.com/opensearch-project/OpenSearch/pull/7919)) - Improved performance of parsing floating point numbers ([#7909](https://github.com/opensearch-project/OpenSearch/pull/7909)) - Move span actions to Scope ([#8411](https://github.com/opensearch-project/OpenSearch/pull/8411)) +- Add wrapper tracer implementation ### Deprecated @@ -164,6 +165,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Enabling compression levels for zstd and zstd_no_dict ([#8312](https://github.com/opensearch-project/OpenSearch/pull/8312)) - Optimize Metadata build() to skip redundant computations as part of ClusterState build ([#7853](https://github.com/opensearch-project/OpenSearch/pull/7853)) - Add safeguard limits for file cache during node level allocation ([#8208](https://github.com/opensearch-project/OpenSearch/pull/8208)) +- Move span actions to Scope ([#8411](https://github.com/opensearch-project/OpenSearch/pull/8411)) +- Add wrapper tracer implementation ([#8565](https://github.com/opensearch-project/OpenSearch/pull/8565)) ### Deprecated diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/Telemetry.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/Telemetry.java index 6f50699528b6b..65c974a0d0c36 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/Telemetry.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/Telemetry.java @@ -13,6 +13,8 @@ /** * Interface defining telemetry + * + * @opensearch.internal */ public interface Telemetry { diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/AbstractSpan.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/AbstractSpan.java index 316edc971913e..150a32b14d0f8 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/AbstractSpan.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/AbstractSpan.java @@ -10,6 +10,8 @@ /** * Base span + * + * @opensearch.internal */ public abstract class AbstractSpan implements Span { diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultSpanScope.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultSpanScope.java index 58e9e0abad739..356b72187de74 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultSpanScope.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultSpanScope.java @@ -12,8 +12,10 @@ /** * Default implementation of Scope + * + * @opensearch.internal */ -public class DefaultSpanScope implements SpanScope { +final class DefaultSpanScope implements SpanScope { private final Span span; diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultTracer.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultTracer.java index 783edd238c1c2..ea59eec645420 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultTracer.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/DefaultTracer.java @@ -16,9 +16,9 @@ * The default tracer implementation. It handles tracing context propagation between spans by maintaining * current active span in its storage * - * + * @opensearch.internal */ -public class DefaultTracer implements Tracer { +class DefaultTracer implements Tracer { static final String THREAD_NAME = "th_name"; private final TracingTelemetry tracingTelemetry; diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Span.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Span.java index d60b4e60adece..6cb1c8234f3de 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Span.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Span.java @@ -11,7 +11,9 @@ /** * An interface that represents a tracing span. * Spans are created by the Tracer.startSpan method. - * Span must be ended by calling Tracer.endSpan which internally calls Span's endSpan. + * Span must be ended by calling SpanScope.close which internally calls Span's endSpan. + * + * @opensearch.internal */ public interface Span { diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/SpanReference.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/SpanReference.java index 99d1bd3c93c84..180136ecf7a57 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/SpanReference.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/SpanReference.java @@ -10,8 +10,10 @@ /** * Wrapper class to hold reference of Span + * + * @opensearch.internal */ -public class SpanReference { +final class SpanReference { private Span span; diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/TracerContextStorage.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/TracerContextStorage.java index eb93006835332..d85b404b0ce41 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/TracerContextStorage.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/TracerContextStorage.java @@ -12,6 +12,8 @@ * Storage interface used for storing tracing context * @param key type * @param value type + * + * @opensearch.internal */ public interface TracerContextStorage { /** diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/TracingContextPropagator.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/TracingContextPropagator.java index 1152e3aedfa88..3e4a377d33a3d 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/TracingContextPropagator.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/TracingContextPropagator.java @@ -13,6 +13,8 @@ /** * Interface defining the tracing related context propagation + * + * @opensearch.internal */ public interface TracingContextPropagator { diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/TracingTelemetry.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/TracingTelemetry.java index 16c76bd0cc141..bce955fc2d99e 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/TracingTelemetry.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/TracingTelemetry.java @@ -12,6 +12,8 @@ /** * Interface for tracing telemetry providers + * + * @opensearch.internal */ public interface TracingTelemetry extends Closeable { diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopSpanScope.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopSpanScope.java index c0dbaf65ba48b..a1d16d1d80d00 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopSpanScope.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopSpanScope.java @@ -12,6 +12,8 @@ /** * No-op implementation of SpanScope + * + * @opensearch.internal */ public final class NoopSpanScope implements SpanScope { diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopTracer.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopTracer.java index a66cbcf4fef52..a1768d7d59116 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopTracer.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/noop/NoopTracer.java @@ -13,6 +13,8 @@ /** * No-op implementation of Tracer + * + * @opensearch.internal */ public class NoopTracer implements Tracer { diff --git a/server/src/main/java/org/opensearch/telemetry/tracing/NoopTracerFactory.java b/server/src/main/java/org/opensearch/telemetry/tracing/NoopTracerFactory.java index 3d7f8133788ce..f82a390dc1754 100644 --- a/server/src/main/java/org/opensearch/telemetry/tracing/NoopTracerFactory.java +++ b/server/src/main/java/org/opensearch/telemetry/tracing/NoopTracerFactory.java @@ -14,6 +14,8 @@ /** * No-op implementation of TracerFactory + * + * @opensearch.internal */ public class NoopTracerFactory extends TracerFactory { public NoopTracerFactory() { diff --git a/server/src/main/java/org/opensearch/telemetry/tracing/ThreadContextBasedTracerContextStorage.java b/server/src/main/java/org/opensearch/telemetry/tracing/ThreadContextBasedTracerContextStorage.java index 0d0b795fdc715..c009ab2391aab 100644 --- a/server/src/main/java/org/opensearch/telemetry/tracing/ThreadContextBasedTracerContextStorage.java +++ b/server/src/main/java/org/opensearch/telemetry/tracing/ThreadContextBasedTracerContextStorage.java @@ -18,6 +18,8 @@ /** * Core's ThreadContext based TracerContextStorage implementation + * + * @opensearch.internal */ public class ThreadContextBasedTracerContextStorage implements TracerContextStorage, ThreadContextStatePropagator { diff --git a/server/src/main/java/org/opensearch/telemetry/tracing/TracerFactory.java b/server/src/main/java/org/opensearch/telemetry/tracing/TracerFactory.java index 8228cded4c822..d8fe812c82f53 100644 --- a/server/src/main/java/org/opensearch/telemetry/tracing/TracerFactory.java +++ b/server/src/main/java/org/opensearch/telemetry/tracing/TracerFactory.java @@ -21,7 +21,7 @@ /** * TracerManager represents a single global class that is used to access tracers. - * + *

* The Tracer singleton object can be retrieved using tracerManager.getTracer(). The TracerManager object * is created during class initialization and cannot subsequently be changed. */ @@ -30,21 +30,20 @@ public class TracerFactory implements Closeable { private static final Logger logger = LogManager.getLogger(TracerFactory.class); private final TelemetrySettings telemetrySettings; - private final Tracer defaultTracer; + private final Tracer tracer; public TracerFactory(TelemetrySettings telemetrySettings, Optional telemetry, ThreadContext threadContext) { this.telemetrySettings = telemetrySettings; - this.defaultTracer = telemetry.map(Telemetry::getTracingTelemetry) - .map(tracingTelemetry -> createDefaultTracer(tracingTelemetry, threadContext)) - .orElse(NoopTracer.INSTANCE); + this.tracer = tracer(telemetry, threadContext); } /** * Returns the tracer instance + * * @return tracer instance */ public Tracer getTracer() { - return telemetrySettings.isTracingEnabled() ? defaultTracer : NoopTracer.INSTANCE; + return tracer; } /** @@ -53,12 +52,19 @@ public Tracer getTracer() { @Override public void close() { try { - defaultTracer.close(); + tracer.close(); } catch (IOException e) { logger.warn("Error closing tracer", e); } } + private Tracer tracer(Optional telemetry, ThreadContext threadContext) { + return telemetry.map(Telemetry::getTracingTelemetry) + .map(tracingTelemetry -> createDefaultTracer(tracingTelemetry, threadContext)) + .map(defaultTracer -> createWrappedTracer(defaultTracer)) + .orElse(NoopTracer.INSTANCE); + } + private Tracer createDefaultTracer(TracingTelemetry tracingTelemetry, ThreadContext threadContext) { TracerContextStorage tracerContextStorage = new ThreadContextBasedTracerContextStorage( threadContext, @@ -67,4 +73,8 @@ private Tracer createDefaultTracer(TracingTelemetry tracingTelemetry, ThreadCont return new DefaultTracer(tracingTelemetry, tracerContextStorage); } + private Tracer createWrappedTracer(Tracer defaultTracer) { + return new WrappedTracer(telemetrySettings, defaultTracer); + } + } diff --git a/server/src/main/java/org/opensearch/telemetry/tracing/WrappedTracer.java b/server/src/main/java/org/opensearch/telemetry/tracing/WrappedTracer.java new file mode 100644 index 0000000000000..0ba9a8ea5fd88 --- /dev/null +++ b/server/src/main/java/org/opensearch/telemetry/tracing/WrappedTracer.java @@ -0,0 +1,52 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.telemetry.tracing; + +import org.opensearch.telemetry.TelemetrySettings; +import org.opensearch.telemetry.tracing.noop.NoopTracer; + +import java.io.IOException; + +/** + * Wrapper implementation of Tracer. This delegates call to right tracer based on the tracer settings + * + * @opensearch.internal + */ +final class WrappedTracer implements Tracer { + + private final Tracer defaultTracer; + private final TelemetrySettings telemetrySettings; + + /** + * Creates WrappedTracer instance + * + * @param telemetrySettings telemetry settings + * @param defaultTracer default tracer instance + */ + public WrappedTracer(TelemetrySettings telemetrySettings, Tracer defaultTracer) { + this.defaultTracer = defaultTracer; + this.telemetrySettings = telemetrySettings; + } + + @Override + public SpanScope startSpan(String spanName) { + Tracer delegateTracer = getDelegateTracer(); + return delegateTracer.startSpan(spanName); + } + + @Override + public void close() throws IOException { + defaultTracer.close(); + } + + // visible for testing + Tracer getDelegateTracer() { + return telemetrySettings.isTracingEnabled() ? defaultTracer : NoopTracer.INSTANCE; + } +} diff --git a/server/src/test/java/org/opensearch/telemetry/tracing/TracerFactoryTests.java b/server/src/test/java/org/opensearch/telemetry/tracing/TracerFactoryTests.java index df9cdd6669d23..0ffccee505d43 100644 --- a/server/src/test/java/org/opensearch/telemetry/tracing/TracerFactoryTests.java +++ b/server/src/test/java/org/opensearch/telemetry/tracing/TracerFactoryTests.java @@ -36,19 +36,20 @@ public void close() { tracerFactory.close(); } - public void testGetTracerWithTracingDisabledReturnsNoopTracer() { + public void testGetTracerWithUnavailableTracingTelemetryReturnsNoopTracer() { Settings settings = Settings.builder().put(TelemetrySettings.TRACER_ENABLED_SETTING.getKey(), false).build(); TelemetrySettings telemetrySettings = new TelemetrySettings(settings, new ClusterSettings(settings, getClusterSettings())); Telemetry mockTelemetry = mock(Telemetry.class); when(mockTelemetry.getTracingTelemetry()).thenReturn(mock(TracingTelemetry.class)); - tracerFactory = new TracerFactory(telemetrySettings, Optional.of(mockTelemetry), new ThreadContext(Settings.EMPTY)); + tracerFactory = new TracerFactory(telemetrySettings, Optional.empty(), new ThreadContext(Settings.EMPTY)); Tracer tracer = tracerFactory.getTracer(); + assertTrue(tracer instanceof NoopTracer); assertTrue(tracer.startSpan("foo") == SpanScope.NO_OP); } - public void testGetTracerWithTracingEnabledReturnsDefaultTracer() { + public void testGetTracerWithAvailableTracingTelemetryReturnsWrappedTracer() { Settings settings = Settings.builder().put(TelemetrySettings.TRACER_ENABLED_SETTING.getKey(), true).build(); TelemetrySettings telemetrySettings = new TelemetrySettings(settings, new ClusterSettings(settings, getClusterSettings())); Telemetry mockTelemetry = mock(Telemetry.class); @@ -56,7 +57,7 @@ public void testGetTracerWithTracingEnabledReturnsDefaultTracer() { tracerFactory = new TracerFactory(telemetrySettings, Optional.of(mockTelemetry), new ThreadContext(Settings.EMPTY)); Tracer tracer = tracerFactory.getTracer(); - assertTrue(tracer instanceof DefaultTracer); + assertTrue(tracer instanceof WrappedTracer); } diff --git a/server/src/test/java/org/opensearch/telemetry/tracing/WrappedTracerTests.java b/server/src/test/java/org/opensearch/telemetry/tracing/WrappedTracerTests.java new file mode 100644 index 0000000000000..d1abc5a4d98aa --- /dev/null +++ b/server/src/test/java/org/opensearch/telemetry/tracing/WrappedTracerTests.java @@ -0,0 +1,69 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.telemetry.tracing; + +import org.opensearch.common.settings.ClusterSettings; +import org.opensearch.common.settings.Setting; +import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.FeatureFlags; +import org.opensearch.telemetry.TelemetrySettings; +import org.opensearch.telemetry.tracing.noop.NoopTracer; +import org.opensearch.test.OpenSearchTestCase; + +import java.io.IOException; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + +public class WrappedTracerTests extends OpenSearchTestCase { + + public void testStartSpanWithTracingDisabledInvokesNoopTracer() throws Exception { + Settings settings = Settings.builder().put(TelemetrySettings.TRACER_ENABLED_SETTING.getKey(), false).build(); + TelemetrySettings telemetrySettings = new TelemetrySettings(settings, new ClusterSettings(settings, getClusterSettings())); + DefaultTracer mockDefaultTracer = mock(DefaultTracer.class); + + try (WrappedTracer wrappedTracer = new WrappedTracer(telemetrySettings, mockDefaultTracer)) { + wrappedTracer.startSpan("foo"); + assertTrue(wrappedTracer.getDelegateTracer() instanceof NoopTracer); + verify(mockDefaultTracer, never()).startSpan("foo"); + } + } + + public void testStartSpanWithTracingEnabledInvokesDefaultTracer() throws Exception { + Settings settings = Settings.builder().put(TelemetrySettings.TRACER_ENABLED_SETTING.getKey(), true).build(); + TelemetrySettings telemetrySettings = new TelemetrySettings(settings, new ClusterSettings(settings, getClusterSettings())); + DefaultTracer mockDefaultTracer = mock(DefaultTracer.class); + + try (WrappedTracer wrappedTracer = new WrappedTracer(telemetrySettings, mockDefaultTracer)) { + wrappedTracer.startSpan("foo"); + + assertTrue(wrappedTracer.getDelegateTracer() instanceof DefaultTracer); + verify(mockDefaultTracer).startSpan("foo"); + } + } + + public void testClose() throws IOException { + DefaultTracer mockDefaultTracer = mock(DefaultTracer.class); + WrappedTracer wrappedTracer = new WrappedTracer(null, mockDefaultTracer); + + wrappedTracer.close(); + + verify(mockDefaultTracer).close(); + } + + private Set> getClusterSettings() { + Set> allTracerSettings = new HashSet<>(); + ClusterSettings.FEATURE_FLAGGED_CLUSTER_SETTINGS.get(List.of(FeatureFlags.TELEMETRY)).stream().forEach((allTracerSettings::add)); + return allTracerSettings; + } +}