Skip to content

Commit

Permalink
Address review comment
Browse files Browse the repository at this point in the history 
Signed-off-by: Gagan Juneja <[email protected]>
  • Loading branch information
Gagan Juneja committed Oct 4, 2023
1 parent ba603a3 commit 9f7182d
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 44 deletions.
13 changes: 11 additions & 2 deletions plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/OTelTelemetryPlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
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
Expand Down Expand Up @@ -69,8 +71,15 @@ public String getName() {
private Telemetry telemetry(TelemetrySettings telemetrySettings) {
final OpenTelemetrySdk openTelemetry = OTelResourceProvider.get(telemetrySettings, settings);
return new OTelTelemetry(
new OTelTracingTelemetry(openTelemetry, () -> openTelemetry.getSdkTracerProvider().close()),
new OTelMetricsTelemetry(openTelemetry, () -> openTelemetry.getSdkMeterProvider().close())
new OTelTracingTelemetry<SdkTracerProvider>(
openTelemetry.getSdkTracerProvider(),
openTelemetry,
() -> openTelemetry.getSdkTracerProvider().close()
),
new OTelMetricsTelemetry<SdkMeterProvider>(
openTelemetry.getSdkMeterProvider(),
() -> openTelemetry.getSdkMeterProvider().close()
)
);
}

Expand Down
12 changes: 5 additions & 7 deletions ...s/telemetry-otel/src/main/java/org/opensearch/telemetry/metrics/OTelMetricsTelemetry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,28 +16,26 @@
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;
import io.opentelemetry.api.metrics.MeterProvider;

/**
* OTel implementation for {@link MetricsTelemetry}
*/
public class OTelMetricsTelemetry implements MetricsTelemetry {
public class OTelMetricsTelemetry<T extends MeterProvider & Closeable> implements MetricsTelemetry {
private static final Logger logger = LogManager.getLogger(OTelMetricsTelemetry.class);
private final OpenTelemetry openTelemetry;
private final Meter otelMeter;
private final Closeable metricsProviderClosable;

/**
* Creates OTel based {@link MetricsTelemetry}.
* @param openTelemetry OpenTelemetry instance
* @param meterProvider OpenTelemetry instance
* @param meterProviderCloseable closable to close the meter.
*/
public OTelMetricsTelemetry(OpenTelemetry openTelemetry, Closeable meterProviderCloseable) {
this.openTelemetry = openTelemetry;
this.otelMeter = openTelemetry.getMeter(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
public OTelMetricsTelemetry(T meterProvider, Closeable meterProviderCloseable) {
this.otelMeter = meterProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
this.metricsProviderClosable = meterProviderCloseable;
}

Expand Down
12 changes: 7 additions & 5 deletions ...s/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelTracingTelemetry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,16 @@
import org.opensearch.telemetry.OTelTelemetryPlugin;

import java.io.Closeable;
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 implements TracingTelemetry {
public class OTelTracingTelemetry<T extends TracerProvider & Closeable> implements TracingTelemetry {

private static final Logger logger = LogManager.getLogger(OTelTracingTelemetry.class);
private final OpenTelemetry openTelemetry;
Expand All @@ -30,20 +32,20 @@ public class OTelTracingTelemetry implements TracingTelemetry {

/**
* Creates OTel based {@link TracingTelemetry}
* @param openTelemetry OpenTelemetry instance
* @param tracerProvider OpenTelemetry instance
* @param tracerProviderCloseable closable to close the tracer
*/
public OTelTracingTelemetry(OpenTelemetry openTelemetry, Closeable tracerProviderCloseable) {
public OTelTracingTelemetry(T tracerProvider, OpenTelemetry openTelemetry, Closeable tracerProviderCloseable) {
this.otelTracer = tracerProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
this.openTelemetry = openTelemetry;
this.otelTracer = openTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME);
this.tracerProviderClosable = tracerProviderCloseable;
}

@Override
public void close() {
try {
tracerProviderClosable.close();
} catch (Exception e) {
} catch (IOException e) {
logger.warn("Error while closing Opentelemetry TracerProvider", e);
}
}
Expand Down
10 changes: 5 additions & 5 deletions plugins/telemetry-otel/src/test/java/org/opensearch/telemetry/OTelTelemetryPluginTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@

public class OTelTelemetryPluginTests extends OpenSearchTestCase {

private OTelTelemetryPlugin oTelTelemetryModulePlugin;
private OTelTelemetryPlugin oTelTelemetryPlugin;
private Optional<Telemetry> telemetry;
private TracingTelemetry tracingTelemetry;

Expand All @@ -48,8 +48,8 @@ public void setup() {
// TRACER_EXPORTER_DELAY_SETTING should always be less than 10 seconds because
// io.opentelemetry.sdk.OpenTelemetrySdk.close waits only for 10 seconds for shutdown to complete.
Settings settings = Settings.builder().put(TRACER_EXPORTER_DELAY_SETTING.getKey(), "1s").build();
oTelTelemetryModulePlugin = new OTelTelemetryPlugin(settings);
telemetry = oTelTelemetryModulePlugin.getTelemetry(
oTelTelemetryPlugin = new OTelTelemetryPlugin(settings);
telemetry = oTelTelemetryPlugin.getTelemetry(
new TelemetrySettings(Settings.EMPTY, new ClusterSettings(settings, Set.of(TRACER_ENABLED_SETTING, TRACER_SAMPLER_PROBABILITY)))
);
tracingTelemetry = telemetry.get().getTracingTelemetry();
Expand All @@ -59,7 +59,7 @@ public void setup() {
public void testGetTelemetry() {
Set<Setting<?>> allTracerSettings = new HashSet<>();
ClusterSettings.FEATURE_FLAGGED_CLUSTER_SETTINGS.get(List.of(FeatureFlags.TELEMETRY)).stream().forEach((allTracerSettings::add));
assertEquals(OTEL_TRACER_NAME, oTelTelemetryModulePlugin.getName());
assertEquals(OTEL_TRACER_NAME, oTelTelemetryPlugin.getName());
assertTrue(tracingTelemetry instanceof OTelTracingTelemetry);
assertTrue(metricsTelemetry instanceof OTelMetricsTelemetry);
assertEquals(
Expand All @@ -70,7 +70,7 @@ public void testGetTelemetry() {
OTEL_TRACER_SPAN_EXPORTER_CLASS_SETTING,
OTEL_METRICS_EXPORTER_CLASS_SETTING
),
oTelTelemetryModulePlugin.getSettings()
oTelTelemetryPlugin.getSettings()
);

}
Expand Down
25 changes: 12 additions & 13 deletions ...emetry-otel/src/test/java/org/opensearch/telemetry/metrics/OTelMetricsTelemetryTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@

import java.util.concurrent.atomic.AtomicBoolean;

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.api.metrics.MeterProvider;

import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
Expand All @@ -34,14 +34,13 @@ public void testCounter() {
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);

when(mockOpenTelemetry.getMeter(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockMeter);
MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(mockOpenTelemetry, () -> {});
MeterProvider meterProvider = mock(MeterProvider.class);
when(meterProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockMeter);
MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(meterProvider, () -> {});
when(mockMeter.counterBuilder(counterName)).thenReturn(mockOTelLongCounterBuilder);
when(mockOTelLongCounterBuilder.setDescription(description)).thenReturn(mockOTelLongCounterBuilder);
when(mockOTelLongCounterBuilder.setUnit(unit)).thenReturn(mockOTelLongCounterBuilder);
Expand All @@ -60,14 +59,14 @@ 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);

when(mockOpenTelemetry.getMeter(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockMeter);
MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(mockOpenTelemetry, () -> {});
MeterProvider meterProvider = mock(MeterProvider.class);
when(meterProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockMeter);
MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(meterProvider, () -> {});
when(mockMeter.counterBuilder(counterName)).thenReturn(mockOTelLongCounterBuilder);
when(mockOTelLongCounterBuilder.setDescription(description)).thenReturn(mockOTelLongCounterBuilder);
when(mockOTelLongCounterBuilder.setUnit(unit)).thenReturn(mockOTelLongCounterBuilder);
Expand All @@ -83,14 +82,14 @@ 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);

when(mockOpenTelemetry.getMeter(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockMeter);
MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(mockOpenTelemetry, () -> {});
MeterProvider meterProvider = mock(MeterProvider.class);
when(meterProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockMeter);
MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(meterProvider, () -> {});
when(mockMeter.upDownCounterBuilder(counterName)).thenReturn(mockOTelLongUpDownCounterBuilder);
when(mockOTelLongUpDownCounterBuilder.setDescription(description)).thenReturn(mockOTelLongUpDownCounterBuilder);
when(mockOTelLongUpDownCounterBuilder.setUnit(unit)).thenReturn(mockOTelLongUpDownCounterBuilder);
Expand All @@ -106,9 +105,9 @@ public void testUpDownCounter() {
}

public void testClose() {
OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
final AtomicBoolean closed = new AtomicBoolean(false);
MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(mockOpenTelemetry, () -> closed.set(true));
MeterProvider meterProvider = mock(MeterProvider.class);
MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry(meterProvider, () -> closed.set(true));
metricsTelemetry.close();
assertTrue(closed.get());
}
Expand Down
27 changes: 15 additions & 12 deletions ...emetry-otel/src/test/java/org/opensearch/telemetry/tracing/OTelTracingTelemetryTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,13 @@
import org.opensearch.telemetry.tracing.attributes.Attributes;
import org.opensearch.test.OpenSearchTestCase;

import java.util.Collections;
import java.util.Map;
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;
Expand All @@ -30,16 +29,16 @@
public class OTelTracingTelemetryTests extends OpenSearchTestCase {
public void testCreateSpanWithoutParent() {
OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
TracerProvider tracerProvider = mock(TracerProvider.class);
Tracer mockTracer = mock(Tracer.class);
when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
when(tracerProvider.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);
Map<String, String> attributeMap = Collections.singletonMap("name", "value");
Attributes attributes = Attributes.create().addAttribute("name", "value");
TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, () -> {});
TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(tracerProvider, mockOpenTelemetry, () -> {});
Span span = tracingTelemetry.createSpan(SpanCreationContext.internal().name("span_name").attributes(attributes), null);
verify(mockSpanBuilder, never()).setParent(any());
verify(mockSpanBuilder).setAllAttributes(createAttribute(attributes));
Expand All @@ -49,7 +48,8 @@ public void testCreateSpanWithoutParent() {
public void testCreateSpanWithParent() {
OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
Tracer mockTracer = mock(Tracer.class);
when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
TracerProvider tracerProvider = mock(TracerProvider.class);
when(tracerProvider.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);
Expand All @@ -59,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(tracerProvider, mockOpenTelemetry, () -> {});
Attributes attributes = Attributes.create().addAttribute("name", 1l);
Span span = tracingTelemetry.createSpan(SpanCreationContext.internal().name("span_name").attributes(attributes), parentSpan);

Expand All @@ -73,7 +73,8 @@ public void testCreateSpanWithParent() {
public void testCreateSpanWithParentWithMultipleAttributes() {
OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
Tracer mockTracer = mock(Tracer.class);
when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
TracerProvider tracerProvider = mock(TracerProvider.class);
when(tracerProvider.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);
Expand All @@ -83,7 +84,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(tracerProvider, mockOpenTelemetry, () -> {});
Attributes attributes = Attributes.create()
.addAttribute("key1", 1l)
.addAttribute("key2", 2.0)
Expand Down Expand Up @@ -119,17 +120,19 @@ private io.opentelemetry.api.common.Attributes createAttributeLong(Attributes at
public void testGetContextPropagator() {
OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
Tracer mockTracer = mock(Tracer.class);
when(mockOpenTelemetry.getTracer(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);
TracerProvider tracerProvider = mock(TracerProvider.class);
when(tracerProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockTracer);

TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, () -> {});
TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(tracerProvider, mockOpenTelemetry, () -> {});

assertTrue(tracingTelemetry.getContextPropagator() instanceof OTelTracingContextPropagator);
}

public void testClose() {
OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class);
TracerProvider tracerProvider = mock(TracerProvider.class);
final AtomicBoolean closed = new AtomicBoolean(false);
TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(mockOpenTelemetry, () -> closed.set(true));
TracingTelemetry tracingTelemetry = new OTelTracingTelemetry(tracerProvider, mockOpenTelemetry, () -> closed.set(true));
tracingTelemetry.close();
assertTrue(closed.get());
}
Expand Down

0 comments on commit 9f7182d

Please sign in to comment.