Address review comment

Signed-off-by: Gagan Juneja <[email protected]>
Gaganjuneja · Oct 4, 2023 · 1287c98 · 1287c98
1 parent 8dd61db
commit 1287c98
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 52 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
@@ -22,7 +22,6 @@
 import java.util.Optional;
 
 import io.opentelemetry.sdk.OpenTelemetrySdk;
-import io.opentelemetry.sdk.metrics.SdkMeterProvider;
 
 /**
  * Telemetry plugin based on Otel
@@ -70,11 +69,8 @@ public String getName() {
     private Telemetry telemetry(TelemetrySettings telemetrySettings) {
         final OpenTelemetrySdk openTelemetry = OTelResourceProvider.get(telemetrySettings, settings);
         return new OTelTelemetry(
-            new OTelTracingTelemetry<OpenTelemetrySdk>(openTelemetry, () -> openTelemetry.getSdkTracerProvider().close()),
-            new OTelMetricsTelemetry<SdkMeterProvider>(
-                openTelemetry.getSdkMeterProvider(),
-                () -> openTelemetry.getSdkMeterProvider().close()
-            )
+            new OTelTracingTelemetry<>(openTelemetry, openTelemetry.getSdkTracerProvider()),
+            new OTelMetricsTelemetry<>(openTelemetry.getSdkMeterProvider())
         );
     }
 

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
@@ -28,16 +28,15 @@
 public class OTelMetricsTelemetry<T extends MeterProvider & Closeable> implements MetricsTelemetry {
     private static final Logger logger = LogManager.getLogger(OTelMetricsTelemetry.class);
     private final Meter otelMeter;
-    private final Closeable metricsProviderClosable;
+    private final T meterProvider;
 
     /**
      * Creates OTel based {@link MetricsTelemetry}.
      * @param meterProvider {@link MeterProvider} instance
-     * @param meterProviderCloseable closable to close the meter.
      */
-    public OTelMetricsTelemetry(T meterProvider, Closeable meterProviderCloseable) {
+    public OTelMetricsTelemetry(T meterProvider) {
+        this.meterProvider = meterProvider;
         this.otelMeter = meterProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
-        this.metricsProviderClosable = meterProviderCloseable;
     }
 
     @Override
@@ -66,6 +65,6 @@ public Counter createUpDownCounter(String name, String description, String unit)
 
     @Override
     public void close() throws IOException {
-        metricsProviderClosable.close();
+        meterProvider.close();
     }
 }
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
@@ -17,32 +17,33 @@
 import java.io.IOException;
 
 import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.trace.TracerProvider;
 import io.opentelemetry.context.Context;
 
 /**
  * OTel based Telemetry provider
  */
