Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert histogram buckets #4736

Merged
merged 2 commits into from Sep 8, 2022
Merged

Revert histogram buckets #4736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e90c0df
Revert "Restore prior histogram defaults (#4717)"
jack-berg Sep 8, 2022
449d69d
Revert "Update bucket hist defaults to match spec (#4684)"
jack-berg Sep 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions sdk/metrics/build.gradle.kts
View file
Open in desktop
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
Open in desktop

This file was deleted.

0 .../descriptor/InstrumentDescriptorTest.java → .../descriptor/InstrumentDescriptorTest.java
View file
Open in desktop
File renamed without changes.
0 ...opentelemetry/testing/SourceInfoTest.java → ...opentelemetry/testing/SourceInfoTest.java
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion .../jmh/java/io/opentelemetry/sdk/metrics/internal/aggregator/HistogramAggregationParam.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop

This file was deleted.