Address review comment

Signed-off-by: Gagan Juneja <[email protected]>
Gaganjuneja · Oct 3, 2023 · f196658 · f196658
1 parent 2b85bdd
commit f196658
Show file tree

Hide file tree

Showing 16 changed files with 34 additions and 286 deletions.
diff --git a/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/OTelTelemetryPlugin.java b/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/OTelTelemetryPlugin.java
@@ -21,7 +21,7 @@
 import java.util.List;
 import java.util.Optional;
 
-import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.sdk.OpenTelemetrySdk;
 
 /**
  * Telemetry plugin based on Otel
@@ -64,7 +64,7 @@ public String getName() {
     }
 
     private Telemetry telemetry(TelemetrySettings telemetrySettings) {
-        final OpenTelemetry openTelemetry = OTelResourceProvider.get(telemetrySettings, settings);
+        final OpenTelemetrySdk openTelemetry = OTelResourceProvider.get(telemetrySettings, settings);
         return new OTelTelemetry(new OTelTracingTelemetry(openTelemetry), new OTelMetricsTelemetry(openTelemetry));
     }
 

diff --git a/...s/telemetry-otel/src/main/java/org/opensearch/telemetry/metrics/OTelMetricsTelemetry.java b/...s/telemetry-otel/src/main/java/org/opensearch/telemetry/metrics/OTelMetricsTelemetry.java
@@ -15,7 +15,6 @@
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 
-import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.api.metrics.DoubleCounter;
 import io.opentelemetry.api.metrics.DoubleUpDownCounter;
 import io.opentelemetry.api.metrics.Meter;
@@ -26,14 +25,14 @@
  */
 public class OTelMetricsTelemetry implements MetricsTelemetry {
     private static final Logger logger = LogManager.getLogger(OTelMetricsTelemetry.class);
-    private final OpenTelemetry openTelemetry;
+    private final OpenTelemetrySdk openTelemetry;
     private final Meter otelMeter;
 
     /**
      * Constructor.
      * @param openTelemetry telemetry.
      */
-    public OTelMetricsTelemetry(OpenTelemetry openTelemetry) {
+    public OTelMetricsTelemetry(OpenTelemetrySdk openTelemetry) {
         this.openTelemetry = openTelemetry;
         this.otelMeter = openTelemetry.getMeter(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
     }
@@ -66,7 +65,7 @@ public Counter createUpDownCounter(String name, String description, String unit)
     public void close() {
         // There is no harm closing the openTelemetry multiple times.
         try {
-            ((OpenTelemetrySdk) openTelemetry).getSdkMeterProvider().close();
+            openTelemetry.getSdkMeterProvider().close();
         } catch (Exception e) {
             logger.warn("Error while closing Opentelemetry", e);
         }

diff --git a/...s/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelResourceProvider.java b/...s/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelResourceProvider.java
@@ -19,7 +19,6 @@
 import java.security.PrivilegedAction;
 import java.util.concurrent.TimeUnit;
 
-import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator;
 import io.opentelemetry.context.propagation.ContextPropagators;
@@ -47,11 +46,11 @@ private OTelResourceProvider() {}
      * Creates OpenTelemetry instance with default configuration
      * @param telemetrySettings telemetry settings
      * @param settings cluster settings
-     * @return OpenTelemetry instance
+     * @return OpenTelemetrySdk instance
      */
-    public static OpenTelemetry get(TelemetrySettings telemetrySettings, Settings settings) {
+    public static OpenTelemetrySdk get(TelemetrySettings telemetrySettings, Settings settings) {
         return AccessController.doPrivileged(
-            (PrivilegedAction<OpenTelemetry>) () -> get(
+            (PrivilegedAction<OpenTelemetrySdk>) () -> get(
                 settings,
                 OTelSpanExporterFactory.create(settings),
                 ContextPropagators.create(W3CTraceContextPropagator.getInstance()),
@@ -66,9 +65,14 @@ public static OpenTelemetry get(TelemetrySettings telemetrySettings, Settings se
      * @param spanExporter span exporter instance
      * @param contextPropagators context propagator instance
      * @param sampler sampler instance
-     * @return Opentelemetry instance
+     * @return OpenTelemetrySdk instance
      */
-    public static OpenTelemetry get(Settings settings, SpanExporter spanExporter, ContextPropagators contextPropagators, Sampler sampler) {
+    public static OpenTelemetrySdk get(
+        Settings settings,
+        SpanExporter spanExporter,
+        ContextPropagators contextPropagators,
+        Sampler sampler
+    ) {
         Resource resource = Resource.create(Attributes.of(ResourceAttributes.SERVICE_NAME, "OpenSearch"));
         SdkTracerProvider sdkTracerProvider = createSdkTracerProvider(settings, spanExporter, sampler, resource);
         SdkMeterProvider sdkMeterProvider = createSdkMetricProvider(settings, resource);

diff --git a/...s/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelTracingTelemetry.java b/...s/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelTracingTelemetry.java
@@ -13,7 +13,6 @@
 import org.opensearch.telemetry.OTelAttributesConverter;
 import org.opensearch.telemetry.OTelTelemetryPlugin;
 
-import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.sdk.OpenTelemetrySdk;
 
@@ -23,14 +22,14 @@
 public class OTelTracingTelemetry implements TracingTelemetry {
 
     private static final Logger logger = LogManager.getLogger(OTelTracingTelemetry.class);
-    private final OpenTelemetry openTelemetry;
+    private final OpenTelemetrySdk openTelemetry;
     private final io.opentelemetry.api.trace.Tracer otelTracer;
 
     /**
      * Creates OTel based Telemetry
      * @param openTelemetry OpenTelemetry instance
      */
-    public OTelTracingTelemetry(OpenTelemetry openTelemetry) {
+    public OTelTracingTelemetry(OpenTelemetrySdk openTelemetry) {
         this.openTelemetry = openTelemetry;
         this.otelTracer = openTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
 
@@ -39,7 +38,7 @@ public OTelTracingTelemetry(OpenTelemetry openTelemetry) {
     @Override
     public void close() {
         try {
-            ((OpenTelemetrySdk) openTelemetry).getSdkTracerProvider().close();
+            openTelemetry.getSdkTracerProvider().close();
         } catch (Exception e) {
             logger.warn("Error while closing Opentelemetry", e);
         }

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
@@ -32,7 +32,6 @@
 import static org.opensearch.telemetry.OTelTelemetrySettings.TRACER_EXPORTER_BATCH_SIZE_SETTING;
 import static org.opensearch.telemetry.OTelTelemetrySettings.TRACER_EXPORTER_DELAY_SETTING;
 import static org.opensearch.telemetry.OTelTelemetrySettings.TRACER_EXPORTER_MAX_QUEUE_SIZE_SETTING;
-import static org.opensearch.telemetry.TelemetrySettings.METRICS_ENABLED_SETTING;
 import static org.opensearch.telemetry.TelemetrySettings.TRACER_ENABLED_SETTING;
 import static org.opensearch.telemetry.TelemetrySettings.TRACER_SAMPLER_PROBABILITY;
 
@@ -51,10 +50,7 @@ public void setup() {
         Settings settings = Settings.builder().put(TRACER_EXPORTER_DELAY_SETTING.getKey(), "1s").build();
         oTelTelemetryModulePlugin = new OTelTelemetryPlugin(settings);
         telemetry = oTelTelemetryModulePlugin.getTelemetry(
-            new TelemetrySettings(
-                Settings.EMPTY,
-                new ClusterSettings(settings, Set.of(TRACER_ENABLED_SETTING, TRACER_SAMPLER_PROBABILITY, METRICS_ENABLED_SETTING))
-            )
+            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/...emetry-otel/src/test/java/org/opensearch/telemetry/metrics/OTelMetricsTelemetryTests.java b/...emetry-otel/src/test/java/org/opensearch/telemetry/metrics/OTelMetricsTelemetryTests.java
@@ -13,14 +13,14 @@
 import org.opensearch.telemetry.metrics.tags.Tags;
 import org.opensearch.test.OpenSearchTestCase;
 
-import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.api.metrics.DoubleCounter;
 import io.opentelemetry.api.metrics.DoubleCounterBuilder;
 import io.opentelemetry.api.metrics.DoubleUpDownCounter;
 import io.opentelemetry.api.metrics.DoubleUpDownCounterBuilder;
 import io.opentelemetry.api.metrics.LongCounterBuilder;
 import io.opentelemetry.api.metrics.LongUpDownCounterBuilder;
 import io.opentelemetry.api.metrics.Meter;
+import io.opentelemetry.sdk.OpenTelemetrySdk;
 
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
@@ -32,7 +32,7 @@ public void testCounter() {
         String counterName = "test-counter";
         String description = "test";
         String unit = "1";
-        OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
+        OpenTelemetrySdk mockOpenTelemetry = mock(OpenTelemetrySdk.class);
         Meter mockMeter = mock(Meter.class);
         DoubleCounter mockOTelDoubleCounter = mock(DoubleCounter.class);
         LongCounterBuilder mockOTelLongCounterBuilder = mock(LongCounterBuilder.class);
@@ -58,7 +58,7 @@ public void testCounterNegativeValue() {
         String counterName = "test-counter";
         String description = "test";
         String unit = "1";
-        OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
+        OpenTelemetrySdk mockOpenTelemetry = mock(OpenTelemetrySdk.class);
         Meter mockMeter = mock(Meter.class);
         DoubleCounter mockOTelDoubleCounter = mock(DoubleCounter.class);
         LongCounterBuilder mockOTelLongCounterBuilder = mock(LongCounterBuilder.class);
@@ -81,7 +81,7 @@ public void testUpDownCounter() {
         String counterName = "test-counter";
         String description = "test";
         String unit = "1";
-        OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
+        OpenTelemetrySdk mockOpenTelemetry = mock(OpenTelemetrySdk.class);
         Meter mockMeter = mock(Meter.class);
         DoubleUpDownCounter mockOTelUpDownDoubleCounter = mock(DoubleUpDownCounter.class);
         LongUpDownCounterBuilder mockOTelLongUpDownCounterBuilder = mock(LongUpDownCounterBuilder.class);

diff --git a/...emetry-otel/src/test/java/org/opensearch/telemetry/tracing/OTelTracingTelemetryTests.java b/...emetry-otel/src/test/java/org/opensearch/telemetry/tracing/OTelTracingTelemetryTests.java
@@ -15,10 +15,10 @@
 import java.util.Collections;
 import java.util.Map;
 
-import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.api.common.AttributesBuilder;
 import io.opentelemetry.api.trace.SpanBuilder;
 import io.opentelemetry.api.trace.Tracer;
+import io.opentelemetry.sdk.OpenTelemetrySdk;
 
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
@@ -29,7 +29,7 @@
 public class OTelTracingTelemetryTests extends OpenSearchTestCase {
 
     public void testCreateSpanWithoutParent() {
-        OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
+        OpenTelemetrySdk mockOpenTelemetry = mock(OpenTelemetrySdk.class);
         Tracer mockTracer = mock(Tracer.class);
         when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
         SpanBuilder mockSpanBuilder = mock(SpanBuilder.class);
@@ -47,7 +47,7 @@ public void testCreateSpanWithoutParent() {
     }
 
     public void testCreateSpanWithParent() {
-        OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
+        OpenTelemetrySdk mockOpenTelemetry = mock(OpenTelemetrySdk.class);
         Tracer mockTracer = mock(Tracer.class);
         when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
         SpanBuilder mockSpanBuilder = mock(SpanBuilder.class);
@@ -71,7 +71,7 @@ public void testCreateSpanWithParent() {
     }
 
     public void testCreateSpanWithParentWithMultipleAttributes() {
-        OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
+        OpenTelemetrySdk mockOpenTelemetry = mock(OpenTelemetrySdk.class);
         Tracer mockTracer = mock(Tracer.class);
         when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
         SpanBuilder mockSpanBuilder = mock(SpanBuilder.class);
@@ -117,7 +117,7 @@ private io.opentelemetry.api.common.Attributes createAttributeLong(Attributes at
     }
 
     public void testGetContextPropagator() {
-        OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
+        OpenTelemetrySdk mockOpenTelemetry = mock(OpenTelemetrySdk.class);
         Tracer mockTracer = mock(Tracer.class);
         when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
 

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
@@ -18,7 +18,6 @@
 import io.opentelemetry.sdk.trace.samplers.Sampler;
 
 import static org.opensearch.telemetry.OTelTelemetrySettings.TRACER_EXPORTER_DELAY_SETTING;
-import static org.opensearch.telemetry.TelemetrySettings.METRICS_ENABLED_SETTING;
 import static org.opensearch.telemetry.TelemetrySettings.TRACER_ENABLED_SETTING;
 import static org.opensearch.telemetry.TelemetrySettings.TRACER_SAMPLER_PROBABILITY;
 
@@ -34,7 +33,7 @@ public void testDefaultGetSampler() {
         Settings settings = Settings.builder().put(TRACER_EXPORTER_DELAY_SETTING.getKey(), "1s").build();
         TelemetrySettings telemetrySettings = new TelemetrySettings(
             Settings.EMPTY,
-            new ClusterSettings(settings, Set.of(TRACER_SAMPLER_PROBABILITY, TRACER_ENABLED_SETTING, METRICS_ENABLED_SETTING))
+            new ClusterSettings(settings, Set.of(TRACER_SAMPLER_PROBABILITY, TRACER_ENABLED_SETTING))
         );
 
         // Probabilistic Sampler
@@ -48,7 +47,7 @@ public void testGetSamplerWithUpdatedSamplingRatio() {
         Settings settings = Settings.builder().put(TRACER_EXPORTER_DELAY_SETTING.getKey(), "1s").build();
         TelemetrySettings telemetrySettings = new TelemetrySettings(
             Settings.EMPTY,
-            new ClusterSettings(settings, Set.of(TRACER_SAMPLER_PROBABILITY, TRACER_ENABLED_SETTING, METRICS_ENABLED_SETTING))
+            new ClusterSettings(settings, Set.of(TRACER_SAMPLER_PROBABILITY, TRACER_ENABLED_SETTING))
         );
 
         // Probabilistic Sampler

diff --git a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java
@@ -692,10 +692,6 @@ public void apply(Settings value, Settings current, Settings previous) {
             SearchService.CONCURRENT_SEGMENT_SEARCH_TARGET_MAX_SLICE_COUNT_SETTING
         ),
         List.of(FeatureFlags.TELEMETRY),
-        List.of(
-            TelemetrySettings.TRACER_ENABLED_SETTING,
-            TelemetrySettings.TRACER_SAMPLER_PROBABILITY,
-            TelemetrySettings.METRICS_ENABLED_SETTING
-        )
+        List.of(TelemetrySettings.TRACER_ENABLED_SETTING, TelemetrySettings.TRACER_SAMPLER_PROBABILITY)
     );
 }
diff --git a/server/src/main/java/org/opensearch/telemetry/TelemetrySettings.java b/server/src/main/java/org/opensearch/telemetry/TelemetrySettings.java
@@ -28,13 +28,6 @@ public class TelemetrySettings {
         Setting.Property.Dynamic
     );
 
-    public static final Setting<Boolean> METRICS_ENABLED_SETTING = Setting.boolSetting(
-        "telemetry.metrics.enabled",
-        false,
-        Setting.Property.NodeScope,
-        Setting.Property.Dynamic
-    );
-
     /**
      * Probability of sampler
      */
@@ -60,16 +53,12 @@ public class TelemetrySettings {
     private volatile boolean tracingEnabled;
     private volatile double samplingProbability;
 
-    private volatile boolean metricsEnabled;
-
     public TelemetrySettings(Settings settings, ClusterSettings clusterSettings) {
         this.tracingEnabled = TRACER_ENABLED_SETTING.get(settings);
         this.samplingProbability = TRACER_SAMPLER_PROBABILITY.get(settings);
-        this.metricsEnabled = METRICS_ENABLED_SETTING.get(settings);
 
         clusterSettings.addSettingsUpdateConsumer(TRACER_ENABLED_SETTING, this::setTracingEnabled);
         clusterSettings.addSettingsUpdateConsumer(TRACER_SAMPLER_PROBABILITY, this::setSamplingProbability);
-        clusterSettings.addSettingsUpdateConsumer(METRICS_ENABLED_SETTING, this::setMetricsEnabled);
     }
 
     public void setTracingEnabled(boolean tracingEnabled) {
@@ -94,21 +83,4 @@ public void setSamplingProbability(double samplingProbability) {
     public double getSamplingProbability() {
         return samplingProbability;
     }
-
-    /**
-     * update the metrics enabled property.
-     * @param metricsEnabled metrics enabled.
-     */
-    public void setMetricsEnabled(boolean metricsEnabled) {
-        this.metricsEnabled = metricsEnabled;
-    }
-
-    /**
-     * Returns whether metrics are enabled or not.
-     * @return enabled/disabled flag.
-     */
-    public boolean isMetricsEnabled() {
-        return metricsEnabled;
-    }
-
 }
diff --git a/server/src/main/java/org/opensearch/telemetry/metrics/MetricsRegistryFactory.java b/server/src/main/java/org/opensearch/telemetry/metrics/MetricsRegistryFactory.java
@@ -65,7 +65,6 @@ public void close() {
     private MetricsRegistry metricsRegistry(Optional<Telemetry> telemetry) {
         MetricsRegistry metricsRegistry = telemetry.map(Telemetry::getMetricsTelemetry)
             .map(metricsTelemetry -> createDefaultMetricsRegistry(metricsTelemetry))
-            .map(defaultMetricsRegistry -> createWrappedMetricsRegistry(defaultMetricsRegistry))
             .orElse(NoopMetricsRegistry.INSTANCE);
         return metricsRegistry;
     }
@@ -74,8 +73,4 @@ private MetricsRegistry createDefaultMetricsRegistry(MetricsTelemetry metricsTel
         return new DefaultMetricsRegistry(metricsTelemetry);
     }
 
-    private MetricsRegistry createWrappedMetricsRegistry(MetricsRegistry metricsRegistry) {
-        return new WrappedMetricsRegistry(telemetrySettings, metricsRegistry);
-    }
-
 }
diff --git a/server/src/main/java/org/opensearch/telemetry/metrics/WrappedCounter.java b/server/src/main/java/org/opensearch/telemetry/metrics/WrappedCounter.java