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

jack-berg · 2023-02-15T00:24:57Z

A prototype extension to the metric API to allow instrumentation authors to optionally provide advice on histogram bucket boundaries. Advice only applies if there are no matching views and if the reader has no preference for the histogram bucket boundaries.

Here's a code snippet that demonstrates usage and behavior:

    InMemoryMetricReader reader = InMemoryMetricReader.create();
    meterProvider = SdkMeterProvider.builder().registerMetricReader(reader).build();
    DoubleHistogram doubleHistogram =
        ((ExtendedDoubleHistogramBuilder) meterProvider
            .get("meter")
            .histogramBuilder("histogram"))
            .setAdvice(
                advice -> advice.setExplicitBucketBoundaries(Arrays.asList(10.0, 20.0, 30.0)))
            .build();

    doubleHistogram.record(5.0);
    doubleHistogram.record(15.0);
    doubleHistogram.record(25.0);
    doubleHistogram.record(35.0);

    // Bucket bounds from advice should be used
    assertThat(reader.collectAllMetrics())
        .satisfiesExactly(
            metric ->
                assertThat(metric)
                    .hasName("histogram")
                    .hasHistogramSatisfying(
                        histogram ->
                            histogram.hasPointsSatisfying(
                                point ->
                                    point
                                        .hasBucketCounts(1, 1, 1, 1)
                                        .hasBucketBoundaries(10.0, 20.0, 30.0))));

codecov · 2023-02-15T00:45:16Z

Codecov Report

Patch coverage: 95.12% and project coverage change: -0.03 ⚠️

Comparison is base (2babc69) 90.98% compared to head (34b6a73) 90.96%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5217      +/-   ##
============================================
- Coverage     90.98%   90.96%   -0.03%     
- Complexity     4908     4918      +10     
============================================
  Files           552      555       +3     
  Lines         14489    14518      +29     
  Branches       1372     1373       +1     
============================================
+ Hits          13183    13206      +23     
- Misses          906      911       +5     
- Partials        400      401       +1

Impacted Files	Coverage Δ
...ubator/metrics/ExtendedDoubleHistogramBuilder.java	`0.00% <0.00%> (ø)`
...ncubator/metrics/ExtendedLongHistogramBuilder.java	`0.00% <0.00%> (ø)`
...io/opentelemetry/sdk/metrics/SdkDoubleCounter.java	`100.00% <ø> (ø)`
...ntelemetry/sdk/metrics/SdkDoubleUpDownCounter.java	`100.00% <ø> (ø)`
...opentelemetry/sdk/metrics/SdkLongGaugeBuilder.java	`100.00% <ø> (ø)`
...lemetry/sdk/metrics/AbstractInstrumentBuilder.java	`98.14% <100.00%> (+0.23%)`	⬆️
.../opentelemetry/sdk/metrics/SdkDoubleHistogram.java	`100.00% <100.00%> (ø)`
...io/opentelemetry/sdk/metrics/SdkLongHistogram.java	`100.00% <100.00%> (ø)`
...emetry/sdk/metrics/internal/descriptor/Advice.java	`100.00% <100.00%> (ø)`
...rics/internal/descriptor/InstrumentDescriptor.java	`100.00% <100.00%> (ø)`
... and 3 more

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

mateuszrzeszutek

Awesome!

mateuszrzeszutek · 2023-02-16T10:25:48Z

sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/AdviceTest.java

+
+class AdviceTest {
+
+  private SdkMeterProvider meterProvider = SdkMeterProvider.builder().build();


Nit: this value does not seem to be used anywhere

mateuszrzeszutek · 2023-02-16T10:26:57Z

api/all/src/main/java/io/opentelemetry/api/metrics/DoubleHistogramBuilder.java

@@ -32,6 +34,11 @@ public interface DoubleHistogramBuilder {
   */
  DoubleHistogramBuilder setUnit(String unit);

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


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

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.

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(...)

jkwatson · 2023-02-16T17:17:06Z

api/all/src/main/java/io/opentelemetry/api/metrics/HistogramAdviceConfigurer.java

+import java.util.List;
+
+/** Configure advice for implementations of {@link LongHistogram} and {@link DoubleHistogram}. */
+public interface HistogramAdviceConfigurer {


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

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.

…y-java into histogram-hint-api

jack-berg · 2023-03-09T21:17:44Z

Marked this PR as ready for review. The idea has reasonable support in the specification, and I will continue to drive it. In the meantime, I've adjusted the PR to move the advice APIs to opentelemetry-extension-incubator, so its actually in a reasonable state to merge.

mateuszrzeszutek

This is awesome 💯🚀

jack-berg · 2023-04-07T22:13:00Z

Hey @open-telemetry/java-instrumentation-maintainers if the advice API were to be merged in this form, in which its exposed in the extensions API component, would we be able to use it in instrumentation? Also, would we be comfortable using an experimental API in instrumentation?

I think it would be really useful in cases like the gc duration histogram metric, which would benefit a lot from tuning the buckets to match the duration we typically expect for garbage collection events.

Fixes #2229. Related to #3061 (lays groundwork but does not resolve). Related to #2977, which may use this new API to have `http.server.duration` report in seconds instead of ms without changing / breaking default bucket boundaries. Summary of the change: - Proposes a new parameter to optionally include when creating instruments, called "advice". - For the moment, advice only has one parameter for specifying the bucket boundaries of explicit bucket histogram. - Advice can be expanded with additional parameters in the future (e.g. default attributes to retain). The parameters may be general (aka applicable to all instruments) or specific to a particular instrument kind, like bucket boundaries. - Advice parameters can influence the [default aggregation](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#default-aggregation), which is used if there is no matching view and if the reader does not specify a preferred aggregation. - Not clear that all advice will be oriented towards configuring aggregation, so I've intentionally left the scope of what they can influence open ended. I've prototyped this in java [here](open-telemetry/opentelemetry-java#5217). Example usage: ``` DoubleHistogram doubleHistogram = meterProvider .get("meter") .histogramBuilder("histogram") .setUnit("foo") .setDescription("bar") .setAdvice( advice -> advice.setBoundaries(Arrays.asList(10.0, 20.0, 30.0))) .build(); ``` Advice could easily be changed to "hint" with everything else being equal. I thought "advice" clearly described what we're trying to accomplish, which is advice / recommend the implementation in providing useful output with minimal configuration. --------- Co-authored-by: Reiley Yang <reyang@microsoft.com>

mateuszrzeszutek · 2023-04-11T13:20:22Z

would we be able to use it in instrumentation?

I think so; we'll just have to put extensions on the bootstrap classpath.

Also, would we be comfortable using an experimental API in instrumentation?

Personally: as long as we're not exposing this in any public API, sure 💯. I've been hoping to use the advice API in the micrometer bridge for a while.

jack-berg · 2023-04-11T15:37:09Z

That's great @mateuszrzeszutek.

@jkwatson can you take a look at this when you have a chance? The spec PR proposing this change has been merged.

…telemetry#3216) Fixes open-telemetry#2229. Related to open-telemetry#3061 (lays groundwork but does not resolve). Related to open-telemetry#2977, which may use this new API to have `http.server.duration` report in seconds instead of ms without changing / breaking default bucket boundaries. Summary of the change: - Proposes a new parameter to optionally include when creating instruments, called "advice". - For the moment, advice only has one parameter for specifying the bucket boundaries of explicit bucket histogram. - Advice can be expanded with additional parameters in the future (e.g. default attributes to retain). The parameters may be general (aka applicable to all instruments) or specific to a particular instrument kind, like bucket boundaries. - Advice parameters can influence the [default aggregation](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#default-aggregation), which is used if there is no matching view and if the reader does not specify a preferred aggregation. - Not clear that all advice will be oriented towards configuring aggregation, so I've intentionally left the scope of what they can influence open ended. I've prototyped this in java [here](open-telemetry/opentelemetry-java#5217). Example usage: ``` DoubleHistogram doubleHistogram = meterProvider .get("meter") .histogramBuilder("histogram") .setUnit("foo") .setDescription("bar") .setAdvice( advice -> advice.setBoundaries(Arrays.asList(10.0, 20.0, 30.0))) .build(); ``` Advice could easily be changed to "hint" with everything else being equal. I thought "advice" clearly described what we're trying to accomplish, which is advice / recommend the implementation in providing useful output with minimal configuration. --------- Co-authored-by: Reiley Yang <reyang@microsoft.com>

Prototype histogram advice API

678d230

Rename to setAdvice

30c161a

jack-berg changed the title ~~Prototype histogram advice API~~ Prototype histogram advice API (i.e. Hints) Feb 15, 2023

jack-berg added 2 commits February 15, 2023 18:04

Advice influences default aggregation

1a448f7

Rename to setBoundaries

b1062df

jack-berg mentioned this pull request Feb 16, 2023

Propose histogram bucket boundary metric advice (aka hint API) open-telemetry/opentelemetry-specification#3216

Merged

mateuszrzeszutek approved these changes Feb 16, 2023

View reviewed changes

japicmp

385b13c

jkwatson reviewed Feb 16, 2023

View reviewed changes

jack-berg added the prototype label Feb 27, 2023

jack-berg added 2 commits March 9, 2023 14:14

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

aec5ce6

…y-java into histogram-hint-api

Move histogrma advice to :extensions:incubator

7d4d24e

jack-berg marked this pull request as ready for review March 9, 2023 21:16

jack-berg force-pushed the histogram-hint-api branch from 077463c to 7d4d24e Compare March 9, 2023 21:16

jack-berg requested a review from a team March 9, 2023 21:16

streamline test

34b6a73

mateuszrzeszutek approved these changes Mar 29, 2023

View reviewed changes

jkwatson approved these changes Apr 11, 2023

View reviewed changes

jack-berg removed the prototype label Apr 11, 2023

jack-berg merged commit 2870209 into open-telemetry:main Apr 11, 2023

JamesNK mentioned this pull request Jun 5, 2023

Add "hints" in Metric API to provide things like histogram bounds dotnet/runtime#63650

Closed

sebastian-alfers mentioned this pull request Jun 22, 2023

ApplicationDoubleHistogramBuilder cannot be cast to ExtendedDoubleHistogramBuilder microsoft/ApplicationInsights-Java#3151

Closed

mateuszrzeszutek mentioned this pull request Aug 25, 2023

Update Java compliance matrix with metrics advice open-telemetry/opentelemetry-specification#3675

Merged

trask mentioned this pull request Sep 12, 2023

Mark ExplicitBucketBoundaries hint API as stable open-telemetry/opentelemetry-specification#3391

Closed

trask mentioned this pull request Sep 18, 2023

Stabilize ExplicitBucketBoundaries instrument advisory parameter open-telemetry/opentelemetry-specification#3694

Merged

lsfischer mentioned this pull request Aug 21, 2024

Implement the histogram bucket advise API open-telemetry/opentelemetry-python#4140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jack-berg commented Feb 15, 2023 •

edited

Loading

codecov bot commented Feb 15, 2023 •

edited

Loading

mateuszrzeszutek left a comment

mateuszrzeszutek Feb 16, 2023

mateuszrzeszutek Feb 16, 2023

jack-berg Feb 23, 2023

jack-berg Mar 9, 2023

jkwatson Feb 16, 2023

jack-berg Feb 23, 2023

jack-berg commented Mar 9, 2023

mateuszrzeszutek left a comment

jack-berg commented Apr 7, 2023

mateuszrzeszutek commented Apr 11, 2023

jack-berg commented Apr 11, 2023


		class AdviceTest {

		private SdkMeterProvider meterProvider = SdkMeterProvider.builder().build();

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

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

Conversation

jack-berg commented Feb 15, 2023 • edited Loading

codecov bot commented Feb 15, 2023 • edited Loading

Codecov Report

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

mateuszrzeszutek Feb 16, 2023

Choose a reason for hiding this comment

mateuszrzeszutek Feb 16, 2023

Choose a reason for hiding this comment

jack-berg Feb 23, 2023

Choose a reason for hiding this comment

jack-berg Mar 9, 2023

Choose a reason for hiding this comment

jkwatson Feb 16, 2023

Choose a reason for hiding this comment

jack-berg Feb 23, 2023

Choose a reason for hiding this comment

jack-berg commented Mar 9, 2023

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

jack-berg commented Apr 7, 2023

mateuszrzeszutek commented Apr 11, 2023

jack-berg commented Apr 11, 2023

jack-berg commented Feb 15, 2023 •

edited

Loading

codecov bot commented Feb 15, 2023 •

edited

Loading