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.

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

Add Exemplar to metricdata package #3849

Merged
merged 14 commits into from
Mar 14, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Mar 8, 2023

Part of #3834

  • Add the new Exemplar[N int64 | float64] struct to sdk/metric/metricdata
  • Add Exemplars []Exemplar[N] field to DataPoint in sdk/metric/metricdata
  • Add Exemplars []Exemplar[N] field to HistogramDataPoint in sdk/metric/metricdata
    • Update HistogramDataPoint to be defined generically as HistogramDataPoint[N int64 | float64]
    • Update Histogram to be defined generically as Histogram[N int64 | float64]
  • Update metricdatatest to support Exemplar
  • Update metricdatatest to support HistogramDataPoint[N]
  • Update sdk/metric histogram aggregator to produce Histogram[N]
  • Update exporters (stdout, Prometheus, OTLP) to compile with Histogram[N]
    • These exporters still need to be updated to export Exemplar
  • Update OpenCensus bridge to support Histogram[N] (only produced Histogram[float64])

@MrAlias MrAlias force-pushed the metricdata-exemplars branch from 2e6cf74 to 35ad3cb Compare March 8, 2023 20:12
@MrAlias MrAlias added this to the Metric SDK v0.38.0 milestone Mar 8, 2023
@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Mar 8, 2023
@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #3849 (8e6506a) into main (b62eb2c) will decrease coverage by 0.2%.
The diff coverage is 72.6%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3849     +/-   ##
=======================================
- Coverage   81.7%   81.6%   -0.2%     
=======================================
  Files        169     169             
  Lines      12581   12723    +142     
=======================================
+ Hits       10288   10387     +99     
- Misses      2078    2117     +39     
- Partials     215     219      +4     
Impacted Files Coverage Δ
sdk/metric/metricdata/data.go 57.1% <0.0%> (ø)
exporters/stdout/stdoutmetric/exporter.go 92.7% <50.0%> (-4.5%) ⬇️
...dk/metric/metricdata/metricdatatest/comparisons.go 88.2% <62.7%> (-9.4%) ⬇️
bridge/opencensus/internal/ocmetric/metric.go 93.3% <100.0%> (ø)
...s/otlp/otlpmetric/internal/transform/metricdata.go 100.0% <100.0%> (ø)
exporters/prometheus/exporter.go 83.7% <100.0%> (+0.1%) ⬆️
sdk/metric/internal/histogram.go 100.0% <100.0%> (ø)
sdk/metric/metricdata/metricdatatest/assertion.go 87.0% <100.0%> (+2.9%) ⬆️

... and 2 files with indirect coverage changes

@MrAlias MrAlias force-pushed the metricdata-exemplars branch 2 times, most recently from f06d7aa to 0d2173e Compare March 8, 2023 20:24
@MrAlias MrAlias force-pushed the metricdata-exemplars branch from 0d2173e to 215fdb9 Compare March 8, 2023 20:30
@MrAlias MrAlias marked this pull request as ready for review March 8, 2023 23:18
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

General question, just to clarify: do we want to implement "Exemplar" before it is Stable in the specification?

Reference: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exemplar

sdk/metric/metricdata/data.go Show resolved Hide resolved
@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 9, 2023

General question, just to clarify: do we want to implement "Exemplar" before it is Stable in the specification?

Reference: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exemplar

I'm not sure we want to implement it given the unstable nature of the specification around it. That and I think the specification has some evolving to do regarding the exemplar.

That said, I do want to make sure we can add support in the future. These changes are necessary now so we don't have to implement something sub-par to support them in the future or introduce backwards incompatible changes later.

As pointed out in your other comment, I think there is still some follow-up work to address here.

  1. The other fields of the HistogramDataPoint can now utilize the generic argument to give native values
  2. The exporters can be updated to "support" exemplars as they will be in the data structure passed to them (though the SDK will not produce them yet)

I think (2) is helpful on the path to supporting exemplars, but not needed for the GA of the specification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:metrics Part of OpenTelemetry Metrics pkg:SDK Related to an SDK package
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants