Repository navigation
Conversation
Add DB_MAX_OPEN_CONNS and DB_MAX_IDLE_CONNS env vars to config.Config so operators can tune the Postgres connection pool at runtime without rebuilding. db.Open gains two new parameters; passing 0 preserves the prior hardcoded defaults (10 open / 2 idle for Postgres, 1 for SQLite). All callers updated. Add test/load/main.go: a standalone stdlib-only load-test binary that drives realistic MCP tool-call traffic (70/25/5 list_dialogs/get_messages/send dry-run) against a staging deployment, produces per-tool p50/p95/p99 latency tables, scrapes /metrics every 5s for peak pool/RSS/goroutine data, and writes a JSON results file. Build with: go build ./test/load/ Tests: config_test.go covers DB_MAX_* env loading; db_test.go covers Open pool parameter behaviour on SQLite and (when TEST_DATABASE_URL is set) Postgres. Deferred (require live staging runs): docs/load-test-beta.md, deploy/profiles/beta.env, docs/hpa.md 2 GiB tier row (tasks 4-8). Proposal: platform-gitops/agents-state/mctl-telegram/proposals/issue-90-beta-capacity-profile-load-test-tuned-co/ Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Claude finished @mashkovd's task in 3m 16s —— View job Code Review
Summary1 P1 · 2 P2 · 1 P3 — changes requested The
The P1 and first P2 are functional correctness issues — the send-sequence path as written is either dangerous (if |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // No "mode" field: server defaults to draft. No message is delivered. | ||
| id2 := int(counter.Add(1)) | ||
| _, lat2, err2 := callTool(ctx, client, target, token, id2, toolSendMessage, map[string]any{ | ||
| "peer": peer, | ||
| "text": "load test dry run", | ||
| "confirmation_id": confirmID, | ||
| }) |
There was a problem hiding this comment.
P1 — real messages may be delivered in staging
The comment "server defaults to draft. No message is delivered." relies entirely on undocumented server-side behavior and is not enforced by the client. If the staging deployment has ALLOW_SEND=true (which is expected for testing the send path), calling send_message with a valid confirmation_id will deliver a real Telegram message to the -peer target. With 100 VUs over 10 min at 5% weight, this is potentially hundreds of real messages.
There is no dry-run parameter in the MCP send_message tool today. Either:
- Drop the
send_messagestep entirely and only measureprepare_send_messagelatency, or - Guard this block behind an explicit
-allow-sendflag that defaults tofalseand documents the risk.
| func extractSSEData(body []byte) []byte { | ||
| for _, line := range strings.Split(string(body), "\n") { | ||
| line = strings.TrimSpace(line) | ||
| if strings.HasPrefix(line, "data:") { | ||
| return []byte(strings.TrimSpace(strings.TrimPrefix(line, "data:"))) | ||
| } | ||
| } | ||
| return body | ||
| } |
There was a problem hiding this comment.
P2 — SSE extraction only returns the first data: line
MCP over SSE sends multiple events: intermediate progress or keepalive frames followed by the final result. This implementation returns on the first data: line encountered. If prepare_send_message emits a progress event before the final result, extractConfirmationID will fail (no confirmation_id in a progress frame), the send sequence silently short-circuits, and send_message is never measured.
The fix is to scan all data: lines and return the last non-empty one (the final result in standard MCP SSE):
func extractSSEData(body []byte) []byte {
var last []byte
for _, line := range strings.Split(string(body), "\n") {
line = strings.TrimSpace(line)
if strings.HasPrefix(line, "data:") {
if payload := []byte(strings.TrimSpace(strings.TrimPrefix(line, "data:"))); len(payload) > 0 {
last = payload
}
}
}
if last != nil {
return last
}
return body
}
| // Start metrics poller during the hold phase. | ||
| pollerCtx, pollerCancel := context.WithCancel(context.Background()) | ||
| go pollMetrics(pollerCtx, client, *flagTarget, pm) |
There was a problem hiding this comment.
P2 — metrics poller starts after the full ramp completes, missing peak data
The ramp loop on lines 573–583 runs synchronously in main and takes the full -ramp duration (default 2 min) before reaching this line. The metrics poller therefore never scrapes during ramp-up, which is exactly when goroutine count and connection pool size typically peak.
Move the go pollMetrics(...) call to before the ramp loop (after the client/s/pm setup, before for i := 0; i < numUsers; i++).
| // DBMaxOpenConns caps the Postgres connection pool. 0 means use the | ||
| // driver default (10). Set via DB_MAX_OPEN_CONNS. | ||
| DBMaxOpenConns int | ||
| // DBMaxIdleConns sets the Postgres idle connection count. 0 means use | ||
| // the driver default (2). Set via DB_MAX_IDLE_CONNS. | ||
| DBMaxIdleConns int |
There was a problem hiding this comment.
P3 — "driver default" is misleading
database/sql's actual defaults are unlimited open connections and 2 idle. 10 and 2 are the prior hardcoded values in db.Open, not the driver defaults. The sentinel comment should read "use the prior default (10)" / "use the prior default (2)" to avoid confusion when the driver's own defaults change.
| // DBMaxOpenConns caps the Postgres connection pool. 0 means use the | |
| // driver default (10). Set via DB_MAX_OPEN_CONNS. | |
| DBMaxOpenConns int | |
| // DBMaxIdleConns sets the Postgres idle connection count. 0 means use | |
| // the driver default (2). Set via DB_MAX_IDLE_CONNS. | |
| DBMaxIdleConns int | |
| // DBMaxOpenConns caps the Postgres connection pool. 0 means keep the | |
| // prior default of 10. Set via DB_MAX_OPEN_CONNS. | |
| DBMaxOpenConns int | |
| // DBMaxIdleConns sets the Postgres idle connection count. 0 means keep | |
| // the prior default of 2. Set via DB_MAX_IDLE_CONNS. | |
| DBMaxIdleConns int |
The db.Open signature gained maxOpenConns/maxIdleConns; two test helpers still called the 2-arg form, breaking 'go test ./internal/db/...' compilation. Pass 0, 0 (driver defaults), matching db_test.go.
|
Claude finished @mashkovd's task in 2m 58s —— View job Re-review (synchronize — commit
|
| Severity | File | Finding | Status |
|---|---|---|---|
| P1 | test/load/main.go:295 |
send_message delivers real messages when ALLOW_SEND=true; no -allow-send guard |
still open |
| P2 | test/load/main.go:247 |
extractSSEData returns first data: line; final SSE result is in the last line |
still open |
| P2 | test/load/main.go:588 |
Metrics poller starts after ramp loop; ramp-phase peak data never captured | still open |
| P3 | internal/config/config.go:61 |
Comment says "driver default (10/2)"; those are prior hardcoded values, not database/sql defaults |
still open |
No new issues found in the test/load/main.go or internal/db/db.go changes. The db.Open pool-knob logic and config parsing are clean.
|
@claude review This implements #90 tasks 1-3 only (DB pool config knobs, db.Open pool params, and the test/load/ binary). Tasks 4-8 (live load-test runs + docs/load-test-beta.md + deploy/profiles/beta.env + hpa.md capacity table) are intentionally deferred — they need measured numbers from real staging load tests and must not be fabricated. Note: I pushed a fixup (3da9e64) because the implementer missed updating two db.Open() callers (store_migration_test.go, store_test.go) which broke 'go test ./internal/db/...' compilation. Please focus on: the db.Open pool-param wiring + caller correctness, the config sentinel (0 = driver default), and the test/load/ binary's PromQL/Prometheus parsing and graceful handling of unreachable targets. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef47b02d98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, target+"/mcp", bytes.NewReader(body)) | ||
| if err != nil { | ||
| return nil, 0, fmt.Errorf("new request: %w", err) | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") |
There was a problem hiding this comment.
This load client sends tools/call requests directly but never performs the Streamable HTTP initialize step or forwards an Mcp-Session-Id on subsequent calls, even though this repo’s own probe path treats initialization as required (cmd/canary/main.go, step 2). Against the deployed MCP transport, this causes requests to be rejected as invalid/uninitialized sessions, so the run can degrade into mostly errors and produce unusable capacity numbers.
Useful? React with 👍 / 👎.
| case "mctl_telegram_flood_wait_events_total": | ||
| if tool := labels["tool"]; tool != "" { | ||
| if value > pm.floodWaitByTool[tool] { | ||
| pm.floodWaitByTool[tool] = value |
There was a problem hiding this comment.
mctl_telegram_flood_wait_events_total is a cumulative Prometheus counter (defined as such in internal/metrics/metrics.go), but this code records the maximum absolute value and reports it as run-local “Events”. If the server already had flood-wait history before the test, the output overstates this run’s incidents and makes comparisons between runs invalid; capture a baseline at start and report end-minus-start per tool.
Useful? React with 👍 / 👎.
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return | ||
| } | ||
| defer resp.Body.Close() | ||
| body, err := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
The metrics scraper ignores HTTP status codes and keeps parsing whatever body is returned, so a protected endpoint (for example when METRICS_ALLOW_CIDR rejects the caller with 403 in cmd/server/main.go) silently produces zero/partial peaks instead of signaling invalid telemetry. That can make the load report look healthy while server-side peak metrics were never actually collected.
Useful? React with 👍 / 👎.
| if rpcResp.Error != nil { | ||
| return nil, latency, fmt.Errorf("rpc error: %s", rpcResp.Error.Message) | ||
| } | ||
| return rpcResp.Result, latency, nil |
There was a problem hiding this comment.
This client returns success whenever JSON-RPC error is nil, but MCP tool failures are commonly returned inside result with isError=true (this repo’s tools use toolErr(...) for many failure paths). As written, denied/invalid tool executions are recorded as successful calls, which materially underreports error rates and can invalidate the load-test conclusions.
Useful? React with 👍 / 👎.
| "text": "load test dry run", | ||
| "confirmation_id": confirmID, | ||
| }) | ||
| s.stats[toolSendMessage].record(callResult{latency: lat2, isErr: err2 != nil}) |
There was a problem hiding this comment.
P1 (unaddressed) — send_message will deliver real Telegram messages when ALLOW_SEND=true
The comment "server defaults to draft. No message is delivered." relies on undocumented server-side behavior that is explicitly overridden by ALLOW_SEND=true, which staging deployments use for end-to-end send testing. With 100 VUs over 10 min at 5% weight, this is potentially hundreds of real messages sent.
Previous review requested an -allow-send flag defaulting to false. That change has not been made. Either:
- Drop the
send_messagestep and only benchmarkprepare_send_message, or - Add
-allow-send boolflag defaulting tofalseand gate the call behind it.
Fix this →%20flag.%20In%20runVirtualUser%2C%20gate%20the%20send_message%20call%20at%20line%20289-295%20behind%20an%20allowSend%20parameter%20passed%20in%20from%20main.%20Default%20is%20false%20so%20no%20real%20messages%20are%20sent%20unless%20explicitly%20opted%20in.%20Branch%3A%20feat%2Fagents-issue-90-beta-capacity-profile-load-test-tuned-co&repo=mctlhq%2Fmctl-telegram)
| } | ||
| } | ||
| return body | ||
| } |
There was a problem hiding this comment.
P2 (unaddressed) — extractSSEData returns the first data: line, not the last
MCP over SSE emits progress/keepalive frames before the final result event. Returning on the first data: line means extractConfirmationID will parse a progress frame (no confirmation_id), silently short-circuit, and send_message is never exercised. The fix is to scan all lines and return the last non-empty payload:
| } | |
| func extractSSEData(body []byte) []byte { | |
| var last []byte | |
| for _, line := range strings.Split(string(body), "\n") { | |
| line = strings.TrimSpace(line) | |
| if strings.HasPrefix(line, "data:") { | |
| if payload := []byte(strings.TrimSpace(strings.TrimPrefix(line, "data:"))); len(payload) > 0 { | |
| last = payload | |
| } | |
| } | |
| } | |
| if last != nil { | |
| return last | |
| } | |
| return body | |
| } |
|
|
||
| // Start metrics poller during the hold phase. | ||
| pollerCtx, pollerCancel := context.WithCancel(context.Background()) | ||
| go pollMetrics(pollerCtx, client, *flagTarget, pm) |
There was a problem hiding this comment.
P2 (unaddressed) — metrics poller starts after the ramp completes, missing peak data
The ramp loop above (lines 573–583) is synchronous and takes the full -ramp duration (default 2 min) before reaching this line. The poller never scrapes during ramp-up, which is precisely when goroutine count and connection pool size peak. The comment was updated to say "during the hold phase" but that confirms rather than fixes the bug.
Move the pollerCtx setup and go pollMetrics(...) call to before the for i := 0; i < numUsers loop so ramp-phase peak metrics are captured.
| DBMaxOpenConns int | ||
| // DBMaxIdleConns sets the Postgres idle connection count. 0 means use | ||
| // the driver default (2). Set via DB_MAX_IDLE_CONNS. | ||
| DBMaxIdleConns int |
There was a problem hiding this comment.
P3 (unaddressed) — "driver default" is incorrect
database/sql's actual defaults are unlimited open connections (0) and 2 idle. 10 and 2 are the prior hardcoded values in db.Open, not driver defaults. The comment should say "prior default" to avoid confusing future readers:
| DBMaxIdleConns int | |
| // DBMaxOpenConns caps the Postgres connection pool. 0 means keep the | |
| // prior default of 10. Set via DB_MAX_OPEN_CONNS. | |
| DBMaxOpenConns int | |
| // DBMaxIdleConns sets the Postgres idle connection count. 0 means keep | |
| // the prior default of 2. Set via DB_MAX_IDLE_CONNS. | |
| DBMaxIdleConns int |
Address review findings on the load-test binary: - P1: gate send_message behind -allow-send (default false) so the load test never delivers real Telegram messages unless explicitly opted in - P2: extractSSEData returns the last non-empty data: payload (final MCP result), not the first (which may be a progress frame) - P2: start the metrics poller before the ramp loop so ramp-phase peaks (goroutines, pool size) are captured - P3: config comment says 'prior default' not 'driver default'
|
Claude finished @mashkovd's task in 3m 9s —— View job Re-review (synchronize — commit
|
| Severity | File | Finding |
|---|---|---|
| P2 | test/load/main.go:234 |
callTool doesn't check isError: true in the MCP result body; tool-level failures (FLOOD_WAIT, invalid peer, auth rejection) are counted as successes and their latencies contaminate percentiles |
| P2 | test/load/main.go:399 |
scrapeMetrics ignores non-200 HTTP status; a 403 from METRICS_ALLOW_CIDR silently leaves all peak metrics at zero with no operator warning |
| P2 | test/load/main.go:152 |
mctl_telegram_flood_wait_events_total is a cumulative counter; tracking the max reports the server-lifetime total, not the run-local delta — capture a baseline on first scrape |
No new issues in internal/db/db.go or internal/config/config.go.
| if rpcResp.Error != nil { | ||
| return nil, latency, fmt.Errorf("rpc error: %s", rpcResp.Error.Message) | ||
| } | ||
| return rpcResp.Result, latency, nil |
There was a problem hiding this comment.
P2 — MCP isError: true results counted as successful calls
callTool only checks the JSON-RPC-level error field. MCP tools return application-level failures as {"result": {"isError": true, "content": [...]}} with no JSON-RPC error — for example when Telegram returns FLOOD_WAIT, when the peer is invalid, or when auth.Provider rejects the call. As written, those are counted as successful with their latency in the percentile bucket, understating error rates and contaminating p50/p95/p99.
Add an isError check before returning:
| return rpcResp.Result, latency, nil | |
| if rpcResp.Error != nil { | |
| return nil, latency, fmt.Errorf("rpc error: %s", rpcResp.Error.Message) | |
| } | |
| var toolRes mcpToolResult | |
| if json.Unmarshal(rpcResp.Result, &toolRes) == nil && toolRes.IsError { | |
| return nil, latency, fmt.Errorf("tool error (isError=true)") | |
| } | |
| return rpcResp.Result, latency, nil |
| } | ||
| defer resp.Body.Close() | ||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { |
There was a problem hiding this comment.
P2 — non-200 responses silently produce zero peak metrics
If /metrics is guarded by METRICS_ALLOW_CIDR and the load-test host is outside that range, the server returns 403. parsePrometheusLine returns ok=false for every HTML error-body line, so all peak values stay at zero and the report looks healthy while server-side metrics were never collected. There is no warning to the operator.
| if err != nil { | |
| defer resp.Body.Close() | |
| if resp.StatusCode != http.StatusOK { | |
| fmt.Fprintf(os.Stderr, "warning: /metrics returned HTTP %d; peak metrics will be incomplete\n", resp.StatusCode) | |
| return | |
| } |
| case "mctl_telegram_flood_wait_events_total": | ||
| if tool := labels["tool"]; tool != "" { | ||
| if value > pm.floodWaitByTool[tool] { | ||
| pm.floodWaitByTool[tool] = value |
There was a problem hiding this comment.
P2 — flood-wait counter reports server-lifetime total, not run-local delta
mctl_telegram_flood_wait_events_total is a monotonic Prometheus counter (defined as prometheus.CounterVec in internal/metrics/metrics.go). Tracking the running maximum reports the cumulative total since the server started, not the events triggered by this specific load test run. If the server already handled 1 000 flood-wait events before the test, the report will show 1 000+ regardless of what the test caused.
Capture a baseline on the first scrape and subtract it in buildReport. The simplest approach: add a baseline map[string]float64 to peakMetrics that is populated on the first non-zero scrape, then subtract in buildReport when producing FloodWaits.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40f68504e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if i > 0 && rampInterval > 0 { | ||
| time.Sleep(rampInterval) | ||
| } | ||
| token := tokens[i%len(tokens)] |
There was a problem hiding this comment.
This line reuses credentials with i%len(tokens) whenever -users exceeds the token file size, so multiple goroutines run under the same identity. Because the server enforces per-identity request limits (internal/audit/ratelimit.go middleware), the load test will produce synthetic 429/error rates and lower throughput that reflect credential reuse, not system capacity; fail fast (or at least warn) when users outnumber distinct tokens.
Useful? React with 👍 / 👎.
| return nil, 0, fmt.Errorf("marshal: %w", err) | ||
| } | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, target+"/mcp", bytes.NewReader(body)) |
There was a problem hiding this comment.
The load client always posts to target + "/mcp", but the server mounts MCP on the configurable cfg.MCPPath (cmd/server/main.go), so any deployment using a non-default path will fail every tool call despite a healthy service. This makes the benchmark unusable outside the default routing and should be configurable (or discovered) in the load test binary.
Useful? React with 👍 / 👎.
| return nil, 0, fmt.Errorf("marshal: %w", err) | ||
| } | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, target+"/mcp", bytes.NewReader(body)) |
There was a problem hiding this comment.
Concatenating target + "/mcp" without trimming can produce //mcp when operators pass a base URL ending in / (for example, https://host/), and this route is not mounted by the server router. In that case the load test reports universal tool-call failures caused by URL construction, not service behavior, so the target should be normalized (or joined with URL APIs) before request creation.
Useful? React with 👍 / 👎.
| "peer": peer, | ||
| "text": "load test dry run", | ||
| "confirmation_id": confirmID, |
There was a problem hiding this comment.
In the -allow-send branch this request includes confirmation_id but omits mode, and the server defaults send_message to mode="draft" (internal/mcp/tools.go), so the run never exercises real Telegram sends even when ALLOW_SEND is enabled. That underloads the destructive path and produces misleading capacity numbers for the intended send test scenario.
Useful? React with 👍 / 👎.
… flood-wait Round-3 review fixes: - callTool now treats MCP isError=true results as errors so FLOOD_WAIT / invalid-peer / auth failures don't inflate success counts or percentiles - scrapeMetrics warns and bails on non-200 (e.g. 403 from METRICS_ALLOW_CIDR) instead of silently reporting zero peaks - flood-wait peak is now reported as a run-local delta (peak minus first-scrape baseline) since the metric is a monotonic counter
|
Claude finished @mashkovd's task in 3m 12s —— View job Re-review (synchronize — commit
|
| Severity | File | Finding | Status |
|---|---|---|---|
| P1 | test/load/main.go:211 |
Missing MCP initialize handshake + Mcp-Session-Id header — server rejects tools/call without a valid session; cmd/canary/main.go:225 explicitly documents this requirement |
new |
| P2 | test/load/main.go:626 |
Token reuse when -users > len(tokens) silently applies per-user rate limits to multiple goroutines, producing artificial errors |
new |
| P3 | test/load/main.go:327 |
-allow-send sends no real message because mode defaults to "draft"; flag description implies real sends |
new |
The db.Open pool-knob wiring, config sentinel, and all test call-site updates are clean.
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
| req.Header.Set("Accept", "application/json, text/event-stream") | ||
| req.Header.Set("Authorization", "Bearer "+token) |
There was a problem hiding this comment.
P1 — missing MCP initialize handshake and Mcp-Session-Id header
callTool sends tools/call directly with no prior initialize call and no Mcp-Session-Id header. cmd/canary/main.go (line 225) explicitly documents: "The MCP Streamable HTTP transport requires session initialization before any tools/call request" — it calls initMCPSession first, then passes the returned Mcp-Session-Id on every subsequent request (canary line 327).
Without a valid session, the server is expected to reject every tools/call, producing ~100% JSON-RPC errors and no usable capacity data.
Fix: add a per-virtual-user initialize call (analogous to initMCPSession in canary) at the start of runVirtualUser, and set Mcp-Session-Id on every callTool request. Each goroutine should obtain its own session.
| if i > 0 && rampInterval > 0 { | ||
| time.Sleep(rampInterval) | ||
| } | ||
| token := tokens[i%len(tokens)] |
There was a problem hiding this comment.
P2 — silent token reuse when -users exceeds token count
tokens[i%len(tokens)] silently reuses identities when numUsers > len(tokens). Multiple goroutines sharing the same bearer token run under the same user identity. This matters because internal/audit/ratelimit.go enforces per-identity request limits: the goroutines collectively exhaust the per-user quota, producing artificial 429-equivalent errors and lower throughput that reflects credential collision, not system capacity.
Either fail fast or emit a prominent warning:
| token := tokens[i%len(tokens)] | |
| if i >= len(tokens) { | |
| fmt.Fprintf(os.Stderr, "warning: only %d tokens available for %d users; tokens will be reused — results may reflect per-user rate-limit collisions\n", len(tokens), numUsers) | |
| } | |
| token := tokens[i%len(tokens)] |
| "text": "load test dry run", | ||
| "confirmation_id": confirmID, | ||
| }) | ||
| s.stats[toolSendMessage].record(callResult{latency: lat2, isErr: err2 != nil}) |
There was a problem hiding this comment.
P3 — -allow-send never sends a real message (missing mode="send")
The -allow-send flag description says it "delivers REAL messages when the server runs with ALLOW_SEND=true", but no mode argument is passed. The server's send_message tool defaults to mode="draft" (internal/mcp/tools.go), so no actual Telegram delivery happens even when opted in. The flag's intent and its behavior are inconsistent.
Either document that draft mode is intentional for load testing (update the flag description), or add "mode": "send" to the arguments when allowSend is true:
| s.stats[toolSendMessage].record(callResult{latency: lat2, isErr: err2 != nil}) | |
| _, lat2, err2 := callTool(ctx, client, target, token, id2, toolSendMessage, map[string]any{ | |
| "peer": peer, | |
| "text": "load test dry run", | |
| "confirmation_id": confirmID, | |
| "mode": "send", | |
| }) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 060112dbed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case <-ctx.Done(): | ||
| return | ||
| case <-ticker.C: | ||
| scrapeMetrics(ctx, client, target, pm) |
There was a problem hiding this comment.
buildReport computes flood-wait events as peak - first_scrape_baseline, but pollMetrics does not scrape until the first 5s tick. That means any mctl_telegram_flood_wait_events_total increments that happen during startup/ramp before the first tick are absorbed into the baseline and never counted for this run; very short runs can report zero events even when floods occurred. Trigger one immediate scrapeMetrics call before entering the ticker loop so the baseline is captured at test start.
Useful? React with 👍 / 👎.
| pollerCancel() | ||
| wg.Wait() |
There was a problem hiding this comment.
The report is generated after canceling the metrics poller, so any peak/flood-wait changes in the final polling interval are dropped. In practice, if a spike happens between the last 5s tick and test end (or while virtual users are draining after cancellation), Peak* and flood-wait values are underreported. Keep polling until after workers exit and/or do one last synchronous scrapeMetrics before buildReport.
Useful? React with 👍 / 👎.
Round-4 review fixes: - P1: perform the MCP initialize handshake per virtual user and send the Mcp-Session-Id on every tools/call (Streamable HTTP rejects calls without it) — same requirement the canary documents - P2: warn when -users exceeds the token count, since reused tokens share per-user rate limits and skew results - P3: pass mode=draft explicitly on send_message and reword -allow-send so it no longer implies real message delivery
|
Claude finished @mashkovd's task in 2m 27s —— View job Re-review (synchronize — commit
|
| Status | File | Finding |
|---|---|---|
| FIXED | test/load/main.go |
P1: Missing MCP initialize handshake |
| FIXED | test/load/main.go |
P2: Silent token reuse when -users > token count |
| FIXED | test/load/main.go |
P3: -allow-send flag description mismatched behaviour |
| FIXED | internal/config/config.go |
P3: "driver default" comment |
Two new P3 observations (neither blocks merge):
| Severity | File | Finding |
|---|---|---|
| P3 | test/load/main.go:691 |
pollMetrics first tick fires 5 s after start; flood-wait events in the first 5 s of ramp are excluded from the run-local delta |
| P3 | test/load/main.go:711 |
Poller cancelled before wg.Wait(); peak metrics during the VU drain window are not captured |
| // Start the metrics poller before the ramp loop so ramp-phase peaks | ||
| // (goroutine count, pool size) are captured, not just the hold phase. | ||
| pollerCtx, pollerCancel := context.WithCancel(context.Background()) | ||
| go pollMetrics(pollerCtx, client, *flagTarget, pm) |
There was a problem hiding this comment.
P3 — baseline captured 5 s late; first-tick flood-wait events are under-counted
pollMetrics waits for the first ticker fire (5 s) before scraping. The floodWaitBaseline is therefore set to the server-lifetime total plus any events that occur in the first 5 s of the ramp — those events are silently excluded from the report delta.
For a 10-min hold this is usually negligible, but if you want a precise t=0 baseline, one synchronous scrape before starting the goroutine is enough:
| go pollMetrics(pollerCtx, client, *flagTarget, pm) | |
| // Take a baseline scrape at t=0 so any flood-wait events during the | |
| // ramp phase are included in the run-local delta, not absorbed into | |
| // the baseline. | |
| scrapeMetrics(context.Background(), client, *flagTarget, pm) | |
| go pollMetrics(pollerCtx, client, *flagTarget, pm) |
| time.Sleep(*flagHold) | ||
|
|
||
| holdCancel() | ||
| pollerCancel() |
There was a problem hiding this comment.
P3 — poller cancelled before VUs drain; events during drain window are lost
pollerCancel() fires before wg.Wait(), so any flood-wait or peak-goroutine changes while VUs finish their current in-flight call are not captured. A final synchronous scrape after wg.Wait() closes the gap:
| pollerCancel() | |
| holdCancel() | |
| pollerCancel() | |
| wg.Wait() | |
| // Final scrape after all VUs have exited to capture any metrics that | |
| // changed during the drain window. | |
| scrapeMetrics(context.Background(), client, *flagTarget, pm) | |
| fmt.Println("hold phase complete; generating report") |
Implements accepted proposal
mctl-telegram/issue-90-beta-capacity-profile-load-test-tuned-co.Spec: https://github.com/mctlhq/mctl-gitops/tree/main/platform-gitops/agents-state/mctl-telegram/proposals/issue-90-beta-capacity-profile-load-test-tuned-co/
Generated by mctl-agents Tier 2 implementer. Review carefully — this PR was opened automatically and the spec may have gaps.
Closes #90