-public class OTelTracingTelemetry<T extends OpenTelemetry & Closeable> implements TracingTelemetry {
+public class OTelTracingTelemetry<T extends TracerProvider & Closeable> implements TracingTelemetry {
 
     private static final Logger logger = LogManager.getLogger(OTelTracingTelemetry.class);
-    private final T openTelemetry;
-    private final Closeable tracerProviderClosable;
+    private final OpenTelemetry openTelemetry;
+    private final T tracerProvider;
     private final io.opentelemetry.api.trace.Tracer otelTracer;
 
     /**
      * Creates OTel based {@link TracingTelemetry}
      * @param openTelemetry OpenTelemetry instance
-     * @param tracerProviderCloseable closable to close the tracer
+     * @param tracerProvider {@link TracerProvider} instance.
      */
-    public OTelTracingTelemetry(T openTelemetry, Closeable tracerProviderCloseable) {
+    public OTelTracingTelemetry(OpenTelemetry openTelemetry, T tracerProvider) {
         this.openTelemetry = openTelemetry;
-        this.otelTracer = openTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
-        this.tracerProviderClosable = tracerProviderCloseable;
+        this.tracerProvider = tracerProvider;
+        this.otelTracer = tracerProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
     }
 
     @Override
     public void close() throws IOException {
-        tracerProviderClosable.close();
+        tracerProvider.close();
     }
 
     @Override

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,9 +13,6 @@
 import org.opensearch.telemetry.metrics.tags.Tags;
 import org.opensearch.test.OpenSearchTestCase;
 
-import java.io.IOException;
-import java.util.concurrent.atomic.AtomicBoolean;
-
 import io.opentelemetry.api.metrics.DoubleCounter;
 import io.opentelemetry.api.metrics.DoubleCounterBuilder;
 import io.opentelemetry.api.metrics.DoubleUpDownCounter;
@@ -31,6 +28,7 @@
 
 public class OTelMetricsTelemetryTests extends OpenSearchTestCase {
 
+    @SuppressWarnings({ "rawtypes", "unchecked" })
     public void testCounter() {
         String counterName = "test-counter";
         String description = "test";
@@ -41,7 +39,7 @@ public void testCounter() {
         DoubleCounterBuilder mockOTelDoubleCounterBuilder = mock(DoubleCounterBuilder.class);
         MeterProvider meterProvider = mock(MeterProvider.class);
         when(meterProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockMeter);
-        MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(meterProvider, () -> {});
+        MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(meterProvider);
         when(mockMeter.counterBuilder(counterName)).thenReturn(mockOTelLongCounterBuilder);
         when(mockOTelLongCounterBuilder.setDescription(description)).thenReturn(mockOTelLongCounterBuilder);
         when(mockOTelLongCounterBuilder.setUnit(unit)).thenReturn(mockOTelLongCounterBuilder);
@@ -56,6 +54,7 @@ public void testCounter() {
         verify(mockOTelDoubleCounter).add(2.0, OTelAttributesConverter.convert(tags));
     }
 
+    @SuppressWarnings({ "rawtypes", "unchecked" })
     public void testCounterNegativeValue() {
         String counterName = "test-counter";
         String description = "test";
@@ -67,7 +66,7 @@ public void testCounterNegativeValue() {
 
         MeterProvider meterProvider = mock(MeterProvider.class);
         when(meterProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockMeter);
-        MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(meterProvider, () -> {});
+        MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(meterProvider);
         when(mockMeter.counterBuilder(counterName)).thenReturn(mockOTelLongCounterBuilder);
         when(mockOTelLongCounterBuilder.setDescription(description)).thenReturn(mockOTelLongCounterBuilder);
         when(mockOTelLongCounterBuilder.setUnit(unit)).thenReturn(mockOTelLongCounterBuilder);
@@ -79,6 +78,7 @@ public void testCounterNegativeValue() {
         verify(mockOTelDoubleCounter).add(-1.0);
     }
 
+    @SuppressWarnings({ "rawtypes", "unchecked" })
     public void testUpDownCounter() {
         String counterName = "test-counter";
         String description = "test";
@@ -90,7 +90,7 @@ public void testUpDownCounter() {
 
         MeterProvider meterProvider = mock(MeterProvider.class);
         when(meterProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockMeter);
-        MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(meterProvider, () -> {});
+        MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(meterProvider);
         when(mockMeter.upDownCounterBuilder(counterName)).thenReturn(mockOTelLongUpDownCounterBuilder);
         when(mockOTelLongUpDownCounterBuilder.setDescription(description)).thenReturn(mockOTelLongUpDownCounterBuilder);
         when(mockOTelLongUpDownCounterBuilder.setUnit(unit)).thenReturn(mockOTelLongUpDownCounterBuilder);
@@ -104,12 +104,4 @@ public void testUpDownCounter() {
         counter.add(-2.0, tags);
         verify(mockOTelUpDownDoubleCounter).add((-2.0), OTelAttributesConverter.convert(tags));
     }
-
-    public void testClose() throws IOException {
-        final AtomicBoolean closed = new AtomicBoolean(false);
-        MeterProvider meterProvider = mock(MeterProvider.class);
-        MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(meterProvider, () -> closed.set(true));
-        metricsTelemetry.close();
-        assertTrue(closed.get());
-    }
 }
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
@@ -12,13 +12,11 @@
 import org.opensearch.telemetry.tracing.attributes.Attributes;
 import org.opensearch.test.OpenSearchTestCase;
 
-import java.io.IOException;
-import java.util.concurrent.atomic.AtomicBoolean;
-
 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.api.trace.TracerProvider;
 
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
@@ -27,27 +25,31 @@
 import static org.mockito.Mockito.when;
 
 public class OTelTracingTelemetryTests extends OpenSearchTestCase {
+    @SuppressWarnings({ "rawtypes", "unchecked" })
     public void testCreateSpanWithoutParent() {
         OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
         Tracer mockTracer = mock(Tracer.class);
-        when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
+        TracerProvider mockTracerProvider = mock(TracerProvider.class);
+        when(mockTracerProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
         SpanBuilder mockSpanBuilder = mock(SpanBuilder.class);
         when(mockTracer.spanBuilder("span_name")).thenReturn(mockSpanBuilder);
         when(mockSpanBuilder.setAllAttributes(any(io.opentelemetry.api.common.Attributes.class))).thenReturn(mockSpanBuilder);
         when(mockSpanBuilder.startSpan()).thenReturn(mock(io.opentelemetry.api.trace.Span.class));
         when(mockSpanBuilder.setSpanKind(any(io.opentelemetry.api.trace.SpanKind.class))).thenReturn(mockSpanBuilder);
         Attributes attributes = Attributes.create().addAttribute("name", "value");
-        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, () -> {});
+        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, mockTracerProvider);
         Span span = tracingTelemetry.createSpan(SpanCreationContext.internal().name("span_name").attributes(attributes), null);
         verify(mockSpanBuilder, never()).setParent(any());
         verify(mockSpanBuilder).setAllAttributes(createAttribute(attributes));
         assertNull(span.getParentSpan());
     }
 
+    @SuppressWarnings({ "rawtypes", "unchecked" })
     public void testCreateSpanWithParent() {
         OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
         Tracer mockTracer = mock(Tracer.class);
-        when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
+        TracerProvider mockTracerProvider = mock(TracerProvider.class);
+        when(mockTracerProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
         SpanBuilder mockSpanBuilder = mock(SpanBuilder.class);
         when(mockTracer.spanBuilder("span_name")).thenReturn(mockSpanBuilder);
         when(mockSpanBuilder.setParent(any())).thenReturn(mockSpanBuilder);
@@ -57,7 +59,7 @@ public void testCreateSpanWithParent() {
 
         Span parentSpan = new OTelSpan("parent_span", mock(io.opentelemetry.api.trace.Span.class), null);
 
-        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, () -> {});
+        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, mockTracerProvider);
         Attributes attributes = Attributes.create().addAttribute("name", 1l);
         Span span = tracingTelemetry.createSpan(SpanCreationContext.internal().name("span_name").attributes(attributes), parentSpan);
 
@@ -68,10 +70,12 @@ public void testCreateSpanWithParent() {
         assertEquals("parent_span", span.getParentSpan().getSpanName());
     }
 
+    @SuppressWarnings({ "rawtypes", "unchecked" })
     public void testCreateSpanWithParentWithMultipleAttributes() {
         OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
         Tracer mockTracer = mock(Tracer.class);
-        when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
+        TracerProvider mockTracerProvider = mock(TracerProvider.class);
+        when(mockTracerProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
         SpanBuilder mockSpanBuilder = mock(SpanBuilder.class);
         when(mockTracer.spanBuilder("span_name")).thenReturn(mockSpanBuilder);
         when(mockSpanBuilder.setParent(any())).thenReturn(mockSpanBuilder);
@@ -81,7 +85,7 @@ public void testCreateSpanWithParentWithMultipleAttributes() {
 
         Span parentSpan = new OTelSpan("parent_span", mock(io.opentelemetry.api.trace.Span.class), null);
 
-        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, () -> {});
+        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, mockTracerProvider);
         Attributes attributes = Attributes.create()
             .addAttribute("key1", 1l)
             .addAttribute("key2", 2.0)
@@ -114,22 +118,16 @@ private io.opentelemetry.api.common.Attributes createAttributeLong(Attributes at
         return attributesBuilder.build();
     }
 
+    @SuppressWarnings({ "rawtypes", "unchecked" })
     public void testGetContextPropagator() {
         OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
         Tracer mockTracer = mock(Tracer.class);
-        when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
+        TracerProvider mockTracerProvider = mock(TracerProvider.class);
+        when(mockTracerProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
 
-        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, () -> {});
+        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, mockTracerProvider);
 
         assertTrue(tracingTelemetry.getContextPropagator() instanceof OTelTracingContextPropagator);
     }
 
-    public void testClose() throws IOException {
-        OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
-        final AtomicBoolean closed = new AtomicBoolean(false);
-        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, () -> closed.set(true));
-        tracingTelemetry.close();
-        assertTrue(closed.get());
-    }
-
 }