Repository navigation
Conversation
|
@tikue any chance you can look into it? |
|
Hi, thanks for the contribution! I will try to look at this in the next week or so. |
|
@tikue sorry for pinging, would really appreciate if you'd find a minute to review the PR :) |
tikue
left a comment
tikue
left a comment
There was a problem hiding this comment.
Apologies for the delay in reviewing!
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| opentelemetry: ["", "--no-default-features"] |
There was a problem hiding this comment.
Should this have --features opentelemetry?
|
|
||
| /// An extension trait for [`tracing::Span`] for propagating tarpc Contexts. | ||
| pub(crate) trait SpanExt { | ||
| fn trace_context(&self) -> Result<trace::Context, trace::NoActiveSpan>; |
There was a problem hiding this comment.
Please document why this method exists. AFAICT, it exists purely as a shim because TryFrom<&tracing::Span> can't be impl'd when the crate is compiled without the tracing dependency.
| /// An extension trait for [`tracing::Span`] for propagating tarpc Contexts. | ||
| pub(crate) trait SpanExt { | ||
| fn trace_context(&self) -> Result<trace::Context, trace::NoActiveSpan>; | ||
| fn deadline(&self) -> Option<Instant>; |
There was a problem hiding this comment.
Same thing as above -- please document that this exists purely as a shim over the tracing crate, or as a stub / empty impl when the tracing dependency is not present.
| fn deadline(&self) -> Option<Instant>; | ||
| /// Sets the given context on this span. Newly-created spans will be children of the given | ||
| /// context's trace context. | ||
| fn set_context(&self, context: &Context); |
There was a problem hiding this comment.
Now that this can fail, please make it return a Result. It seems error prone to silently fail.
| } | ||
| } | ||
|
|
||
| impl From<opentelemetry::trace::TraceId> for TraceId { |
There was a problem hiding this comment.
Can this file be branched from trace.rs so that these lines don't appear as new?
There was a problem hiding this comment.
Is it possible for this file to preserve the commit history of trace.rs so that the unchanged lines don't appear as new?
Fixes #564
Not all projects require opentelemetry, and besides being extra code to compile, it also makes supply chain security harder (see example).
I've kept opentelemetry enabled by default so that the change is backward compatible, hope that works.