Address review comment

Signed-off-by: Gagan Juneja <[email protected]>
Gaganjuneja · Oct 4, 2023 · 376191f · 376191f
1 parent ad95b57
commit 376191f
Show file tree

Hide file tree

Showing 8 changed files with 25 additions and 51 deletions.
diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/MetricsTelemetry.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/MetricsTelemetry.java
@@ -19,8 +19,5 @@
  */
 @ExperimentalApi
 public interface MetricsTelemetry extends MetricsRegistry, Closeable {
-    /**
-     * closes the resource
-     */
-    void close();
+
 }
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
@@ -35,9 +35,4 @@ public interface TracingTelemetry extends Closeable {
      */
     TracingContextPropagator getContextPropagator();
 
-    /**
-     * closes the resource
-     */
-    void close();
-
 }
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
@@ -23,7 +23,6 @@
 
 import io.opentelemetry.sdk.OpenTelemetrySdk;
 import io.opentelemetry.sdk.metrics.SdkMeterProvider;
-import io.opentelemetry.sdk.trace.SdkTracerProvider;
 
 /**
  * Telemetry plugin based on Otel
@@ -71,11 +70,7 @@ public String getName() {
     private Telemetry telemetry(TelemetrySettings telemetrySettings) {
         final OpenTelemetrySdk openTelemetry = OTelResourceProvider.get(telemetrySettings, settings);
         return new OTelTelemetry(
-            new OTelTracingTelemetry<SdkTracerProvider>(
-                openTelemetry.getSdkTracerProvider(),
-                openTelemetry,
-                () -> openTelemetry.getSdkTracerProvider().close()
-            ),
+            new OTelTracingTelemetry<OpenTelemetrySdk>(openTelemetry, () -> openTelemetry.getSdkTracerProvider().close()),
             new OTelMetricsTelemetry<SdkMeterProvider>(
                 openTelemetry.getSdkMeterProvider(),
                 () -> openTelemetry.getSdkMeterProvider().close()

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
@@ -65,11 +65,7 @@ public Counter createUpDownCounter(String name, String description, String unit)
     }
 
     @Override
-    public void close() {
-        try {
-            metricsProviderClosable.close();
-        } catch (IOException e) {
-            logger.warn("Error while closing Opentelemetry MeterProvider", e);
-        }
+    public void close() throws IOException {
+        metricsProviderClosable.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,38 +17,32 @@
 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 TracerProvider & Closeable> implements TracingTelemetry {
+public class OTelTracingTelemetry<T extends OpenTelemetry & Closeable> implements TracingTelemetry {
 
     private static final Logger logger = LogManager.getLogger(OTelTracingTelemetry.class);
-    private final OpenTelemetry openTelemetry;
+    private final T openTelemetry;
     private final Closeable tracerProviderClosable;
     private final io.opentelemetry.api.trace.Tracer otelTracer;
 
     /**
      * Creates OTel based {@link TracingTelemetry}
-     * @param tracerProvider {@link TracerProvider} instance.
      * @param openTelemetry OpenTelemetry instance
      * @param tracerProviderCloseable closable to close the tracer
      */
