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

[PoC] Implement Metric Producer specification #5678

Closed
wants to merge 8 commits into from

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Aug 3, 2023

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

It includes open-telemetry/opentelemetry-specification#3613, as producers are passed to readers as configuration, rather than via a registration method.

The tricky part was how to handle Resource (see open-telemetry/opentelemetry-specification#3636). This is tricky because getResource is part of MetricData, but we would rather the OpenCensus shim not need to understand resource. In this PoC, i've opted to split the MetricProducer interface into two interfaces: SdkMetricProducer, and MetricProducer.

  • SdkMetricProducer can provide the resource to readers, and returns batches of metrics that already include Resource (MetricData)
  • MetricProducer returns batches of metrics that do not include Resource (ScopeMetricData).

Alternatively, the produce() function on MetricProducer could accept Resource as a parameter, and return MetricData. That would introduce less of an API surface, but could be misused.

example usage:

SdkMeterProvider.builder().registerMetricReader(PeriodicMetricReader.builder(exporter).addMetricProducer(OpenCensusMetricProducer.create()).build())

TODOs:

  • Unit tests
  • Fix the prometheus unit test

@jack-berg @jsuereth

@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Patch coverage: 57.54% and project coverage change: -0.70% ⚠️

Comparison is base (7ae32c9) 91.31% compared to head (39b347b) 90.61%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5678      +/-   ##
============================================
- Coverage     91.31%   90.61%   -0.70%     
- Complexity     4978     4988      +10     
============================================
  Files           554      555       +1     
  Lines         14731    14855     +124     
  Branches       1376     1388      +12     
============================================
+ Hits          13451    13461      +10     
- Misses          884      969      +85     
- Partials        396      425      +29     
Files Changed Coverage Δ
...ry/sdk/metrics/internal/export/MetricProducer.java 0.00% <ø> (-50.00%) ⬇️
...pentelemetry/sdk/metrics/data/ScopeMetricData.java 27.27% <27.27%> (ø)
...porter/prometheus/PrometheusHttpServerBuilder.java 59.09% <40.00%> (-40.91%) ⬇️
...lemetry/sdk/testing/assertj/ScopeMetricAssert.java 43.75% <43.75%> (ø)
...metry/sdk/metrics/export/PeriodicMetricReader.java 78.48% <46.15%> (-7.24%) ⬇️
...dk/metrics/export/PeriodicMetricReaderBuilder.java 86.95% <50.00%> (-13.05%) ⬇️
...etry/exporter/prometheus/PrometheusHttpServer.java 55.93% <53.33%> (-24.81%) ⬇️
...sdk/metrics/internal/export/SdkMetricProducer.java 77.77% <77.77%> (ø)
...y/opencensusshim/OpenTelemetryMetricsExporter.java 85.29% <87.50%> (+1.96%) ⬆️
...etrics/internal/data/ImmutableScopeMetricData.java 88.88% <88.88%> (ø)
... and 7 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant