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 Metric Producer as a new interface, which returns scope metrics #3524

Merged
merged 15 commits into from
Dec 15, 2022

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Dec 7, 2022

Issue: #3504

Implements open-telemetry/opentelemetry-specification#2951.

This adds metric.Producer as an additional interface beyond the current producer interface used for the SDK. It has the advantage of scoping metrics from the Producer to only ScopeMetrics, instead of ResourceMetrics.

As an alternative, I explored #3523, which would re-use the existing producer interface, and make it shared by the SDK and bridge implementations.

@MrAlias
Copy link
Contributor

MrAlias commented Dec 8, 2022

Reviewers: looking for high-level feedback on interface design here.

@dashpole dashpole force-pushed the other_metricproducer branch from a88a52e to 8631d4e Compare December 9, 2022 16:36
@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Merging #3524 (09eef44) into main (4e76347) will increase coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3524     +/-   ##
=======================================
+ Coverage   77.9%   78.0%   +0.1%     
=======================================
  Files        164     164             
  Lines      11703   11762     +59     
=======================================
+ Hits        9123    9182     +59     
  Misses      2385    2385             
  Partials     195     195             
Impacted Files Coverage Δ
sdk/metric/reader.go 100.0% <ø> (ø)
sdk/metric/config.go 94.5% <100.0%> (+0.2%) ⬆️
sdk/metric/manual_reader.go 84.6% <100.0%> (+5.9%) ⬆️
sdk/metric/periodic_reader.go 91.3% <100.0%> (+1.8%) ⬆️

@dashpole
Copy link
Contributor Author

dashpole commented Dec 9, 2022

This is now ready for review.

@dashpole dashpole changed the title PoC: add Metric Producer as a new interface, and scope to only scope metrics Add Metric Producer as a new interface, which returns scope metrics Dec 9, 2022
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Looks good overall, only minor questions/suggestions.

CHANGELOG.md Outdated Show resolved Hide resolved
sdk/metric/manual_reader.go Outdated Show resolved Hide resolved
sdk/metric/periodic_reader.go Show resolved Hide resolved
Co-authored-by: Tyler Yahn <[email protected]>
@MrAlias MrAlias mentioned this pull request Dec 9, 2022
3 tasks
@MrAlias MrAlias merged commit 14a17b3 into open-telemetry:main Dec 15, 2022
@MrAlias MrAlias added this to the Metric v0.35.0 milestone Jan 27, 2023
@MrAlias MrAlias mentioned this pull request Jan 28, 2023
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.

5 participants