Address review comment

Signed-off-by: Gagan Juneja <[email protected]>
Gaganjuneja · Oct 16, 2023 · 9a67369 · 9a67369
1 parent d63c38c
commit 9a67369
Show file tree

Hide file tree

Showing 19 changed files with 76 additions and 126 deletions.
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
@@ -85,6 +85,11 @@ public SpanScope withSpanInScope(Span span) {
         return DefaultSpanScope.create(span, tracerContextStorage).attach();
     }
 
+    @Override
+    public boolean isRecording() {
+        return true;
+    }
+
     private Span createSpan(SpanCreationContext spanCreationContext, Span parentSpan) {
         return tracingTelemetry.createSpan(spanCreationContext, parentSpan);
     }

diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Tracer.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/tracing/Tracer.java
@@ -53,4 +53,10 @@ public interface Tracer extends HttpTracer, Closeable {
      */
     SpanScope withSpanInScope(Span span);
 
+    /**
+     * Tells if the traces are being recorded or not
+     * @return boolean
+     */
+    boolean isRecording();
+
 }
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
@@ -54,6 +54,11 @@ public SpanScope withSpanInScope(Span span) {
         return SpanScope.NO_OP;
     }
 
+    @Override
+    public boolean isRecording() {
+        return false;
+    }
+
     @Override
     public void close() {
 

diff --git a/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultTracerTests.java b/libs/telemetry/src/test/java/org/opensearch/telemetry/tracing/DefaultTracerTests.java
@@ -62,6 +62,7 @@ public void testCreateSpan() {
 
         String spanName = defaultTracer.getCurrentSpan().getSpan().getSpanName();
         assertEquals("span_name", spanName);
+        assertTrue(defaultTracer.isRecording());
     }
 
     @SuppressWarnings("unchecked")

diff --git a/...alClusterTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsEnabledCounterIT.java b/...alClusterTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsEnabledCounterIT.java
@@ -15,6 +15,7 @@
 import org.opensearch.telemetry.OTelTelemetrySettings;
 import org.opensearch.telemetry.TelemetrySettings;
 import org.opensearch.test.OpenSearchSingleNodeTestCase;
+import org.junit.After;
 
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -60,4 +61,25 @@ public void testCounter() throws Exception {
             .getValue();
         assertEquals(1.0, value, 0.0);
     }
+
+    public void testUpDownCounter() throws Exception {
+
+        MetricsRegistry metricsRegistry = node().injector().getInstance(MetricsRegistry.class);
+
+        Counter counter = metricsRegistry.createUpDownCounter("test-up-down-counter", "test", "1");
+        counter.add(1.0);
+        counter.add(-2.0);
+        // Sleep for about 2s to wait for metrics to be published.
+        Thread.sleep(2000);
+
+        InMemorySingletonMetricsExporter exporter = InMemorySingletonMetricsExporter.INSTANCE;
+        double value = ((DoublePointData) ((ArrayList) exporter.getFinishedMetricItems().get(0).getDoubleSumData().getPoints()).get(0))
+            .getValue();
+        assertEquals(-1.0, value, 0.0);
+    }
+
+    @After
+    public void reset() {
+        InMemorySingletonMetricsExporter.INSTANCE.reset();
+    }
 }
