Skip to content

feat(nodejs): make Node.js event loop tracing opt-in, default off - #331

Merged
mayankpande88 merged 1 commit into
mainfrom
feat/nodejs-tracing-opt-in
Sep 10, 2026
Merged

mayankpande88 merged 1 commit into
mainfrom
feat/nodejs-tracing-opt-in

Conversation

@mayankpande88

Copy link
Copy Markdown
Contributor

Why

Attaching the libuv probes reads the entire ELF symbol table of every Node.js process's binary, per pid. node is a large statically linked binary with V8 embedded, so this is expensive — a customer CPU profile attributed 17.5% of a core to it:

Process.instrumentNodejs → AttachNodejsProbes
  → ELFFile.GetSymbol → readSymbols        17.5%

What it buys is a single metric: container_nodejs_event_loop_blocked_time_seconds_total.

An org-wide code search returns exactly one hit — the metric's own definition. No dashboard, alert, query, or application code consumes it. It is collected and stored (24 series on our dev cluster) but never read.

Given no current use case, the cost isn't justified by default.

Change

ENABLE_NODEJS_TRACING, default false. Opt-in matches ENABLE_DOTNET_TRACING rather than the always-on Python probes.

Gated before nodejsChecked, so enabling the flag and restarting instruments processes that were skipped while it was off.

Blast radius

Verified, not assumed — limited to that one metric:

  • NodejsStats carries only EventLoopBlockedTime
  • The probes attach nothing but uv_io_poll_enter/exit and uv_io_cb_enter/exit
  • TLS interception for Node.js processes is unaffectedtls.go has no Node-specific handling and works via the libssl/gotls paths
  • L7 tracing is unaffected — it reads syscalls, not libuv

Two things reviewers should weigh:

  • Anyone with a customer-built dashboard on this metric loses it silently on upgrade.
  • If the AI/troubleshooting features discover metrics dynamically by name rather than hardcoding them, a code search wouldn't reveal that. Worth confirming with whoever owns that before merge.

This does not fix the symbol-table cost generally

python.go and tls.go take the same uncached per-pid full-table path and remain always-on:

python.go:58   OpenELFFile → GetSymbol("pthread_cond_timedwait")
tls.go:142     OpenELFFile → GetSymbol(p.symbol)
tls.go:250     OpenELFFile → GetSymbol(writeSymbol/readSymbol)

This PR removes the Node.js share, which happened to dominate that particular customer's profile. #330 caches the lookups by binary identity and is the general fix — the two are complementary, and #330 still matters for the paths that stay always-on (and for anyone who turns Node.js tracing back on).

Testing

go build ./...                    ok  (whole repo)
go vet ./containers/ ./flags/     clean

No behavioural test added — the change is a flag guard on a code path that requires live Node.js processes and eBPF attachment to exercise meaningfully.

Attaching the libuv probes reads the entire ELF symbol table of every
Node.js process's binary, per pid. node is a large statically linked binary
with V8 embedded, so this is expensive: a customer CPU profile attributed
17.5% of a core to instrumentNodejs -> GetSymbol -> readSymbols.

What it buys is a single metric,
container_nodejs_event_loop_blocked_time_seconds_total. An org-wide code
search returns exactly one hit, the metric's own definition — no dashboard,
alert, query or application code consumes it. It is collected and stored
(24 series on dev) but never read.

ENABLE_NODEJS_TRACING defaults to false, matching ENABLE_DOTNET_TRACING
rather than the always-on Python probes.

Blast radius is limited to that metric. NodejsStats carries only
EventLoopBlockedTime, and the probes attach nothing but uv_io_poll_* and
uv_io_cb_*. TLS interception for Node.js processes is unaffected — tls.go
has no Node-specific handling and works through the libssl/gotls paths — as
is L7 tracing, which reads the syscalls rather than libuv.

Checked before nodejsChecked so that turning the flag on and restarting
instruments processes that were skipped while it was off.

Note this does not remove the symbol-table cost generally: python.go and
tls.go take the same uncached per-pid path and remain always-on.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new configuration flag, EnableNodejsTracing, which is disabled by default, to control Node.js event loop tracing. It also updates the instrumentNodejs function in containers/process.go to respect this flag, ensuring that Node.js processes are only instrumented when the flag is enabled. I have no feedback to provide.

@mayankpande88
mayankpande88 merged commit f3168fe into main Sep 10, 2026
7 checks passed
@mayankpande88
mayankpande88 deleted the feat/nodejs-tracing-opt-in branch September 10, 2026 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants