WIP: counter latency monitor#5011
Conversation
|
🚅 Deployed to the rivet-pr-5011 environment in rivet-frontend
|
|
Review: WIP Counter Latency Monitor This PR adds two new files to the kitchen-sink example: a latency load-test script (counter-latency.ts) and a raw WebSocket ping-pong actor (ping-pong-counter.ts). Overall the script is well-structured and clearly documented. The two-mode design (rtt vs. concurrent) covers the main latency measurement use cases. A few issues need attention before merge. Bugs / Correctness pingPongCounter is not registered in src/index.ts The actor in ping-pong-counter.ts is defined but never imported or added to the registry. It cannot be reached by any client. Either register it or explain in the PR description why it is intentionally excluded. inflight array grows unbounded in infinite mode In runRttMode, resolved promises are pushed into inflight but never removed. When BATCHES=0 (infinite, the default), the loop never exits and await Promise.all(inflight) is unreachable, so the array grows without bound for the process lifetime. Settled promises are small but this will accumulate over hours or days of running. Fix: drain completed promises periodically, or track in-flight count with a simple counter instead of retaining promise handles. Relationship between the two new files is unclear The latency script uses client.counter (the existing counter actor with increment / noop). The new pingPongCounter actor has neither of those actions and is not referenced anywhere in the script. If the intent is to eventually swap the script to use pingPongCounter, that work should be completed here; otherwise one of the two files is unnecessary. Code Quality any types in ping-pong-counter.ts If the WebSocket type is not yet exported by the framework, import or inline a minimal interface rather than using any. At minimum, let parsed: unknown with a proper type guard would be safer. Comment says orange but implementation uses a smooth gradient The top-of-file comment describes three discrete color thresholds (green/orange/red), but the actual implementation uses a continuous green-to-red gradient via gradientColor. The comment should be updated to match what the code actually does. Log helpers duplicate timestamp/prefix construction logConnect, logIncrement, logDisconnect, and logConnectError each rebuild the same ts + prefix string inline. Extracting a small formatPrefix(worker) helper would remove the repetition. BATCHES env var parsing lacks validation If BATCHES is set to a non-numeric string (e.g. BATCHES=foo), this silently becomes NaN. That makes BATCHES === 0 false and workerId < BATCHES always false, so the loop exits immediately with no error. Add an explicit guard with a clear error message, matching the validation pattern already used for --interval. Minor / Style
Summary The PR is draft with all checklist items unchecked, so the above are expected to be resolved before merge. The highest-priority items are: wiring pingPongCounter into the registry (or removing it), fixing the inflight unbounded growth, and clarifying the intended relationship between the two new files. |
1 participant
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: