Restore prior histogram defaults (#4717)

* Add property for configuring legacy explicit bucket histogram bucket boundaries * Add clarifying comment * Spotless
open-telemetry · Aug 25, 2022 · 160af1c · 160af1c
1 parent def1c85
commit 160af1c
Show file tree

Hide file tree

Showing 9 changed files with 93 additions and 7 deletions.
diff --git a/sdk/metrics/build.gradle.kts b/sdk/metrics/build.gradle.kts
@@ -32,11 +32,11 @@ dependencies {
 
 testing {
   suites {
-    val debugEnabledTest by registering(JvmTestSuite::class) {
+    val configurationTest by registering(JvmTestSuite::class) {
       targets {
         all {
           testTask.configure {
-            jvmArgs("-Dotel.experimental.sdk.metrics.debug=true")
+            jvmArgs("-Dotel.experimental.sdk.metrics.debug=true", "-Dotel.java.histogram.legacy.buckets.enabled=true")
           }
         }
       }

diff --git a/...va/io/opentelemetry/sdk/metrics/internal/aggregator/ExplicitBucketHistogramUtilsTest.java b/...va/io/opentelemetry/sdk/metrics/internal/aggregator/ExplicitBucketHistogramUtilsTest.java
@@ -0,0 +1,28 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.sdk.metrics.internal.aggregator;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Arrays;
+import org.junit.jupiter.api.Test;
+
+class ExplicitBucketHistogramUtilsTest {
+
+  /**
+   * {@link ExplicitBucketHistogramUtils#getDefaultBucketBoundaries()} returns different values when
+   * {@code otel.java.histogram.legacy.buckets.enabled=true}, which is set for this test in {@code
+   * build.gradle.kts}.
+   */
+  @Test
+  void defaultBucketBoundaries() {
+    assertThat(ExplicitBucketHistogramUtils.getDefaultBucketBoundaries())
+        .isEqualTo(
+            Arrays.asList(
+                5d, 10d, 25d, 50d, 75d, 100d, 250d, 500d, 750d, 1_000d, 2_500d, 5_000d, 7_500d,
+                10_000d));
+  }
+}
diff --git a/.../descriptor/InstrumentDescriptorTest.java → .../descriptor/InstrumentDescriptorTest.java b/.../descriptor/InstrumentDescriptorTest.java → .../descriptor/InstrumentDescriptorTest.java
diff --git a/...opentelemetry/testing/SourceInfoTest.java → ...opentelemetry/testing/SourceInfoTest.java b/...opentelemetry/testing/SourceInfoTest.java → ...opentelemetry/testing/SourceInfoTest.java
diff --git a/.../jmh/java/io/opentelemetry/sdk/metrics/internal/aggregator/HistogramAggregationParam.java b/.../jmh/java/io/opentelemetry/sdk/metrics/internal/aggregator/HistogramAggregationParam.java
@@ -15,7 +15,7 @@ public enum HistogramAggregationParam {
   EXPLICIT_DEFAULT_BUCKET(
       new DoubleExplicitBucketHistogramAggregator(
           ExplicitBucketHistogramUtils.createBoundaryArray(
-              ExplicitBucketHistogramUtils.DEFAULT_HISTOGRAM_BUCKET_BOUNDARIES),
+              ExplicitBucketHistogramUtils.getDefaultBucketBoundaries()),
           ExemplarReservoir::doubleNoSamples)),
   EXPLICIT_SINGLE_BUCKET(
       new DoubleExplicitBucketHistogramAggregator(

diff --git a/...rc/jmh/java/io/opentelemetry/sdk/metrics/internal/aggregator/HistogramValueGenerator.java b/...rc/jmh/java/io/opentelemetry/sdk/metrics/internal/aggregator/HistogramValueGenerator.java
@@ -64,7 +64,7 @@ private static double[] explicitDefaultBucketPool() {
     // Add minimal recording value.
     fixedBoundaries.add(0.0);
     // Add the bucket LE bucket boundaries (starts at 5).
-    fixedBoundaries.addAll(ExplicitBucketHistogramUtils.DEFAULT_HISTOGRAM_BUCKET_BOUNDARIES);
+    fixedBoundaries.addAll(ExplicitBucketHistogramUtils.getDefaultBucketBoundaries());
     // Add Double max value as our other extreme.
     fixedBoundaries.add(Double.MAX_VALUE);
     return ExplicitBucketHistogramUtils.createBoundaryArray(fixedBoundaries);

diff --git a/...n/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExplicitBucketHistogramUtils.java b/...n/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExplicitBucketHistogramUtils.java
@@ -8,6 +8,9 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.Locale;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 /**
  * Utilities for interacting with explicit bucket histograms.
@@ -16,11 +19,46 @@
  * at any time.
  */
 public final class ExplicitBucketHistogramUtils {
-  private ExplicitBucketHistogramUtils() {}
 
-  public static final List<Double> DEFAULT_HISTOGRAM_BUCKET_BOUNDARIES =
+  private static final Logger logger =
+      Logger.getLogger(ExplicitBucketHistogramUtils.class.getName());
+  private static final String LEGACY_BUCKETS_ENABLED = "otel.java.histogram.legacy.buckets.enabled";
+  private static final List<Double> DEFAULT_HISTOGRAM_BUCKET_BOUNDARIES =
       Collections.unmodifiableList(
           Arrays.asList(0d, 5d, 10d, 25d, 50d, 75d, 100d, 250d, 500d, 1_000d));
+  private static final List<Double> LEGACY_HISTOGRAM_BUCKET_BOUNDARIES =
+      Collections.unmodifiableList(
+          Arrays.asList(
+              5d, 10d, 25d, 50d, 75d, 100d, 250d, 500d, 750d, 1_000d, 2_500d, 5_000d, 7_500d,
+              10_000d));
+
+  private static final List<Double> defaultBucketBoundaries;
+
+  static {
+    // TODO: remove support for configuring legacy bucket boundaries after 1.24.0
+    boolean legacyBucketsEnabled =
+        Boolean.parseBoolean(System.getProperty(LEGACY_BUCKETS_ENABLED))
+            || Boolean.parseBoolean(
+                System.getenv(LEGACY_BUCKETS_ENABLED.toUpperCase(Locale.ROOT).replace(".", "_")));
+    if (legacyBucketsEnabled) {
+      logger.log(
+          Level.WARNING,
+          "Legacy explicit bucket histogram buckets have been enabled. Support will be removed "
+              + "after version 1.24.0. If you depend on the legacy bucket boundaries, please "
+              + "use the View API as described in "
+              + "https://opentelemetry.io/docs/instrumentation/java/manual/#views.");
+      defaultBucketBoundaries = LEGACY_HISTOGRAM_BUCKET_BOUNDARIES;
+    } else {
+      defaultBucketBoundaries = DEFAULT_HISTOGRAM_BUCKET_BOUNDARIES;
+    }
+  }
+
+  private ExplicitBucketHistogramUtils() {}
+
+  /** Returns the default explicit bucket histogram bucket boundaries. */
+  public static List<Double> getDefaultBucketBoundaries() {
+    return defaultBucketBoundaries;
+  }
 
   /** Converts bucket boundary "convenient" configuration into the "more efficient" array. */
   public static double[] createBoundaryArray(List<Double> boundaries) {

diff --git a/...n/java/io/opentelemetry/sdk/metrics/internal/view/ExplicitBucketHistogramAggregation.java b/...n/java/io/opentelemetry/sdk/metrics/internal/view/ExplicitBucketHistogramAggregation.java
@@ -27,7 +27,7 @@ public final class ExplicitBucketHistogramAggregation implements Aggregation, Ag
 
   private static final Aggregation DEFAULT =
       new ExplicitBucketHistogramAggregation(
-          ExplicitBucketHistogramUtils.DEFAULT_HISTOGRAM_BUCKET_BOUNDARIES);
+          ExplicitBucketHistogramUtils.getDefaultBucketBoundaries());
 
   public static Aggregation getDefault() {
     return DEFAULT;

diff --git a/...va/io/opentelemetry/sdk/metrics/internal/aggregator/ExplicitBucketHistogramUtilsTest.java b/...va/io/opentelemetry/sdk/metrics/internal/aggregator/ExplicitBucketHistogramUtilsTest.java
@@ -0,0 +1,20 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.sdk.metrics.internal.aggregator;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Arrays;
+import org.junit.jupiter.api.Test;
+
+class ExplicitBucketHistogramUtilsTest {
+
+  @Test
+  void defaultBucketBoundaries() {
+    assertThat(ExplicitBucketHistogramUtils.getDefaultBucketBoundaries())
+        .isEqualTo(Arrays.asList(0d, 5d, 10d, 25d, 50d, 75d, 100d, 250d, 500d, 1_000d));
+  }
+}