diff --git a/...terTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsEnabledUpDownCounterIT.java b/...terTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsEnabledUpDownCounterIT.java
diff --git a/plugins/telemetry-otel/src/test/java/org/opensearch/telemetry/OTelTelemetryPluginTests.java b/plugins/telemetry-otel/src/test/java/org/opensearch/telemetry/OTelTelemetryPluginTests.java
@@ -51,10 +51,7 @@ public void setup() {
         Settings settings = Settings.builder().put(TRACER_EXPORTER_DELAY_SETTING.getKey(), "1s").build();
         oTelTelemetryPlugin = new OTelTelemetryPlugin(settings);
         telemetry = oTelTelemetryPlugin.getTelemetry(
-            TelemetrySettings.create(
-                Settings.EMPTY,
-                new ClusterSettings(settings, Set.of(TRACER_ENABLED_SETTING, TRACER_SAMPLER_PROBABILITY))
-            )
+            new TelemetrySettings(Settings.EMPTY, new ClusterSettings(settings, Set.of(TRACER_ENABLED_SETTING, TRACER_SAMPLER_PROBABILITY)))
         );
         tracingTelemetry = telemetry.get().getTracingTelemetry();
         metricsTelemetry = telemetry.get().getMetricsTelemetry();

diff --git a/...tel/src/test/java/org/opensearch/telemetry/tracing/sampler/ProbabilisticSamplerTests.java b/...tel/src/test/java/org/opensearch/telemetry/tracing/sampler/ProbabilisticSamplerTests.java
@@ -31,7 +31,7 @@ public void testProbabilisticSamplerWithNullSettings() {
 
     public void testDefaultGetSampler() {
         Settings settings = Settings.builder().put(TRACER_EXPORTER_DELAY_SETTING.getKey(), "1s").build();
-        TelemetrySettings telemetrySettings = TelemetrySettings.create(
+        TelemetrySettings telemetrySettings = new TelemetrySettings(
             Settings.EMPTY,
             new ClusterSettings(settings, Set.of(TRACER_SAMPLER_PROBABILITY, TRACER_ENABLED_SETTING))
         );
@@ -45,7 +45,7 @@ public void testDefaultGetSampler() {
 
     public void testGetSamplerWithUpdatedSamplingRatio() {
         Settings settings = Settings.builder().put(TRACER_EXPORTER_DELAY_SETTING.getKey(), "1s").build();
-        TelemetrySettings telemetrySettings = TelemetrySettings.create(
+        TelemetrySettings telemetrySettings = new TelemetrySettings(
             Settings.EMPTY,
             new ClusterSettings(settings, Set.of(TRACER_SAMPLER_PROBABILITY, TRACER_ENABLED_SETTING))
         );

diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java
@@ -598,15 +598,15 @@ protected Node(
             TracerFactory tracerFactory;
             MetricsRegistryFactory metricsRegistryFactory;
             if (FeatureFlags.isEnabled(TELEMETRY)) {
-                final TelemetrySettings telemetrySettings = TelemetrySettings.create(settings, clusterService.getClusterSettings());
+                final TelemetrySettings telemetrySettings = new TelemetrySettings(settings, clusterService.getClusterSettings());
                 List<TelemetryPlugin> telemetryPlugins = pluginsService.filterPlugins(TelemetryPlugin.class);
                 TelemetryModule telemetryModule = new TelemetryModule(telemetryPlugins, telemetrySettings);
-                if (TelemetrySettings.isTracerFeatureEnabled()) {
+                if (telemetrySettings.isTracerFeatureEnabled()) {
                     tracerFactory = new TracerFactory(telemetrySettings, telemetryModule.getTelemetry(), threadPool.getThreadContext());
                 } else {
                     tracerFactory = new NoopTracerFactory();
                 }
-                if (TelemetrySettings.isMetricsFeatureEnabled()) {
+                if (telemetrySettings.isMetricsFeatureEnabled()) {
                     metricsRegistryFactory = new MetricsRegistryFactory(telemetrySettings, telemetryModule.getTelemetry());
                 } else {
                     metricsRegistryFactory = new NoopMetricsRegistryFactory();

diff --git a/server/src/main/java/org/opensearch/telemetry/TelemetrySettings.java b/server/src/main/java/org/opensearch/telemetry/TelemetrySettings.java
@@ -67,14 +67,10 @@ public class TelemetrySettings {
     private volatile boolean tracingEnabled;
     private volatile double samplingProbability;
 
-    private static Settings settingz;
+    private Settings settingz;
 
-    public static TelemetrySettings create(Settings settings, ClusterSettings clusterSettings) {
-        settingz = settings;
-        return new TelemetrySettings(settings, clusterSettings);
-    }
-
-    private TelemetrySettings(Settings settings, ClusterSettings clusterSettings) {
+    public TelemetrySettings(Settings settings, ClusterSettings clusterSettings) {
+        this.settingz = settings;
         this.tracingEnabled = TRACER_ENABLED_SETTING.get(settings);
         this.samplingProbability = TRACER_SAMPLER_PROBABILITY.get(settings);
 
@@ -105,19 +101,11 @@ public double getSamplingProbability() {
         return samplingProbability;
     }
 
-    public static boolean isTracerFeatureEnabled() {
-        if (settingz != null) {
-            return TRACER_FEATURE_ENABLED_SETTING.get(settingz);
-        } else {
-            return false;
-        }
+    public boolean isTracerFeatureEnabled() {
+        return TRACER_FEATURE_ENABLED_SETTING.get(settingz);
     }
 
-    public static boolean isMetricsFeatureEnabled() {
-        if (settingz != null) {
-            return METRICS_FEATURE_ENABLED_SETTING.get(settingz);
-        } else {
-            return false;
-        }
+    public boolean isMetricsFeatureEnabled() {
+        return METRICS_FEATURE_ENABLED_SETTING.get(settingz);
     }
 }
diff --git a/server/src/main/java/org/opensearch/telemetry/tracing/WrappedTracer.java b/server/src/main/java/org/opensearch/telemetry/tracing/WrappedTracer.java
@@ -59,6 +59,11 @@ public SpanScope withSpanInScope(Span span) {
         return getDelegateTracer().withSpanInScope(span);
     }
 
+    @Override
+    public boolean isRecording() {
+        return getDelegateTracer().isRecording();
+    }
+
     @Override
     public void close() throws IOException {
         defaultTracer.close();

diff --git a/server/src/main/java/org/opensearch/telemetry/tracing/channels/TraceableHttpChannel.java b/server/src/main/java/org/opensearch/telemetry/tracing/channels/TraceableHttpChannel.java
@@ -8,11 +8,9 @@
 
 package org.opensearch.telemetry.tracing.channels;
 
-import org.opensearch.common.util.FeatureFlags;
 import org.opensearch.core.action.ActionListener;
 import org.opensearch.http.HttpChannel;
 import org.opensearch.http.HttpResponse;
-import org.opensearch.telemetry.TelemetrySettings;
 import org.opensearch.telemetry.tracing.Span;
 import org.opensearch.telemetry.tracing.Tracer;
 import org.opensearch.telemetry.tracing.listener.TraceableActionListener;
@@ -50,7 +48,7 @@ private TraceableHttpChannel(HttpChannel delegate, Span span, Tracer tracer) {
      * @return http channel
      */
     public static HttpChannel create(HttpChannel delegate, Span span, Tracer tracer) {
-        if (FeatureFlags.isEnabled(FeatureFlags.TELEMETRY) == true && TelemetrySettings.isTracerFeatureEnabled()) {
+        if (tracer.isRecording()) {
             return new TraceableHttpChannel(delegate, span, tracer);
         } else {
             return delegate;

diff --git a/server/src/main/java/org/opensearch/telemetry/tracing/channels/TraceableRestChannel.java b/server/src/main/java/org/opensearch/telemetry/tracing/channels/TraceableRestChannel.java
@@ -9,13 +9,11 @@
 package org.opensearch.telemetry.tracing.channels;
 
 import org.opensearch.common.io.stream.BytesStreamOutput;
-import org.opensearch.common.util.FeatureFlags;
 import org.opensearch.core.xcontent.MediaType;
 import org.opensearch.core.xcontent.XContentBuilder;
 import org.opensearch.rest.RestChannel;
 import org.opensearch.rest.RestRequest;
 import org.opensearch.rest.RestResponse;
-import org.opensearch.telemetry.TelemetrySettings;
 import org.opensearch.telemetry.tracing.Span;
 import org.opensearch.telemetry.tracing.SpanScope;
 import org.opensearch.telemetry.tracing.Tracer;
@@ -54,7 +52,7 @@ private TraceableRestChannel(RestChannel delegate, Span span, Tracer tracer) {
      * @return rest channel
      */
     public static RestChannel create(RestChannel delegate, Span span, Tracer tracer) {
-        if (FeatureFlags.isEnabled(FeatureFlags.TELEMETRY) == true && TelemetrySettings.isTracerFeatureEnabled()) {
+        if (tracer.isRecording()) {
             return new TraceableRestChannel(delegate, span, tracer);
         } else {
             return delegate;

diff --git a/...src/main/java/org/opensearch/telemetry/tracing/channels/TraceableTcpTransportChannel.java b/...src/main/java/org/opensearch/telemetry/tracing/channels/TraceableTcpTransportChannel.java
@@ -9,10 +9,8 @@
 package org.opensearch.telemetry.tracing.channels;
 
 import org.opensearch.Version;
-import org.opensearch.common.util.FeatureFlags;
 import org.opensearch.core.action.ActionListener;
 import org.opensearch.core.transport.TransportResponse;
-import org.opensearch.telemetry.TelemetrySettings;
 import org.opensearch.telemetry.tracing.Span;
 import org.opensearch.telemetry.tracing.SpanScope;
 import org.opensearch.telemetry.tracing.Tracer;
@@ -53,7 +51,7 @@ public TraceableTcpTransportChannel(TcpTransportChannel delegate, Span span, Tra
      * @return transport channel
      */
     public static TransportChannel create(TcpTransportChannel delegate, final Span span, final Tracer tracer) {
-        if (FeatureFlags.isEnabled(FeatureFlags.TELEMETRY) == true && TelemetrySettings.isTracerFeatureEnabled()) {
+        if (tracer.isRecording()) {
             delegate.getChannel().addCloseListener(new ActionListener<Void>() {
                 @Override
                 public void onResponse(Void unused) {

diff --git a/...main/java/org/opensearch/telemetry/tracing/handler/TraceableTransportResponseHandler.java b/...main/java/org/opensearch/telemetry/tracing/handler/TraceableTransportResponseHandler.java
@@ -8,10 +8,8 @@
 
 package org.opensearch.telemetry.tracing.handler;
 
-import org.opensearch.common.util.FeatureFlags;
 import org.opensearch.core.common.io.stream.StreamInput;
 import org.opensearch.core.transport.TransportResponse;
-import org.opensearch.telemetry.TelemetrySettings;
 import org.opensearch.telemetry.tracing.Span;
 import org.opensearch.telemetry.tracing.SpanScope;
 import org.opensearch.telemetry.tracing.Tracer;
@@ -56,7 +54,7 @@ public static <S extends TransportResponse> TransportResponseHandler<S> create(
         Span span,
         Tracer tracer
     ) {
-        if (FeatureFlags.isEnabled(FeatureFlags.TELEMETRY) == true && TelemetrySettings.isTracerFeatureEnabled()) {
+        if (tracer.isRecording()) {
             return new TraceableTransportResponseHandler<S>(delegate, span, tracer);
         } else {
             return delegate;

diff --git a/server/src/main/java/org/opensearch/telemetry/tracing/listener/TraceableActionListener.java b/server/src/main/java/org/opensearch/telemetry/tracing/listener/TraceableActionListener.java
@@ -8,9 +8,7 @@
 
 package org.opensearch.telemetry.tracing.listener;
 
-import org.opensearch.common.util.FeatureFlags;
 import org.opensearch.core.action.ActionListener;
-import org.opensearch.telemetry.TelemetrySettings;
 import org.opensearch.telemetry.tracing.Span;
 import org.opensearch.telemetry.tracing.SpanScope;
 import org.opensearch.telemetry.tracing.Tracer;
@@ -48,7 +46,7 @@ private TraceableActionListener(ActionListener<Response> delegate, Span span, Tr
      * @return action listener
      */
     public static <Response> ActionListener<Response> create(ActionListener<Response> delegate, Span span, Tracer tracer) {
-        if (FeatureFlags.isEnabled(FeatureFlags.TELEMETRY) == true && TelemetrySettings.isTracerFeatureEnabled()) {
+        if (tracer.isRecording()) {
             return new TraceableActionListener<Response>(delegate, span, tracer);
         } else {
             return delegate;

diff --git a/server/src/test/java/org/opensearch/telemetry/metrics/MetricsRegistryFactoryTests.java b/server/src/test/java/org/opensearch/telemetry/metrics/MetricsRegistryFactoryTests.java
@@ -39,7 +39,7 @@ public void close() {
 
     public void testGetMeterRegistryWithUnavailableMetricsTelemetry() {
         Settings settings = Settings.builder().put(TelemetrySettings.TRACER_ENABLED_SETTING.getKey(), false).build();
-        TelemetrySettings telemetrySettings = TelemetrySettings.create(settings, new ClusterSettings(settings, getClusterSettings()));
+        TelemetrySettings telemetrySettings = new TelemetrySettings(settings, new ClusterSettings(settings, getClusterSettings()));
         Telemetry mockTelemetry = mock(Telemetry.class);
         when(mockTelemetry.getTracingTelemetry()).thenReturn(mock(TracingTelemetry.class));
         metricsRegistryFactory = new MetricsRegistryFactory(telemetrySettings, Optional.empty());
@@ -53,7 +53,7 @@ public void testGetMeterRegistryWithUnavailableMetricsTelemetry() {
 
     public void testGetMetricsWithAvailableMetricsTelemetry() {
         Settings settings = Settings.builder().put(TelemetrySettings.TRACER_ENABLED_SETTING.getKey(), true).build();
-        TelemetrySettings telemetrySettings = TelemetrySettings.create(settings, new ClusterSettings(settings, getClusterSettings()));
+        TelemetrySettings telemetrySettings = new TelemetrySettings(settings, new ClusterSettings(settings, getClusterSettings()));
         Telemetry mockTelemetry = mock(Telemetry.class);
         when(mockTelemetry.getMetricsTelemetry()).thenReturn(mock(MetricsTelemetry.class));
         metricsRegistryFactory = new MetricsRegistryFactory(telemetrySettings, Optional.of(mockTelemetry));
@@ -65,7 +65,7 @@ public void testGetMetricsWithAvailableMetricsTelemetry() {
 
     public void testNullMetricsTelemetry() {
         Settings settings = Settings.builder().put(TelemetrySettings.METRICS_FEATURE_ENABLED_SETTING.getKey(), false).build();
-        TelemetrySettings telemetrySettings = TelemetrySettings.create(settings, new ClusterSettings(settings, getClusterSettings()));
+        TelemetrySettings telemetrySettings = new TelemetrySettings(settings, new ClusterSettings(settings, getClusterSettings()));
         Telemetry mockTelemetry = mock(Telemetry.class);
         when(mockTelemetry.getMetricsTelemetry()).thenReturn(null);
         metricsRegistryFactory = new MetricsRegistryFactory(telemetrySettings, Optional.of(mockTelemetry));