Don't log errors when connect fails due to a user error#6665
Don't log errors when connect fails due to a user error#6665
Conversation
This should address the logs we're getting when connect is invoked without a connect handler being present
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR suppresses LOG_ERROR_PERIODICALLY for socket proxy errors that are tagged as user errors (e.g., missing connect() handler), in both handleProxyStatus overloads.
- Stale comment (low): The comment at line 436-438 only mentions "disconnected exception" but the filter now also covers user errors. Should be updated to reflect the new condition.
- Missing
DISCONNECTEDfilter in second overload (low): The first overload filters bothDISCONNECTEDandEXCEPTION_IS_USER_ERROR, but the second overload (line 484) only filtersEXCEPTION_IS_USER_ERROR. This is a pre-existing gap but since you're touching this code, it might be worth aligning both overloads for consistency.
| // Let's not log errors when we have a disconnected exception. | ||
| // If we don't filter this out, whenever connect() fails, we'll | ||
| // have noisy errors even though the user catches the error on JS side. |
There was a problem hiding this comment.
This comment only describes the DISCONNECTED filter but should also mention the EXCEPTION_IS_USER_ERROR check you're adding:
| // Let's not log errors when we have a disconnected exception. | |
| // If we don't filter this out, whenever connect() fails, we'll | |
| // have noisy errors even though the user catches the error on JS side. | |
| // Let's not log errors when we have a disconnected exception or a user error. | |
| // If we don't filter this out, whenever connect() fails, we'll | |
| // have noisy errors even though the user catches the error on JS side. |
| if (e.getDetail(jsg::EXCEPTION_IS_USER_ERROR) == kj::none) { | ||
| LOG_ERROR_PERIODICALLY("Socket proxy disconnected abruptly", e); | ||
| } |
There was a problem hiding this comment.
The first handleProxyStatus overload filters both DISCONNECTED and EXCEPTION_IS_USER_ERROR, but this one only filters EXCEPTION_IS_USER_ERROR. Should the DISCONNECTED check be added here too for consistency?
| if (e.getDetail(jsg::EXCEPTION_IS_USER_ERROR) == kj::none) { | |
| LOG_ERROR_PERIODICALLY("Socket proxy disconnected abruptly", e); | |
| } | |
| if (e.getType() != kj::Exception::Type::DISCONNECTED && | |
| e.getDetail(jsg::EXCEPTION_IS_USER_ERROR) == kj::none) { | |
| LOG_ERROR_PERIODICALLY("Socket proxy disconnected abruptly", e); | |
| } |
|
I'm Bonk, and I've done a quick review of your PR. This PR suppresses Two low-severity items posted as inline suggestions on the PR:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6665 +/- ##
==========================================
- Coverage 66.53% 66.51% -0.03%
==========================================
Files 405 405
Lines 115964 115967 +3
Branches 19407 19409 +2
==========================================
- Hits 77153 77131 -22
- Misses 27229 27255 +26
+ Partials 11582 11581 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
This should address the logs we're getting when connect is invoked without a connect handler being present
================
Not sure if this is the right approach here – might be better to change how exception handling works for connect? I have some tests available locally, but they only reproduce the issue, they're not able to check for the error not being logged.