Metrics/gauge #129
Metrics/gauge #129
Conversation
biryukovmaxim
force-pushed
the
metrics/gauge
branch
from
b1ff97b to
45ab3dc
Compare
src/metrics.rs
Outdated
| @@ -2,6 +2,8 @@ use std::{collections::HashMap, fmt, sync::RwLock}; | |||
| use tracing::{field::Visit, Subscriber}; | |||
| use tracing_core::{Field, Interest, Metadata}; | |||
|
|
|||
| #[cfg(feature="otel_unstable")] | |||
There was a problem hiding this comment.
Nit: please use spaces around = in attributes (for all of the added guards).
There was a problem hiding this comment.
I'm sure rustfmt can handle this.
I raised a new issue at #143 and is working a PR to fix it.
Please fix the CI 😆
src/metrics.rs
Outdated
| #[cfg(feature="otel_unstable")] | ||
| GaugeU64(u64), | ||
| #[cfg(feature="otel_unstable")] | ||
| GaugeF64(f64), | ||
| #[cfg(feature="otel_unstable")] | ||
| GaugeI64(i64), |
There was a problem hiding this comment.
Nit: ordering above suggests that i64 goes before f64 -- there are no examples of ordering between u64 and i64 but it probably makes sense to keep it in that order.
src/metrics.rs
Outdated
| @@ -125,6 +142,34 @@ impl Instruments { | |||
| |rec| rec.record(value, attributes), | |||
| ); | |||
| } | |||
| #[cfg(feature="otel_unstable")] | |||
| InstrumentType::GaugeF64(value) => { | |||
src/metrics.rs
Outdated
| if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_MONOTONIC_COUNTER) { | ||
| self.visited_metrics | ||
| .push((metric_name, InstrumentType::CounterU64(value))); | ||
| } else if let Some(metric_name) = field.name().strip_prefix(METRIC_PREFIX_COUNTER) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Not sure why we should drop the else if here?
There was a problem hiding this comment.
I guess it was done because there is no way to guard else if let by feature gate. I will rollback else if as it was, and will add additional gated if let on the top of function
Cargo.toml
Outdated
| @@ -23,6 +23,8 @@ rust-version = "1.65.0" | |||
| default = ["tracing-log", "metrics"] | |||
| # Enables support for exporting OpenTelemetry metrics | |||
| metrics = ["opentelemetry/metrics","opentelemetry_sdk/metrics", "smallvec"] | |||
| # Enables support for unstable OpenTelemetry metric instruments | |||
| otel_unstable = ["opentelemetry/otel_unstable"] | |||
There was a problem hiding this comment.
Exporting otel_unstable in tracing-opentelemetry seems a bit odd to me.
What do you think about renaming it to something more descriptive like metrics_gauge_unstable? This way, we can remove it once gauge stabilizes without revealing its origin from opentelemetry/otel_unstable.
There was a problem hiding this comment.
yes, sounds reasonable
|
one more thing I want to share. Due to original pr workflow didn't show errors in tests, I added #[cfg(feature = "feature")]
struct Foo{}
struct Bar(Foo) // not feature gated - compilation errorI would suggest using feature matrix in ci to handle it. Can be done with |
|
Thanks! I think using an external crate tool for this is overkill for now, but it would be nice to add steps that check |
Motivation
The opentelemetry crate actually supports Gauge metric behind otel_unstable feature.
Solution
Extend the current implementation with the treatment for the new Gauge metric.
I fixed tests in pr. adds all-features flag to cicd
I also made a pr in the repo of original contributor academiaresf#1