Clean up socks logging #1285
Clean up socks logging #1285
Conversation
WalkthroughThe recent update focuses on improving the error handling and logging mechanisms within the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/socks/src/helpers/wss.ts (4 hunks)
Additional Context Used
Path-based Instructions (1)
indexer/services/socks/src/helpers/wss.ts (1)
**/**:
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (3)
indexer/services/socks/src/helpers/wss.ts (3)
112-114: The changes from lines 112 to 114 introduce a stats increment forwrite EPIPEerrors. This is a good practice as it helps in monitoring and identifying the frequency of these errors. However, it's important to ensure that the stats are being monitored and acted upon. Consider setting up alerts or thresholds that trigger investigations when these errors exceed normal levels.
119-121: The handling ofwrite ECONNRESETerrors from lines 119 to 121 is improved by incrementing stats and closing the WebSocket connection with a specific close code and reason. This is a proactive approach to managing abrupt client disconnections. Ensure that the close codeWS_CLOSE_CODE_ABNORMAL_CLOSUREis appropriately documented and understood by client applications to facilitate better error handling on their end.
112-121: The error handling forwrite EPIPEandwrite ECONNRESETerrors includes incrementing stats but does not log these errors. While this approach reduces log volume and potentially mitigates memory spikes, it's crucial to have a mechanism in place for debugging these errors when necessary. Consider logging these errors at a lower log level (e.g., debug) or implementing a feature to enable detailed logging temporarily for troubleshooting purposes.
| stats.increment( | ||
| `${config.SERVICE_NAME}.ws_send.write_epipe_errors`, | ||
| ); | ||
| } else if (error?.message.includes?.('write ECONNRESET')) { | ||
| // This error means that the client abruptly disconnected without sending a proper "close" | ||
| // message (or the message is delayed). In this case, we should terminate the connection | ||
| // immediately. | ||
| stats.increment( | ||
| `${config.SERVICE_NAME}.ws_send.write_econn_reset_errors`, | ||
| ); |
There was a problem hiding this comment.
For both write EPIPE and write ECONNRESET errors, stats are incremented without specifying a count or tags. While incrementing by a default value of 1 might be intended, explicitly specifying the count ensures clarity and prevents misunderstandings. Additionally, consider adding tags or labels to these stats to provide more context, such as the WebSocket connection ID or other relevant identifiers.
- stats.increment(
- `${config.SERVICE_NAME}.ws_send.write_epipe_errors`,
- );
+ stats.increment(
+ `${config.SERVICE_NAME}.ws_send.write_epipe_errors`,
+ 1,
+ { connectionId: connectionId },
+ );
- stats.increment(
- `${config.SERVICE_NAME}.ws_send.write_econn_reset_errors`,
- );
+ stats.increment(
+ `${config.SERVICE_NAME}.ws_send.write_econn_reset_errors`,
+ 1,
+ { connectionId: connectionId },
+ );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.
| stats.increment( | |
| `${config.SERVICE_NAME}.ws_send.write_epipe_errors`, | |
| ); | |
| } else if (error?.message.includes?.('write ECONNRESET')) { | |
| // This error means that the client abruptly disconnected without sending a proper "close" | |
| // message (or the message is delayed). In this case, we should terminate the connection | |
| // immediately. | |
| stats.increment( | |
| `${config.SERVICE_NAME}.ws_send.write_econn_reset_errors`, | |
| ); | |
| stats.increment( | |
| `${config.SERVICE_NAME}.ws_send.write_epipe_errors`, | |
| 1, | |
| { connectionId: connectionId }, | |
| ); | |
| } else if (error?.message.includes?.('write ECONNRESET')) { | |
| // This error means that the client abruptly disconnected without sending a proper "close" | |
| // message (or the message is delayed). In this case, we should terminate the connection | |
| // immediately. | |
| stats.increment( | |
| `${config.SERVICE_NAME}.ws_send.write_econn_reset_errors`, | |
| 1, | |
| { connectionId: connectionId }, | |
| ); |
* Add subaccountNumber to PerpetualPositionResponseObject (#1274) Signed-off-by: Shrenuj Bansal <[email protected]> * [CT-712] send updates for both normal order matches and liquidation (#1280) * Remove volatile market (#1263) * [SKI-21] Bump slinky version to v0.3.1 (#1275) * Bump slinky version * Add cp number method * clean up socks logging (#1285) * [CT-681] fix liquidated side and offsetting side for indexer delevera… (#1284) * [CT-681] fix liquidated side and offsetting side for indexer deleveraging events * fix test * fix test * fix lint * [TRA-105] Add API for parent subaccount perpetual positions (#1282) Signed-off-by: Shrenuj Bansal <[email protected]> * Use sample rate with stream destroyed stats. (#1294) * Revert "[CT-708] Indexer track e2e latency (#1237)" (#1292) This reverts commit 60b94df. * Fix swagger generation makefile command / regen swagger docs (#1299) * pull dydx fork to generate swagger properly * remove the print * remove vault constants (#1293) * Remove custom ping message from socks (#1301) * Add subaccountNumber to the OrderResponseObject (#1296) Signed-off-by: Shrenuj Bansal <[email protected]> * sample more metrics (#1304) * [OTE-256] Add upgrade handler to initialize OI during upgrade handler (#1302) * Add upgrade handler to initialize OI during upgrade handler * nits * Fix lib.ErrorLogWithError: properly pass in args (#1306) * fix broken tests (#1312) * Explicitly close websockets on errors (#1290) * Increase the number of allowed connections to 8000 (#1317) * [TRA-104] Add parentSubaccountNumber API for orders (#1313) Signed-off-by: Shrenuj Bansal <[email protected]> * Improve Slinky logs to prevent unnecessary logs (#1289) * [SKI-26]: Prevent funding index update with no oracle prices from (#1321) halting indexer * Skip equity tier limit check in PlaceShortTermOrder (#1318) * Skip equity tier limit check in PlaceShortTermOrder * remove tests * Add comment * fix lint (#1323) --------- Signed-off-by: Shrenuj Bansal <[email protected]> Co-authored-by: shrenujb <[email protected]> Co-authored-by: jayy04 <[email protected]> Co-authored-by: Eric Warehime <[email protected]> Co-authored-by: vincentwschau <[email protected]> Co-authored-by: Jonathan Fung <[email protected]> Co-authored-by: Tian <[email protected]> Co-authored-by: Teddy Ding <[email protected]> Co-authored-by: roy-dydx <[email protected]> Co-authored-by: Christopher-Li <[email protected]>
Changelist
Logging is causing memory spikes. See discussion here for details.
Test Plan
N/A
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.