Skip to content

Commit

Permalink
Revert histogram buckets (#4736)
Browse files Browse the repository at this point in the history 
* Revert "Restore prior histogram defaults (#4717)"

This reverts commit 160af1c.

* Revert "Update bucket hist defaults to match spec (#4684)"

This reverts commit 8445642.
  • Loading branch information
@jack-berg
jack-berg committed Sep 8, 2022
1 parent b5ef538 commit 928d112
Show file tree
Hide file tree
Showing 12 changed files with 51 additions and 113 deletions.
4 changes: 2 additions & 2 deletions sdk/metrics/build.gradle.kts
View file
Expand Up @@ -32,11 +32,11 @@ dependencies {

testing {
suites {
val configurationTest by registering(JvmTestSuite::class) {
val debugEnabledTest by registering(JvmTestSuite::class) {
targets {
all {
testTask.configure {
jvmArgs("-Dotel.experimental.sdk.metrics.debug=true", "-Dotel.java.histogram.legacy.buckets.enabled=true")
jvmArgs("-Dotel.experimental.sdk.metrics.debug=true")
}
}
}
Expand Down
28 changes: 0 additions & 28 deletions ...va/io/opentelemetry/sdk/metrics/internal/aggregator/ExplicitBucketHistogramUtilsTest.java
View file

This file was deleted.

0 .../descriptor/InstrumentDescriptorTest.java → .../descriptor/InstrumentDescriptorTest.java
View file
File renamed without changes.
0 ...opentelemetry/testing/SourceInfoTest.java → ...opentelemetry/testing/SourceInfoTest.java
View file
File renamed without changes.
2 changes: 1 addition & 1 deletion .../jmh/java/io/opentelemetry/sdk/metrics/internal/aggregator/HistogramAggregationParam.java
View file
Expand Up @@ -15,7 +15,7 @@ public enum HistogramAggregationParam {
EXPLICIT_DEFAULT_BUCKET(
new DoubleExplicitBucketHistogramAggregator(
ExplicitBucketHistogramUtils.createBoundaryArray(
ExplicitBucketHistogramUtils.getDefaultBucketBoundaries()),
ExplicitBucketHistogramUtils.DEFAULT_HISTOGRAM_BUCKET_BOUNDARIES),
ExemplarReservoir::doubleNoSamples)),
EXPLICIT_SINGLE_BUCKET(
new DoubleExplicitBucketHistogramAggregator(
Expand Down
2 changes: 1 addition & 1 deletion ...rc/jmh/java/io/opentelemetry/sdk/metrics/internal/aggregator/HistogramValueGenerator.java
View file
Expand Up @@ -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.getDefaultBucketBoundaries());
fixedBoundaries.addAll(ExplicitBucketHistogramUtils.DEFAULT_HISTOGRAM_BUCKET_BOUNDARIES);
// Add Double max value as our other extreme.
fixedBoundaries.add(Double.MAX_VALUE);
return ExplicitBucketHistogramUtils.createBoundaryArray(fixedBoundaries);
Expand Down
40 changes: 2 additions & 38 deletions ...n/java/io/opentelemetry/sdk/metrics/internal/aggregator/ExplicitBucketHistogramUtils.java
View file
Expand Up @@ -8,9 +8,6 @@
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.
Expand All @@ -19,47 +16,14 @@
* at any time.
*/
public final class ExplicitBucketHistogramUtils {
private ExplicitBucketHistogramUtils() {}

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 =
public static final List<Double> DEFAULT_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) {
return validateBucketBoundaries(boundaries.stream().mapToDouble(i -> i).toArray());
Expand Down
2 changes: 1 addition & 1 deletion ...n/java/io/opentelemetry/sdk/metrics/internal/view/ExplicitBucketHistogramAggregation.java
View file
Expand Up @@ -27,7 +27,7 @@ public final class ExplicitBucketHistogramAggregation implements Aggregation, Ag

private static final Aggregation DEFAULT =
new ExplicitBucketHistogramAggregation(
ExplicitBucketHistogramUtils.getDefaultBucketBoundaries());
ExplicitBucketHistogramUtils.DEFAULT_HISTOGRAM_BUCKET_BOUNDARIES);

public static Aggregation getDefault() {
return DEFAULT;
Expand Down
30 changes: 20 additions & 10 deletions sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogramTest.java
View file
Expand Up @@ -112,8 +112,10 @@ void collectMetrics_WithEmptyAttributes() {
.hasCount(2)
.hasSum(24)
.hasBucketBoundaries(
0, 5, 10, 25, 50, 75, 100, 250, 500, 1_000)
.hasBucketCounts(0, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0))));
5, 10, 25, 50, 75, 100, 250, 500, 750, 1_000, 2_500,
5_000, 7_500, 10_000)
.hasBucketCounts(
0, 0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0))));
}

@Test
Expand Down Expand Up @@ -147,15 +149,17 @@ void collectMetrics_WithMultipleCollects() {
.hasEpochNanos(testClock.now())
.hasCount(3)
.hasSum(566.3d)
.hasBucketCounts(0, 0, 0, 0, 0, 0, 0, 2, 1, 0, 0)
.hasBucketCounts(
0, 0, 0, 0, 0, 0, 2, 1, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(attributeEntry("K", "V")),
point ->
point
.hasStartEpochNanos(startTime)
.hasEpochNanos(testClock.now())
.hasCount(2)
.hasSum(22.2d)
.hasBucketCounts(0, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0)
.hasBucketCounts(
0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(Attributes.empty()))));

// Histograms are cumulative by default.
Expand All @@ -178,15 +182,17 @@ void collectMetrics_WithMultipleCollects() {
.hasEpochNanos(testClock.now())
.hasCount(4)
.hasSum(788.3)
.hasBucketCounts(0, 0, 0, 0, 0, 0, 0, 3, 1, 0, 0)
.hasBucketCounts(
0, 0, 0, 0, 0, 0, 3, 1, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(attributeEntry("K", "V")),
point ->
point
.hasStartEpochNanos(startTime)
.hasEpochNanos(testClock.now())
.hasCount(3)
.hasSum(39.2)
.hasBucketCounts(0, 0, 1, 2, 0, 0, 0, 0, 0, 0, 0)
.hasBucketCounts(
0, 1, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(Attributes.empty()))));
} finally {
bound.unbind();
Expand Down Expand Up @@ -380,31 +386,35 @@ void stressTest_WithDifferentLabelSet() {
.hasEpochNanos(testClock.now())
.hasCount(4_000)
.hasSum(40_000)
.hasBucketCounts(0, 0, 2000, 2000, 0, 0, 0, 0, 0, 0, 0)
.hasBucketCounts(
0, 2000, 2000, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(attributeEntry(keys[0], values[0])),
point ->
point
.hasStartEpochNanos(testClock.now())
.hasEpochNanos(testClock.now())
.hasCount(4_000)
.hasSum(40_000)
.hasBucketCounts(0, 0, 2000, 2000, 0, 0, 0, 0, 0, 0, 0)
.hasBucketCounts(
0, 2000, 2000, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(attributeEntry(keys[1], values[1])),
point ->
point
.hasStartEpochNanos(testClock.now())
.hasEpochNanos(testClock.now())
.hasCount(4_000)
.hasSum(40_000)
.hasBucketCounts(0, 0, 2000, 2000, 0, 0, 0, 0, 0, 0, 0)
.hasBucketCounts(
0, 2000, 2000, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(attributeEntry(keys[2], values[2])),
point ->
point
.hasStartEpochNanos(testClock.now())
.hasEpochNanos(testClock.now())
.hasCount(4_000)
.hasSum(40_000)
.hasBucketCounts(0, 0, 2000, 2000, 0, 0, 0, 0, 0, 0, 0)
.hasBucketCounts(
0, 2000, 2000, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(attributeEntry(keys[3], values[3])))));
}

Expand Down
30 changes: 20 additions & 10 deletions sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/SdkLongHistogramTest.java
View file
Expand Up @@ -112,8 +112,10 @@ void collectMetrics_WithEmptyAttributes() {
.hasCount(2)
.hasSum(24)
.hasBucketBoundaries(
0, 5, 10, 25, 50, 75, 100, 250, 500, 1_000)
.hasBucketCounts(0, 0, 0, 2, 0, 0, 0, 0, 0, 0, 0))));
5, 10, 25, 50, 75, 100, 250, 500, 750, 1_000, 2_500,
5_000, 7_500, 10_000)
.hasBucketCounts(
0, 0, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0))));
}

