-
Notifications
You must be signed in to change notification settings - Fork 485
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
New metrics SDK #1000
New metrics SDK #1000
Conversation
b04f672
to
970884a
Compare
This patch updates the metrics SDK to the latest spec. The following breaking changes are introduced. Metrics API changes: * Move `AttributeSet` to SDK as it's not mentioned in the spec or used in the api * Consolidate `AsyncCounter`, `AsyncUpDownCounter`, and `AsyncGauge` into `AsyncInstrument` trait and add downcasting for observer callbacks. * Add `AsyncInstrumentBuilder` to allow per-instrument callback configuration. * Allow metric `name` and `description` fields to be `Cow<'static, str>` * Warn on metric misconfiguration when using instrument builder `init` rather than returning error * Update `Meter::register_callback` to take a list of async instruments and validate they are registered in the callback through the associated `Observer` * Allow registered callbacks to be unregistered. Metrics SDK changes: * Introduce `Scope` as type alias for `InstrumentationLibrary` * Update `Aggregation` to match aggregation spec * Refactor `BasicController` to spec compliant `ManualReader` * Refactor `PushController` to spec compliant `PeriodicReader` * Update metric data fields to match spec, including exemplars. * Split `MetricsExporter` into `Reader`s and `PushMetricExporter`s * Add `View` implementation * Remove `AtomicNumber`s * Refactor `Processor`s into `Pipeline` Metrics exporter changes: * Update otlp exporter to match new metrics data * Update otlp exporter configuration to allow aggregation and temporality selectors to be optional. * Update prometheus exporter to match new metrics data Example changes: * Update otlp metrics and prometheus examples. * Remove basic example as we should be focusing on the OTLP variants
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.
Great work overall! Don't see any blockers, and I think anything that we missed on this go we can iterate on 😄
Sorry was little busy recently. I will finish reviewing this weekend if that's OK. Or we can merge this and address any issue in the followup PR. Overall great work 🎉 |
I guess I thought we were waiting until after 0.19.0 to merge this? Or are we just not releasing 0.19.0 😅 |
Haha yeah, we want to after 0.19. I was worried I will block others from merging the release PR and then this PR. I will #1002 today |
# Conflicts: # opentelemetry-dynatrace/Cargo.toml # opentelemetry-otlp/Cargo.toml # opentelemetry-prometheus/Cargo.toml
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.
Mostly nits and can be addressed in follow-up PRs
Co-authored-by: Zhongyang Wu <zhongyang.wu@outlook.com>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1000 +/- ##
========================================
- Coverage 69.9% 57.2% -12.8%
========================================
Files 116 143 +27
Lines 9027 17461 +8434
========================================
+ Hits 6316 9999 +3683
- Misses 2711 7462 +4751
... and 104 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
@TommyCpp any other comments or we good to merge? |
Nope good to merge. Great work! |
* chore(deps): cargo * wip: use axum & cargo fmt * wip: remove `once_cell` * wip: upgrade opentelemetry ref: open-telemetry/opentelemetry-rust#1000 ref: open-telemetry/opentelemetry-rust#2221 ref: open-telemetry/opentelemetry-rust#2085 * chore: cargo clippy * chore(deps): upgrade go * version: upgrade to 0.6.0 * chore(deps): use rkyv * chore(deps): update * chore(deps): update * wip: split mods into crates * wip: remove unused dependencies * chore(ci): upgrade rust version in Dockerfile * fix(ci): update cargo install path * chore: tidy up main.rs * fix(test): review snaps & fix tests * chore(deps): update * fix(cgroup): early stop when use `map_while` * chore: init logger before setup cgroup * chore(deps): ebpf version restore to v0.12.3 * chore(lazylock): use `LazyLock::force` to init cache * fix(cache): remove unnecessary async * chore(config): make default functions const
This patch updates the metrics SDK to the latest spec. The following breaking changes are introduced.
Metrics API changes:
AttributeSet
to SDK as it's not mentioned in the spec or used in the apiAsyncCounter
,AsyncUpDownCounter
, andAsyncGauge
intoAsyncInstrument
trait and add downcasting for observer callbacks.AsyncInstrumentBuilder
to allow per-instrument callback configuration.name
anddescription
fields to beCow<'static, str>
init
rather than returning errorMeter::register_callback
to take a list of async instruments and validate they are registered in the callback through the associatedObserver
Metrics SDK changes:
Scope
as type alias forInstrumentationLibrary
Aggregation
to match aggregation specBasicController
to spec compliantManualReader
PushController
to spec compliantPeriodicReader
MetricsExporter
intoReader
s andPushMetricExporter
sView
implementationAtomicNumber
sProcessor
s intoPipeline
Metrics exporter changes:
Example changes:
Given the size of the changes I would have preferred to land them individually, but as they all rely on each other it was not practical.
Resolves #897