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

Update metrics API and SDK #819

Merged
merged 5 commits into from Jul 25, 2022
Merged

Conversation

jtescher
Copy link
Member

@jtescher jtescher commented Jun 21, 2022

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

Resolves #733

@jtescher jtescher requested a review from a team as a code owner June 21, 2022 17:47
@jtescher
Copy link
Member Author

This change would have been nicer in smaller pieces, but many of them rely on each other.

@jtescher jtescher force-pushed the metrics-update branch 3 times, most recently from 6228f84 to c5fd910 Compare June 22, 2022 19:25
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
@jtescher
Copy link
Member Author

cc @hdost

@hdost
Copy link
Contributor

hdost commented Jun 28, 2022

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.

@hdost
Copy link
Contributor

hdost commented Jun 28, 2022

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.

@TommyCpp
Copy link
Contributor

Sorry I have been busy moving. Will try to take a look today

Copy link
Contributor

@TommyCpp TommyCpp left a 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

opentelemetry-api/src/metrics/meter.rs Outdated Show resolved Hide resolved
opentelemetry-api/src/metrics/meter.rs Show resolved Hide resolved
opentelemetry-dynatrace/src/metric.rs Outdated Show resolved Hide resolved
opentelemetry-sdk/src/metrics/aggregators/histogram.rs Outdated Show resolved Hide resolved
examples/dynatrace/src/main.rs Outdated Show resolved Hide resolved
opentelemetry-sdk/src/metrics/sdk_api/instrument_kind.rs Outdated Show resolved Hide resolved
@jtescher
Copy link
Member Author

@TommyCpp / @hdost any last issues or should we merge this and iterate from here?

Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jtescher jtescher merged commit f20c9b4 into open-telemetry:main Jul 25, 2022
@jtescher jtescher deleted the metrics-update branch July 25, 2022 16:05
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.

Asynch Instruments: Spec Conformance
4 participants