feat(loadgen): room-read max-rps workload#266
Conversation
|
Warning Review limit reached
More reviews will be available in 52 minutes and 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR adds a new ChangesRoom-read workload for loadgen max-RPS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/loadgen/README.md (1)
289-291:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate max-rps flag table to include
room-read.The
--workloadand default-steps rows still document onlymessages|history; this now misstates the CLI surface after adding room-read.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/loadgen/README.md` around lines 289 - 291, Update the README table row describing `--workload` and `--steps` to include the new `room-read` workload and its corresponding default `--steps` values: add `room-read` to the `--workload` cell (so it reads `messages`, `history` or `room-read`) and add the appropriate default steps for `room-read` in the `--steps` cell (matching how `messages` and `history` are documented, e.g., list the ordered RPS values or `k` suffix rules for `room-read`); ensure `--preset` remains documented as required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-06-02-loadgen-room-read-maxrps.md`:
- Line 33: The heading "### Task 1: Read-state backfill fixtures" jumps a level
and triggers MD001; update that heading to the correct intermediate level to
match the surrounding document hierarchy (e.g., change it to "#### Task 1:
Read-state backfill fixtures" or "## Task 1: Read-state backfill fixtures" as
appropriate) so the markdown heading levels progress sequentially and satisfy
linting.
- Line 1000: The markdown contains a blank line inside a blockquote continuation
which triggers MD028; locate the multi-line note block (the lines starting with
">" that form the note) and remove the empty line between the quoted lines so
the blockquote is contiguous, or if the blank line is intentional, prefix it
with ">" to keep it as part of the blockquote; ensure the entire note block
remains a continuous sequence of lines beginning with ">" to satisfy MD028.
In `@docs/superpowers/specs/2026-06-02-loadgen-room-read-maxrps-design.md`:
- Around line 104-106: The spec currently says RunStep uses runFor but the
implementation uses runRoomReadFor (runFor is typed to *HistoryGenerator), so
update the spec to reference runRoomReadFor instead of runFor; mention that
RunStep runs warmup then hold via runRoomReadFor, and ensure the note about
buildRoomReadInputs producing rpsStepInputs remains intact so the text matches
the tools/loadgen implementation and symbols RunStep, runRoomReadFor, runFor,
HistoryGenerator, and buildRoomReadInputs are consistent.
- Around line 159-160: The docs incorrectly reference a non-existent Makefile
target `run-roomread`; update the wiring text to reference the actual deploy
invocation `run-max-rps WORKLOAD=room-read` (and, if listing related targets,
mention `seed-roomread` and `teardown-roomread` only as applicable) so the
documentation matches the current deploy flow and Makefile targets.
In `@tools/loadgen/README.md`:
- Around line 168-184: The fenced code blocks that show the make commands for
running and tearing down the loadgen (the blocks containing "make -C
tools/loadgen/deploy up", "make -C tools/loadgen/deploy seed-roomread
PRESET=medium", "make -C tools/loadgen/deploy run-max-rps WORKLOAD=room-read
PRESET=medium", the block showing the STEPS override, and the teardown block)
need a language identifier; update each triple-backtick fence to use bash (e.g.,
```bash) so the command snippets are correctly marked as shell commands.
In `@tools/loadgen/roomread_generator.go`:
- Around line 87-96: The MaxInFlight<=0 branch currently calls g.requestOne(ctx)
inline on each tick (using g.cfg.MaxInFlight and tick.C), which serializes
requests and is therefore single in-flight rather than “unbounded”; to fix it,
change that branch to spawn a goroutine per tick (e.g. on tick.C do go func() {
g.requestOne(ctx) }()) so requests run concurrently without the semaphore, and
keep the existing ctx.Done() select so the loop still exits on cancellation;
update any comment that claims “unbounded” if present to reflect the new
behavior.
---
Outside diff comments:
In `@tools/loadgen/README.md`:
- Around line 289-291: Update the README table row describing `--workload` and
`--steps` to include the new `room-read` workload and its corresponding default
`--steps` values: add `room-read` to the `--workload` cell (so it reads
`messages`, `history` or `room-read`) and add the appropriate default steps for
`room-read` in the `--steps` cell (matching how `messages` and `history` are
documented, e.g., list the ordered RPS values or `k` suffix rules for
`room-read`); ensure `--preset` remains documented as required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd9b3f11-8f25-44fd-9068-6d41711d3ec4
📒 Files selected for processing (15)
docs/superpowers/plans/2026-06-02-loadgen-room-read-maxrps.mddocs/superpowers/specs/2026-06-02-loadgen-room-read-maxrps-design.mdtools/loadgen/README.mdtools/loadgen/deploy/Makefiletools/loadgen/main.gotools/loadgen/maxrps.gotools/loadgen/maxrps_roomread.gotools/loadgen/maxrps_roomread_test.gotools/loadgen/roomread_collector.gotools/loadgen/roomread_collector_test.gotools/loadgen/roomread_generator.gotools/loadgen/roomread_generator_test.gotools/loadgen/roomread_integration_test.gotools/loadgen/roomread_seed.gotools/loadgen/roomread_seed_test.go
|
|
||
| --- | ||
|
|
||
| ### Task 1: Read-state backfill fixtures |
There was a problem hiding this comment.
Fix heading level jump to satisfy markdown linting.
### Task 1 jumps a level after the preceding structure; adjust heading hierarchy (or insert the missing intermediate level) to avoid MD001 violations.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 33-33: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-06-02-loadgen-room-read-maxrps.md` at line 33,
The heading "### Task 1: Read-state backfill fixtures" jumps a level and
triggers MD001; update that heading to the correct intermediate level to match
the surrounding document hierarchy (e.g., change it to "#### Task 1: Read-state
backfill fixtures" or "## Task 1: Read-state backfill fixtures" as appropriate)
so the markdown heading levels progress sequentially and satisfy linting.
| ``` | ||
|
|
||
| > The `seed` arg is accepted for signature symmetry with the other workloads and to keep the call sites uniform; teardown drops collections wholesale so it does not need to rebuild fixtures. Keep the parameter (the switch passes `*seed`). | ||
|
|
There was a problem hiding this comment.
Remove blank line inside the blockquote continuation.
There’s a blank line within the > note block that triggers MD028; keep the blockquote contiguous.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 1000-1000: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-06-02-loadgen-room-read-maxrps.md` at line 1000,
The markdown contains a blank line inside a blockquote continuation which
triggers MD028; locate the multi-line note block (the lines starting with ">"
that form the note) and remove the empty line between the quoted lines so the
blockquote is contiguous, or if the blank line is intentional, prefix it with
">" to keep it as part of the blockquote; ensure the entire note block remains a
continuous sequence of lines beginning with ">" to satisfy MD028.
| ``` | ||
| make -C tools/loadgen/deploy up | ||
| make -C tools/loadgen/deploy seed-roomread PRESET=medium | ||
| make -C tools/loadgen/deploy run-max-rps WORKLOAD=room-read PRESET=medium | ||
| ``` | ||
|
|
||
| Override the ramp with `STEPS` (default `200,500,1000,2000,5000`): | ||
|
|
||
| ``` | ||
| make -C tools/loadgen/deploy run-max-rps WORKLOAD=room-read PRESET=medium STEPS=500,1k,2k,5k | ||
| ``` | ||
|
|
||
| Tear down the fixtures: | ||
|
|
||
| ``` | ||
| make -C tools/loadgen/deploy teardown-roomread PRESET=medium | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks.
The new room-read command snippets are missing fence languages (MD040). Use ```bash for these command blocks.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 168-168: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 176-176: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 182-182: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/loadgen/README.md` around lines 168 - 184, The fenced code blocks that
show the make commands for running and tearing down the loadgen (the blocks
containing "make -C tools/loadgen/deploy up", "make -C tools/loadgen/deploy
seed-roomread PRESET=medium", "make -C tools/loadgen/deploy run-max-rps
WORKLOAD=room-read PRESET=medium", the block showing the STEPS override, and the
teardown block) need a language identifier; update each triple-backtick fence to
use bash (e.g., ```bash) so the command snippets are correctly marked as shell
commands.
| if g.cfg.MaxInFlight <= 0 { | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil | ||
| case <-tick.C: | ||
| g.requestOne(ctx) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find MaxInFlight config default / env binding and how it's set for room-read
rg -nC3 'MaxInFlight'Repository: hmchangw/chat
Length of output: 32180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print requestOne for roomread generator
rg -n --context 2 "func \\(g \\*.*\\) requestOne" tools/loadgen/roomread_generator.go
# Print roomread_generator.go around the MaxInFlight<=0 branch
sed -n '60,120p' tools/loadgen/roomread_generator.go
# Inspect maxrps_roomread wiring for MaxInFlight
sed -n '1,140p' tools/loadgen/maxrps_roomread.go
# Find where room-read generator is constructed with MaxInFlight (CLI/options)
rg -n --context 3 "roomread.*MaxInFlight|MaxInFlight.*roomread|maxrps_roomread" tools/loadgen | head -n 80Repository: hmchangw/chat
Length of output: 6289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print requestOne for roomread generator
rg -n --context 2 "func \\(g \\*.*\\) requestOne" tools/loadgen/roomread_generator.go
# Print roomread_generator.go around the MaxInFlight<=0 branch
sed -n '60,140p' tools/loadgen/roomread_generator.go
# Inspect maxrps_roomread wiring for MaxInFlight
sed -n '1,180p' tools/loadgen/maxrps_roomread.go
# Find where room-read generator is constructed with MaxInFlight (CLI/options)
rg -n --context 3 "roomread.*MaxInFlight|MaxInFlight.*roomread|maxrps_roomread" tools/loadgen | head -n 120Repository: hmchangw/chat
Length of output: 6615
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print requestOne for roomread generator
rg -n --context 2 "func \\(g \\*.*\\) requestOne" tools/loadgen/roomread_generator.go
# Print roomread_generator.go around the MaxInFlight<=0 branch
sed -n '60,140p' tools/loadgen/roomread_generator.go
# Inspect maxrps_roomread wiring for MaxInFlight
sed -n '1,180p' tools/loadgen/maxrps_roomread.go
# Find where room-read generator is constructed with MaxInFlight (CLI/options)
rg -n --context 3 "roomread.*MaxInFlight|MaxInFlight.*roomread|maxrps_roomread" tools/loadgen | head -n 120Repository: hmchangw/chat
Length of output: 6615
Fix the claim about “unbounded in-flight” in MaxInFlight <= 0 path for room-read.
tools/loadgen/roomread_generator.go calls g.requestOne(ctx) inline on each tick.C when g.cfg.MaxInFlight <= 0 (no semaphore, no goroutine), so this branch is effectively single in-flight rather than “unbounded”. The achievable RPS is therefore limited by request latency in this mode (and any max-RPS logic assuming concurrency would be invalid). Room-read workload wiring passes MaxInFlight: w.cfg.MaxInFlight from the shared config, so this only becomes an issue if real runs allow MAX_IN_FLIGHT <= 0.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/loadgen/roomread_generator.go` around lines 87 - 96, The MaxInFlight<=0
branch currently calls g.requestOne(ctx) inline on each tick (using
g.cfg.MaxInFlight and tick.C), which serializes requests and is therefore single
in-flight rather than “unbounded”; to fix it, change that branch to spawn a
goroutine per tick (e.g. on tick.C do go func() { g.requestOne(ctx) }()) so
requests run concurrently without the semaphore, and keep the existing
ctx.Done() select so the loop still exits on cancellation; update any comment
that claims “unbounded” if present to reflect the new behavior.
Brainstormed design for a third max-rps workload that measures the sustainable RPS ceiling of marking a room as read (room-service message.read RPC), reusing the existing messages presets with a read-state backfill that keeps the floor-recompute path live.
hmchangw
force-pushed
the
claude/loadgen-room-read-maxrps-9eeLu
branch
from
f671725 to
3516ae6
Compare
Mint a fresh X-Request-ID per message.read request in the room-read generator (like a real client) and have the shared NATS requester carry it on the message header via natsutil.NewMsg. The header is nil when ctx has no request ID, so the history workload is unaffected. Room-service already propagates the inbound header onward (wrappedCtx + NewMsg), so benchmark traffic is now correlatable end-to-end in logs and traces.
3 participants
Summary
Adds a third
room-readworkload to thetools/loadgenmax-rpsbenchmark that measures the maximum sustainable RPS for marking a room as read (room-service'smessage.readrequest/reply RPC), under a realistic read pattern.BuildRoomReadFixturesstamps every room'slastMsgAtto a timestamp ahead of the whole ramp window, spreads members'lastSeenAtbehind it, and sets each room'sminUserLastSeenAtfloor. Because a read stampslastSeenAt = now(the present, still before the futurelastMsgAt), the reader never "catches up", so the room read-floor recompute path (MinSubscriptionLastSeenByRoomIDscan + conditionalUpdateRoomMinUserLastSeenAtwrite) stays live on every request — the realistic steady state.rpsWorkloadinterface, inheriting the ramp engine, SLO gating, verdict, report, and CSV output. It's synchronous request/reply, so it's gated on p95/p99 latency + error rate only (no consumer-pending signal). Defaults match thehistoryworkload (--slo-p95=100ms,--slo-p99=250ms,--slo-error-rate=0.001; steps200,500,1000,2000,5000).seed/teardown/max-rps --workload=room-read, plus deploy Makefile targets (seed-roomread,teardown-roomread) and a README section.New files:
roomread_seed.go,roomread_collector.go,roomread_generator.go,maxrps_roomread.go,roomread_integration_test.go(+ unit tests for each). Edits wire it intomaxrps.go/main.goand adddeploy/Makefiletargets + README docs. Nodocs/client-api.mdchange (themessage.readschema is unchanged) and no new dependencies.Built spec-first (design + plan under
docs/superpowers/), implemented task-by-task with per-task spec + code-quality review and a final holistic review.Test plan
make test SERVICE=tools/loadgen— unit tests pass with-racemake lint— clean (0 issues)roomread_integration_test.go) compiles under-tags integration(runtime requires Docker/testcontainers: real Mongo seed + stubmessage.readresponder over NATS)make -C tools/loadgen/deploy up && make -C tools/loadgen/deploy seed-roomread PRESET=medium && make -C tools/loadgen/deploy run-max-rps WORKLOAD=room-read PRESET=mediumhttps://claude.ai/code/session_015e29swZ1GqH9qX7B9G6dh8
Generated by Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation