Count dropped execs instead of losing them silently - #4
Merged
Conversation
Jacob's two seeding fixes made a third bug visible. With the scope finally
covering pre-existing subtrees, a fork storm produces far more events than
before, and most of them were disappearing with nothing on screen to say so:
96,000 execs fired across 24 workers, ~36,000 shown, ~60,000 gone.
The loss itself is a real limit rather than a defect. Ring-buffer delivery is
bound by bytes moved, not events, and a record carries a 1 KiB argument
window, so throughput caps around 2,700 execs/s. Measured with an empty
callback, so it is the runtime's ceiling and not this script's per-event work
(normalize is ~4ms per 40k records, model.add ~12ms; neither is the
bottleneck). A `make -j24` can outrun it. A normal npm install, at tens per
second, cannot come close.
What was wrong is that it was silent, so every count read as a total when it
was a floor. bpf_ringbuf_output's return value was discarded. A stats_map
ARRAY now counts emitted and dropped, JS polls it, and a non-zero drop count
appears on the verdict line and in the headless report, which also says
outright that the numbers below it are a floor.
test/drops.sh asserts the accounting closes: captured + dropped must be at
least what actually ran, drops must be non-zero at storm scale (otherwise the
drop path silently goes untested), and the report must explain what the number
means. Verified: 36,490 + 59,536 = 96,026 against 96,000 fired.
Two things tried and rejected, recorded so they are not retried:
- Shrinking ARGV_BUF to 256 raises the ceiling to ~10,200/s, a 3.8x gain,
but truncates 12% of real records (measured against the checked-in
captures: p50 argv is 48 bytes, p99 is 1026). Wrong trade for a tool whose
job is showing you what ran.
- Emitting a variable-length record (header + args_len, mean 143 bytes
instead of a fixed 1076) delivers nothing at all. The ring is bound with
`btf_struct: "exec_event"`, so the consumer decodes fixed-size records and
a short write is not seen. Left in a comment at the emit site.
Also removes setCgroup and its unread target_cgid filter. Nothing ever called
it, so container mode has always been a plain pid subtree; now that the
existing tree is seeded from the process graph, the tgid set IS the scope and
a second overlapping filter would only be a way for the two to disagree.
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.
Follows #3. Those two seeding fixes made a third bug visible: with the scope finally covering pre-existing subtrees, a fork storm produces far more events, and most were disappearing with nothing on screen to say so.
96,000 execs fired across 24 workers. ~36,000 shown. ~60,000 gone.
The loss is a limit; the silence was the bug
Ring-buffer delivery is bound by bytes moved rather than events, and a record carries a 1 KiB argument window, so throughput caps around 2,700 execs/s. I measured that with an empty callback, so it is the runtime's ceiling and not this script's per-event work (
normalizeis ~4ms per 40k records,model.add~12ms; neither is close to the bottleneck). Amake -j24can outrun it. A realnpm install, at tens per second, cannot come near it.What was wrong is that
bpf_ringbuf_output's return value was discarded, so every count read as a total when it was a floor. Astats_mapARRAY now counts emitted and dropped, JS polls it, and a non-zero count appears on the verdict line:The headless report says it outright: the ring buffer filled faster than it drained, so every count below is a floor.
Tried and rejected
Recorded so nobody spends the afternoon I did:
args_len, mean 143 bytes vs a fixed 1076) delivers nothing at all. The ring is bound withbtf_struct: "exec_event", so the consumer decodes fixed-size records and a short write is never seen. Comment left at the emit site.test/drops.sh
Asserts the accounting closes rather than that the number looks plausible: captured + dropped ≥ what actually ran, drops must be non-zero at storm scale (else the drop path silently goes untested), and the report must explain what the number means.
Also
Removes
setCgroupand its unreadtarget_cgidfilter, the other loose end from #3. Nothing ever called it, so container mode has always been a plain pid subtree. Now that the existing tree is seeded from the process graph, the tgid set is the scope, and a second overlapping filter would only be a way for the two to disagree.heuristics.test.mjsandcapture.shboth still pass. Not merging.