fix(tracing): propagate span context #166
Conversation
The tracing span context set in consumers of the `TokioExecutor` was sometimes lost (if the task was run in a different thread). This adds a call to `in_current_span()` to preserve it.
|
Interesting, I remember something like this a while ago. I think an issue when doing this automatically is that some tasks that are spawned live longer than the initial context. For example, if a new connection is created when trying to serve a request, and then that connection is put in a pool and used for later requests, the context is no longer the same. |
My understanding is that this isn't a concern for HTTP/2--connection establishment occurs separately from request dispatch; IIUC, the HTTP/1 connection pooling behavior was changed in the 1.0 refactor... what would be the proper way to audit this? Presumably we don't want to look for uses of executor as much as we want to look into HTTP/1 connection reuse? I'm especially interested in making this change so that libraries like kube-rs (cc @clux) can get this behavior without us having to go submit patches to every library that builds a client... |
|
I'm happy to include something like this. Are you saying that if we made this a wrapping We could alternatively just use |
It would require upstream changes, but I think that would be an acceptable middle-ground (at least we can document the trade offs in the util crate).
I like this approach because it means that applications can control the behavior independently of the underlying libraries; but maybe it makes sense to force library authors to make a choice? I can see both sides of that. |
this commit removes the `linkerd-http-executor` crate, and replaces all usage of its `TracingExecutor` type with the `TokioExecutor` type provided by `hyper-util`. this work is based upon hyperium/hyper-util#166. that change, included in the 0.1.11 release, altered the `TokioExecutor` type so that it propagates tracing context when the `tracing` feature is enabled. with that change made, our `TracingExecutor` type is now redundant. * hyperium/hyper-util#166 * https://github.com/hyperium/hyper-util/blob/master/CHANGELOG.md#0111-2025-03-31 Signed-off-by: katelyn martin <kate@buoyant.io>
this commit removes the `linkerd-http-executor` crate, and replaces all usage of its `TracingExecutor` type with the `TokioExecutor` type provided by `hyper-util`. this work is based upon hyperium/hyper-util#166. that change, included in the 0.1.11 release, altered the `TokioExecutor` type so that it propagates tracing context when the `tracing` feature is enabled. with that change made, our `TracingExecutor` type is now redundant. * hyperium/hyper-util#166 * https://github.com/hyperium/hyper-util/blob/master/CHANGELOG.md#0111-2025-03-31 Signed-off-by: katelyn martin <kate@buoyant.io>
This enables the `tracing` feature in the hyper-util dependency, allowing for the span context to properly propagate and avoiding having entries like this in the log, deprived from all the labels:
```log
2025-04-01T18:02:28.180957Z DEBUG hyper_util::client::legacy::pool: pooling idle connection for ("https", 10.43.0.1)
```
An entry with full context conserved should look something like this instead:
```log
2025-04-01T18:28:45.647991Z DEBUG serverauthorizations:HTTP{http.method=GET http.url=https://10.43.0.1/apis/policy.linkerd.io/v1beta1/serverauthorizations?&watch=true&timeoutSeconds=290&allowWatchBookmarks=true&resourceVersion=2684 otel.name="watch" otel.kind="client" http.status_code=200}: hyper_util::client::legacy::pool: pooling idle connection for ("https", 10.43.0.1)
```
See hyperium/hyper-util#166
the latest release of `hyper-util` includes hyperium/hyper-util#166, which changes the behavior of the `TokioExecutor` when the `tracing` feature flag has been enabled. when this flag is enabled, the `TokioExecutor` will now propagate the current tracing span to spawned tasks. this commit adds this new `hyper-util` feature to the dependency used by the `kube-client` crate. * hyperium/hyper-util#166 Signed-off-by: katelyn martin <me+cratelyn@katelyn.world>
This enables the `tracing` feature in the hyper-util dependency, allowing for the span context to properly propagate and avoiding having entries like this in the log, deprived from all the labels:
```log
2025-04-01T18:02:28.180957Z DEBUG hyper_util::client::legacy::pool: pooling idle connection for ("https", 10.43.0.1)
```
An entry with full context conserved should look something like this instead:
```log
2025-04-01T18:28:45.647991Z DEBUG serverauthorizations:HTTP{http.method=GET http.url=https://10.43.0.1/apis/policy.linkerd.io/v1beta1/serverauthorizations?&watch=true&timeoutSeconds=290&allowWatchBookmarks=true&resourceVersion=2684 otel.name="watch" otel.kind="client" http.status_code=200}: hyper_util::client::legacy::pool: pooling idle connection for ("https", 10.43.0.1)
```
See hyperium/hyper-util#166
- feat(server): support 'auto_date_header', 'max_local_error_reset_streams', and 'ignore_invalid_headers'. (hyperium/hyper-util#161) - refactor: replace manual implementations of 'ReadBufCursor' methods (hyperium/hyper-util#181) - feat(rt): add 'tracing' feature that will propagate span context in TokioExecutor (hyperium/hyper-util#166) Co-authored-by: Finn Bear <finntbear@gmail.com> Co-authored-by: Paolo Barbolini <paolo@paolo565.org> Co-authored-by: Alejandro Pedraza <alejandro.pedraza@gmail.com>
- feat(server): support 'auto_date_header', 'max_local_error_reset_streams', and 'ignore_invalid_headers'. (hyperium/hyper-util#161) - refactor: replace manual implementations of 'ReadBufCursor' methods (hyperium/hyper-util#181) - feat(rt): add 'tracing' feature that will propagate span context in TokioExecutor (hyperium/hyper-util#166) Co-authored-by: Finn Bear <finntbear@gmail.com> Co-authored-by: Paolo Barbolini <paolo@paolo565.org> Co-authored-by: Alejandro Pedraza <alejandro.pedraza@gmail.com>
Hi Sean! 👋
The tracing span context set in consumers of the
TokioExecutoris sometimes lost (if the task is run in a different thread). This change adds a call toin_current_span()to preserve it.As a side-effect, it adds a dependency on tracing for the tokio feature. Should I instead allow the new behavior only if tracing is enabled? Or use a separate
TracingExecutorfor this?