@Test
Expand Down Expand Up @@ -147,15 +149,17 @@ void collectMetrics_WithMultipleCollects() {
.hasEpochNanos(testClock.now())
.hasCount(3)
.hasSum(445)
.hasBucketCounts(0, 1, 0, 0, 0, 0, 0, 1, 1, 0, 0)
.hasBucketCounts(
1, 0, 0, 0, 0, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(attributeEntry("K", "V")),
point ->
point
.hasStartEpochNanos(startTime)
.hasEpochNanos(testClock.now())
.hasCount(2)
.hasSum(23)
.hasBucketCounts(0, 0, 1, 1, 0, 0, 0, 0, 0, 0, 0)
.hasBucketCounts(
0, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(Attributes.empty()))));

// Histograms are cumulative by default.
Expand All @@ -178,15 +182,17 @@ void collectMetrics_WithMultipleCollects() {
.hasEpochNanos(testClock.now())
.hasCount(4)
.hasSum(667)
.hasBucketCounts(0, 1, 0, 0, 0, 0, 0, 2, 1, 0, 0)
.hasBucketCounts(
1, 0, 0, 0, 0, 0, 2, 1, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(attributeEntry("K", "V")),
point ->
point
.hasStartEpochNanos(startTime)
.hasEpochNanos(testClock.now())
.hasCount(3)
.hasSum(40)
.hasBucketCounts(0, 0, 1, 2, 0, 0, 0, 0, 0, 0, 0)
.hasBucketCounts(
0, 1, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(Attributes.empty()))));
} finally {
bound.unbind();
Expand Down Expand Up @@ -381,31 +387,35 @@ void stressTest_WithDifferentLabelSet() {
.hasEpochNanos(testClock.now())
.hasCount(2_000)
.hasSum(20_000)
.hasBucketCounts(0, 0, 1000, 1000, 0, 0, 0, 0, 0, 0, 0)
.hasBucketCounts(
0, 1000, 1000, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(attributeEntry(keys[0], values[0])),
point ->
point
.hasStartEpochNanos(testClock.now())
.hasEpochNanos(testClock.now())
.hasCount(2_000)
.hasSum(20_000)
.hasBucketCounts(0, 0, 1000, 1000, 0, 0, 0, 0, 0, 0, 0)
.hasBucketCounts(
0, 1000, 1000, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(attributeEntry(keys[1], values[1])),
point ->
point
.hasStartEpochNanos(testClock.now())
.hasEpochNanos(testClock.now())
.hasCount(2_000)
.hasSum(20_000)
.hasBucketCounts(0, 0, 1000, 1000, 0, 0, 0, 0, 0, 0, 0)
.hasBucketCounts(
0, 1000, 1000, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(attributeEntry(keys[2], values[2])),
point ->
point
.hasStartEpochNanos(testClock.now())
.hasEpochNanos(testClock.now())
.hasCount(2_000)
.hasSum(20_000)
.hasBucketCounts(0, 0, 1000, 1000, 0, 0, 0, 0, 0, 0, 0)
.hasBucketCounts(
0, 1000, 1000, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
.hasAttributes(attributeEntry(keys[3], values[3])))));
}

Expand Down
6 changes: 4 additions & 2 deletions sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/SdkMeterProviderTest.java
View file
Expand Up @@ -130,7 +130,8 @@ void collectAllSyncInstruments() {
.hasAttributes(Attributes.empty())
.hasCount(1)
.hasSum(10.1)
.hasBucketCounts(0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0))),
.hasBucketCounts(
0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0))),
metric ->
assertThat(metric)
.hasName("testDoubleCounter")
Expand Down Expand Up @@ -158,7 +159,8 @@ void collectAllSyncInstruments() {
.hasAttributes(Attributes.empty())
.hasCount(1)
.hasSum(10)
.hasBucketCounts(0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0))),
.hasBucketCounts(
0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0))),
metric ->
assertThat(metric)
.hasName("testLongUpDownCounter")
Expand Down
20 changes: 0 additions & 20 deletions ...va/io/opentelemetry/sdk/metrics/internal/aggregator/ExplicitBucketHistogramUtilsTest.java
View file

This file was deleted.

0 comments on commit 928d112

Please sign in to comment.