Address PR comments

Signed-off-by: suranjay <surajkumar.tu@gmail.com>
suranjay · Jun 26, 2023 · 59b54df · 59b54df
1 parent c15cc62
commit 59b54df
Show file tree

Hide file tree

Showing 11 changed files with 55 additions and 42 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
@@ -106,7 +106,7 @@ private void setCurrentSpanInContext(Span span) {
         tracerContextStorage.put(CURRENT_SPAN, span);
     }
 
-    private void addDefaultAttributes(Span span) {
+    protected void addDefaultAttributes(Span span) {
         span.addAttribute(THREAD_NAME, Thread.currentThread().getName());
     }
 

diff --git a/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/OTelTelemetryModulePlugin.java b/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/OTelTelemetryModulePlugin.java
@@ -15,8 +15,8 @@
 import org.opensearch.plugins.TelemetryPlugin;
 import org.opensearch.telemetry.metrics.MetricsTelemetry;
 import org.opensearch.telemetry.tracing.OTelResourceProvider;
-import org.opensearch.telemetry.tracing.OtelTelemetryImpl;
-import org.opensearch.telemetry.tracing.OtelTracingTelemetry;
+import org.opensearch.telemetry.tracing.OTelTelemetry;
+import org.opensearch.telemetry.tracing.OTelTracingTelemetry;
 
 import java.util.Arrays;
 import java.util.List;
@@ -85,7 +85,7 @@ public String getName() {
     }
 
     private Telemetry telemetry() {
-        return new OtelTelemetryImpl(new OtelTracingTelemetry(OTelResourceProvider.get(settings)), new MetricsTelemetry() {
+        return new OTelTelemetry(new OTelTracingTelemetry(OTelResourceProvider.get(settings)), new MetricsTelemetry() {
         });
     }
 

diff --git a/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelSpan.java b/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelSpan.java
@@ -16,55 +16,55 @@
  */
 class OTelSpan extends AbstractSpan {
 
-    private final Span otelSpan;
+    private final Span oTelSpan;
 
     public OTelSpan(String spanName, Span span, org.opensearch.telemetry.tracing.Span parentSpan) {
         super(spanName, parentSpan);
-        this.otelSpan = span;
+        this.oTelSpan = span;
     }
 
     @Override
     public void endSpan() {
-        otelSpan.end();
+        oTelSpan.end();
     }
 
     @Override
     public void addAttribute(String key, String value) {
-        otelSpan.setAttribute(key, value);
+        oTelSpan.setAttribute(key, value);
     }
 
     @Override
     public void addAttribute(String key, Long value) {
-        otelSpan.setAttribute(key, value);
+        oTelSpan.setAttribute(key, value);
     }
 
     @Override
     public void addAttribute(String key, Double value) {
-        otelSpan.setAttribute(key, value);
+        oTelSpan.setAttribute(key, value);
     }
 
     @Override
     public void addAttribute(String key, Boolean value) {
-        otelSpan.setAttribute(key, value);
+        oTelSpan.setAttribute(key, value);
     }
 
     @Override
     public void addEvent(String event) {
-        otelSpan.addEvent(event);
+        oTelSpan.addEvent(event);
     }
 
     @Override
     public String getTraceId() {
-        return otelSpan.getSpanContext().getTraceId();
+        return oTelSpan.getSpanContext().getTraceId();
     }
 
     @Override
     public String getSpanId() {
-        return otelSpan.getSpanContext().getSpanId();
+        return oTelSpan.getSpanContext().getSpanId();
     }
 
-    io.opentelemetry.api.trace.Span getOtelSpan() {
-        return otelSpan;
+    io.opentelemetry.api.trace.Span getoTelSpan() {
+        return oTelSpan;
     }
 
 }
diff --git a/.../telemetry/tracing/OtelTelemetryImpl.java → ...arch/telemetry/tracing/OTelTelemetry.java b/.../telemetry/tracing/OtelTelemetryImpl.java → ...arch/telemetry/tracing/OTelTelemetry.java
@@ -14,7 +14,7 @@
 /**
  * Otel implementation of Telemetry
  */
-public class OtelTelemetryImpl implements Telemetry {
+public class OTelTelemetry implements Telemetry {
 
     private final TracingTelemetry tracingTelemetry;
     private final MetricsTelemetry metricsTelemetry;
@@ -24,7 +24,7 @@ public class OtelTelemetryImpl implements Telemetry {
      * @param tracingTelemetry tracing telemetry
      * @param metricsTelemetry metrics telemetry
      */
-    public OtelTelemetryImpl(TracingTelemetry tracingTelemetry, MetricsTelemetry metricsTelemetry) {
+    public OTelTelemetry(TracingTelemetry tracingTelemetry, MetricsTelemetry metricsTelemetry) {
         this.tracingTelemetry = tracingTelemetry;
         this.metricsTelemetry = metricsTelemetry;
     }

diff --git a/...tracing/OtelTracingContextPropagator.java → ...tracing/OTelTracingContextPropagator.java b/...tracing/OtelTracingContextPropagator.java → ...tracing/OTelTracingContextPropagator.java
@@ -19,15 +19,15 @@
 /**
  * Otel implementation of TracingContextPropagator
  */
-public class OtelTracingContextPropagator implements TracingContextPropagator {
+public class OTelTracingContextPropagator implements TracingContextPropagator {
 
     private final OpenTelemetry openTelemetry;
 
     /**
-     * Creates OtelTracingContextPropagator instance
+     * Creates OTelTracingContextPropagator instance
      * @param openTelemetry Otel OpenTelemetry instance
      */
-    public OtelTracingContextPropagator(OpenTelemetry openTelemetry) {
+    public OTelTracingContextPropagator(OpenTelemetry openTelemetry) {
         this.openTelemetry = openTelemetry;
     }
 
@@ -48,7 +48,7 @@ public void inject(Span currentSpan, BiConsumer<String, String> setter) {
     }
 
     private static Context context(OTelSpan oTelSpan) {
-        return Context.current().with(io.opentelemetry.api.trace.Span.wrap(oTelSpan.getOtelSpan().getSpanContext()));
+        return Context.current().with(io.opentelemetry.api.trace.Span.wrap(oTelSpan.getoTelSpan().getSpanContext()));
     }
 
     private static final TextMapSetter<BiConsumer<String, String>> TEXT_MAP_SETTER = (carrier, key, value) -> {

diff --git a/...lemetry/tracing/OtelTracingTelemetry.java → ...lemetry/tracing/OTelTracingTelemetry.java b/...lemetry/tracing/OtelTracingTelemetry.java → ...lemetry/tracing/OTelTracingTelemetry.java
@@ -19,9 +19,9 @@
 /**
  * OTel based Telemetry provider
  */
-public class OtelTracingTelemetry implements TracingTelemetry {
+public class OTelTracingTelemetry implements TracingTelemetry {
 
-    private static final Logger logger = LogManager.getLogger(OtelTracingTelemetry.class);
+    private static final Logger logger = LogManager.getLogger(OTelTracingTelemetry.class);
 
     private final OpenTelemetry openTelemetry;
     private final io.opentelemetry.api.trace.Tracer otelTracer;
@@ -30,7 +30,7 @@ public class OtelTracingTelemetry implements TracingTelemetry {
      * Creates OTel based Telemetry
      * @param openTelemetry OpenTelemetry instance
      */
-    public OtelTracingTelemetry(OpenTelemetry openTelemetry) {
+    public OTelTracingTelemetry(OpenTelemetry openTelemetry) {
         this.openTelemetry = openTelemetry;
         this.otelTracer = openTelemetry.getTracer("os-tracer");
 
@@ -52,7 +52,7 @@ public Span createSpan(String spanName, Span parentSpan) {
 
     @Override
     public TracingContextPropagator getContextPropagator() {
-        return new OtelTracingContextPropagator(openTelemetry);
+        return new OTelTracingContextPropagator(openTelemetry);
     }
 
     private Span createOtelSpan(String spanName, Span parentSpan) {
@@ -63,6 +63,6 @@ private Span createOtelSpan(String spanName, Span parentSpan) {
     io.opentelemetry.api.trace.Span otelSpan(String spanName, Span parentOTelSpan) {
         return parentOTelSpan == null || !(parentOTelSpan instanceof OTelSpan)
             ? otelTracer.spanBuilder(spanName).startSpan()
-            : otelTracer.spanBuilder(spanName).setParent(Context.current().with(((OTelSpan) parentOTelSpan).getOtelSpan())).startSpan();
+            : otelTracer.spanBuilder(spanName).setParent(Context.current().with(((OTelSpan) parentOTelSpan).getoTelSpan())).startSpan();
     }
 }
diff --git a/...telemetry-otel/src/test/java/org/opensearch/telemetry/OTelTelemetryModulePluginTests.java b/...telemetry-otel/src/test/java/org/opensearch/telemetry/OTelTelemetryModulePluginTests.java
@@ -12,16 +12,20 @@
 import org.opensearch.common.settings.Setting;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.util.FeatureFlags;
-import org.opensearch.telemetry.tracing.OtelTracingTelemetry;
+import org.opensearch.telemetry.tracing.OTelTracingTelemetry;
 import org.opensearch.telemetry.tracing.TracingTelemetry;
 import org.opensearch.test.OpenSearchTestCase;
 
+import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
 import java.util.Optional;
 
 import static org.opensearch.telemetry.OTelTelemetryModulePlugin.OTEL_TRACER_NAME;
+import static org.opensearch.telemetry.OTelTelemetryModulePlugin.TRACER_EXPORTER_BATCH_SIZE_SETTING;
+import static org.opensearch.telemetry.OTelTelemetryModulePlugin.TRACER_EXPORTER_DELAY_SETTING;
+import static org.opensearch.telemetry.OTelTelemetryModulePlugin.TRACER_EXPORTER_MAX_QUEUE_SIZE_SETTING;
 
 public class OTelTelemetryModulePluginTests extends OpenSearchTestCase {
 
@@ -34,7 +38,11 @@ public void testGetTelemetry() {
 
         assertEquals(OTEL_TRACER_NAME, oTelTracerModulePlugin.getName());
         TracingTelemetry tracingTelemetry = tracer.get().getTracingTelemetry();
-        assertTrue(tracingTelemetry instanceof OtelTracingTelemetry);
+        assertTrue(tracingTelemetry instanceof OTelTracingTelemetry);
+        assertEquals(
+            Arrays.asList(TRACER_EXPORTER_BATCH_SIZE_SETTING, TRACER_EXPORTER_DELAY_SETTING, TRACER_EXPORTER_MAX_QUEUE_SIZE_SETTING),
+            oTelTracerModulePlugin.getSettings()
+        );
         tracingTelemetry.close();
 
     }

diff --git a/...ng/OtelTracingContextPropagatorTests.java → ...ng/OTelTracingContextPropagatorTests.java b/...ng/OtelTracingContextPropagatorTests.java → ...ng/OTelTracingContextPropagatorTests.java
@@ -23,7 +23,7 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-public class OtelTracingContextPropagatorTests extends OpenSearchTestCase {
+public class OTelTracingContextPropagatorTests extends OpenSearchTestCase {
 
     private static final String TRACE_ID = "4aa59968f31dcbff7807741afa9d7d62";
     private static final String SPAN_ID = "bea205cd25756b5e";
@@ -35,7 +35,7 @@ public void testAddTracerContextToHeader() {
         Map<String, String> requestHeaders = new HashMap<>();
         OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
         when(mockOpenTelemetry.getPropagators()).thenReturn(ContextPropagators.create(W3CTraceContextPropagator.getInstance()));
-        TracingContextPropagator tracingContextPropagator = new OtelTracingContextPropagator(mockOpenTelemetry);
+        TracingContextPropagator tracingContextPropagator = new OTelTracingContextPropagator(mockOpenTelemetry);
 
         tracingContextPropagator.inject(span, (key, value) -> requestHeaders.put(key, value));
         assertEquals("00-" + TRACE_ID + "-" + SPAN_ID + "-00", requestHeaders.get("traceparent"));
@@ -46,7 +46,7 @@ public void testExtractTracerContextFromHeader() {
         requestHeaders.put("traceparent", "00-" + TRACE_ID + "-" + SPAN_ID + "-00");
         OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
         when(mockOpenTelemetry.getPropagators()).thenReturn(ContextPropagators.create(W3CTraceContextPropagator.getInstance()));
-        TracingContextPropagator tracingContextPropagator = new OtelTracingContextPropagator(mockOpenTelemetry);
+        TracingContextPropagator tracingContextPropagator = new OTelTracingContextPropagator(mockOpenTelemetry);
         org.opensearch.telemetry.tracing.Span span = tracingContextPropagator.extract(requestHeaders);
         assertEquals(TRACE_ID, span.getTraceId());
         assertEquals(SPAN_ID, span.getSpanId());

diff --git a/...ry/tracing/OtelTracingTelemetryTests.java → ...ry/tracing/OTelTracingTelemetryTests.java b/...ry/tracing/OtelTracingTelemetryTests.java → ...ry/tracing/OTelTracingTelemetryTests.java
@@ -19,7 +19,7 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
-public class OtelTracingTelemetryTests extends OpenSearchTestCase {
+public class OTelTracingTelemetryTests extends OpenSearchTestCase {
 
     public void testCreateSpanWithoutParent() {
         OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
@@ -29,7 +29,7 @@ public void testCreateSpanWithoutParent() {
         when(mockTracer.spanBuilder("span_name")).thenReturn(mockSpanBuilder);
         when(mockSpanBuilder.startSpan()).thenReturn(mock(io.opentelemetry.api.trace.Span.class));
 
-        TracingTelemetry tracingTelemetry = new OtelTracingTelemetry(mockOpenTelemetry);
+        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry);
         Span span = tracingTelemetry.createSpan("span_name", null);
 
         verify(mockSpanBuilder, never()).setParent(any());
@@ -47,12 +47,22 @@ 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);
         Span span = tracingTelemetry.createSpan("span_name", parentSpan);
 
         verify(mockSpanBuilder).setParent(any());
         assertNotNull(span.getParentSpan());
         assertEquals("parent_span", span.getParentSpan().getSpanName());
     }
 
+    public void testGetContextPropagator() {
+        OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
+        Tracer mockTracer = mock(Tracer.class);
+        when(mockOpenTelemetry.getTracer("os-tracer")).thenReturn(mockTracer);
+
+        TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry);
+
+        assertTrue(tracingTelemetry.getContextPropagator() instanceof OTelTracingContextPropagator);
+    }
+
 }
diff --git a/server/src/main/java/org/opensearch/telemetry/TelemetryModule.java b/server/src/main/java/org/opensearch/telemetry/TelemetryModule.java
@@ -33,12 +33,7 @@ public TelemetryModule(List<TelemetryPlugin> telemetryPlugins, TelemetrySettings
     }
 
     public Optional<Telemetry> getTelemetry() {
-        // if only default(Otel) telemetry is registered, return it
-        if (telemetry != null) {
-            return Optional.of(telemetry);
-        } else {
-            return Optional.empty();
-        }
+        return Optional.ofNullable(telemetry);
     }
 
     private void registerTelemetry(Telemetry factory) {

diff --git a/server/src/main/java/org/opensearch/telemetry/tracing/TracerFactory.java b/server/src/main/java/org/opensearch/telemetry/tracing/TracerFactory.java
@@ -79,7 +79,7 @@ private Tracer getOrCreateDefaultTracerInstance() {
                 if (defaultTracer == null) {
                     logger.info("Creating default tracer...");
                     TracingTelemetry tracingTelemetry = telemetry.get().getTracingTelemetry();
-                    TracerContextStorage tracerContextStorage = new ThreadContextBasedTracerContextStorage(
+                    TracerContextStorage<String, Span> tracerContextStorage = new ThreadContextBasedTracerContextStorage(
                         threadPool.getThreadContext(),
                         tracingTelemetry
                     );