Add per-client queue length metrics #2754
Draft
Conversation
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
It's a metric that tracks the size of the incoming per-client `message_queue`. We've been concerned about not having visibility into this queue's length, as currently we only have the length of the per-database reducer queue, but each client can only have a single reducer in that queue, and may have additional messages waiting in its per-client queue. The new metric, `spacetime_client_connection_incoming_queue_length`, is an `IntGaugeVec` with the labels: `db: Identity, client_identity: Identity, connection_id: ConnectionId`. My theory is that in our viewer we can inspect the average and the sum per database, and it also may be interesting to be able to look at individual clients, as e.g. the BitCraft mob monitor may be a notable outlier. This is not the same pattern as most of our other metrics, though, which tend to only offer per-database granularity. It's possible that this new metric should also have the last two labels removed, and be labeled only on `db: Identity`.
joshua-spacetime
approved these changes
May 19, 2025
Comment on lines
+234
to
+237
| scopeguard::defer!( | ||
| if let Err(e) = WORKER_METRICS | ||
| .client_connection_incoming_queue_length | ||
| .remove_label_values(&addr, &client_identity, &connection_id) { |
There was a problem hiding this comment.
In lieu of a test, I'd just add a comment here about resetting the gauge when the connection ends since that's something that has plagued us before.
jsdt
reviewed
May 19, 2025
|
|
||
| #[name = spacetime_client_connection_incoming_queue_length] | ||
| #[help = "The number of client -> server WebSocket messages waiting in a client connection's incoming queue"] | ||
| #[labels(db: Identity, client_identity: Identity, connection_id: ConnectionId)] |
There was a problem hiding this comment.
I think we should avoid adding any metrics with per-client or per-connection labels. I'm not sure how big the impact would be on our metrics collection/storage/querying performance, but the cardinality of a stat like this can blow up very quickly.
There was a problem hiding this comment.
As discussed out-of-band, I'll alter this to be a per-database metric, which I suppose will be the sum of the length of all clients' queues. I expect to open a separate PR for that, since we want to use this PR in testing.
Like the previous metric added in this branch, it's per-client, so we'll use it for testing, but likely not merge it into master. I'll follow up in a separate PR with a version that's per-database instead.
gefjon
changed the title
client_connection_incoming_queue_length metric
gefjon
added a commit
that referenced
this pull request
May 21, 2025
This commit alters the message queue length metrics introduced by #2754 to be per-database, rather than per-client. This should limit the cardinality of these metrics, and better lines up with the labels of our other metrics. Because the metrics are now per-database rather than per-client, it's no longer correct to just drop the label when the client disconnects. Instead, care must be taken to decrement the metric by the number of messages which were waiting in the queue at the time of the disconnection. I've added comments to call attention to this complexity.
Included resolving merge conflicts in client_connection.rs, moving the sendtx queue length metric into the `ClientConnectionMetrics`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of Changes
This PR adds two new metrics,
spacetime_client_connection_incoming_queue_lengthandspacetime_client_connection_outgoing_queue_length, which track the lengths of the per-client incoming and outgoing message queues. We expect to use these metrics in testing, incl. bot testing, but not to merge them, as we expect the cardinality of per-client metrics to be too high. I will open a separate PR with per-database versions of these metrics which we can merge.