fix(electric-telemetry): Fix process_type parsing for request processes that have a request id that is not the standard 20 bytes.#4658
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4658 +/- ##
==========================================
+ Coverage 59.99% 60.18% +0.18%
==========================================
Files 395 410 +15
Lines 43747 44348 +601
Branches 12582 12581 -1
==========================================
+ Hits 26246 26689 +443
- Misses 17423 17580 +157
- Partials 78 79 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Claude Code ReviewSummaryThis PR fixes a real, unbounded What's Working Well
Issues FoundCritical (Must Fix)None. Important (Should Fix)None. Suggestions (Nice to Have)
Issue ConformanceNo linked issue, but the PR description thoroughly documents both the telemetry problem/mechanism/fix and the CI fix's root cause. Implementation matches with no scope creep beyond the intentionally-included CI fix. Previous Review StatusIterations 1-2 found no Critical/Important issues in the telemetry change; that still holds (byte-for-byte unchanged). Resolution of prior discussion threads is unchanged:
New this round: the LGTM. Review iteration: 3 | 2026-06-30 |
| _ -> | ||
| truncate_label(label) |
There was a problem hiding this comment.
I'm wondering if we should have a fixed label, say "Unparseable Request" and to log a warning with the full label, to prevent an unbounded process type in this scenario
There was a problem hiding this comment.
What specific problems does unbounded process type cardinality cause? Is it controlled by the client, i.e. does it have a DoS potential?
There was a problem hiding this comment.
It came up as DoS potential when investigating promethius issues, but it turns out the issue was to do with promethus library not aggregating until it is drained. It doesn't matter whether the label is unbounded or not, if the prometheus metrics are not aggregated, they grow unbounded. So I'll leave this as is.
robacourt
changed the title
…plied request ids parse_binary_label/1 only stripped the request id from process labels when it was exactly 20 bytes (the length Plug.RequestId self-generates). A proxy- or load-balancer-supplied x-request-id (UUID, Envoy/nginx trace id, etc.) has a different length, so the raw per-request id leaked into process_type, adding a permanent new series to telemetry events like vm.monitor.long_gc and vm.monitor.long_schedule for every distinct id. Keep the fast fixed-width match for the common 20-byte case and add a slow path that splits on the " - " delimiter regardless of request-id length, collapsing all requests to a route back to a single process_type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `start_consumer_for_handle/2 starts a consumer plus dependencies` test called `wait_until(1000, fn -> ... end)`, but #4635 changed the signature to `wait_until(fun, timeout_ms \\ 500)` (function first). The new call site was added by #4651 using the old arg order; the two merged cleanly in git but not in meaning, so `1000` was bound to `fun`, failed the `is_function(fun, 0)` guard, and raised FunctionClauseError whenever the slow suite ran on main. Swap the arguments to match the current signature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
robacourt
force-pushed
the
rob/request-process-type-fix
branch
from
119d222 to
767b117
Compare
2 participants
Summary
Fixes an unbounded
process_typecardinality leak in telemetry for request-handling processes.Problem
process_typefor request-handling processes is derived from the process label set byElectric.Plug.LabelProcessPlug, which looks like:parse_binary_label/1is meant to strip the request id and query, collapsing this to a boundedGET /v1/shape. But it did so with a fixed-width binary match that assumes the request id is exactly 20 bytes:That holds for ids
Plug.RequestIdself-generates, butPlug.RequestIdreuses an incomingx-request-idheader when a client/proxy supplies one. A UUID (36 chars), an Envoy/nginx id, or an LB trace id is not 20 bytes, so the clause falls through to the generic branch that returns the first 20 bytes of the whole label — i.e.Request <first 12 chars of the id>. Every distinct incoming request id that GCs slowly then adds a permanent new series toprometheus_metrics_distfor events likevm.monitor.long_gcandvm.monitor.long_schedule. Request handlers building large shape responses are exactly the processes that trip those monitors, so the table grows without bound when a fronting proxy injectsx-request-id.Solution
Keep the fast fixed-width binary match for the common 20-byte case (
Plug.RequestId's self-generated ids), and add a slow path that only runs when the id is a different length: it splits on the" - "delimiter regardless of request-id length, then strips the query string. All requests to a route collapse to a singleprocess_type(e.g.GET /v1/shape) regardless of request-id source or length. A"Request "label with no delimiter falls back to truncation.Also includes: unrelated CI fix
CI for this PR was failing on
Electric.ShapeCacheTest(insync-service, unrelated to the telemetry change). The cause is a semantic merge collision already onmain: #4635 changedwait_until's signature towait_until(fun, timeout_ms)(function first), while #4651 added a new call site atshape_cache_test.exs:1582using the old arg orderwait_until(1000, fn -> ... end). The two merged cleanly in git but not in meaning, so the slow suite raised aFunctionClauseErroron every run. This PR swaps the arguments to match the current signature, unblocking bothmainand this PR.