Skip to content

Commit 044a8d5

Browse files
robacourtclaude
andauthored
fix(electric-telemetry): Fix process_type parsing for request processes that have a request id that is not the standard 20 bytes. (#4658)
## Summary Fixes an unbounded `process_type` cardinality leak in telemetry for request-handling processes. ## Problem `process_type` for request-handling processes is derived from the process label set by `Electric.Plug.LabelProcessPlug`, which looks like: ``` Request F-jPUudNHxbD8lIAABQG - GET /v1/shape?table=users&offset=-1 ``` `parse_binary_label/1` is meant to strip the request id and query, collapsing this to a bounded `GET /v1/shape`. But it did so with a fixed-width binary match that assumes the request id is exactly 20 bytes: ```elixir "Request " <> <<_req_id::binary-20, " - ", rest::binary>> -> ... ``` That holds for ids `Plug.RequestId` self-generates, but `Plug.RequestId` reuses an incoming `x-request-id` header 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 to `prometheus_metrics_dist` for events like `vm.monitor.long_gc` and `vm.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 injects `x-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 single `process_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` (in `sync-service`, unrelated to the telemetry change). The cause is a semantic merge collision already on `main`: #4635 changed `wait_until`'s signature to `wait_until(fun, timeout_ms)` (function first), while #4651 added a new call site at `shape_cache_test.exs:1582` using the old arg order `wait_until(1000, fn -> ... end)`. The two merged cleanly in git but not in meaning, so the slow suite raised a `FunctionClauseError` on every run. This PR swaps the arguments to match the current signature, unblocking both `main` and this PR. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 0130c4a commit 044a8d5

4 files changed

Lines changed: 65 additions & 19 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@core/electric-telemetry": patch
3+
---
4+
5+
Fix `process_type` parsing for request processes that have a request id that is not the standard 20 bytes.

packages/electric-telemetry/lib/electric/telemetry/processes.ex

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -262,26 +262,43 @@ defmodule ElectricTelemetry.Processes do
262262
defp parse_binary_label(label) do
263263
case label do
264264
"Request " <> <<_req_id::binary-20, " - ", rest::binary>> ->
265+
# Fast path: the request id is exactly 20 bytes, as generated by Plug.RequestId.
265266
# This is a request process with the label assigned by Electric.Plug.LabelProcessPlug.
266267
# We group all requests together to be able to see their aggregated mem footprint.
268+
parse_request_label(rest)
269+
270+
"Request " <> rest ->
271+
# Slow path: the request id has a non-default length (e.g. a UUID or a proxy-supplied
272+
# x-request-id), so the fixed-width match above failed. Split on the " - " delimiter
273+
# instead so all requests to a route collapse to a single label regardless of id length.
274+
case :binary.split(rest, " - ") do
275+
[_req_id, request] -> parse_request_label(request)
276+
_ -> truncate_label(label)
277+
end
267278

268-
# Cut off the URL query part, leaving only the `<method> <path>` prefix.
269-
part_len =
270-
case :binary.match(rest, "?") do
271-
{pos, _} ->
272-
pos
279+
_ ->
280+
truncate_label(label)
281+
end
282+
end
273283

274-
:nomatch ->
275-
# No query string means only path is present. It is either a request to the health endpoint or something unexpected.
276-
byte_size(rest)
277-
end
284+
defp parse_request_label(request) do
285+
# Cut off the URL query part, leaving only the `<method> <path>` prefix.
286+
part_len =
287+
case :binary.match(request, "?") do
288+
{pos, _} ->
289+
pos
278290

279-
:binary.part(rest, 0, part_len)
291+
:nomatch ->
292+
# No query string means only path is present. It is either a request to the health endpoint or something unexpected.
293+
byte_size(request)
294+
end
280295

281-
_ ->
282-
# Opportunistic grouping by truncating long labels.
283-
part_len = min(20, byte_size(label))
284-
:binary.part(label, 0, part_len)
285-
end
296+
:binary.part(request, 0, part_len)
297+
end
298+
299+
defp truncate_label(label) do
300+
# Opportunistic grouping by truncating long labels.
301+
part_len = min(20, byte_size(label))
302+
:binary.part(label, 0, part_len)
286303
end
287304
end

packages/electric-telemetry/test/electric/telemetry/processes_test.exs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,27 @@ defmodule ElectricTelemetry.ProcessesTest do
1616
assert "GET /v1/health" = proc_type(pid)
1717
end
1818

19+
test "groups request labels with a non-default-length request id (e.g. a proxy-supplied UUID)" do
20+
pid =
21+
spawn_with_label(
22+
"Request 123e4567-e89b-12d3-a456-426614174000 - GET /v1/shape?table=users&offset=-1"
23+
)
24+
25+
assert "GET /v1/shape" = proc_type(pid)
26+
end
27+
28+
test "groups request labels with a shorter-than-default request id" do
29+
pid = spawn_with_label("Request abc123 - GET /v1/health")
30+
31+
assert "GET /v1/health" = proc_type(pid)
32+
end
33+
34+
test "request label with no \" - \" delimiter is truncated" do
35+
pid = spawn_with_label("Request something-without-delimiter")
36+
37+
assert "Request something-wi" = proc_type(pid)
38+
end
39+
1940
test "non-request binary labels are truncated to 20 chars" do
2041
pid = spawn_with_label("some_long_label_that_exceeds_twenty_characters")
2142

packages/sync-service/test/electric/shape_cache_test.exs

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1579,10 +1579,13 @@ defmodule Electric.ShapeCacheTest do
15791579
# explicit `start_consumer_for_handle/2` call is required. The
15801580
# continue runs asynchronously after `start_supervised!` returns,
15811581
# so we wait for the registry to populate.
1582-
assert wait_until(1000, fn ->
1583-
not is_nil(Electric.Shapes.ConsumerRegistry.whereis(stack_id, shape_handle)) and
1584-
not is_nil(Electric.Shapes.ConsumerRegistry.whereis(stack_id, dep_handle))
1585-
end)
1582+
assert wait_until(
1583+
fn ->
1584+
not is_nil(Electric.Shapes.ConsumerRegistry.whereis(stack_id, shape_handle)) and
1585+
not is_nil(Electric.Shapes.ConsumerRegistry.whereis(stack_id, dep_handle))
1586+
end,
1587+
1000
1588+
)
15861589

15871590
# Materializer should be started
15881591
assert Process.alive?(

0 commit comments

Comments
 (0)