Repository navigation
fix: register uprobe programs before attaching, add opt-in --instrumentation-delay (B4a of #369) - #378
Open
mayankpande88 wants to merge 3 commits into
Open
fix: register uprobe programs before attaching, add opt-in --instrumentation-delay (B4a of #369)#378mayankpande88 wants to merge 3 commits into
mayankpande88 wants to merge 3 commits into
Conversation
Port of coroot/coroot-node-agent@2c72586 ("register uprobes before reporting the running processes"). Uprobe programs were registered in t.uprobes inside the attach loop, interleaved with attaching tracepoints and kprobes. Events start flowing once the first of those is attached, and handleEvents (already running) could attach TLS probes for a process before its program was registered: the attach failed, the process was marked as checked and never retried. Registering them right after the collection loads also removes concurrent writes to t.uprobes while handleEvents reads it.
…instrumentation (cherry picked from commit 44e3e8ef582f04dabe44fa7ed78d0838c5e43aae) In this fork the delay covers Python GIL probes and the opt-in Node.js and .NET instrumentation; TLS probes are attached per connection and are not delayed. Upstream's instrumentDone channel belongs to its uprobe dedupe and is not taken.
Upstream delays Python GIL and Node.js event-loop instrumentation by 30s to save the attach cost on short-lived processes. Here Node.js tracing is already opt-in, and with about 300 short-lived Python processes a minute the delay saved about 2.6 millicores of agent CPU (2290 vs 2134 ms over 60s), while hiding GIL metrics for each process's first 30s. Keep the flag and leave instrumentation immediate by default.
12 of 34 tasks
There was a problem hiding this comment.
Code Review
This pull request introduces an instrumentation delay configuration (--instrumentation-delay) to delay Python GIL and Node.js event loop instrumentation after a process starts. It also updates the eBPF tracer to register uprobe and uretprobe programs before tracepoints or kprobes are attached, ensuring no events are missed. Feedback suggests replacing time.After with time.NewTimer in the delay logic to prevent potential timer leaks if the process context is cancelled before the delay expires.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First part of B4 (#369): the uprobe-lifecycle fixes from upstream coroot-node-agent that apply here without its uprobe deduplication.
handleEventswas already running. A process handled before its uprobe program was registered failed to attach, was marked as checked and was never retried. It also meantt.uprobeswas written whilehandleEventsread it. Now every uprobe program is registered right after the collection loads, before anything is attached.--instrumentation-delay0(off) here; upstream uses 30s. TLS probes aren't affected.Not taken:
Process.Close): fix(tls): count TLS capture losses and fix the gaps they exposed #353 already closes links that arrive afterClose.Engineering detail
Why the delay defaults to 0: upstream added it to save attach cost on short-lived processes. Here Node.js tracing is already opt-in (
--enable-nodejs-tracing), so the delay mostly covers the Python GIL probes. With about 300 short-lived Python processes a minute, it saved about 2.6 millicores of agent CPU (2290 vs 2134 ms over 60s), at the price of losing GIL metrics for each process's first 30s.44e3e8e conflicts: upstream's
instrumentDonechannel belongs to its deduplication, so it isn't taken. The flag line follows this fork's flag style.CI: gofmt, goimports, vet, golangci-lint,
go test(excluding/containers) and the build all pass in a Linux container with Go 1.26.5.Local e2e: I built agent binaries from this branch and from main and ran them as systemd services on a local Debian 12 VM (kernel 6.1).