-
Notifications
You must be signed in to change notification settings - Fork 821
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
feat(metrics-sdk): bootstrap views api #2625
Conversation
d6d62c1
to
2cdfc4a
Compare
2cdfc4a
to
c635ba6
Compare
Codecov Report
@@ Coverage Diff @@
## main #2625 +/- ##
==========================================
- Coverage 93.26% 93.08% -0.18%
==========================================
Files 115 123 +8
Lines 4645 4761 +116
Branches 1036 1061 +25
==========================================
+ Hits 4332 4432 +100
- Misses 313 329 +16
|
experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts
Outdated
Show resolved
Hide resolved
return this._sharedState.resource; | ||
} | ||
|
||
getMeter(name: string, version = '', options: metrics.MeterOptions = {}): metrics.Meter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these are whitespace changes: changed 4 spaces indentation to 2 spaces indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I prefer your approach of passing down a shared state class instead of exposing an aggregation function on the meter.
I don't see any handling of the case where there is no view registered. Am I just missing it?
experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/Meter.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/view/ViewRegistry.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/view/Predicate.ts
Show resolved
Hide resolved
It's handled in the And there are test coverages: https://github.com/open-telemetry/opentelemetry-js/pull/2625/files#diff-95e2da7680fa6c3f0dbc3e43fc9ec175ebe45f651e9bfb8e18f31717f2347551R43 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@open-telemetry/javascript-approvers Please take a look at this PR it is holding up other work until it can be merged |
Which problem is this PR solving?
Bootstrap the skeleton of the views API and basic views matching.
Unblocks the metrics stream implementation: provides a way to configure Aggregations.
This PR is not a complete view implementation.
Related: #2592
Short description of the changes
Processor.prototype.aggregatorFor
).Type of change
Checklist: