feat(rt): add TracingExecutor and 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?
| /// An executor that propagates the current tracing span to its futures. | ||
| /// | ||
| /// The span is captured when [`execute`](Executor::execute) is called, and is | ||
| /// entered each time the future is polled or dropped. Execution is delegated to | ||
| /// the wrapped executor, without requiring a particular runtime. | ||
| /// | ||
| /// Requires the `tracing` feature. | ||
| /// | ||
| /// # Example | ||
| /// | ||
| /// ``` | ||
| /// # #[cfg(feature = "tokio")] | ||
| /// # { | ||
| /// use hyper_util::rt::{TokioExecutor, TracingExecutor}; | ||
| /// | ||
| /// let executor = TracingExecutor::new(TokioExecutor::new()); | ||
| /// # } | ||
| /// ``` | ||
| #[derive(Clone, Copy, Debug, Default)] | ||
| pub struct TracingExecutor<E> { | ||
| inner: E, | ||
| } |
There was a problem hiding this comment.
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 automaticin_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.
(emphasis added)
i also agree that adding the automatic in_current_span() calls to TokioExecutor was a mistake. as you point out, both behaviors are desired by different users. my understanding is that this stems from different lifecycle semantics of various tracing_core::subscriber::Subscriber implementations.
users exporting traces via OpenTelemetry for example, must contend with the fact that only finished spans are sent to the collector. thus, users that wrap their subscriber in a tracing_opentelemetry::OpenTelemetryLayer layer will accordingly need to organize the tracing spans in their application code differently from users of subscribers like tracing_subscriber::fmt::Subscriber printing to stdout.
conversely, not all tracing subscribers have a way to visualize causal relationships between spans. whether an executor should spawn futures in a separate span that follows_from() the current span or within the current span depends on how and where events are being formatted.
to help account for both of these groups, i would propose that we expose more than one "tracing" executor from the perspective of hyper-util. i've opened #323 as a draft against this branch, exploring what that could look like.
for now i've only added a WithSpanExecutor<E> to instrument executed futures with a provided span, but i can imagine a FollowsFromExecutor<E> to build closely upon that. i am curious what you think of this direction!
There was a problem hiding this comment.
yea, I did wonder if I should even include an additional executor wrapper, or leave that for a separate thing. I don't have to merge it at the same time, if it's less clear what a good set to provide is. I don't feel strongly, I don't have a need for these types myself, but figured I'd include it as an example and possibly useful thing. wdyt?
|
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>
3 participants
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