Addresses review comments

Signed-off-by: Gagan Juneja <[email protected]>
Gaganjuneja · Feb 2, 2024 · e6e786d · e6e786d
1 parent 5494e7b
commit e6e786d
Show file tree

Hide file tree

Showing 16 changed files with 136 additions and 100 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
@@ -9,6 +9,7 @@
 package org.opensearch.telemetry.metrics;
 
 import java.io.IOException;
+import java.util.List;
 
 /**
  * Default implementation for {@link MetricsRegistry}
@@ -39,6 +40,11 @@ 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<Double> 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/Histogram.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/Histogram.java
@@ -8,6 +8,8 @@
 
 package org.opensearch.telemetry.metrics;
 
+import java.util.Arrays;
+import java.util.List;
 import org.opensearch.common.annotation.ExperimentalApi;
 import org.opensearch.telemetry.metrics.tags.Tags;
 

diff --git a/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/HistogramType.java b/libs/telemetry/src/main/java/org/opensearch/telemetry/metrics/HistogramType.java
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
@@ -8,6 +8,7 @@
 
 package org.opensearch.telemetry.metrics;
 
+import java.util.List;
 import org.opensearch.common.annotation.ExperimentalApi;
 
 import java.io.Closeable;
@@ -38,12 +39,25 @@ public interface MetricsRegistry extends Closeable {
     Counter createUpDownCounter(String name, String description, String unit);
 
     /**
-     * Creates the histogram type of Metric.
+     * Creates the histogram type of Metric. Implementation framework will take care
+     * of the bucketing strategy.
      *
      * @param name        name of the histogram.
      * @param description any description about the metric.
      * @param unit        unit of the metric.
      * @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<Double> 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
@@ -8,6 +8,7 @@
 
 package org.opensearch.telemetry.metrics.noop;
 
+import java.util.List;
 import org.opensearch.common.annotation.InternalApi;
 import org.opensearch.telemetry.metrics.Counter;
 import org.opensearch.telemetry.metrics.Histogram;
@@ -44,6 +45,11 @@ public Histogram createHistogram(String name, String description, String unit) {
         return NoopHistogram.INSTANCE;
     }
 
+    @Override
+    public Histogram createHistogram(String name, String description, String unit, List<Double> buckets) {
+        return NoopHistogram.INSTANCE;
+    }
+
     @Override
     public void close() throws IOException {
 

diff --git a/...telemetry/src/test/java/org/opensearch/telemetry/metrics/DefaultMetricsRegistryTests.java b/...telemetry/src/test/java/org/opensearch/telemetry/metrics/DefaultMetricsRegistryTests.java
@@ -8,6 +8,8 @@
 
 package org.opensearch.telemetry.metrics;
 
+import java.util.Arrays;
+import java.util.List;
 import org.opensearch.test.OpenSearchTestCase;
 
 import static org.mockito.ArgumentMatchers.any;
@@ -59,4 +61,16 @@ 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), any(List.class))).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/...alClusterTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsDisabledSanityIT.java b/...alClusterTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsDisabledSanityIT.java
@@ -56,11 +56,14 @@ 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/...nalClusterTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsEnabledSanityIT.java b/...nalClusterTest/java/org/opensearch/telemetry/metrics/TelemetryMetricsEnabledSanityIT.java
@@ -8,6 +8,9 @@
 
 package org.opensearch.telemetry.metrics;
 
+import io.opentelemetry.sdk.metrics.data.MetricData;
+import io.opentelemetry.sdk.metrics.internal.data.ImmutableExponentialHistogramPointData;
+import java.util.List;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.unit.TimeValue;
 import org.opensearch.plugins.Plugin;
@@ -104,6 +107,31 @@ public void testHistogram() throws Exception {
         // Sleep for about 2s to wait for metrics to be published.
         Thread.sleep(2000);
 
+        InMemorySingletonMetricsExporter exporter = InMemorySingletonMetricsExporter.INSTANCE;
+        ImmutableExponentialHistogramPointData histogramPointData = ((ImmutableExponentialHistogramPointData) ((ArrayList) exporter.getFinishedMetricItems()
+            .stream()
+            .filter(a -> a.getName().contains("test-histogram"))
+            .collect(Collectors.toList())
+            .get(0)
+            .getExponentialHistogramData()
+            .getPoints()).get(0));
+        assertEquals(1.0, histogramPointData.getSum(), 6.0);
+        assertEquals(1.0, histogramPointData.getMax(), 3.0);
+        assertEquals(1.0, histogramPointData.getMin(), 1.0);
+    }
+
+    public void testHistogramWithExplicitBuckets() throws Exception {
+        MetricsRegistry metricsRegistry = internalCluster().getInstance(MetricsRegistry.class);
+        InMemorySingletonMetricsExporter.INSTANCE.reset();
+
+        List<Double> 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()
@@ -115,6 +143,7 @@ public void testHistogram() throws Exception {
         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

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
@@ -53,10 +53,7 @@ public List<Setting<?>> getSettings() {
             OTelTelemetrySettings.TRACER_EXPORTER_DELAY_SETTING,
             OTelTelemetrySettings.TRACER_EXPORTER_MAX_QUEUE_SIZE_SETTING,
             OTelTelemetrySettings.OTEL_TRACER_SPAN_EXPORTER_CLASS_SETTING,
-            OTelTelemetrySettings.OTEL_METRICS_EXPORTER_CLASS_SETTING,
-            OTelTelemetrySettings.OTEL_METRICS_HISTOGRAM_EXPONENTIAL_MAX_SCALE,
-            OTelTelemetrySettings.OTEL_METRICS_HISTOGRAM_EXPONENTIAL_MAX_BUCKETS,
-            OTelTelemetrySettings.OTEL_METRICS_HISTOGRAM_FIXED_BUCKETS
+            OTelTelemetrySettings.OTEL_METRICS_EXPORTER_CLASS_SETTING
         );
     }
 

diff --git a/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/OTelTelemetrySettings.java b/plugins/telemetry-otel/src/main/java/org/opensearch/telemetry/OTelTelemetrySettings.java
@@ -112,37 +112,4 @@ private OTelTelemetrySettings() {}
         Setting.Property.NodeScope,
         Setting.Property.Final
     );
-
-    /**
-     * Max scale setting for the {@link io.opentelemetry.sdk.metrics.internal.view.Base2ExponentialHistogramAggregation}
-     */
-    public static final Setting<Integer> OTEL_METRICS_HISTOGRAM_EXPONENTIAL_MAX_SCALE = Setting.intSetting(
-        "telemetry.otel.metrics.histogram.exponential.max.scale",
-        20,
-        -10,
-        Setting.Property.NodeScope,
-        Setting.Property.Final
-    );
-
-    /**
-     * Max buckets setting for the {@link io.opentelemetry.sdk.metrics.internal.view.Base2ExponentialHistogramAggregation}
-     */
-    public static final Setting<Integer> OTEL_METRICS_HISTOGRAM_EXPONENTIAL_MAX_BUCKETS = Setting.intSetting(
-        "telemetry.otel.metrics.histogram.exponential.max.buckets",
-        160,
-        1,
-        Setting.Property.NodeScope,
-        Setting.Property.Final
-    );
-
-    /**
-     * Explicit bucket list setting for the {@link io.opentelemetry.sdk.metrics.internal.view.ExplicitBucketHistogramAggregation}
-     */
-    public static final Setting<List<Double>> OTEL_METRICS_HISTOGRAM_FIXED_BUCKETS = Setting.listSetting(
-        "telemetry.otel.metrics.histogram.fixed.buckets",
-        Arrays.asList("0", "5", "10", "25", "50", "75", "100", "250", "500", "750", "1000", "2500", "5000", "7500", "10000"),
-        Double::parseDouble,
-        Setting.Property.NodeScope,
-        Setting.Property.Final
-    );
 }
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
@@ -8,6 +8,7 @@
 
 package org.opensearch.telemetry.metrics;
 
