feat(rt): change TokioExecutor to not trace, add rt-tracing-exec-force feature - #322
seanmonstar wants to merge 1 commit into
Conversation
daefed3 to
1970559
Compare
There was a problem hiding this comment.
thank you for opening this, and for your time thinking through a solution to this situation! i strongly support removing the calls to Instrument::in_current_span() from <TokioExecutor as Executor>::execute().
the temporary opt-in rt-tracing-exec-force feature flag appears to be a reasonable stopgap to allow transitive dependents of hyper-util, i.e. users of libraries like kube-rs or the AWS SDK that might create a hyper client internally on behalf of a caller, to continue to opt-in to the existing behavior after a hyper-util release including this change. i support this as well.
most substantively, i have a question below about how we might go about supporting the alternate span propagation behaviors that different groups of users want. #323 is a draft based upon this branch that explores that space a bit.
This is a soft behavioral breaking change.
one meta question i have is whether this change should warrant a v0.2.0 release. i would lean towards the latter, to be explicit that this could potentially cause breakage for some users, myself. what do you think?
|
Agree. In retrospect, the addition of There's #311 too, but I'm not convinced with the changes there. It's adding more creep when the issue is in |
see #322 for more information. this commit introduces an additional `Executor` implementation to accompany the `CurrentSpanExecutor` that executes spawned futures within the current span at time of execution. this would provide an alternative for users that wish to provide tracing information, but do not want to run background futures in the current span, which can interfere with some observability systems. Signed-off-by: katelyn martin <git@katelyn.world>
see #322 for more information. this commit introduces an additional `Executor` implementation to accompany the `CurrentSpanExecutor` that executes spawned futures within the current span at time of execution. this would provide an alternative for users that wish to provide tracing information, but do not want to run background futures in the current span, which can interfere with some observability systems. Signed-off-by: katelyn martin <git@katelyn.world>
1970559 to
044c3c9
Compare
TokioExecutor to not trace, add rt-tracing-exec-force feature
cratelyn
left a comment
There was a problem hiding this comment.
this looks great. i'm excited to have an answer to this long-standing issue. thank you for opening this, @seanmonstar!
see #322 for more information. this commit introduces an additional `Executor` implementation to accompany the `CurrentSpanExecutor` that executes spawned futures within the current span at time of execution. this would provide an alternative for users that wish to provide tracing information, but do not want to run background futures in the current span, which can interfere with some observability systems. Signed-off-by: katelyn martin <git@katelyn.world>
see #322 for more information. this commit introduces an additional `Executor` implementation to accompany the `CurrentSpanExecutor` that executes spawned futures within the current span at time of execution. this would provide an alternative for users that wish to provide tracing information, but do not want to run background futures in the current span, which can interfere with some observability systems. Signed-off-by: katelyn martin <git@katelyn.world>
I've been giving some thought to the long-running span. I can sympathize that it causes problems for many people, and there's basically no way to stop it. I'm leaning towards adding the automatic
in_current_spanwas a mistake, at least in that it had unintended consequences. What made fixing this slow is that both behaviors are desired, and changing it could be considered breaking.But, so be it. This is a soft behavioral breaking change. But, because the alternative is that people basically cannot fix it. Here's what it does.
tracingfeature no longer automatically makes theTokioExecutorspawn the future within_current_span.rt::TracingExecutorthat can wrap any other executor to add that functionality in easily, if so desired.rt-tracing-exec-force, which will add back in the previous behavior, in case there's a library creating a client that you cannot otherwise customize, but you badly want the spans connected.This seems like the least bad outcome. But curious what others think. @cratelyn @dswij? anyone else?
Closes hyperium/hyper#3904