feat(metrics): count HTTP/2 frames by type (without logging payloads) - #320
Merged
Conversation
The pipeline counters from #317 localised the failure but not its cause. External HTTP/2, 10 minutes on dev: L7 events 1,155,643 internal control: 844,250 stream_created 53 8,052 response_status 0 5,478 completed 0 5,158 That is ~22,000 external events per stream created, against ~105 internally — a 200x difference — and response_status is exactly zero. Zero matters because :status decodes from the HPACK static table (index 8), which survives a degraded decoder; the internal control produces 5,478 of them on the same code path. Server-side HEADERS are therefore never decoded externally at all. Also ruled out by that run, so they need no further attention: parser cap drops were 0, and stale fd reuse was 3 events externally in 10 minutes. Two explanations remain for the ratio, and direction separates them cleanly: server-frame events scarce -> responses never reach the parser server-frame events plentiful -> the bytes are not HTTP/2 and the eBPF port-based detection (is_likely_http2_port, 443/8443, plus a frame-shape check) is over-matching TLS traffic The second would also recast node_agent_hpack_decode_errors_total as a symptom of feeding non-HTTP/2 bytes to an HPACK decoder rather than a cause. Adds a direction label ("client"/"server", "-" where inapplicable) to node_agent_l7_events_total. Measurement only.
Frame-type counts, not payload dumps. The obvious way to answer "what are these
bytes" is to log the payloads, but these events carry decrypted application
traffic — dumping them would put Authorization headers and request bodies from
GitHub and every other external API into agent logs. Frame type, flags and
length are structural and disclose nothing, and they answer the question just
as well.
The question: external HTTP/2 delivers ~44k client-frame events per 5 minutes
yielding ~71 streams, against ~161k events and ~10.8k streams internally — 86x
worse — while response_status is exactly zero externally and 5.5k internally.
node_agent_http2_frames_total{type,destination} discriminates:
"invalid" dominant -> the bytes are not HTTP/2; the eBPF port
heuristic (443/8443 + frame-shape check) is
over-matching, and the 6.28M HPACK errors are
a symptom of feeding it non-HTTP/2 data
DATA/WINDOW_UPDATE dominant,
HEADERS absent -> the bytes are HTTP/2 but request headers are
lost before the parser sees them
Also adds two tests that rule out a hypothesis rather than support one. After a
truncated event the parser was assumed to lose frame alignment permanently;
these show it recovers, because eBPF truncates a single SSL_write and the next
event is the next write, which begins on a frame boundary. Misalignment is
therefore not the cause and #316 was sufficient for what it addressed. Keeping
them as regression cover for the truncation path.
Measurement only.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Prometheus metric, node_agent_http2_frames_total, to track parsed HTTP/2 frame types and help diagnose connection alignment issues. It also adds unit tests to verify parser behavior under truncated events. The review feedback highlights three important issues: first, calling WithLabelValues on every frame can cause significant CPU overhead and lock contention, which can be resolved by pre-allocating the counters; second, a logical flaw in TestHttp2MidFrameContinuationIsMisread places the fake header in the discarded chunk, making the test a false positive; and third, the check h.Type > 9 will incorrectly flag valid HTTP/2 extension frames (such as ALTSVC or ORIGIN) as invalid, leading to unintended connection desynchronization.
…g builds All image builds began failing with a 404 on a specific package version: E: Failed to fetch .../libperl5.32_5.32.1-4+deb11u5_amd64.deb 404 Not Found The base image ships a package index that can reference .debs the mirror has already pruned after a point release. apt-get update issues a conditional GET which the CDN may answer from cache, so the stale index survives and install then asks for a version the pool no longer serves. Two consecutive builds failed identically, so this is not transient. Drops the cached lists before updating to force a genuinely fresh index, adds Acquire::Retries to ride out single-node staleness, and cleans lists afterwards to keep the layer smaller. bullseye is oldstable, so archive rotation will keep happening; if this recurs the durable answer is pinning snapshot.debian.org, which is a larger change than unblocking the build warrants right now.
--no-install-recommends, added in the previous commit to slim the layer, drops ca-certificates because it is only a recommended dependency of curl rather than a required one. The Go toolchain download in the next layer then fails TLS verification with curl exit 77. The apt index fix itself worked — the 404 on a pruned .deb is gone.
96% of external HTTP/2 events yield no parseable frame (8,881 frames from ~221k
events) against 44% internally. Parse() can only produce nothing for three
reasons: an empty payload, fewer than 9 bytes, or a first frame header that
fails validation — and the third is already counted as type="invalid". So the
answer is in the size distribution, which nothing currently reports.
node_agent_http2_payload_size_total{bucket,destination,direction} splits it.
Bucket boundaries are chosen for the parser, not for readability: 0 and 1-8
cannot produce a frame at all, and 9-16 is a bare frame header with little or no
payload attached.
That middle bucket is the hypothesis. A correct HTTP/2 reader does
io.ReadFull(header[:9]) and then reads the frame payload as a separate call, so
SSL_read returns header-sized and payload-only chunks rather than whole frames.
Parse() assumes every event begins on a frame boundary and contains complete
frames. If external reads are predominantly 9 bytes, that assumption is the bug,
and the fix is to treat each connection as a continuous byte stream rather than
a sequence of frame-aligned events.
Stated as a hypothesis: six previous diagnoses were wrong, and this measurement
can refute this one as easily as confirm it.
Scoped to HTTP/2 to keep label cardinality small.
…ions
External HTTP/2 produced no decoded requests at all: 84% of its frames failed
validation (vs 12% internally), response_status was zero against 8,669
internally, and the cluster logged 6.28M HPACK errors per 12h. Six earlier
diagnoses — mid-stream join, attach ordering, payload truncation, parser cap,
stale fd reuse, undersized reads — were each measured and refuted.
The cause is detection, not parsing. looks_like_http2_frame requires only
frame_type <= 9, a clear reserved bit, a HEADERS type byte and one HPACK byte
with static index 1-14. Ported to Go and run over 2M random 64-byte buffers,
that accepts non-HTTP/2 data once every ~9,174 buffers.
That rate alone is survivable. conn->protocol is what makes it fatal: the
verdict is cached for the connection's lifetime, so a single false positive
converts every later event on that connection into an HTTP/2 event permanently.
A large binary transfer over HTTPS/1.1 — image layers, S3 objects, exactly what
the CI runners do — performs tens of thousands of reads, making at least one
false positive near-certain (66% at 10k reads, 99.6% at 50k). Those connections
then feed the HPACK decoder garbage forever, which is the 6.28M errors, and
drown the genuine HTTP/2 traffic we care about.
Two changes:
eBPF. The frame-shape heuristic now only runs within the first 64KB of a
connection. Real HTTP/2 announces itself immediately with the client preface and
SETTINGS on stream 0, so early detection is sufficient. Nothing recoverable is
lost: a connection joined mid-stream has unrecoverable HPACK dynamic-table
state, so today it is "detected" only to emit undecodable garbage.
Userspace. HTTP/2 now feeds trackParseFail/protocolOverride. That mechanism
exists for precisely this ("eBPF protocol misidentification where weak
heuristics tag a connection permanently") and was wired for Postgres, ClickHouse
and Zookeeper — but never for HTTP/2, which has the weakest heuristic of the
four. A connection yielding no structurally valid frame is reclassified after
parseFailThreshold events. Empty payloads are not counted as failures.
ebpf.go regenerated via `cd ebpftracer && make build`; the .c change has no
effect otherwise, since the main Dockerfile only runs `go build`.
Verification is the metrics already deployed: node_agent_http2_frames_total
type="invalid" should collapse for destination="external", and
node_agent_http2_stage_total stage="response_status" should become non-zero.
…g builds deb.debian.org publishes a bullseye-security index advertising .debs already pruned from the pool after a point release, so builds fail with a 404 on an exact version (2026-09-05: git 1:2.30.2-1+deb11u5; earlier: libperl5.32 5.32.1-4+deb11u5). It is a mirror-side inconsistency, not a cache or CDN-node problem: clearing /var/lib/apt/lists and retrying five times were both tried and both failed on every attempt, locally and in CI. snapshot.debian.org serves index and pool as a matched pair at a fixed timestamp, making the build reproducible and immune to rotation. Check-Valid-Until is disabled because a pinned snapshot's Release file is deliberately older than apt's freshness window. Verified locally: apt now completes and the build proceeds to compiling the agent. bullseye is oldstable and its archive keeps rotating, so without this every build in the repo stays exposed — releases included, which is why this should land on main independently of the diagnostic work it is currently branched with.
Contributor
Author
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves HTTP/2 protocol detection and parser resilience by restricting the eBPF detection heuristic to an early window of the connection, tracking parse failures to handle misidentified connections, and adding detailed metrics for HTTP/2 frame types and payload sizes. The review feedback highlights two key issues: first, in ebpftracer/l7/http2.go, breaking the loop on an invalid frame without updating the offset causes the remaining invalid payload to be saved as a partial frame and re-parsed on subsequent calls; second, in ebpftracer/l7/http2_resync_test.go, the test TestHttp2MidFrameContinuationIsMisread has a logical flaw where the fake header is placed in the truncated portion of the payload, meaning it is never parsed and the test passes for the wrong reason.
…nters Review caught a regression I introduced. The frame loop treated any type > 9 as invalid, which breaks out of the loop and discards the rest of the payload. ALTSVC (0x0a), ORIGIN (0x0c) and PRIORITY_UPDATE (0x10) are standard extensions that GitHub and Google both send, and RFC 9113 4.1 requires unknown types to be ignored and skipped rather than treated as errors. This mattered more than a spec deviation: callers now reclassify connections that yield no structurally valid frame, so a legitimate HTTP/2 connection whose payload merely led with an extension frame could have been dropped as if it had been misdetected — undoing the fix it was meant to protect. Types 0x0a-0x10 are now skipped and counted as extension. Beyond 0x10 there is no registered type, so those still count as invalid and stop the loop; that boundary keeps the misdetection signal intact, which a blanket skip would have destroyed (caught by TestHttp2SawValidFrameDistinguishesGarbage failing). Writing the test first also caught an offset bug in the fix: offset still points at the frame header at that point, so skipping only h.Length left the parser mid-frame. It now advances header+payload. Replaced TestHttp2MidFrameContinuationIsMisread, which review correctly identified as a false positive — it passed only because the second chunk was all zeros and never reached the fake header, so it never exercised its own claim. Also pre-resolves the frame counters. OnHttp2Frame fires per frame (~1.6k/s) and WithLabelValues hashes labels and takes the vector read lock on every call; the label sets are small and fixed, so they are resolved once at startup.
…pture # Conflicts: # containers/container.go
Review caught that both invalid-frame paths break with offset still at frameStart. The partial-frame save at the end of Parse then buffers the garbage and prepends it to the next call, so a misdetected connection re-parses the same invalid bytes on every event — burning CPU and never draining. That is worse here than it looks: this branch reclassifies connections yielding no valid frame, and replaying the same garbage keeps the invalid-frame counters elevated for a connection that has effectively already been given up on. Both paths now set offset = len(payload) before breaking. Mutation-tested: restoring the plain break fails TestHttp2InvalidFrameDoesNotAccumulate with 18 bytes retained.
blue4209211
approved these changes
Sep 5, 2026
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
2 participants
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.
Frame-type counts instead of payload dumps
The natural way to answer "what are these bytes?" is to log the payloads. I did not do that: these L7 events carry decrypted application traffic, so a dump would put
Authorizationheaders and request bodies from GitHub and every other external API into agent logs and into whatever ships them.Frame type, flags and length are structural metadata. They disclose nothing and answer the question just as well.
The question
Server frames are plentiful, so responses do reach the parser — that rules out the "responses never arrive" branch. But external events are 86× less likely to produce a stream, and
response_statusis exactly zero while the internal control on the same code path runs at 83%.What this discriminates
node_agent_http2_frames_total{type,destination}:invaliddominantis_likely_http2_port, 443/8443, plus a frame-shape check) is over-matching TLS traffic, and the 6.28M HPACK errors are a symptom of feeding non-HTTP/2 data to an HPACK decoderEither way it is one deploy, and the branch-build support from #319 means no release tag.
Also: two tests that killed a hypothesis
I expected truncation to cost frame alignment permanently — eBPF discards the tail of a write, so the next event should start mid-frame.
TestHttp2TruncationLosesFrameAlignmentandTestHttp2MidFrameContinuationIsMisreadshow it does not. eBPF truncates a singleSSL_write, and the next event is the next write, which begins on a frame boundary because applications write whole frames. Misalignment is not the cause, and #316 was sufficient for what it addressed.Worth noting these ran locally in seconds —
ebpftracer/l7builds on macOS, unlikecontainers. Testing that hypothesis in the cluster would have cost another build-deploy cycle. Keeping both as regression cover for the truncation path.Status
Measurement only, no behaviour change.
gofmt,GOOS=linux gopls check, full CI suite pass.Five diagnoses have been wrong so far. This does not assert a cause — it is the cheapest measurement that splits the two remaining ones, and it deliberately avoids the payload-logging shortcut.