Emit otel metrics directly instead of through tracing#3623
Conversation
Signed-off-by: Ryan Levick <rlevick@akamai.com>
| /// spin_telemetry::metrics::monotonic_counter_f64!(spin.metric_name = 1.5, metric_attribute = "value"); | ||
| /// ``` | ||
| #[macro_export] | ||
| macro_rules! monotonic_counter_f64 { |
There was a problem hiding this comment.
I think we should omit this (and strip _u64 from the other one). I can't think of any use for a float counter in this kind of project. What does counting even mean?!
This seems more robust than what tracing-opentelemetry does anyway: https://docs.rs/tracing-opentelemetry/latest/tracing_opentelemetry/struct.MetricsLayer.html#floating-point-numbers |
| let meter_provider = metrics::metrics_provider(spin_version.clone(), histogram_buckets) | ||
| .context("failed to initialize otel metrics")?; | ||
| opentelemetry::global::set_meter_provider(meter_provider); | ||
| } |
There was a problem hiding this comment.
I'm not 100% confident but I think we could replace this (and most of metrics_provider(...) with the default behavior that pulls config from env vars. I think we had this explicit check previously because the extra tracing layer had a significant effect on tracing performance under high load.
There was a problem hiding this comment.
@calebschoepp do you remember what the setup in metrics_provider is all about?
There was a problem hiding this comment.
I'd say lets just not mess with this. You might be right Lann, but this would be a bigger refactor across tracing and logs too and also risk more potentially breaking DX changes.
| /// spin_telemetry::metrics::counter_f64!(spin.metric_name = -1.5, metric_attribute = "value"); | ||
| /// ``` | ||
| #[macro_export] | ||
| macro_rules! counter_f64 { |
lann
left a comment
lann
left a comment
There was a problem hiding this comment.
Minor quibbles. I think this is better than tracing.
| pub use propagation::extract_trace_context; | ||
| pub use propagation::inject_trace_context; | ||
|
|
||
| /// Initializes telemetry for Spin using the [tracing] library. |
There was a problem hiding this comment.
I suppose this should be updated to mention that it sets the global metrics provider. An LLM might read it...
calebschoepp
left a comment
calebschoepp
left a comment
There was a problem hiding this comment.
I like this approach a lot more
| pub use gauge; | ||
| pub use histogram; | ||
| pub use monotonic_counter; | ||
| pub use counter_f64; |
There was a problem hiding this comment.
Nit: I think it's more clear to expose these with names matching the underlying OTel types i.e. Counter and UpDownCounter vs. MonotonicCounter and Counter. Not blocking and feel free to stick with your opinion - just thought I'd voice that it's a bit confusing to me.
| /// [Layer] emits [tracing] events to stderr, another sends spans to an OTel collector, and another | ||
| /// sends metrics to an OTel collector. |
There was a problem hiding this comment.
This metrics layer is gone now
| /// spin_telemetry::metrics::monotonic_counter_u64!(spin.metric_name = 1, metric_attribute = "value"); | ||
| /// ``` | ||
| #[macro_export] | ||
| macro_rules! monotonic_counter_u64 { |
There was a problem hiding this comment.
I think it's important to document clearly somewhere where the consumers of these macros will see that to emit a metric from multiple locations they need to wrap the macro in a function and call that function instead (because of the static nature). You kind of allude to this but I think it should be more clear.
| let meter_provider = metrics::metrics_provider(spin_version.clone(), histogram_buckets) | ||
| .context("failed to initialize otel metrics")?; | ||
| opentelemetry::global::set_meter_provider(meter_provider); | ||
| } |
There was a problem hiding this comment.
I'd say lets just not mess with this. You might be right Lann, but this would be a bigger refactor across tracing and logs too and also risk more potentially breaking DX changes.
3 participants
Alternative to #3619
Instead of doing some compile time magic, we instead get rid of our dependency on
tracingfor emitting metrics and instead emit the otel metrics directly.This does require duplicating macros to specify types as as far as I can tell we can't infer the type param we need to pass to the
opentelemetry::metricstypes.I also deleted some tests in crates/connection-semaphore/src/lib.rs where were testing that we were emitting specific metrics. This is harder to test with the new implementation, and I'm not convinced we actually got that much out of those tests.