Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/process-type-request-id-cardinality.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@core/electric-telemetry": patch
---

Fix `process_type` parsing for request processes that have a request id that is not the standard 20 bytes.
47 changes: 32 additions & 15 deletions packages/electric-telemetry/lib/electric/telemetry/processes.ex
Original file line number Diff line number Diff line change
Expand Up @@ -262,26 +262,43 @@ defmodule ElectricTelemetry.Processes do
defp parse_binary_label(label) do
case label do
"Request " <> <<_req_id::binary-20, " - ", rest::binary>> ->
# Fast path: the request id is exactly 20 bytes, as generated by Plug.RequestId.
# This is a request process with the label assigned by Electric.Plug.LabelProcessPlug.
# We group all requests together to be able to see their aggregated mem footprint.
parse_request_label(rest)

"Request " <> rest ->
# Slow path: the request id has a non-default length (e.g. a UUID or a proxy-supplied
# x-request-id), so the fixed-width match above failed. Split on the " - " delimiter
# instead so all requests to a route collapse to a single label regardless of id length.
case :binary.split(rest, " - ") do
[_req_id, request] -> parse_request_label(request)
_ -> truncate_label(label)
end

# Cut off the URL query part, leaving only the `<method> <path>` prefix.
part_len =
case :binary.match(rest, "?") do
{pos, _} ->
pos
_ ->
truncate_label(label)
Comment on lines +279 to +280

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What specific problems does unbounded process type cardinality cause? Is it controlled by the client, i.e. does it have a DoS potential?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end

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

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

_ ->
# Opportunistic grouping by truncating long labels.
part_len = min(20, byte_size(label))
:binary.part(label, 0, part_len)
end
:binary.part(request, 0, part_len)
end

defp truncate_label(label) do
# Opportunistic grouping by truncating long labels.
part_len = min(20, byte_size(label))
:binary.part(label, 0, part_len)
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,27 @@ defmodule ElectricTelemetry.ProcessesTest do
assert "GET /v1/health" = proc_type(pid)
end

test "groups request labels with a non-default-length request id (e.g. a proxy-supplied UUID)" do
pid =
spawn_with_label(
"Request 123e4567-e89b-12d3-a456-426614174000 - GET /v1/shape?table=users&offset=-1"
)

assert "GET /v1/shape" = proc_type(pid)
end

test "groups request labels with a shorter-than-default request id" do
pid = spawn_with_label("Request abc123 - GET /v1/health")

assert "GET /v1/health" = proc_type(pid)
end

test "request label with no \" - \" delimiter is truncated" do
pid = spawn_with_label("Request something-without-delimiter")

assert "Request something-wi" = proc_type(pid)
end

test "non-request binary labels are truncated to 20 chars" do
pid = spawn_with_label("some_long_label_that_exceeds_twenty_characters")

Expand Down
11 changes: 7 additions & 4 deletions packages/sync-service/test/electric/shape_cache_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -1579,10 +1579,13 @@ defmodule Electric.ShapeCacheTest do
# explicit `start_consumer_for_handle/2` call is required. The
# continue runs asynchronously after `start_supervised!` returns,
# so we wait for the registry to populate.
assert wait_until(1000, fn ->
not is_nil(Electric.Shapes.ConsumerRegistry.whereis(stack_id, shape_handle)) and
not is_nil(Electric.Shapes.ConsumerRegistry.whereis(stack_id, dep_handle))
end)
assert wait_until(
fn ->
not is_nil(Electric.Shapes.ConsumerRegistry.whereis(stack_id, shape_handle)) and
not is_nil(Electric.Shapes.ConsumerRegistry.whereis(stack_id, dep_handle))
end,
1000
)

# Materializer should be started
assert Process.alive?(
Expand Down
Loading