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

Prototype histogram advice API (i.e. Hints) #5217

Merged
merged 8 commits into from
Apr 11, 2023
Merged

Prototype histogram advice API (i.e. Hints) #5217

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
678d230
Prototype histogram advice API
jack-berg Feb 15, 2023
30c161a
Rename to setAdvice
jack-berg Feb 15, 2023
1a448f7
Advice influences default aggregation
jack-berg Feb 16, 2023
b1062df
Rename to setBoundaries
jack-berg Feb 16, 2023
385b13c
japicmp
jack-berg Feb 16, 2023
aec5ce6
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
jack-berg Mar 9, 2023
7d4d24e
Move histogrma advice to :extensions:incubator
jack-berg Mar 9, 2023
34b6a73
streamline test
jack-berg Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions api/all/src/main/java/io/opentelemetry/api/metrics/DoubleHistogramBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

package io.opentelemetry.api.metrics;

import java.util.function.Consumer;

/**
* Builder class for {@link DoubleHistogram}.
*
Expand Down Expand Up @@ -32,6 +34,11 @@ public interface DoubleHistogramBuilder {
*/
DoubleHistogramBuilder setUnit(String unit);

/** Specify advice for histogram implementations. */
default DoubleHistogramBuilder setAdvice(Consumer<HistogramAdviceConfigurer> adviceConsumer) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: are we planning to add this to the API right away? Or perhaps introduce it in an internal interface first?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to different options. Could make it an internal interface, or add it to the api incubator, or wait on merging until the spec is marked stable.

I think I like the idea of an internal interface the best.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've split out the interfaces to :extensions:incubator.

To use, add a dependency on opentelemetry-extension-incubator, and cast the histogram builder to the extended version of the interface:
-((ExtendedDoubleHistogramBuilder) meter.histogramBuilder("histogram")).setAdvice(...)
-((ExtendedLongHistogramBuilder) meter.histogramBuilder("histogram").ofLongs()).setAdvice(...)

return this;
}

/** Sets the Counter for recording {@code long} values. */
LongHistogramBuilder ofLongs();

Expand Down
15 changes: 15 additions & 0 deletions api/all/src/main/java/io/opentelemetry/api/metrics/HistogramAdviceConfigurer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.api.metrics;

import java.util.List;

/** Configure advice for implementations of {@link LongHistogram} and {@link DoubleHistogram}. */
public interface HistogramAdviceConfigurer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you envision this having more methods added, or could we add @FunctionalInterface to it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think more methods will be added. One that seems likely is a set of attribute keys to retain by default (spec Issue #3061). The idea being that instrumentation can record a wider set of attributes while only retaining a smaller set by default. Users can then opt into additional attributes via views.


/** Specify recommended set of explicit bucket boundaries for this histogram. */
HistogramAdviceConfigurer setBoundaries(List<Double> bucketBoundaries);
}
7 changes: 7 additions & 0 deletions api/all/src/main/java/io/opentelemetry/api/metrics/LongHistogramBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

package io.opentelemetry.api.metrics;

import java.util.function.Consumer;

/**
* Builder class for {@link LongHistogram}.
*
Expand All @@ -31,6 +33,11 @@ public interface LongHistogramBuilder {
*/
LongHistogramBuilder setUnit(String unit);

/** Specify advice for histogram implementations. */
default LongHistogramBuilder setAdvice(Consumer<HistogramAdviceConfigurer> adviceConsumer) {
return this;
}

/**
* Builds and returns a Histogram instrument with the configuration.
*
Expand Down
11 changes: 10 additions & 1 deletion docs/apidiffs/current_vs_latest/opentelemetry-api.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,11 @@
Comparing source compatibility of against
No changes.
*** MODIFIED INTERFACE: PUBLIC ABSTRACT io.opentelemetry.api.metrics.DoubleHistogramBuilder (not serializable)
=== CLASS FILE FORMAT VERSION: 52.0 <- 52.0
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.api.metrics.DoubleHistogramBuilder setAdvice(java.util.function.Consumer)
+++ NEW INTERFACE: PUBLIC(+) ABSTRACT(+) io.opentelemetry.api.metrics.HistogramAdviceConfigurer (not serializable)
+++ CLASS FILE FORMAT VERSION: 52.0 <- n.a.
+++ NEW SUPERCLASS: java.lang.Object
+++ NEW METHOD: PUBLIC(+) ABSTRACT(+) io.opentelemetry.api.metrics.HistogramAdviceConfigurer setExplicitBucketBoundaries(java.util.List)
*** MODIFIED INTERFACE: PUBLIC ABSTRACT io.opentelemetry.api.metrics.LongHistogramBuilder (not serializable)
=== CLASS FILE FORMAT VERSION: 52.0 <- 52.0
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.api.metrics.LongHistogramBuilder setAdvice(java.util.function.Consumer)
5 changes: 4 additions & 1 deletion docs/apidiffs/current_vs_latest/opentelemetry-sdk-testing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,5 @@
Comparing source compatibility of against
No changes.
*** MODIFIED CLASS: PUBLIC io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader (not serializable)
=== CLASS FILE FORMAT VERSION: 52.0 <- 52.0
+++ NEW METHOD: PUBLIC(+) STATIC(+) io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader create(io.opentelemetry.sdk.metrics.export.AggregationTemporalitySelector, io.opentelemetry.sdk.metrics.export.DefaultAggregationSelector)
+++ NEW METHOD: PUBLIC(+) io.opentelemetry.sdk.metrics.Aggregation getDefaultAggregation(io.opentelemetry.sdk.metrics.InstrumentType)
3 changes: 3 additions & 0 deletions sdk/all/src/test/java/io/opentelemetry/sdk/OpenTelemetrySdkTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,11 @@
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.junit.jupiter.MockitoExtension;
import org.mockito.junit.jupiter.MockitoSettings;
import org.mockito.quality.Strictness;

@ExtendWith(MockitoExtension.class)
@MockitoSettings(strictness = Strictness.LENIENT)
class OpenTelemetrySdkTest {

@RegisterExtension
Expand Down
52 changes: 44 additions & 8 deletions ...dTest/java/io/opentelemetry/sdk/metrics/internal/descriptor/InstrumentDescriptorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,44 +22,80 @@ class InstrumentDescriptorTest {
void equals() {
InstrumentDescriptor descriptor =
InstrumentDescriptor.create(
"name", "description", "unit", InstrumentType.COUNTER, InstrumentValueType.LONG);
"name",
"description",
"unit",
InstrumentType.COUNTER,
InstrumentValueType.LONG,
Advice.empty());

assertThat(descriptor)
.isEqualTo(
InstrumentDescriptor.create(
"name", "description", "unit", InstrumentType.COUNTER, InstrumentValueType.LONG));
"name",
"description",
"unit",
InstrumentType.COUNTER,
InstrumentValueType.LONG,
Advice.empty()));

// Validate getSourceInfo() is not equal for otherwise equal descriptors
assertThat(descriptor.getSourceInfo())
.isNotEqualTo(
InstrumentDescriptor.create(
"name", "description", "unit", InstrumentType.COUNTER, InstrumentValueType.LONG)
"name",
"description",
"unit",
InstrumentType.COUNTER,
InstrumentValueType.LONG,
Advice.empty())
.getSourceInfo());

// Validate that name, description, unit, type, and value type are considered in equals
assertThat(descriptor)
.isNotEqualTo(
InstrumentDescriptor.create(
"foo", "description", "unit", InstrumentType.COUNTER, InstrumentValueType.LONG));
"foo",
"description",
"unit",
InstrumentType.COUNTER,
InstrumentValueType.LONG,
Advice.empty()));
assertThat(descriptor)
.isNotEqualTo(
InstrumentDescriptor.create(
"name", "foo", "unit", InstrumentType.COUNTER, InstrumentValueType.LONG));
"name",
"foo",
"unit",
InstrumentType.COUNTER,
InstrumentValueType.LONG,
Advice.empty()));
assertThat(descriptor)
.isNotEqualTo(
InstrumentDescriptor.create(
"name", "description", "foo", InstrumentType.COUNTER, InstrumentValueType.LONG));
"name",
"description",
"foo",
InstrumentType.COUNTER,
InstrumentValueType.LONG,
Advice.empty()));
assertThat(descriptor)
.isNotEqualTo(
InstrumentDescriptor.create(
"name",
"description",
"unit",
InstrumentType.UP_DOWN_COUNTER,
InstrumentValueType.LONG));
InstrumentValueType.LONG,
Advice.empty()));
assertThat(descriptor)
.isNotEqualTo(
InstrumentDescriptor.create(
"name", "description", "unit", InstrumentType.COUNTER, InstrumentValueType.DOUBLE));
"name",
"description",
"unit",
InstrumentType.COUNTER,
InstrumentValueType.DOUBLE,
Advice.empty()));
}
}
23 changes: 15 additions & 8 deletions sdk/metrics/src/debugEnabledTest/java/io/opentelemetry/testing/SourceInfoTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import io.opentelemetry.sdk.metrics.InstrumentValueType;
import io.opentelemetry.sdk.metrics.View;
import io.opentelemetry.sdk.metrics.internal.debug.SourceInfo;
import io.opentelemetry.sdk.metrics.internal.descriptor.Advice;
import io.opentelemetry.sdk.metrics.internal.descriptor.InstrumentDescriptor;
import io.opentelemetry.sdk.metrics.internal.descriptor.MetricDescriptor;
import io.opentelemetry.sdk.metrics.internal.state.DebugUtils;
Expand All @@ -23,10 +24,10 @@ class SourceInfoTest {

@Test
void sourceInfoFindsStackTrace() {
assertThat(info.shortDebugString()).isEqualTo("SourceInfoTest.java:22");
assertThat(info.shortDebugString()).isEqualTo("SourceInfoTest.java:23");
assertThat(info.multiLineDebugString())
.startsWith(
"\tat io.opentelemetry.testing.SourceInfoTest.<clinit>(SourceInfoTest.java:22)\n");
"\tat io.opentelemetry.testing.SourceInfoTest.<clinit>(SourceInfoTest.java:23)\n");
}

@Test
Expand All @@ -40,7 +41,8 @@ void testDuplicateExceptionMessage_pureInstruments() {
"description",
"unit",
InstrumentType.OBSERVABLE_COUNTER,
InstrumentValueType.DOUBLE));
InstrumentValueType.DOUBLE,
Advice.empty()));
MetricDescriptor simpleWithNewDescription =
MetricDescriptor.create(
View.builder().build(),
Expand All @@ -50,7 +52,8 @@ void testDuplicateExceptionMessage_pureInstruments() {
"description2",
"unit2",
InstrumentType.COUNTER,
InstrumentValueType.LONG));
InstrumentValueType.LONG,
Advice.empty()));
assertThat(DebugUtils.duplicateMetricErrorMessage(simple, simpleWithNewDescription))
.contains("Found duplicate metric definition: name")
.contains("- InstrumentDescription [description2] does not match [description]")
Expand All @@ -76,7 +79,8 @@ void testDuplicateExceptionMessage_viewBasedConflict() {
"description",
"unit",
InstrumentType.HISTOGRAM,
InstrumentValueType.DOUBLE));
InstrumentValueType.DOUBLE,
Advice.empty()));
MetricDescriptor simpleWithNewDescription =
MetricDescriptor.create(
View.builder().build(),
Expand All @@ -86,7 +90,8 @@ void testDuplicateExceptionMessage_viewBasedConflict() {
"description2",
"unit",
InstrumentType.HISTOGRAM,
InstrumentValueType.DOUBLE));
InstrumentValueType.DOUBLE,
Advice.empty()));
assertThat(DebugUtils.duplicateMetricErrorMessage(simple, simpleWithNewDescription))
.contains("Found duplicate metric definition: name2")
.contains(simple.getSourceInstrument().getSourceInfo().multiLineDebugString())
Expand All @@ -111,7 +116,8 @@ void testDuplicateExceptionMessage_viewBasedConflict2() {
"description",
"unit2",
InstrumentType.HISTOGRAM,
InstrumentValueType.DOUBLE));
InstrumentValueType.DOUBLE,
Advice.empty()));
MetricDescriptor simpleWithNewDescription =
MetricDescriptor.create(
problemView,
Expand All @@ -121,7 +127,8 @@ void testDuplicateExceptionMessage_viewBasedConflict2() {
"description",
"unit",
InstrumentType.HISTOGRAM,
InstrumentValueType.DOUBLE));
InstrumentValueType.DOUBLE,
Advice.empty()));
assertThat(DebugUtils.duplicateMetricErrorMessage(simple, simpleWithNewDescription))
.contains("Found duplicate metric definition: name")
.contains("VIEW defined")
Expand Down
12 changes: 9 additions & 3 deletions sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/AbstractInstrumentBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import io.opentelemetry.api.internal.ValidationUtil;
import io.opentelemetry.api.metrics.ObservableDoubleMeasurement;
import io.opentelemetry.api.metrics.ObservableLongMeasurement;
import io.opentelemetry.sdk.metrics.internal.descriptor.Advice;
import io.opentelemetry.sdk.metrics.internal.descriptor.InstrumentDescriptor;
import io.opentelemetry.sdk.metrics.internal.state.CallbackRegistration;
import io.opentelemetry.sdk.metrics.internal.state.MeterProviderSharedState;
Expand All @@ -28,6 +29,7 @@ abstract class AbstractInstrumentBuilder<BuilderT extends AbstractInstrumentBuil
private final InstrumentValueType valueType;
private String description;
private String unit;
private Advice advice = Advice.empty();

protected final MeterSharedState meterSharedState;
protected final String instrumentName;
Expand Down Expand Up @@ -72,10 +74,14 @@ protected <T> T swapBuilder(SwapBuilder<T> swapper) {
meterProviderSharedState, meterSharedState, instrumentName, description, unit);
}

protected void setAdvice(Advice advice) {
this.advice = advice;
}

final <I extends AbstractInstrument> I buildSynchronousInstrument(
BiFunction<InstrumentDescriptor, WriteableMetricStorage, I> instrumentFactory) {
InstrumentDescriptor descriptor =
InstrumentDescriptor.create(instrumentName, description, unit, type, valueType);
InstrumentDescriptor.create(instrumentName, description, unit, type, valueType, advice);
WriteableMetricStorage storage =
meterSharedState.registerSynchronousMetricStorage(descriptor, meterProviderSharedState);
return instrumentFactory.apply(descriptor, storage);
Expand Down Expand Up @@ -103,15 +109,15 @@ final SdkObservableInstrument registerLongAsynchronousInstrument(

final SdkObservableMeasurement buildObservableMeasurement(InstrumentType type) {
InstrumentDescriptor descriptor =
InstrumentDescriptor.create(instrumentName, description, unit, type, valueType);
InstrumentDescriptor.create(instrumentName, description, unit, type, valueType, advice);
return meterSharedState.registerObservableMeasurement(descriptor);
}

@Override
public String toString() {
return this.getClass().getSimpleName()
+ "{descriptor="
+ InstrumentDescriptor.create(instrumentName, description, unit, type, valueType)
+ InstrumentDescriptor.create(instrumentName, description, unit, type, valueType, advice)
+ "}";
}

Expand Down
18 changes: 17 additions & 1 deletion sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkDoubleHistogram.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,17 @@
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.metrics.DoubleHistogram;
import io.opentelemetry.api.metrics.DoubleHistogramBuilder;
import io.opentelemetry.api.metrics.HistogramAdviceConfigurer;
import io.opentelemetry.api.metrics.LongHistogramBuilder;
import io.opentelemetry.context.Context;
import io.opentelemetry.sdk.internal.ThrottlingLogger;
import io.opentelemetry.sdk.metrics.internal.descriptor.Advice;
import io.opentelemetry.sdk.metrics.internal.descriptor.InstrumentDescriptor;
import io.opentelemetry.sdk.metrics.internal.state.MeterProviderSharedState;
import io.opentelemetry.sdk.metrics.internal.state.MeterSharedState;
import io.opentelemetry.sdk.metrics.internal.state.WriteableMetricStorage;
import java.util.List;
import java.util.function.Consumer;
import java.util.logging.Level;
import java.util.logging.Logger;

Expand Down Expand Up @@ -54,7 +58,7 @@ public void record(double value) {

static final class SdkDoubleHistogramBuilder
extends AbstractInstrumentBuilder<SdkDoubleHistogramBuilder>
implements DoubleHistogramBuilder {
implements DoubleHistogramBuilder, HistogramAdviceConfigurer {

SdkDoubleHistogramBuilder(
MeterProviderSharedState meterProviderSharedState,
Expand All @@ -75,6 +79,12 @@ protected SdkDoubleHistogramBuilder getThis() {
return this;
}

@Override
public SdkDoubleHistogramBuilder setAdvice(Consumer<HistogramAdviceConfigurer> adviceConsumer) {
adviceConsumer.accept(this);
return this;
}

@Override
public SdkDoubleHistogram build() {
return buildSynchronousInstrument(SdkDoubleHistogram::new);
Expand All @@ -84,5 +94,11 @@ public SdkDoubleHistogram build() {
public LongHistogramBuilder ofLongs() {
return swapBuilder(SdkLongHistogram.SdkLongHistogramBuilder::new);
}

@Override
public HistogramAdviceConfigurer setBoundaries(List<Double> bucketBoundaries) {
setAdvice(Advice.create(bucketBoundaries));
return this;
}
}
}
Loading