Supports Histogram Aggregation

[beanliu]

Support fixed bucket histogram aggregation and exporting them with OTLP/Prometheus exporter.

Since the View API is not part of the public Metric API yet (optentelemetry-specification#466), there's no universal way to configure custom aggregators for different instruments. However, the SdkMeterProvider provides a registerView method which allows developers to specify aggregations for certain instruments. This suggests that we could do the following to achieve histogram aggregations:

private static DoubleValueRecorder tryNewHistogramValueRecorder(
    MeterProvider meterProvider,
    Meter meter,
    String name,
    double[] boundaries) {
  if (meterProvider.getClass() == SdkMeterProvider.class) {
    // `registerView` is not part of the public Meter API yet, so here we use a cast to do a bit
    // of trick just to demonstrate the histogram usage.
    SdkMeterProvider sdkMeterProvider = (SdkMeterProvider) meterProvider;
    sdkMeterProvider
        .registerView(InstrumentSelector.builder().setInstrumentNameRegex(name).setInstrumentType(
            InstrumentType.VALUE_RECORDER).build(), AggregatorFactory.histogram(boundaries, /* stateful= */ false));
  }

// in the application/instrumentation code
DoubleValueRecorder mRequestLatency = tryNewHistogramValueRecorder(meterProvider, meter,
        "example_request_latency", new double[] {50, 200, 300});
DoubleValueRecorder mResponseSize = tryNewHistogramValueRecorder(meterProvider, meter,
    "example_response_size", new double[] {1_000, 5_000, 20_000});

And we could upgrade the example to use View API once they are ready.

[jkwatson]

Thanks for doing this work. This PR is a little big to review efficiently. Can you break it up into smaller bits? Maybe

1. Implementation of the core SDK pieces, with no exporter implementation
2. Add the OTLP exporter implementation
3. Add the prometheus exporter implementation

Thanks!

[beanliu]

Thanks for doing this work. This PR is a little big to review efficiently. Can you break it up into smaller bits? Maybe

1. Implementation of the core SDK pieces, with no exporter implementation
2. Add the OTLP exporter implementation
3. Add the prometheus exporter implementation

Thanks!

Thanks for the quick review.
The commits are already organised in this way:

• core SDK implementation of histogram: 62a3a2e.
  • fix up: export non-cumulative counts to fit the definition of OTLP histogram 2a03934.
• Add histogram type to MetricData: 62510be.
• OTLP exporter implementation: a793b26.
• Prometheus exporter implementation: 898ca79.
  • fix up: assume histogram counts are not cumulative while exporting 0967613.

I don't have much experience doing Github reviews myself so I'm not sure if it serves the same. Please let me know if breaking them into separate PRs would provide a better review process.

[codecov]

Codecov Report

Merging #2575 (2a03934) into master (3acbc5a) will increase coverage by 87.30%.
The diff coverage is 93.91%.

@@              Coverage Diff              @@
##             master    #2575       +/-   ##
=============================================
+ Coverage          0   87.30%   +87.30%     
- Complexity        0     2678     +2678     
=============================================
  Files             0      323      +323     
  Lines             0     8953     +8953     
  Branches          0      910      +910     
=============================================
+ Hits              0     7816     +7816     
- Misses            0      846      +846     
- Partials          0      291      +291     

Impacted Files	Coverage Δ	Complexity Δ
...metrics/aggregator/HistogramAggregatorFactory.java	80.00% <80.00%> (ø)	6.00 <6.00> (?)
.../metrics/aggregator/DoubleHistogramAggregator.java	90.90% <90.90%> (ø)	7.00 <7.00> (?)
...entelemetry/exporter/prometheus/MetricAdapter.java	93.61% <100.00%> (ø)	30.00 <2.00> (?)
...telemetry/sdk/extension/otproto/MetricAdapter.java	96.81% <100.00%> (ø)	33.00 <3.00> (?)
...etry/sdk/metrics/aggregator/AggregatorFactory.java	100.00% <100.00%> (ø)	5.00 <1.00> (?)
.../sdk/metrics/aggregator/HistogramAccumulation.java	100.00% <100.00%> (ø)	3.00 <3.00> (?)
...emetry/sdk/metrics/aggregator/MetricDataUtils.java	100.00% <100.00%> (ø)	8.00 <2.00> (?)
...elemetry/sdk/metrics/data/DoubleHistogramData.java	100.00% <100.00%> (ø)	2.00 <2.00> (?)
...try/sdk/metrics/data/DoubleHistogramPointData.java	100.00% <100.00%> (ø)	4.00 <4.00> (?)
.../io/opentelemetry/sdk/metrics/data/MetricData.java	94.59% <100.00%> (ø)	20.00 <3.00> (?)
... and 324 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3acbc5a...2a03934. Read the comment docs.

[jkwatson]

Thanks for doing this work. This PR is a little big to review efficiently. Can you break it up into smaller bits? Maybe

1. Implementation of the core SDK pieces, with no exporter implementation
2. Add the OTLP exporter implementation
3. Add the prometheus exporter implementation

Thanks!

Thanks for the quick review.
The commits are already organised in this way:

• core SDK implementation of histogram: 62a3a2e.

  • fix up: export non-cumulative counts to fit the definition of OTLP histogram 2a03934.

• Add histogram type to MetricData: 62510be.

• OTLP exporter implementation: a793b26.

• Prometheus exporter implementation: 898ca79.

  • fix up: assume histogram counts are not cumulative while exporting 0967613.

I don't have much experience doing Github reviews myself so I'm not sure if it serves the same. Please let me know if breaking them into separate PRs would provide a better review process.

separate PRs would definitely help, if that's possible. thanks!

[beanliu]

separate PRs would definitely help, if that's possible. thanks!

I've separated the PR into the following:

• Histogram Aggregation - core SDK implementation #2581 Histogram Aggregation - core SDK implementation
• Histogram Aggregation - OTLP exporter #2582 Histogram Aggregation - OTLP exporter
• Histogram Aggregation - Prometheus exporter #2583 Histogram Aggregation - Prometheus exporter

The caveat is that they all share two common commits in order to make the checks happy. but they should result no conflict in the merge.

[jkwatson]

awesome. I'll take a look at them in more detail very soon.