Add custom OpenTelemetrySpanExt trait #700
Conversation
crates/utils/src/tracing/mod.rs
Outdated
| pub use opentelemetry::trace::Status as OtelStatus; | ||
| pub use tracing_opentelemetry::OpenTelemetrySpanExt; | ||
| use opentelemetry::trace::Status as OtelStatus; | ||
| use opentelemetry::{Key, Value}; |
There was a problem hiding this comment.
@Mirko-von-Leipzig in the issue you mentioned:
We can make set_attribute more ergonomic by adding a secondary trait ToValue which we can then implement for foreign types. i.e. instead of set_attribute("hash", digest.to_hex()), we can just do set_attribute("hash", &digest).
I was wondering if this is worthwhile considering that crates can already impl From<...> for opentelemetry::Value for their types and rely on the trait as is , value: impl Into<Value>);.
WDYT?
There was a problem hiding this comment.
Most of our core types come from the crates from miden-base - and I don't think introducing open-telemetry there is a reasonable expectation. Though it could make sense if we really want to standardize on it everywhere.
There was a problem hiding this comment.
Actually here we go: https://docs.rs/opentelemetry_sdk/latest/opentelemetry_sdk/testing/trace/index.html
They do actually have test infra :)
There was a problem hiding this comment.
Just checking you meant that this is relevant for this PR and not the other trace-related PR #698.
TestSpan implements trace::Span so I would have to change our Ext trait to be over impl<S: trace::Span + tracing_opentelemetry::OpenTelemetrySpanExt> and then make sure the tracing_opentelemetry::OpenTelemetrySpanExt applies to TestSpan. Unless I am missing something about how to using those testing utilities.
There was a problem hiding this comment.
Ah right; yeah this was intended for their 😅 Will cross-post just for traceability.
There was a problem hiding this comment.
Hmm. Yes good point - we can impl<S: OpenTelemetrySpanExt> because it in turn is implemented for T: tracing::Span. So we just piggy back on theirs? Alternatively we keep as is and add another impl for the TestSpan specifically.
I'd do the former, and then see how it works out in the other PR?
Co-authored-by: Mirko <48352201+Mirko-von-Leipzig@users.noreply.github.com>
Closes #678
tracing_opentelemetry::OpenTelemetrySpanExtwith a custom trait of the same nameopentelemetry::trace::Statusin favour of a trait API that takes&dyn ErrorinsteadToValuetrait and impls for various Miden and core types