fix(k6): record metrics inside callbacks to handle interrupted sessions #2142
Conversation
The k6 WebSocket test was showing all zeros for most metrics because:
1. wsConnectionSuccess was recorded after ws.connect() returns, which only
happens after the socket closes
2. When sessions are interrupted (test ends before SESSION_DURATION_MS),
the code after ws.connect() never runs
Changes:
- Move wsConnectionSuccess.add(1) inside socket.on('open') callback
- Record failure (wsConnectionSuccess.add(0)) in error handler and fallback
- Fix vus_max to use metrics.vus_max or fallback to VUS constant
Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
❌ Deploy Preview for hyprnote-storybook failed.
|
❌ Deploy Preview for hyprnote failed.
|
📝 WalkthroughWalkthroughThe k6 load test script adds explicit WebSocket connection state tracking and new message-related metrics (sent, received, audio_chunks_sent), refines success/failure recording for connection attempts, and updates summary reporting (vus_max source and text summary output). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 1
🧹 Nitpick comments (2)
apps/k6/scripts/stt-live.js (2)
133-144: Confirm thatconnectionOpenedis the right success criterion for reconnects
runSessionnow returnsconnectionOpened, so the default function only incrementswsReconnectsand sleeps longer when the WebSocket never reaches the"open"state:const success = runSession(data); if (!success) { wsReconnects.add(1); sleep(5); } else { sleep(1); }That means sessions that open successfully but then error/close early will not be treated as reconnect cases here (they’ll just start a new iteration after a short sleep).
If your intent is “reconnect aggressively only on connection-establishment failures”, this is fine. If you also want early-terminated sessions (open → immediate error/close) to count as reconnects, you may want a separate flag (e.g.,
sessionCompleted) or adjust the success condition / naming to make this explicit.
147-157: Prefer??over||forvus_maxfallback to avoid treating0as missingHere:
vus_max: data.metrics.vus_max?.values?.max || VUS,using
||will replace a legitimate0withVUS, which can misrepresent runs that reportvus_max = 0(e.g., very short or failed runs).Safer to only fall back when the metric is
null/undefined:- vus_max: data.metrics.vus_max?.values?.max || VUS, + vus_max: data.metrics.vus_max?.values?.max ?? VUS,This keeps edge-case runs accurately reflected while still defaulting when the metric is absent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/k6/scripts/stt-live.js(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
- GitHub Check: Devin
Track WebSocket message activity: - ws_messages_sent: total outgoing messages (audio + keepalive) - ws_messages_received: total incoming messages - ws_audio_chunks_sent: audio chunks specifically These metrics are recorded inside callbacks so they work even when sessions are interrupted before completing. Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/k6/scripts/stt-live.js (1)
63-71: Avoid double-countingws_connection_successfailures between the error handler and the fallbackAs written, a failed connection that triggers
socket.on("error")before"open"will:
- Add
wsConnectionSuccess.add(0)in the error handler when!connectionOpened, and- Add another
wsConnectionSuccess.add(0)in the finalif (!connectionOpened)block afterws.connectreturns.That records two failures for a single attempted session and will skew the
ws_connection_successrate and thresholds.You likely want exactly one success/failure event per
runSessionattempt. A simple way is to track whether the outcome has already been recorded:let firstTranscriptTime = null; let loopCount = 0; let connectionOpened = false; + let connectionResultRecorded = false; @@ socket.on("open", () => { connectionOpened = true; wsConnections.add(1); wsConnectionSuccess.add(1); + connectionResultRecorded = true; @@ socket.on("error", (e) => { if (e.error() !== "websocket: close sent") { wsErrors.add(1); - if (!connectionOpened) { - wsConnectionSuccess.add(0); - } + if (!connectionOpened && !connectionResultRecorded) { + wsConnectionSuccess.add(0); + connectionResultRecorded = true; + } console.log(`[VU ${__VU}] Error: ${e.error()}`); } }); @@ - if (!connectionOpened) { - wsConnectionSuccess.add(0); - } + if (!connectionResultRecorded) { + wsConnectionSuccess.add(0); + }This keeps the Rate metric aligned with “one outcome per attempted session” and satisfies the “no double-counting failures” checklist item from the PR description.
Also applies to: 112-120, 132-141
🧹 Nitpick comments (1)
apps/k6/scripts/stt-live.js (1)
20-22: Message metrics wiring is consistent; consider whether to includeCloseStreamsendsThe new message counters (
wsMessagesSent,wsMessagesReceived,wsAudioChunksSent) are correctly:
- Defined at Lines 20-22.
- Incremented on audio chunk sends and KeepAlive sends (Lines 79-81, 90).
- Incremented on every inbound message (Line 95).
- Surfaced in
handleSummaryandtextSummary(Lines 189-194, 286-290).This gives a coherent view of WS traffic volume.
One small design question: the
{ type: "CloseStream" }send at Line 127 is not counted inwsMessagesSent. If you intend “Messages: Sent” to mean all outbound messages, you might optionally incrementwsMessagesSentthere as well; if you meant to track only steady-state traffic (audio + keepalives), the current behavior is fine, but it could be worth a brief comment in code or PR description for clarity.Also applies to: 79-81, 90-90, 95-95, 189-194, 286-290
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/k6/scripts/stt-live.js(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Devin
🔇 Additional comments (1)
apps/k6/scripts/stt-live.js (1)
163-163:vus_maxfallback looks correct; minor note on|| VUSedge caseUsing
data.metrics.vus_max?.values?.max || VUSfixes the previous undefined access and aligns with k6’svus_maxmetric:vus_max: data.metrics.vus_max?.values?.max || VUS,This will correctly report the observed max VUs when present and fall back to the configured
VUSwhen that metric is missing. The only theoretical edge case is ifvalues.maxcould legitimately be0, in which case|| VUSwould substituteVUS; in practicevus_maxshould never be 0 in a meaningful run, so this is acceptable.
Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
Summary
The k6 WebSocket test was showing all zeros for most metrics because
wsConnectionSuccesswas recorded afterws.connect()returns, which only happens after the socket closes. When sessions are interrupted (test ends before the 5-minute SESSION_DURATION_MS), the code afterws.connect()never runs.From the GitHub Actions logs:
Changes:
wsConnectionSuccess.add(1)insidesocket.on("open")callback so success is recorded immediately when connection opensws.connect()returnsvus_maxto usedata.metrics.vus_maxwith fallback toVUSconstant (was showingundefined)Updates since last revision
Added message count metrics to track WebSocket activity:
ws_messages_sent: total outgoing messages (audio chunks + keepalive)ws_messages_received: total incoming messagesws_audio_chunks_sent: audio chunks specificallyThese are recorded inside callbacks so they work even when sessions are interrupted. Added to both JSON report and text summary output.
Review & Testing Checklist for Human
!connectionOpened, and there's also a fallback check afterws.connect()returns. If an error occurs before connection opens, both paths could execute. Consider if this is the intended behavior.wsConnectionDurationis still recorded insocket.on("close"), so it may still show 0 for interrupted sessions. This was not changed.on("open")and failure in error/fallback maintains correct rate calculationNotes