Address review comment

Signed-off-by: Gagan Juneja <gjjuneja@amazon.com>
opensearch-project · Oct 17, 2023 · c277a2b · c277a2b
1 parent d8c67a0
commit c277a2b
Show file tree

Hide file tree

Showing 6 changed files with 67 additions and 70 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
@@ -9,25 +9,18 @@
 package org.opensearch.telemetry;
 
 import org.opensearch.common.concurrent.RefCountedReleasable;
-import org.opensearch.common.lease.Releasable;
-import org.opensearch.common.lease.Releasables;
 import org.opensearch.common.settings.Setting;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.plugins.Plugin;
 import org.opensearch.plugins.TelemetryPlugin;
-import org.opensearch.telemetry.metrics.OTelMetricsTelemetry;
 import org.opensearch.telemetry.tracing.OTelResourceProvider;
 import org.opensearch.telemetry.tracing.OTelTelemetry;
-import org.opensearch.telemetry.tracing.OTelTracingTelemetry;
 
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
 
 import io.opentelemetry.sdk.OpenTelemetrySdk;
-import io.opentelemetry.sdk.metrics.SdkMeterProvider;
-import io.opentelemetry.sdk.trace.SdkTracerProvider;
 
 /**
  * Telemetry plugin based on Otel
@@ -45,8 +38,6 @@ public class OTelTelemetryPlugin extends Plugin implements TelemetryPlugin {
 
     private RefCountedReleasable<OpenTelemetrySdk> refCountedOpenTelemetry;
 
-    private List<Releasable> resourcesToRelease = new ArrayList<>();
-
     /**
      * Creates Otel plugin
      * @param settings cluster settings
@@ -68,41 +59,27 @@ public List<Setting<?>> getSettings() {
 
     @Override
     public Optional<Telemetry> getTelemetry(TelemetrySettings telemetrySettings) {
+        initializeOpenTelemetrySdk(telemetrySettings);
         return Optional.of(telemetry(telemetrySettings));
     }
 
+    private void initializeOpenTelemetrySdk(TelemetrySettings telemetrySettings) {
+        OpenTelemetrySdk openTelemetrySdk = OTelResourceProvider.get(telemetrySettings, settings);
+        refCountedOpenTelemetry = new RefCountedReleasable<>("openTelemetry", openTelemetrySdk, openTelemetrySdk::close);
+    }
+
     @Override
     public String getName() {
         return OTEL_TRACER_NAME;
     }
 
     private Telemetry telemetry(TelemetrySettings telemetrySettings) {
-        final OpenTelemetrySdk openTelemetry = OTelResourceProvider.get(telemetrySettings, settings);
-        final RefCountedReleasable<SdkTracerProvider> refCountedSdkTracerProvider = new RefCountedReleasable<>(
-            "tracerSdk",
-            openTelemetry.getSdkTracerProvider(),
-            openTelemetry.getSdkTracerProvider()::close
-        );
-        final RefCountedReleasable<SdkMeterProvider> refCountedSdkMeterProvider = new RefCountedReleasable<>(
-            "meterSdk",
-            openTelemetry.getSdkMeterProvider(),
-            openTelemetry.getSdkMeterProvider()::close
-        );
-
-        resourcesToRelease.add(refCountedSdkTracerProvider);
-        resourcesToRelease.add(refCountedSdkMeterProvider);
-
-        return new OTelTelemetry(
-            new OTelTracingTelemetry<>(openTelemetry, refCountedSdkTracerProvider),
-            new OTelMetricsTelemetry<>(refCountedSdkMeterProvider)
-        );
+        return new OTelTelemetry(refCountedOpenTelemetry);
     }
 
     @Override
     public void close() {
-        if (resourcesToRelease.isEmpty() == false) {
-            Releasables.close(resourcesToRelease);
-        }
+        refCountedOpenTelemetry.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
@@ -20,21 +20,25 @@
 import io.opentelemetry.api.metrics.DoubleUpDownCounter;
 import io.opentelemetry.api.metrics.Meter;
 import io.opentelemetry.api.metrics.MeterProvider;
+import io.opentelemetry.sdk.OpenTelemetrySdk;
 
 /**
  * OTel implementation for {@link MetricsTelemetry}
  */
 public class OTelMetricsTelemetry<T extends MeterProvider & Closeable> implements MetricsTelemetry {
+    private final RefCountedReleasable<OpenTelemetrySdk> refCountedOpenTelemetry;
     private final Meter otelMeter;
-    private final RefCountedReleasable<T> refCountedMeterProvider;
+    private final T meterProvider;
 
     /**
      * Creates OTel based {@link MetricsTelemetry}.
+     * @param openTelemetry open telemetry.
      * @param meterProvider {@link MeterProvider} instance
      */
-    public OTelMetricsTelemetry(RefCountedReleasable<T> meterProvider) {
-        this.refCountedMeterProvider = meterProvider;
-        this.otelMeter = meterProvider.get().get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
+    public OTelMetricsTelemetry(RefCountedReleasable<OpenTelemetrySdk> openTelemetry, T meterProvider) {
+        this.refCountedOpenTelemetry = openTelemetry;
+        this.meterProvider = meterProvider;
+        this.otelMeter = meterProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
     }
 
     @Override
@@ -63,8 +67,6 @@ public Counter createUpDownCounter(String name, String description, String unit)
 
     @Override
     public void close() throws IOException {
-        if (refCountedMeterProvider.tryIncRef()) {
-            refCountedMeterProvider.close();
-        }
+        refCountedOpenTelemetry.close();
     }
 }
diff --git a/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelTelemetry.java b/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelTelemetry.java
@@ -8,34 +8,41 @@
 
 package org.opensearch.telemetry.tracing;
 
+import org.opensearch.common.concurrent.RefCountedReleasable;
 import org.opensearch.telemetry.Telemetry;
 import org.opensearch.telemetry.metrics.MetricsTelemetry;
+import org.opensearch.telemetry.metrics.OTelMetricsTelemetry;
+
+import io.opentelemetry.sdk.OpenTelemetrySdk;
 
 /**
  * Otel implementation of Telemetry
  */
 public class OTelTelemetry implements Telemetry {
 
-    private final TracingTelemetry tracingTelemetry;
-    private final MetricsTelemetry metricsTelemetry;
+    private final RefCountedReleasable<OpenTelemetrySdk> refCountedOpenTelemetry;
 
     /**
      * Creates Telemetry instance
-     * @param tracingTelemetry tracing telemetry
-     * @param metricsTelemetry metrics telemetry
+
+     */
+    /**
+     * Creates Telemetry instance
+     * @param refCountedOpenTelemetry open telemetry.
      */
-    public OTelTelemetry(TracingTelemetry tracingTelemetry, MetricsTelemetry metricsTelemetry) {
-        this.tracingTelemetry = tracingTelemetry;
-        this.metricsTelemetry = metricsTelemetry;
+    public OTelTelemetry(RefCountedReleasable<OpenTelemetrySdk> refCountedOpenTelemetry) {
+        this.refCountedOpenTelemetry = refCountedOpenTelemetry;
     }
 
     @Override
     public TracingTelemetry getTracingTelemetry() {
-        return tracingTelemetry;
+        refCountedOpenTelemetry.incRef();
+        return new OTelTracingTelemetry<>(refCountedOpenTelemetry, refCountedOpenTelemetry.get().getSdkTracerProvider());
     }
 
     @Override
     public MetricsTelemetry getMetricsTelemetry() {
-        return metricsTelemetry;
+        refCountedOpenTelemetry.incRef();
+        return new OTelMetricsTelemetry<>(refCountedOpenTelemetry, refCountedOpenTelemetry.get().getSdkMeterProvider());
     }
 }
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
@@ -15,34 +15,32 @@
 import java.io.Closeable;
 import java.io.IOException;
 
-import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.api.trace.TracerProvider;
 import io.opentelemetry.context.Context;
+import io.opentelemetry.sdk.OpenTelemetrySdk;
 
 /**
  * OTel based Telemetry provider
  */
 public class OTelTracingTelemetry<T extends TracerProvider & Closeable> implements TracingTelemetry {
-    private final OpenTelemetry openTelemetry;
-    private final RefCountedReleasable<T> refCountedtTracerProvider;
+    private final RefCountedReleasable<OpenTelemetrySdk> refCountedOpenTelemetry;
+    private final T tracerProvider;
     private final io.opentelemetry.api.trace.Tracer otelTracer;
 
     /**
      * Creates OTel based {@link TracingTelemetry}
-     * @param openTelemetry OpenTelemetry instance
+     * @param refCountedOpenTelemetry OpenTelemetry instance
      * @param tracerProvider {@link TracerProvider} instance.
      */
-    public OTelTracingTelemetry(OpenTelemetry openTelemetry, RefCountedReleasable<T> tracerProvider) {
-        this.openTelemetry = openTelemetry;
-        this.refCountedtTracerProvider = tracerProvider;
-        this.otelTracer = tracerProvider.get().get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
+    public OTelTracingTelemetry(RefCountedReleasable<OpenTelemetrySdk> refCountedOpenTelemetry, T tracerProvider) {
+        this.refCountedOpenTelemetry = refCountedOpenTelemetry;
+        this.tracerProvider = tracerProvider;
+        this.otelTracer = tracerProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
     }
 
     @Override
     public void close() throws IOException {
-        if (refCountedtTracerProvider.tryIncRef()) {
-            refCountedtTracerProvider.close();
-        }
+        refCountedOpenTelemetry.close();
     }
 
     @Override
@@ -52,7 +50,7 @@ public Span createSpan(SpanCreationContext spanCreationContext, Span parentSpan)
 
     @Override
     public TracingContextPropagator getContextPropagator() {
-        return new OTelTracingContextPropagator(openTelemetry);
+        return new OTelTracingContextPropagator(refCountedOpenTelemetry.get());
     }
 
     private Span createOtelSpan(SpanCreationContext spanCreationContext, Span parentSpan) {

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
@@ -14,6 +14,7 @@
 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;
@@ -35,12 +36,16 @@ public void testCounter() {
         String description = "test";
         String unit = "1";
         Meter mockMeter = mock(Meter.class);
+        OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
         DoubleCounter mockOTelDoubleCounter = mock(DoubleCounter.class);
         LongCounterBuilder mockOTelLongCounterBuilder = mock(LongCounterBuilder.class);
         DoubleCounterBuilder mockOTelDoubleCounterBuilder = mock(DoubleCounterBuilder.class);
         MeterProvider meterProvider = mock(MeterProvider.class);
         when(meterProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockMeter);
-        MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(new RefCountedReleasable("meter-provide", meterProvider, () -> {}));
+        MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(
+            new RefCountedReleasable("telemetry", mockOpenTelemetry, () -> {}),
+            meterProvider
+        );
         when(mockMeter.counterBuilder(counterName)).thenReturn(mockOTelLongCounterBuilder);
         when(mockOTelLongCounterBuilder.setDescription(description)).thenReturn(mockOTelLongCounterBuilder);
         when(mockOTelLongCounterBuilder.setUnit(unit)).thenReturn(mockOTelLongCounterBuilder);
@@ -60,14 +65,18 @@ public void testCounterNegativeValue() {
         String counterName = "test-counter";
         String description = "test";
         String unit = "1";
+        OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
         Meter mockMeter = mock(Meter.class);
         DoubleCounter mockOTelDoubleCounter = mock(DoubleCounter.class);
         LongCounterBuilder mockOTelLongCounterBuilder = mock(LongCounterBuilder.class);
         DoubleCounterBuilder mockOTelDoubleCounterBuilder = mock(DoubleCounterBuilder.class);
 
         MeterProvider meterProvider = mock(MeterProvider.class);
         when(meterProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockMeter);
-        MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(new RefCountedReleasable("meter-provide", meterProvider, () -> {}));
+        MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(
+            new RefCountedReleasable("telemetry", mockOpenTelemetry, () -> {}),
+            meterProvider
+        );
         when(mockMeter.counterBuilder(counterName)).thenReturn(mockOTelLongCounterBuilder);
         when(mockOTelLongCounterBuilder.setDescription(description)).thenReturn(mockOTelLongCounterBuilder);
         when(mockOTelLongCounterBuilder.setUnit(unit)).thenReturn(mockOTelLongCounterBuilder);
@@ -84,14 +93,18 @@ public void testUpDownCounter() {
         String counterName = "test-counter";
         String description = "test";
         String unit = "1";
+        OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
         Meter mockMeter = mock(Meter.class);
         DoubleUpDownCounter mockOTelUpDownDoubleCounter = mock(DoubleUpDownCounter.class);
         LongUpDownCounterBuilder mockOTelLongUpDownCounterBuilder = mock(LongUpDownCounterBuilder.class);
         DoubleUpDownCounterBuilder mockOTelDoubleUpDownCounterBuilder = mock(DoubleUpDownCounterBuilder.class);
 
         MeterProvider meterProvider = mock(MeterProvider.class);
         when(meterProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockMeter);
-        MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(new RefCountedReleasable("meter-provide", meterProvider, () -> {}));
+        MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(
+            new RefCountedReleasable("telemetry", mockOpenTelemetry, () -> {}),
+            meterProvider
+        );
         when(mockMeter.upDownCounterBuilder(counterName)).thenReturn(mockOTelLongUpDownCounterBuilder);
         when(mockOTelLongUpDownCounterBuilder.setDescription(description)).thenReturn(mockOTelLongUpDownCounterBuilder);
         when(mockOTelLongUpDownCounterBuilder.setUnit(unit)).thenReturn(mockOTelLongUpDownCounterBuilder);

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
@@ -39,8 +39,8 @@ public void testCreateSpanWithoutParent() {
         when(mockSpanBuilder.setSpanKind(any(io.opentelemetry.api.trace.SpanKind.class))).thenReturn(mockSpanBuilder);
         Attributes attributes = Attributes.create().addAttribute("name", "value");
         TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(
-            mockOpenTelemetry,
-            new RefCountedReleasable("tracing-provide", mockTracerProvider, () -> {})
+            new RefCountedReleasable("telemetry", mockOpenTelemetry, () -> {}),
+            mockTracerProvider
         );
         Span span = tracingTelemetry.createSpan(SpanCreationContext.internal().name("span_name").attributes(attributes), null);
         verify(mockSpanBuilder, never()).setParent(any());
@@ -64,8 +64,8 @@ public void testCreateSpanWithParent() {
         Span parentSpan = new OTelSpan("parent_span", mock(io.opentelemetry.api.trace.Span.class), null);
 
         TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(
-            mockOpenTelemetry,
-            new RefCountedReleasable("tracing-provide", mockTracerProvider, () -> {})
+            new RefCountedReleasable("telemetry", mockOpenTelemetry, () -> {}),
+            mockTracerProvider
         );
         Attributes attributes = Attributes.create().addAttribute("name", 1l);
         Span span = tracingTelemetry.createSpan(SpanCreationContext.internal().name("span_name").attributes(attributes), parentSpan);
@@ -93,8 +93,8 @@ public void testCreateSpanWithParentWithMultipleAttributes() {
         Span parentSpan = new OTelSpan("parent_span", mock(io.opentelemetry.api.trace.Span.class), null);
 
         TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(
-            mockOpenTelemetry,
-            new RefCountedReleasable("tracing-provide", mockTracerProvider, () -> {})
+            new RefCountedReleasable("telemetry", mockOpenTelemetry, () -> {}),
+            mockTracerProvider
         );
         Attributes attributes = Attributes.create()
             .addAttribute("key1", 1l)
@@ -136,8 +136,8 @@ public void testGetContextPropagator() {
         when(mockTracerProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
 
         TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(
-            mockOpenTelemetry,
-            new RefCountedReleasable("tracing-provide", mockTracerProvider, () -> {})
+            new RefCountedReleasable("telemetry", mockOpenTelemetry, () -> {}),
+            mockTracerProvider
         );
 
         assertTrue(tracingTelemetry.getContextPropagator() instanceof OTelTracingContextPropagator);