-    public OTelTracingTelemetry(T tracerProvider, OpenTelemetry openTelemetry, Closeable tracerProviderCloseable) {
-        this.otelTracer = tracerProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
+    public OTelTracingTelemetry(T openTelemetry, Closeable tracerProviderCloseable) {
         this.openTelemetry = openTelemetry;
+        this.otelTracer = openTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
         this.tracerProviderClosable = tracerProviderCloseable;
     }
 
     @Override
-    public void close() {
-        try {
-            tracerProviderClosable.close();
-        } catch (IOException e) {
-            logger.warn("Error while closing Opentelemetry TracerProvider", e);
-        }
+    public void close() throws IOException {
+        tracerProviderClosable.close();
     }
 
     @Override

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
@@ -20,6 +20,7 @@
 import org.junit.After;
 import org.junit.Before;
 
+import java.io.IOException;
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
@@ -76,7 +77,7 @@ public void testGetTelemetry() {
     }
 
     @After
-    public void cleanup() {
+    public void cleanup() throws IOException {
         tracingTelemetry.close();
         metricsTelemetry.close();
     }

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,6 +13,7 @@
 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;
@@ -104,7 +105,7 @@ public void testUpDownCounter() {
         verify(mockOTelUpDownDoubleCounter).add((-2.0), OTelAttributesConverter.convert(tags));
     }
 
-    public void testClose() {
+    public void testClose() throws IOException {
         final AtomicBoolean closed = new AtomicBoolean(false);
         MeterProvider meterProvider = mock(MeterProvider.class);
         MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(meterProvider, () -> closed.set(true));

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,13 @@
 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;
@@ -29,16 +29,15 @@
 public class OTelTracingTelemetryTests extends OpenSearchTestCase {
     public void testCreateSpanWithoutParent() {
         OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
-        TracerProvider tracerProvider = mock(TracerProvider.class);
         Tracer mockTracer = mock(Tracer.class);
-        when(tracerProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
+        when(mockOpenTelemetry.getTracer(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(tracerProvider, mockOpenTelemetry, () -> {});
+        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, () -> {});
         Span span = tracingTelemetry.createSpan(SpanCreationContext.internal().name("span_name").attributes(attributes), null);
         verify(mockSpanBuilder, never()).setParent(any());
         verify(mockSpanBuilder).setAllAttributes(createAttribute(attributes));
@@ -48,8 +47,7 @@ public void testCreateSpanWithoutParent() {
     public void testCreateSpanWithParent() {
         OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
         Tracer mockTracer = mock(Tracer.class);
-        TracerProvider tracerProvider = mock(TracerProvider.class);
-        when(tracerProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
+        when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
         SpanBuilder mockSpanBuilder = mock(SpanBuilder.class);
         when(mockTracer.spanBuilder("span_name")).thenReturn(mockSpanBuilder);
         when(mockSpanBuilder.setParent(any())).thenReturn(mockSpanBuilder);
@@ -59,7 +57,7 @@ public void testCreateSpanWithParent() {
 
         Span parentSpan = new OTelSpan("parent_span", mock(io.opentelemetry.api.trace.Span.class), null);
 
-        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(tracerProvider, mockOpenTelemetry, () -> {});
+        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, () -> {});
         Attributes attributes = Attributes.create().addAttribute("name", 1l);
         Span span = tracingTelemetry.createSpan(SpanCreationContext.internal().name("span_name").attributes(attributes), parentSpan);
 
@@ -73,8 +71,7 @@ public void testCreateSpanWithParent() {
     public void testCreateSpanWithParentWithMultipleAttributes() {
         OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
         Tracer mockTracer = mock(Tracer.class);
-        TracerProvider tracerProvider = mock(TracerProvider.class);
-        when(tracerProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
+        when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
         SpanBuilder mockSpanBuilder = mock(SpanBuilder.class);
         when(mockTracer.spanBuilder("span_name")).thenReturn(mockSpanBuilder);
         when(mockSpanBuilder.setParent(any())).thenReturn(mockSpanBuilder);
@@ -84,7 +81,7 @@ public void testCreateSpanWithParentWithMultipleAttributes() {
 
         Span parentSpan = new OTelSpan("parent_span", mock(io.opentelemetry.api.trace.Span.class), null);
 
-        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(tracerProvider, mockOpenTelemetry, () -> {});
+        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, () -> {});
         Attributes attributes = Attributes.create()
             .addAttribute("key1", 1l)
             .addAttribute("key2", 2.0)
@@ -120,19 +117,17 @@ private io.opentelemetry.api.common.Attributes createAttributeLong(Attributes at
     public void testGetContextPropagator() {
         OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
         Tracer mockTracer = mock(Tracer.class);
-        TracerProvider tracerProvider = mock(TracerProvider.class);
-        when(tracerProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
+        when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
 
-        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(tracerProvider, mockOpenTelemetry, () -> {});
+        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, () -> {});
 
         assertTrue(tracingTelemetry.getContextPropagator() instanceof OTelTracingContextPropagator);
     }
 
-    public void testClose() {
+    public void testClose() throws IOException {
         OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
-        TracerProvider tracerProvider = mock(TracerProvider.class);
         final AtomicBoolean closed = new AtomicBoolean(false);
-        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(tracerProvider, mockOpenTelemetry, () -> closed.set(true));
+        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, () -> closed.set(true));
         tracingTelemetry.close();
         assertTrue(closed.get());
     }