fix(ups): add metrics#4368
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
🚅 Deployed to the rivet-pr-4368 environment in rivet-frontend
|
|
PR Review: fix(ups): add metrics This PR adds new counters/gauges/histograms to the universalpubsub layer and renames existing metrics from plural to singular form (e.g. ups_local_subscribers_count to ups_local_subscriber_count). Overall a solid observability improvement with a few things worth addressing. Breaking Change: Metric Renames The existing gauge names are changing:
Any existing dashboards, Prometheus recording rules, or alerts referencing the old names will silently break. Worth noting in the PR description if these are in active use. Code Quality Repeated inline if-let pattern (3 occurrences in pubsub.rs) The same expression appears three times inside next() and Drop::drop(). A small helper would clean this up: This pattern is already handled cleanly in Subscriber::new() via the subject_str local, just not reused downstream. Shadowed variable in publish_inner These two lines can be collapsed to: let subject_str = T::root().as_deref().unwrap_or("unknown"); TODO comment is a question, not an action item (memory/mod.rs) Per project conventions, comments should be complete sentences. Either answer the question and remove the comment, or convert it to an explanatory note describing why the GC task approach is needed. If follow-up is needed, open a separate issue rather than leaving it inline. Blank lines removed between metric sections in metrics.rs The blank lines between the Memory driver and Postgres driver comment blocks were removed. Restoring them would match the original grouping style. Potential Issue: High-Cardinality Label Documentation SUBSCRIBER_COUNT, ACTIVE_SUBSCRIBER_COUNT, MESSAGE_RECV_COUNT, MESSAGE_SEND_COUNT, and BYTES_PER_MESSAGE all use a subject/root_subject label. The Subject::root() default returns None, so any caller passing a raw &str or &String falls through to "unknown". A doc comment on Subject::root() explaining that implementors should override it to get meaningful per-subject metrics would help future implementors avoid having all traffic bucketed under "unknown". Minor Nit The histogram bucket list in BYTES_PER_MESSAGE is quite long on one line. Breaking it across lines would improve readability. Summary
The new metric additions are well-chosen and the root() approach to controlling cardinality is the right design. Addressing the repeated pattern and the TODO comment would tighten things up before merging. |
6ab4653 to
1ff340f
Compare
1ff340f to
4d4e7d1
Compare
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: