Skip to content

don't instrument long lived tasks 2 - #4682

Merged
MartinquaXD merged 2 commits into
mainfrom
dont-instrument-long-tasks-2
Jul 29, 2026
Merged

don't instrument long lived tasks 2#4682
MartinquaXD merged 2 commits into
mainfrom
dont-instrument-long-tasks-2

Conversation

@MartinquaXD

Copy link
Copy Markdown
Contributor

Description

Follow up to #4680 as I overlooked to background tasks.

Changes

adjusted instrumentation of current block stream and token fetcher
also slightly refactored current block stream to reuse duplicated code

@MartinquaXD
MartinquaXD requested a review from a team as a code owner July 29, 2026 14:00
@MartinquaXD MartinquaXD added the hotfix Labels PRs that should be applied into production right away label Jul 29, 2026
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Claude finished @MartinquaXD's task in 1m 40s —— View job


PR Review: don't instrument long lived tasks 2

  • Read PR diff and changed files
  • Analyze instrumentation changes (block stream + token fetcher)
  • Verify refactor preserves original behavior
  • Check types/signatures consistency

Summary

Looks good. ✅ The change correctly addresses the goal: long-lived spawned tasks (current_block_stream, current_block_ws_stream, token_fetcher) no longer wrap their whole lifetime in a span that never closes. Instead, #[instrument(skip_all)] is applied to the per-iteration units of work (convert_block_and_process, fetch_block_and_process, update_balances), so spans open and close once per block / per poll / per update cycle.

The refactor of the two block-stream loops into the shared handle_new_block helper is a nice deduplication and I verified it preserves the original control flow exactly:

  • update_current_block_metrics still runs for every observed block (before the hash check).
  • Same-hash blocks are ignored; update_block_metrics runs only on hash change; the stream is updated + previous_block advanced only when the block number increases.
  • The panic-on-send-failure behavior is unchanged (both loops).

Types are consistent (AlloyProvider, get_block_at_id(&AlloyProvider, ...)), and the tracing::Instrumenttracing::instrument import swaps match the new usage in both files.

Minor observations (non-blocking, no change needed):

  • The debug log message was unified from "received block" / "polled block" to "observed new block". Since both loops now share code this is expected; the span/task naming still lets you tell WS vs. polling apart.
  • BlockInfo::try_from(block.clone()) still clones to retain block for the error log — pre-existing, not introduced here.

I couldn't run cargo check/cargo +nightly fmt in this environment (command approval not granted), so please rely on CI for the final build/format check.
dont-instrument-long-tasks-2

@MartinquaXD
MartinquaXD enabled auto-merge July 29, 2026 14:04
@MartinquaXD
MartinquaXD added this pull request to the merge queue Jul 29, 2026
Merged via the queue into main with commit 2193c8c Jul 29, 2026
24 checks passed
@MartinquaXD
MartinquaXD deleted the dont-instrument-long-tasks-2 branch July 29, 2026 14:21
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 29, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

hotfix Labels PRs that should be applied into production right away

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants