-
Notifications
You must be signed in to change notification settings - Fork 423
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
Update metrics API and SDK #819
Conversation
This change would have been nicer in smaller pieces, but many of them rely on each other. |
6228f84
to
c5fd910
Compare
This change aligns metrics with the spec, changes include: * Rename `MeterProvider::meter` to `MeterProvider::versioned_meter` for consistency with `TracerProvider` trait. * Move metrics sdk api types to `opentelemetry-sdk` * Consolidate instrument builders into `InstrumentBuilder` * Remove value observers and add gauges. * Move from batch observer to registered callbacks. * Rename `ExportKindFor` to `TemporalitySelector` * Consolidate `PushController` and `PullController` into `BasicController` * Remove `MinMaxSumCountAggregator` and `ArrayAggregator` * Update examples and exporters for new api/sdk
cc @hdost |
for the #733 I think this looks good, However I do wonder if we might want to also add some convenience methods, could be in a followup PR of allowing users to not need to always pass the context. I can open up a separate ticket for this. I'm sure from a performance perspective passing the Context is more performant. |
Although seeing this #828 it's possible this, perhaps there's some further research to be done on ensuring it's working as expected ingeneral. |
Sorry I have been busy moving. Will try to take a look today |
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.
Overall looks good. Just some nits
Co-authored-by: Zhongyang Wu <zhongyang.wu@outlook.com>
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 👍
This change aligns metrics with the spec, changes include:
MeterProvider::meter
toMeterProvider::versioned_meter
forconsistency with
TracerProvider
trait.opentelemetry-sdk
InstrumentBuilder
ExportKindFor
toTemporalitySelector
PushController
andPullController
intoBasicController
MinMaxSumCountAggregator
andArrayAggregator
Resolves #733