Repository navigation
Bound websocket metric labels for issue 337 - #401
Conversation
|
@Jossy-ossy Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
Correct, and it closes the hole: after this the label set for those three metrics is exactly {other, *}, so no client-supplied string can create a series. I checked the three call sites — subscribe, unsubscribe and send — and all three go through pairMetricLabel, which is what matters, because a bounded inc with an unbounded dec would have been worse than neither.
The test is the right shape too. Recording label values through the prom-client mock with vi.hoisted and asserting on what was actually passed is how you keep this fixed — a test that asserts "the counter was incremented" would pass forever while the label regressed.
And you put the rule in docs/http-metrics.md as a general statement rather than a note about websockets. That is the version that stops the next metric reintroducing it.
One thing I am going to follow up on myself, so you do not have to. Folding every pair into other is safe but costs all per-pair visibility — ws_clients_total{pair="other"} and {pair="*"} together are barely more than one number, so "which pair actually has subscribers" is no longer answerable. What I had in mind in #337 was to keep known pairs as labels, since config.pairs is a bounded, server-defined set, and fold only unknown ones into other.
That is a small change on top of yours and the safety property is identical. Merging this now because the unbounded case is the one that actually hurts, and I will add the known-pair refinement in a follow-up.
…ones #401 closed the cardinality hole in ws_clients_total / ws_messages_sent_total / ws_messages_dropped_total by folding every pair-specific subscription into `other`. That is safe — no client-supplied string can mint a series — but it also costs all per-pair visibility: {pair="other"} and {pair="*"} together are barely more than one number, so "which pair actually has subscribers" stops being answerable. config.pairs is operator-defined and fixed for the life of the process, which makes it a bounded label domain. A subscription to a watched pair now keeps its own label; everything else still collapses into `other`. The safety property is unchanged: the whole unbounded space is one series. The test environment configures no pairs, so the test stubs one — without that the watched-pair branch is unreachable and the assertion would pass for the wrong reason. Claude-Session: https://claude.ai/code/session_01USgemLt4Rnz4SGB1Srf3GB
Closes #337. Bounds pair-specific websocket metric labels by mapping unknown pairs to
other, while keeping the*aggregate. Adds coverage for unknown pair cardinality and wildcard counting, and documents the label policy. Vitest was unavailable locally because dependencies are not installed;git diff --checkpassed.