+import java.util.List;
 import org.opensearch.common.concurrent.RefCountedReleasable;
 import org.opensearch.telemetry.OTelTelemetryPlugin;
 
@@ -22,6 +23,7 @@
 import io.opentelemetry.api.metrics.Meter;
 import io.opentelemetry.api.metrics.MeterProvider;
 import io.opentelemetry.sdk.OpenTelemetrySdk;
+import org.opensearch.telemetry.tracing.OTelResourceProvider;
 
 /**
  * OTel implementation for {@link MetricsTelemetry}
@@ -71,16 +73,29 @@ public Counter createUpDownCounter(String name, String description, String unit)
 
     /**
      * Creates the Otel Histogram. It created the default version. In {@link org.opensearch.telemetry.tracing.OTelResourceProvider}
-     * we can configure the bucketing strategy through view.
+     * we can configure the bucketing strategy through view. It appends the OTelResourceProvider.DYNAMIC_HISTOGRAM_METRIC_NAME_SUFFIX
+     * to the metric name. It's needed to differentiate the {@link Histogram} metrics with explictt bucket or dynamic
+     * buckets. This suffix can be removed from the metric name in the collector.
      * @param name        name of the histogram.
      * @param description any description about the metric.
      * @param unit        unit of the metric.
      * @return histogram
      */
     @Override
     public Histogram createHistogram(String name, String description, String unit) {
+        String internalMetricName = name + OTelResourceProvider.DYNAMIC_HISTOGRAM_METRIC_NAME_SUFFIX;
         DoubleHistogram doubleHistogram = AccessController.doPrivileged(
-            (PrivilegedAction<DoubleHistogram>) () -> otelMeter.histogramBuilder(name).setUnit(unit).setDescription(description).build()
+            (PrivilegedAction<DoubleHistogram>) () -> otelMeter.histogramBuilder(internalMetricName).setUnit(unit).setDescription(description)
+                .build()
+        );
+        return new OTelHistogram(doubleHistogram);
+    }
+
+    @Override
+    public Histogram createHistogram(String name, String description, String unit, List<Double> buckets) {
+        DoubleHistogram doubleHistogram = AccessController.doPrivileged(
+            (PrivilegedAction<DoubleHistogram>) () -> otelMeter.histogramBuilder(name).setUnit(unit).setDescription(description)
+                .setExplicitBucketBoundariesAdvice(buckets).build()
         );
         return new OTelHistogram(doubleHistogram);
     }

