feat(tracing): propagate log topic across spawns and label components - #669
varex83agent wants to merge 6 commits into
Conversation
) `MetricsLayer` labels `app_log_{warn,error}_total` with the `topic` field from the nearest enclosing span. Span context is not carried across `tokio::spawn`, and pluto set the `topic` field in only two places, so almost every warn/error was counted under `topic=""`. Add a span-propagating spawn helper (`pluto_tracing::spawn`) that attaches `Span::current()` to the spawned future, restoring context-like topic propagation, and set a `&'static str` `topic` root span on each long-running component, reusing charon's topic names: - sched, tracker, sigagg, bcast (+recast), parsigex, vapi, qbft, p2p, peerinfo, dkg, relay, vmock, and app-start as the catch-all. Adds tests asserting the helper propagates the topic across the task boundary while a bare `tokio::spawn` does not. Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
6782d5d to
7836e72
Compare
The rustdoc build failed with -D warnings because [`spawn`] is ambiguous between the `spawn` module and the `spawn` function. Add parentheses to link to the function. Co-Authored-By: Bohdan Ohorodnii <varex83@users.noreply.github.com>
Co-Authored-By: Bohdan Ohorodnii <varex83@users.noreply.github.com>
emlautarom1
left a comment
There was a problem hiding this comment.
@varex83 Could you confirm that the log level is correct and propagation is working as intended to all potential subtasks? I'm not sure the agent covered all scenarios.
| pub fn spawn<F>(future: F) -> JoinHandle<F::Output> | ||
| where | ||
| F: Future + Send + 'static, | ||
| F::Output: Send + 'static, | ||
| { | ||
| tokio::spawn(future.instrument(tracing::Span::current())) | ||
| } |
There was a problem hiding this comment.
It's extremely easy to forget about this wrapper, preferring to use tokio::spawn directly. Prefer to inline it in the callers (only 2) and document it.
There was a problem hiding this comment.
Done — crates/tracing/src/spawn.rs and the pluto_tracing::spawn re-export are gone. Both callers now inline the wrap with a comment saying why:
// relay-server/src/web.rs
tokio::spawn(
resolve_external_host_periodically(state, external_host, ct)
.instrument(Span::current()),
)The two propagation tests moved to crates/tracing/src/layers/metrics.rs, merged into one that pins both halves of the contract in a single assertion — an instrumented spawn is counted under the topic, a bare tokio::spawn from the same span is not. That's next to the layer that actually reads the span, so it's harder to delete by accident than a test sitting beside a helper nobody calls.
| //! [`tokio::spawn`] does **not** carry the current [`tracing`] span into the | ||
| //! spawned future: the new task starts with an empty span stack. That breaks | ||
| //! the `topic` label used by [`crate::layers::metrics::MetricsLayer`], because | ||
| //! a `warn!`/`error!` emitted from a bare spawn lands on `topic=""` even when | ||
| //! the spawning code is inside a component's `topic` span. |
There was a problem hiding this comment.
What about JoinSet? Are we covering all cases of subtask spawn? The helper here is only used 2 times, and I'm certain that we spawn a lot more tasks than that.
There was a problem hiding this comment.
You were right — two call sites was nowhere near the real surface. I went through every non-test tokio::spawn / JoinSet::spawn / spawn_blocking in the workspace (~90 sites) and checked which ones sit under a topic span and can emit warn!/error!. JoinSet::spawn behaves exactly like tokio::spawn here — empty span stack — so all three needed the same treatment.
Fixed:
| site | kind | before | after |
|---|---|---|---|
app/node/mod.rs — the lifecycle JoinSet (8 tasks) |
JoinSet::spawn / extend |
topic="" |
app-start, via a local background() helper |
consensus/qbft/runner.rs — instance JoinSet (5 tasks) + the core spawn_blocking |
JoinSet::spawn, spawn_blocking |
topic="" |
inherits qbft |
core/scheduler.rs — 2 subscriber loops, 2 per-event callback spawns, duty-broadcast delay, slot ticker |
tokio::spawn |
topic="" |
sched |
core/tracker/inclusion.rs::run |
— | topic="" |
tracker |
app/monitoringapi/checker.rs — version metric + ready checker |
tokio::spawn |
topic="" |
inherits app-start |
priority/prioritiser.rs::start_consensus |
tokio::spawn |
topic="" |
inherits caller |
testutil/validatormock duty JoinSet |
JoinSet::spawn |
already inherited | reformatted (see below) |
Two of those are charon-parity bugs rather than just plumbing:
InclusionChecker::runhad no topic at all. Charon setstrackeron it (core/tracker/inclusion.go:588), so every "Failed to check inclusion" warning was landing ontopic="".- The scheduler's subscriber loops were deliberately left unlabelled in the first pass, on the reasoning that callback errors belong to other components. That doesn't match charon:
emitCoreSlotlogs "Emit scheduled slot event" with the ctx fromScheduler.Run, which issched(core/scheduler/scheduler.go:109). They're labelledschednow.
For the app/node/mod.rs JoinSet I used charon's own mechanism rather than inheritance — lifecycle.Manager hands every background hook a fresh log.WithTopic(context.Background(), "app-start") (app/lifecycle/hook.go:101), so background() opens that span at the one place background tasks are started, and a task with its own topic (sched, tracker, health) shadows it.
Deliberately left alone:
- No
warn!/error!anywhere in the spawned future, so there is nothing to mislabel:qbft/definition.rs:311(round timer),qbft/transport.rs:209,core/aggsigdb/memory.rs:165,core/deadline/mod.rs:216,app/node/wire.rs:{520,809,1137},eth2util/keystore/{load,store}.rs. app/src/sse/mod.rs— 11 warn/error sites, butSseListenercurrently has no consumer outside its own module, so there is no topic to inherit yet. It will need one when it gets wired into the node.cli/commands/test/*— one-shot CLI subcommands; charon gives its equivalents no topic either.
The rule this settles on, which I think is the answer to "easy to forget": a task that is a component opens its own topic span (#[instrument(fields(topic = …))] on the entry point, so it works regardless of who spawned it); a task that is a subtask of its caller re-attaches Span::current() at the spawn. Only the second kind is forgettable, and it is now a handful of sites, each with a comment.
emlautarom1
left a comment
There was a problem hiding this comment.
Also, formatting might need to be revisited.
Addresses review feedback on #669. Drop the `pluto_tracing::spawn` wrapper: a helper that must be remembered at every spawn site is the wrong shape for this. Both callers now inline `tokio::spawn(fut.instrument(Span::current()))` with a comment, and the propagation tests move next to `MetricsLayer`, which is what reads the span. Close the subtask gaps the wrapper's two call sites were hiding. `JoinSet::spawn` and `spawn_blocking` drop the span stack exactly like `tokio::spawn`, so the node's lifecycle JoinSet, the QBFT instance JoinSet and its blocking core, the scheduler's subscriber loops and slot ticker, the readiness checker and the priority consensus spawn all lost their topic. The node's JoinSet now opens `app-start` per task, mirroring charon's `lifecycle.Manager` handing each background hook a `log.WithTopic(context.Background(), "app-start")` context; the rest re-attach the caller's span. Two of these are charon-parity bugs rather than plumbing: - `InclusionChecker::run` carried no topic at all; charon sets `tracker` (core/tracker/inclusion.go). - The scheduler's subscriber loops were left unlabelled on the theory that callback errors belong to other components, but charon logs "Emit scheduled slot event" under `sched` (core/scheduler). Also fix the formatting rustfmt had silently given up on: wrapping a long `async move` block in `Instrument::instrument(fut, span)` left the body at its old indentation while `--check` still passed. Hoisting the future into a named binding first keeps rustfmt in charge. Co-Authored-By: Bohdan Ohorodnii <35969035+varex83@users.noreply.github.com>
|
@emlautarom1 thanks — both inline comments are addressed (replies on the threads), and "Is the log level correct?"No, and it is worse than this PR: the whole
let registry = Registry::default()
.with(env_filter) // <- global, default "info"
.with(fmt_layer)
.with(MetricsLayer);A global I probed it directly against The fix is to make I have not done that here: it restructures subscriber wiring, it fixes a bug that predates this PR, and it deserves its own review. Happy to open a follow-up issue, or to fold it in if you would rather this PR ship working end-to-end — your call. Worth being explicit that until it lands, this PR's labels are correct but unobservable outside "Is propagation working to all potential subtasks?"It wasn't. Full audit in the FormattingFixed. Gates
|
Summary
MetricsLayerlabelsapp_log_warn_total/app_log_error_totalwith atopictaken from the nearest enclosing span, falling back to"". Because tracing span context does not crosstokio::spawn, and pluto set thetopicfield in only two places (health,stacksnipe), effectively every other warn/error landed undertopic="".This PR restores charon-like topic attribution.
1. Every long-running component opens its own
topicspan at its entry point — a&'static strso the label stays bounded — reusing charon's names. Because the span is opened by the task itself, it works regardless of who spawned it.2. Subtasks re-attach their caller's span.
tokio::spawn,JoinSet::spawnandspawn_blockingall start with an empty span stack, so a subtask that has no topic of its own wraps its future in.instrument(Span::current()), inline and commented at each site. This is what charon gets for free by passingcontext.Contextinto a goroutine.3. Background tasks default to
app-start. The node's lifecycleJoinSetopens anapp-startspan per task via a localbackground()helper, mirroring charon'slifecycle.Manager, which hands every background hook alog.WithTopic(context.Background(), "app-start")context (app/lifecycle/hook.go). A task with a more specific topic shadows it, exactly as a nestedlog.WithTopicdoes in Go.Topics / components covered
schedcore/scheduler.rsactorrun, both subscriber loops, per-event callback spawns, duty-broadcast delay, slot tickertrackercore/tracker/mod.rsactorrun,core/tracker/inclusion.rsrunsigaggcore/sigagg.rsaggregatebcastcore/bcast/mod.rsbroadcast,core/bcast/recast.rsslot_tickedparsigexparsigex/behaviour.rsenqueue+notify_subscribersspawnvapicore/validatorapi/router.rsper-request middlewareqbftconsensus/qbft/runner.rspropose/participate/run_instance+ the instanceJoinSetand blocking core,component.rscleanup spawnp2pp2p/p2p.rshandle_eventrelaycli/commands/relay.rsrun,p2p/bootnode.rsrelay-resolver spawn,relay-server/web.rsresolver spawnpeerinfopeerinfo/protocol.rssend_peer_info/recv_peer_infodkgdkg/dkg.rsrun(+ network-driver spawn)vmocktestutil/validatormock/component.rsslot_ticked/run_scheduler+ dutyJoinSetapp-startapp/node/mod.rsrunand its lifecycleJoinSet, readiness checker, priority consensus spawn, dutydb store callbackPre-existing
healthandstacksnipetopics are unchanged.Notes
app-start, matching charon.warn!/error!are left alone — there is nothing to mislabel.app/src/sse/mod.rsdoes log, butSseListenerhas no consumer outside its own module yet, so there is no topic to inherit; it will need one when it is wired into the node.infosynchas no charon topic mapping and was left as-is.Known limitation (pre-existing, not fixed here)
init.rsattachesEnvFilteras a global layer, which gates span construction for every layer includingMetricsLayer. At the defaultinfofilter adebug_span!(…, topic = …)is never created, so these labels — and the pre-existinghealth/stacksnipeones onmain— are only observable underRUST_LOG=debug. Fixing it means movingEnvFilterto a per-layer filter onfmt/Loki and givingMetricsLayerits own; that predates this PR and is tracked separately. Details and a probe in this comment.Quality gates
cargo +nightly fmt --all --check— cleancargo clippy --workspace --all-targets --all-features -- -D warnings— cleancargo test --workspace --all-features— passCloses #588
🤖 Generated with Claude Code