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

Associate views with MeterProvider instead of Reader #3387

Merged
merged 14 commits into from
Oct 31, 2022

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Oct 21, 2022

Resolve #3241
Resolve #3210
Resolve #3085

Follow the v1.14.0 version of the metric SDK specification and register views with a MeterProvider instead of with a Reader.

This change naturally includes a resolution to #3085 as there is no longer a need to map readers to views.

@MrAlias MrAlias added pkg:SDK Related to an SDK package area:metrics Part of OpenTelemetry Metrics labels Oct 21, 2022
@codecov
Copy link

codecov bot commented Oct 21, 2022

Codecov Report

Merging #3387 (d30ca62) into main (c8a13d6) will decrease coverage by 0.0%.
The diff coverage is 84.6%.

❗ Current head d30ca62 differs from pull request most recent head 0692303. Consider uploading reports for the commit 0692303 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #3387     +/-   ##
=======================================
- Coverage   77.9%   77.9%   -0.1%     
=======================================
  Files        164     164             
  Lines      11361   11366      +5     
=======================================
- Hits        8861    8859      -2     
- Misses      2301    2307      +6     
- Partials     199     200      +1     
Impacted Files Coverage Δ
sdk/metric/config.go 94.3% <77.7%> (-5.7%) ⬇️
sdk/metric/pipeline.go 93.4% <100.0%> (ø)
sdk/metric/provider.go 100.0% <100.0%> (ø)
sdk/trace/batch_span_processor.go 80.2% <0.0%> (-1.8%) ⬇️
codes/codes.go 90.4% <0.0%> (ø)

Copy link
Contributor

@MadVikingGod MadVikingGod left a comment

Choose a reason for hiding this comment

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

When it was last discussed, we took a views-per-reader approach specifically to support the use case: High-frequency export of a subset of metrics, and a low-frequency export of a full set. E.g. every 10 seconds get status + response counts, and for every minute get the histogram of response times + everything else.

It doesn't look like we can support this use case with this approach.

That being said, we are the only project that does this.

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 31, 2022

That being said, we are the only project that does this.

I think this is the key. If the OTel community decides to add support for this in the future we can definitely add it back. 👍

@MrAlias MrAlias merged commit d1aca7b into open-telemetry:main Oct 31, 2022
@MrAlias MrAlias deleted the view-refactor branch October 31, 2022 14:55
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
Status: Done
3 participants