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

Metric View API Prototype #1473

Closed
wants to merge 11 commits into from

Conversation

beanliu
Copy link

@beanliu beanliu commented Jan 18, 2021

View API was discussed in the OTEP which is not merged yet (OTEP#89). Having a prototype may push this forward (optentelemetry-specification#466).

Inspired by the Python View API Prototype but the approach for the Golang SDK is a bit different.

View only works for synchronous instruments since the semantics for asynchronous instruments is undefined at the moment. So observers functionality does not change.

The uniqueness of a view is defined by a tuple of instrument, label keys and its aggregator factory. Note that an Ungroup view is the same with defining an empty pair of label keys.

Summary of changes:

  • Move Aggregator, Aggregation and AggregatorSelector interfaces to otel/metric package to avoid concept dependency cycle. (commit)
  • Add View type and RegisterView method as part of the Metric API. (commit)
  • Support labels with values unset since this may be a valid case for views. (commit)
  • View implementation: (commit)
    • If no views are defined for a metric, a default one that's compatible with the previous semantics is registered automatically while taking a measurement.
    • Bound instruments now has a sequence of aggregators if the metric has more than one view. But views registered after the creation of the bound instrument have no effect on the bind object.
    • Views are stored inside the associate instrument object. A measurement would be recorded for each view.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 18, 2021

CLA Signed

The committers are authorized under a signed CLA.

@beanliu beanliu changed the title Metric view api Metric View API Prototype Jan 18, 2021
@codecov
Copy link

codecov bot commented Jan 18, 2021

Codecov Report

Merging #1473 (a56d2ad) into main (1952d7b) will decrease coverage by 0.2%.
The diff coverage is 87.2%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #1473     +/-   ##
=======================================
- Coverage   79.2%   79.0%   -0.3%     
=======================================
  Files        127     129      +2     
  Lines       6659    6768    +109     
=======================================
+ Hits        5280    5350     +70     
- Misses      1124    1163     +39     
  Partials     255     255             
Impacted Files Coverage Δ
exporters/metric/prometheus/prometheus.go 64.8% <ø> (ø)
exporters/otlp/internal/transform/metric.go 80.5% <ø> (ø)
exporters/otlp/otlp.go 90.9% <ø> (ø)
exporters/stdout/metric.go 76.3% <ø> (ø)
internal/global/meter.go 90.3% <ø> (ø)
metric/aggregation/aggregation.go 0.0% <ø> (ø)
metric/metric_sdkapi.go 100.0% <ø> (ø)
metric/viewlabelconfig_string.go 0.0% <0.0%> (ø)
sdk/metric/aggregator/aggregator.go 100.0% <ø> (ø)
sdk/metric/aggregator/aggregatortest/test.go 84.8% <ø> (ø)
... and 26 more

@beanliu beanliu force-pushed the metric-view-api branch 2 times, most recently from d8ed3a0 to 0fba138 Compare January 19, 2021 11:34
Base automatically changed from master to main January 29, 2021 19:39
@MrAlias
Copy link
Contributor

MrAlias commented Apr 6, 2021

Closing as the metric's part of the specification is going through a large refactor currently and we will likely need to reevaluate the entire signal once it stabilizes, including how we want to approach views.

@MrAlias MrAlias closed this Apr 6, 2021
@ProvoK
Copy link

ProvoK commented May 3, 2021

Any news on this? Not being able to specify buckets per single metric(s) is a real blocker for a lot of scenarios.

Is there a way to possible help?

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.

4 participants