fix(deployment): reduce noisy websocket errors in Sentry#3050
fix(deployment): reduce noisy websocket errors in Sentry#3050
Conversation
…27J) Skip error reporting when the abort signal has fired (user navigated away from the logs tab) and downgrade remaining websocket errors to warning severity. Include close code and reason in the error message for better Sentry triage.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefined WebSocket and deployment log error handling: WebSocket errors now include a reason and standardized message; tests updated to match. Deployment log stream catch-handler now checks abort signals, conditions UI state updates, and reports errors with severity and an optional cause. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/deploy-web/src/components/deployments/DeploymentLogs.tsx`:
- Around line 137-145: The catch block for getLogsStream(...) leaves
isLoadingLogs true on non-abort failures causing the skeleton to hang; in the
catch handler (the block referencing abortController.signal.aborted and
errorHandler.reportError) set isLoadingLogs to false (or call the setter that
updates the loading state) before calling errorHandler.reportError so the UI
clears the loading state on any non-abort error; ensure you reference the same
abortController and do not change abort handling logic for the abort case.
🪄 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: ab3f6078-41ce-43e8-a0d1-f85a8215e34f
📒 Files selected for processing (3)
apps/deploy-web/src/components/deployments/DeploymentLogs.tsxapps/deploy-web/src/lib/websocket/createWebsocket.spec.tsapps/deploy-web/src/lib/websocket/createWebsocket.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3050 +/- ##
==========================================
- Coverage 59.71% 59.69% -0.02%
==========================================
Files 1034 1034
Lines 24273 24279 +6
Branches 6003 6007 +4
==========================================
Hits 14494 14494
- Misses 8528 8533 +5
- Partials 1251 1252 +1
🚀 New features to boost your workflow:
|
…e target Extract error handling from DeploymentLogs useEffect into a testable handleLogsStreamError function and add unit tests covering both the abort-skip and warning-report paths.
| setIsLoadingLogs: (v: boolean) => void, | ||
| setIsConnectionEstablished: (v: boolean) => void | ||
| ): void { | ||
| if (signal.aborted) return; |
There was a problem hiding this comment.
why don't you want to report errors if signal is aborted?
There was a problem hiding this comment.
I think this is what is causing the spam of this error no? Why do we want to report an aborted error?
There was a problem hiding this comment.
honestly, idk. i'm just asking. my concern is we could miss some important ones
…ws error message Inline handleLogsStreamError into the component per review feedback, use a stable "websocket error" message for Sentry grouping, and forward cause as extra context.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/deploy-web/src/components/deployments/DeploymentLogs.tsx`:
- Around line 138-147: The reportError call currently fires even for aborted
streams and sets severity inversely; change it so you skip reporting entirely
when abortController.signal.aborted is true, and for non-abort failures set
severity to "warning" (not "error"); update the error payload sent to
errorHandler.reportError (the call in this block) to include the error message
(e.g., error instanceof Error ? error.message : String(error)) as a property or
tag so grouping is stable, and keep using the existing tags { category:
"deployments", label: "followLogs" } while leaving setIsLoadingLogs and
setIsConnectionEstablished behavior unchanged.
🪄 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: f7ac9ae8-d042-4c8d-9a70-bdf7eb902dd9
📒 Files selected for processing (1)
apps/deploy-web/src/components/deployments/DeploymentLogs.tsx
| if (!abortController.signal.aborted) { | ||
| setIsLoadingLogs(false); | ||
| setIsConnectionEstablished(false); | ||
| } | ||
|
|
||
| errorHandler.reportError({ | ||
| error, | ||
| tags: { category: "deployments", label: "followLogs" } | ||
| severity: abortController.signal.aborted ? "warning" : "error", | ||
| tags: { category: "deployments", label: "followLogs" }, | ||
| cause: error instanceof Error ? (error as Error & { cause?: unknown }).cause : undefined |
There was a problem hiding this comment.
Skip reporting aborted streams and downgrade real stream failures to warning.
Line 143 still reports even when abortController.signal.aborted is true, and Line 145 sets non-abort failures to "error". This keeps expected navigation aborts noisy and inverts the intended severity policy. Also, Line 147 adds cause as extra context, but that alone won’t standardize grouping by message.
💡 Proposed fix
).catch(error => {
- if (!abortController.signal.aborted) {
- setIsLoadingLogs(false);
- setIsConnectionEstablished(false);
- }
-
- errorHandler.reportError({
- error,
- severity: abortController.signal.aborted ? "warning" : "error",
- tags: { category: "deployments", label: "followLogs" },
- cause: error instanceof Error ? (error as Error & { cause?: unknown }).cause : undefined
- });
+ if (abortController.signal.aborted) return;
+
+ setIsLoadingLogs(false);
+ setIsConnectionEstablished(false);
+
+ errorHandler.reportError({
+ error,
+ severity: "warning",
+ tags: { category: "deployments", label: "followLogs" }
+ });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!abortController.signal.aborted) { | |
| setIsLoadingLogs(false); | |
| setIsConnectionEstablished(false); | |
| } | |
| errorHandler.reportError({ | |
| error, | |
| tags: { category: "deployments", label: "followLogs" } | |
| severity: abortController.signal.aborted ? "warning" : "error", | |
| tags: { category: "deployments", label: "followLogs" }, | |
| cause: error instanceof Error ? (error as Error & { cause?: unknown }).cause : undefined | |
| ).catch(error => { | |
| if (abortController.signal.aborted) return; | |
| setIsLoadingLogs(false); | |
| setIsConnectionEstablished(false); | |
| errorHandler.reportError({ | |
| error, | |
| severity: "warning", | |
| tags: { category: "deployments", label: "followLogs" } | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/deploy-web/src/components/deployments/DeploymentLogs.tsx` around lines
138 - 147, The reportError call currently fires even for aborted streams and
sets severity inversely; change it so you skip reporting entirely when
abortController.signal.aborted is true, and for non-abort failures set severity
to "warning" (not "error"); update the error payload sent to
errorHandler.reportError (the call in this block) to include the error message
(e.g., error instanceof Error ? error.message : String(error)) as a property or
tag so grouping is stable, and keep using the existing tags { category:
"deployments", label: "followLogs" } while leaving setIsLoadingLogs and
setIsConnectionEstablished behavior unchanged.
There was a problem hiding this comment.
We intentionally kept reporting for aborted streams — the reviewer raised a concern about silently missing important errors. The current approach differentiates by severity: aborted streams get "warning", real failures get "error". This way nothing is silenced, but aborted noise is clearly separated in Sentry. What's your take on this tradeoff vs silencing aborts entirely?
There was a problem hiding this comment.
@baktun14, that's a fair and well-reasoned tradeoff. Here's the breakdown of each side:
Keep reporting at "warning" for aborts (current implementation):
- ✅ Full observability — no errors silently discarded
- ✅ Severity levels in Sentry can be filtered/alerted on independently
⚠️ Still generates Sentry events for every navigation-away (just at a lower level), so the volume reduction from the PR's stated goal (~595 occurrences) may be limited in practice — they shift fromerrortowarningrather than disappearing
Silence aborts entirely (my suggestion):
- ✅ Completely eliminates navigation-abort noise — cleaner signal for real failures
- ✅ Aligns directly with the PR's stated motivation (reduce noisy Sentry errors from
followLogs) ⚠️ Loses visibility into abort-coincident failures — though sinceabortController.abort()is only ever called from theuseEffectcleanup (i.e., a controlled, intentional abort by the component), these are never genuinely unexpected errors
The core question is whether you want Sentry to reflect operational health (where user navigations are irrelevant) or full observability including user behavior patterns (where abort rate itself might be a signal). If the team has no use case for monitoring abort frequency in Sentry, silencing them entirely is the cleaner approach and better matches the PR's motivation. If you want to retain that data for trend analysis, the current implementation is a reasonable compromise.
Given your team's explicit concern about silently missing errors, the current approach is defensible — just worth knowing that warning-level events still contribute to Sentry volume. Either way, I'll defer to your judgment here.
✏️ Learnings added
Learnt from: baktun14
Repo: akash-network/console PR: 3050
File: apps/deploy-web/src/components/deployments/DeploymentLogs.tsx:138-147
Timestamp: 2026-04-10T18:43:35.084Z
Learning: In `DeploymentLogs.tsx` (`apps/deploy-web/src/components/deployments/DeploymentLogs.tsx`), the team intentionally reports aborted WebSocket streams (when `abortController.signal.aborted === true`) to Sentry at `"warning"` severity rather than silencing them entirely. This is a deliberate tradeoff: the team wants full observability and to avoid silently missing errors, while using severity levels to separate expected navigation aborts from real WebSocket failures (`"error"`). Do not suggest silencing aborted stream errors entirely in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ygrishajev
Repo: akash-network/console PR: 2863
File: apps/log-collector/src/services/pod-logs-collector/pod-logs-collector.service.ts:173-180
Timestamp: 2026-03-05T19:43:55.998Z
Learning: In `PodLogsCollectorService` (`apps/log-collector/src/services/pod-logs-collector/pod-logs-collector.service.ts`), the `remainingBuffer` (holding bytes received without a trailing `\n`) is intentionally NOT flushed when `logStream` emits `close`. Two cases cover all close scenarios: (1) AbortSignal fires because the pod was untracked — collection resumes from the last written timestamp on re-discovery, so the partial line is re-delivered; (2) the K8s log stream ends because the container terminated — the partial line is genuinely truncated and malformed. Writing a truncated line in either case would produce malformed output or a redundant duplicate, so the deliberate drop is correct design.
Learnt from: baktun14
Repo: akash-network/console PR: 1725
File: apps/api/src/utils/constants.ts:5-5
Timestamp: 2025-07-24T17:00:52.361Z
Learning: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
Learnt from: stalniy
Repo: akash-network/console PR: 2700
File: apps/deploy-web/src/components/shared/DynamicMonacoEditor/DynamicMonacoEditor.tsx:39-54
Timestamp: 2026-02-11T13:51:34.635Z
Learning: In TypeScript/JSX/TSX files, retain comments that explain the rationale behind non-obvious implementation decisions (e.g., why a workaround or error suppression is necessary). Prefer comments that justify the approach and trade-offs rather than describing what the code already does. Use clear, concise language and ensure the rationale is accessible to future maintainers.
| tags: { category: "deployments", label: "followLogs" } | ||
| severity: abortController.signal.aborted ? "warning" : "error", | ||
| tags: { category: "deployments", label: "followLogs" }, | ||
| cause: error instanceof Error ? (error as Error & { cause?: unknown }).cause : undefined |
There was a problem hiding this comment.
no need to log cause explicitly, sentry walks over .causes, additionally our logger logs error as well and that info becomes a part of sentry issue breadcrumbs
| setIsConnectionEstablished(false); | ||
| } | ||
|
|
||
| errorHandler.reportError({ |
There was a problem hiding this comment.
thought: I doubt that this PR actually reduces websocket errors because all AbortErrors are ignored at errorHandler level ->
There was a problem hiding this comment.
browser websocket on(error) event doesn't expose details about the actual issue. That's why we probably need to send additional http request to websocket, in case of error to get the actual error details (for example, 404, provider network issues, provider is down, etc)
3 participants
Why
Sentry issue DEPLOY-WEB-27J is escalating — 595 occurrences across 79 users in ~2 days. The
Error: websocket errorfires from the deployment logs WebSocket stream (followLogs) on/deployments/[dseq].Most of these errors are caused by users navigating away from the LOGS tab, which aborts the WebSocket connection and triggers the error handler unnecessarily. The remaining genuine errors have a generic message that makes Sentry triage difficult.
Fixes CON-204
What
useEffectcleanup runs (user navigates away), the abort controller fires, closing the WS. Previously this still reported to Sentry; now we checkabortController.signal.abortedbefore reporting.warning— genuine WebSocket connection failures (provider down, network issue) are transient and expected in a streaming context;warningis more appropriate thanerror."websocket error", the message now reads"websocket error: 1006 unknown reason", making Sentry grouping and triage much easier.Summary by CodeRabbit