From 38eb4f336fac951f08210b2f5e0c927ac3ebfb5c Mon Sep 17 00:00:00 2001 From: Gagan Juneja Date: Mon, 5 Feb 2024 21:14:06 +0530 Subject: [PATCH] Removes explicit approach Signed-off-by: Gagan Juneja --- .../metrics/DefaultMetricsRegistry.java | 6 ---- .../telemetry/metrics/MetricsRegistry.java | 13 -------- .../metrics/noop/NoopMetricsRegistry.java | 6 ---- .../metrics/DefaultMetricsRegistryTests.java | 23 ------------- .../TelemetryMetricsDisabledSanityIT.java | 3 -- .../TelemetryMetricsEnabledSanityIT.java | 28 ---------------- .../metrics/OTelMetricsTelemetry.java | 13 -------- .../metrics/OTelMetricsTelemetryTests.java | 33 ------------------- .../test/telemetry/MockTelemetry.java | 7 ---- 9 files changed, 132 deletions(-) diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/DefaultMetricsRegistry.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/DefaultMetricsRegistry.java index 56e78ec1be95c..f38fdd6412d79 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/DefaultMetricsRegistry.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/DefaultMetricsRegistry.java @@ -9,7 +9,6 @@ package org.opensearch.telemetry.metrics; import java.io.IOException; -import java.util.List; /** * Default implementation for {@link MetricsRegistry} @@ -40,11 +39,6 @@ public Histogram createHistogram(String name, String description, String unit) { return metricsTelemetry.createHistogram(name, description, unit); } - @Override - public Histogram createHistogram(String name, String description, String unit, List buckets) { - return metricsTelemetry.createHistogram(name, description, unit, buckets); - } - @Override public void close() throws IOException { metricsTelemetry.close(); diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/MetricsRegistry.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/MetricsRegistry.java index fdb9ab4dcd2c8..94d19bda31f34 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/MetricsRegistry.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/MetricsRegistry.java @@ -11,7 +11,6 @@ import org.opensearch.common.annotation.ExperimentalApi; import java.io.Closeable; -import java.util.List; /** * MetricsRegistry helps in creating the metric instruments. @@ -48,16 +47,4 @@ public interface MetricsRegistry extends Closeable { * @return histogram. */ Histogram createHistogram(String name, String description, String unit); - - /** - * Creates the histogram type of Metric with the explicit buckets defined. This should be used only - * in the scenario where user is very much aware of the buckets otherwise it's advised to use the - * default api. - * @param name name of the histogram. - * @param description any description about the metric. - * @param unit unit of the metric. - * @param buckets list of explicit histogram buckets. - * @return histogram. - */ - Histogram createHistogram(String name, String description, String unit, List buckets); } diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/noop/NoopMetricsRegistry.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/noop/NoopMetricsRegistry.java index 048f754101565..d3dda68cfae71 100644 --- a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/noop/NoopMetricsRegistry.java +++ b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/noop/NoopMetricsRegistry.java @@ -14,7 +14,6 @@ import org.opensearch.telemetry.metrics.MetricsRegistry; import java.io.IOException; -import java.util.List; /** *No-op {@link MetricsRegistry} @@ -45,11 +44,6 @@ public Histogram createHistogram(String name, String description, String unit) { return NoopHistogram.INSTANCE; } - @Override - public Histogram createHistogram(String name, String description, String unit, List buckets) { - return NoopHistogram.INSTANCE; - } - @Override public void close() throws IOException { diff --git a/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/DefaultMetricsRegistryTests.java b/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/DefaultMetricsRegistryTests.java index 3a69800dcc5cd..02f126075845b 100644 --- a/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/DefaultMetricsRegistryTests.java +++ b/libs/telemetry/src/test/java/org/opensearch/telemetry/metrics/DefaultMetricsRegistryTests.java @@ -10,10 +10,6 @@ import org.opensearch.test.OpenSearchTestCase; -import java.util.Arrays; - -import org.mockito.ArgumentMatchers; - import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -63,23 +59,4 @@ public void testHistogram() { assertSame(mockHistogram, histogram); } - public void testHistogramWithExplicitBuckets() { - Histogram mockHistogram = mock(Histogram.class); - when( - defaultMeterRegistry.createHistogram( - any(String.class), - any(String.class), - any(String.class), - ArgumentMatchers.anyList() - ) - ).thenReturn(mockHistogram); - Histogram histogram = defaultMeterRegistry.createHistogram( - "org.opensearch.telemetry.metrics.DefaultMeterRegistryTests.testHistogram", - "test up-down counter", - "ms", - Arrays.asList(1.0, 2.0) - ); - assertSame(mockHistogram, histogram); - } - } diff --git a/plugins/telemetry-otel/src/internalClusterTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsDisabledSanityIT.java b/plugins/telemetry-otel/src/internalClusterTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsDisabledSanityIT.java index 03de897ea3823..e77e69d121036 100644 --- a/plugins/telemetry-otel/src/internalClusterTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsDisabledSanityIT.java +++ b/plugins/telemetry-otel/src/internalClusterTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsDisabledSanityIT.java @@ -56,14 +56,11 @@ public void testSanityChecksWhenMetricsDisabled() throws Exception { Histogram histogram = metricsRegistry.createHistogram("test-histogram", "test", "1"); - Histogram histogramWithBuckets = metricsRegistry.createHistogram("test-histogram", "test", "1", Arrays.asList(1.0)); - Thread.sleep(2000); assertTrue(metricsRegistry instanceof NoopMetricsRegistry); assertTrue(counter instanceof NoopCounter); assertTrue(histogram instanceof NoopHistogram); - assertTrue(histogramWithBuckets instanceof NoopHistogram); } } diff --git a/plugins/telemetry-otel/src/internalClusterTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsEnabledSanityIT.java b/plugins/telemetry-otel/src/internalClusterTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsEnabledSanityIT.java index 0a6c0954554d7..1b8f694709a9c 100644 --- a/plugins/telemetry-otel/src/internalClusterTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsEnabledSanityIT.java +++ b/plugins/telemetry-otel/src/internalClusterTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsEnabledSanityIT.java @@ -20,12 +20,10 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; -import java.util.List; import java.util.stream.Collectors; import io.opentelemetry.sdk.metrics.data.DoublePointData; import io.opentelemetry.sdk.metrics.internal.data.ImmutableExponentialHistogramPointData; -import io.opentelemetry.sdk.metrics.internal.data.ImmutableHistogramPointData; @OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.SUITE, minNumDataNodes = 1) public class TelemetryMetricsEnabledSanityIT extends OpenSearchIntegTestCase { @@ -120,32 +118,6 @@ public void testHistogram() throws Exception { assertEquals(1.0, histogramPointData.getMin(), 1.0); } - public void testHistogramWithExplicitBuckets() throws Exception { - MetricsRegistry metricsRegistry = internalCluster().getInstance(MetricsRegistry.class); - InMemorySingletonMetricsExporter.INSTANCE.reset(); - - List buckets = Arrays.asList(1.0, 5.0, 10.0); - Histogram histogram = metricsRegistry.createHistogram("test-histogram", "test", "ms", buckets); - histogram.record(2.0); - histogram.record(1.0); - histogram.record(3.0); - // Sleep for about 2s to wait for metrics to be published. - Thread.sleep(2000); - - InMemorySingletonMetricsExporter exporter = InMemorySingletonMetricsExporter.INSTANCE; - ImmutableHistogramPointData histogramPointData = ((ImmutableHistogramPointData) ((ArrayList) exporter.getFinishedMetricItems() - .stream() - .filter(a -> a.getName().equals("test-histogram")) - .collect(Collectors.toList()) - .get(0) - .getHistogramData() - .getPoints()).get(0)); - assertEquals(1.0, histogramPointData.getSum(), 6.0); - assertEquals(1.0, histogramPointData.getMax(), 3.0); - assertEquals(1.0, histogramPointData.getMin(), 1.0); - assertEquals(buckets, histogramPointData.getBoundaries()); - } - @After public void reset() { InMemorySingletonMetricsExporter.INSTANCE.reset(); diff --git a/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/metrics/OTelMetricsTelemetry.java b/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/metrics/OTelMetricsTelemetry.java index 8112ebdeef6b2..ec268da9b57a6 100644 --- a/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/metrics/OTelMetricsTelemetry.java +++ b/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/metrics/OTelMetricsTelemetry.java @@ -16,7 +16,6 @@ import java.io.IOException; import java.security.AccessController; import java.security.PrivilegedAction; -import java.util.List; import io.opentelemetry.api.metrics.DoubleCounter; import io.opentelemetry.api.metrics.DoubleHistogram; @@ -93,18 +92,6 @@ public Histogram createHistogram(String name, String description, String unit) { return new OTelHistogram(doubleHistogram); } - @Override - public Histogram createHistogram(String name, String description, String unit, List buckets) { - DoubleHistogram doubleHistogram = AccessController.doPrivileged( - (PrivilegedAction) () -> otelMeter.histogramBuilder(name) - .setUnit(unit) - .setDescription(description) - .setExplicitBucketBoundariesAdvice(buckets) - .build() - ); - return new OTelHistogram(doubleHistogram); - } - @Override public void close() throws IOException { meterProvider.close(); diff --git a/plugins/telemetry-otel/src/test/java/org/opensearch/telemetry/metrics/OTelMetricsTelemetryTests.java b/plugins/telemetry-otel/src/test/java/org/opensearch/telemetry/metrics/OTelMetricsTelemetryTests.java index 370fb5c14e9dc..4b39e3d0d607d 100644 --- a/plugins/telemetry-otel/src/test/java/org/opensearch/telemetry/metrics/OTelMetricsTelemetryTests.java +++ b/plugins/telemetry-otel/src/test/java/org/opensearch/telemetry/metrics/OTelMetricsTelemetryTests.java @@ -14,9 +14,6 @@ import org.opensearch.telemetry.metrics.tags.Tags; import org.opensearch.test.OpenSearchTestCase; -import java.util.Arrays; -import java.util.List; - import io.opentelemetry.api.OpenTelemetry; import io.opentelemetry.api.metrics.DoubleCounter; import io.opentelemetry.api.metrics.DoubleCounterBuilder; @@ -152,34 +149,4 @@ public void testHistogram() { histogram.record(2.0, tags); verify(mockOTelDoubleHistogram).record(2.0, OTelAttributesConverter.convert(tags)); } - - @SuppressWarnings({ "rawtypes", "unchecked" }) - public void testHistogramWithExplicitBuckets() { - String histogramName = "test-histogram"; - String description = "test"; - String unit = "1"; - List buckets = Arrays.asList(1.0, 5.0); - Meter mockMeter = mock(Meter.class); - OpenTelemetry mockOpenTelemetry = mock(OpenTelemetry.class); - DoubleHistogram mockOTelDoubleHistogram = mock(DoubleHistogram.class); - DoubleHistogramBuilder mockOTelDoubleHistogramBuilder = mock(DoubleHistogramBuilder.class); - MeterProvider meterProvider = mock(MeterProvider.class); - when(meterProvider.get(OTelTelemetryPlugin.INSTRUMENTATION_SCOPE_NAME)).thenReturn(mockMeter); - MetricsTelemetry metricsTelemetry = new OTelMetricsTelemetry( - new RefCountedReleasable("telemetry", mockOpenTelemetry, () -> {}), - meterProvider - ); - when(mockMeter.histogramBuilder(histogramName)).thenReturn(mockOTelDoubleHistogramBuilder); - when(mockOTelDoubleHistogramBuilder.setDescription(description)).thenReturn(mockOTelDoubleHistogramBuilder); - when(mockOTelDoubleHistogramBuilder.setUnit(unit)).thenReturn(mockOTelDoubleHistogramBuilder); - when(mockOTelDoubleHistogramBuilder.setExplicitBucketBoundariesAdvice(buckets)).thenReturn(mockOTelDoubleHistogramBuilder); - when(mockOTelDoubleHistogramBuilder.build()).thenReturn(mockOTelDoubleHistogram); - - Histogram histogram = metricsTelemetry.createHistogram(histogramName, description, unit, buckets); - histogram.record(1.0); - verify(mockOTelDoubleHistogram).record(1.0); - Tags tags = Tags.create().addTag("test", "test"); - histogram.record(2.0, tags); - verify(mockOTelDoubleHistogram).record(2.0, OTelAttributesConverter.convert(tags)); - } } diff --git a/test/framework/src/main/java/org/opensearch/test/telemetry/MockTelemetry.java b/test/framework/src/main/java/org/opensearch/test/telemetry/MockTelemetry.java index 06a6b8ca46587..44daf1b1554e0 100644 --- a/test/framework/src/main/java/org/opensearch/test/telemetry/MockTelemetry.java +++ b/test/framework/src/main/java/org/opensearch/test/telemetry/MockTelemetry.java @@ -18,8 +18,6 @@ import org.opensearch.telemetry.tracing.TracingTelemetry; import org.opensearch.test.telemetry.tracing.MockTracingTelemetry; -import java.util.List; - /** * Mock {@link Telemetry} implementation for testing. */ @@ -55,11 +53,6 @@ public Histogram createHistogram(String name, String description, String unit) { return NoopHistogram.INSTANCE; } - @Override - public Histogram createHistogram(String name, String description, String unit, List buckets) { - return NoopHistogram.INSTANCE; - } - @Override public void close() {