diff --git a/...s/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelResourceProvider.java b/...s/telemetry-otel/src/main/java/org/opensearch/telemetry/tracing/OTelResourceProvider.java
@@ -9,9 +9,7 @@
 package org.opensearch.telemetry.tracing;
 
 import org.opensearch.common.settings.Settings;
-import org.opensearch.telemetry.OTelTelemetrySettings;
 import org.opensearch.telemetry.TelemetrySettings;
-import org.opensearch.telemetry.metrics.HistogramType;
 import org.opensearch.telemetry.metrics.exporter.OTelMetricsExporterFactory;
 import org.opensearch.telemetry.tracing.exporter.OTelSpanExporterFactory;
 import org.opensearch.telemetry.tracing.sampler.ProbabilisticSampler;
@@ -47,6 +45,8 @@
  * This class encapsulates all OpenTelemetry related resources
  */
 public final class OTelResourceProvider {
+    public static final String DYNAMIC_HISTOGRAM_METRIC_NAME_SUFFIX = "-dynamic";
+
     private OTelResourceProvider() {}
 
     /**
@@ -99,29 +99,11 @@ private static SdkMeterProvider createSdkMetricProvider(Settings settings, Resou
                     .setInterval(TelemetrySettings.METRICS_PUBLISH_INTERVAL_SETTING.get(settings).getSeconds(), TimeUnit.SECONDS)
                     .build()
             )
-            .registerView(InstrumentSelector.builder().setType(InstrumentType.HISTOGRAM).build(), createHistogramTypeView(settings))
+            .registerView(InstrumentSelector.builder().setName("*"+ DYNAMIC_HISTOGRAM_METRIC_NAME_SUFFIX).setType(InstrumentType.HISTOGRAM).build(), View.builder()
+                .setAggregation(Base2ExponentialHistogramAggregation.getDefault()).build())
             .build();
     }
 
-    private static View createHistogramTypeView(Settings settings) {
-        if (HistogramType.DYNAMIC == TelemetrySettings.METRICS_HISTOGRAM_TYPE.get(settings)) {
-            return View.builder()
-                .setAggregation(
-                    Base2ExponentialHistogramAggregation.create(
-                        OTelTelemetrySettings.OTEL_METRICS_HISTOGRAM_EXPONENTIAL_MAX_BUCKETS.get(settings),
-                        OTelTelemetrySettings.OTEL_METRICS_HISTOGRAM_EXPONENTIAL_MAX_SCALE.get(settings)
-                    )
-                )
-                .build();
-        } else {
-            return View.builder()
-                .setAggregation(
-                    ExplicitBucketHistogramAggregation.create(OTelTelemetrySettings.OTEL_METRICS_HISTOGRAM_FIXED_BUCKETS.get(settings))
-                )
-                .build();
-        }
-    }
-
     private static SdkTracerProvider createSdkTracerProvider(
         Settings settings,
         SpanExporter spanExporter,