fix: enhance panic error logging in SDK by adding stack trace #272
Conversation
WalkthroughThe pull request introduces consistent panic recovery and logging mechanisms across multiple WebSocket client files. The changes focus on enhancing error handling by importing the Changes
Sequence DiagramsequenceDiagram
participant Client
participant WebSocket
participant ErrorHandler
Client->>WebSocket: Initiate Connection
WebSocket->>Client: Connection Established
alt Panic Occurs
WebSocket->>ErrorHandler: Capture Panic
ErrorHandler-->>ErrorHandler: Log Panic Value
ErrorHandler-->>ErrorHandler: Log Stack Trace
ErrorHandler->>Client: Close Connection
end
The sequence diagram illustrates the enhanced error handling workflow, showing how panics are now captured, logged with detailed information, and managed during WebSocket interactions. Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -217,7 +218,9 @@ func (c *WSChannel) flush() { | |||
| err := common.ErrFatalPanicRecovered | |||
| sendErr := c.ProcessError(err) | |||
| if sendErr != nil { | |||
| klog.V(1).Infof("listen: Fatal socket error. Err: %v\n", sendErr) | |||
| klog.V(1).Infof("speak: Fatal socket error. Err: %v\n", sendErr) | |||
There was a problem hiding this comment.
changing log to speak bc this is use in the speak side.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/client/speak/v1/websocket/client_callback.go (1)
223-224: LGTM: Enhanced panic logging with stack trace.The additional logging statements provide valuable context for debugging by capturing both the panic value and the stack trace.
Consider adding structured logging in a future enhancement to make the logs more machine-parseable.
-klog.V(1).Infof("Panic: %v\n", r) -klog.V(1).Infof("Stack trace: %s\n", string(debug.Stack())) +klog.V(1).Infof("panic_details: {\"value\": %q, \"stack_trace\": %q}\n", r, string(debug.Stack()))pkg/client/listen/v1/websocket/client_channel.go (1)
256-257: Consider extracting panic logging to a utility function.The panic logging implementation is duplicated across multiple files and methods. Consider creating a common utility function to improve maintainability and ensure consistent logging format.
// Add to common/v1/logging/panic.go +package logging + +import ( + "runtime/debug" + klog "k8s.io/klog/v2" +) + +// LogPanic logs panic details with stack trace +func LogPanic(r interface{}, prefix string) { + klog.V(1).Infof("%s: Fatal socket error\n", prefix) + klog.V(1).Infof("Panic: %v\n", r) + klog.V(1).Infof("Stack trace: %s\n", string(debug.Stack())) +}Then use it in the panic recovery blocks:
-klog.V(1).Infof("listen: Fatal socket error. Err: %v\n", sendErr) -klog.V(1).Infof("Panic: %v\n", r) -klog.V(1).Infof("Stack trace: %s\n", string(debug.Stack())) +logging.LogPanic(r, "listen")Also applies to: 301-302
pkg/client/common/v1/websocket.go (1)
Line range hint
1-644: Consider extending panic recovery to other critical methods.The enhanced panic logging is currently only implemented in the
listenmethod. Consider adding similar panic recovery blocks to other critical methods that handle WebSocket communication (WriteBinary,WriteJSON) and connection cleanup (closeWs). This would provide consistent error handling and debugging capabilities across the entire WebSocket client implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/client/common/v1/websocket.go(3 hunks)pkg/client/listen/v1/websocket/client_callback.go(3 hunks)pkg/client/listen/v1/websocket/client_channel.go(3 hunks)pkg/client/speak/v1/websocket/client_callback.go(2 hunks)pkg/client/speak/v1/websocket/client_channel.go(2 hunks)
🔇 Additional comments (7)
pkg/client/speak/v1/websocket/client_callback.go (1)
13-13: LGTM: Import added for stack trace functionality.The addition of the
runtime/debugpackage is necessary for implementing the enhanced panic logging feature.pkg/client/speak/v1/websocket/client_channel.go (2)
13-13: LGTM: Import added for stack trace functionality.The addition of the
runtime/debugpackage is consistent with the implementation across other files.
221-223: LGTM: Enhanced panic logging with stack trace.The implementation follows the consistent pattern of logging both panic value and stack trace.
pkg/client/listen/v1/websocket/client_callback.go (2)
14-14: LGTM: Import added for stack trace functionality.The addition of the
runtime/debugpackage is consistent with the implementation across other files.
256-257: LGTM: Enhanced panic logging in both ping and flush methods.The implementation consistently adds panic value and stack trace logging to both methods that can potentially panic.
Also applies to: 301-302
pkg/client/listen/v1/websocket/client_channel.go (1)
14-14: LGTM: Import added for stack trace functionality.The addition of the
runtime/debugpackage is consistent with the implementation across other files.pkg/client/common/v1/websocket.go (1)
16-16: LGTM: Import required for stack trace capture.The addition of the
runtime/debugpackage is necessary for implementing the enhanced panic logging feature.
onur-yildirim-infinitusai
force-pushed
the
adding-stack-trace-for-panics
branch
2 times, most recently
from
e9fbb05 to
2b83715
Compare
onur-yildirim-infinitusai
force-pushed
the
adding-stack-trace-for-panics
branch
from
2b83715 to
5a3960f
Compare
|
@jpvajda and @naomi-lgbt Can I ask you to take a look at this change? |
|
@onur-yildirim-infinitusai Thanks for working on this change. I'm curious are you blocked from using the Go SDK without this change? or is this more of a "quality of life" change you want to introduce to improve panic error handling? Just asking so we can better understand the urgency for your point of view. |
Thanks for taking a look. I think it affects the developer experience a lot especiall when you try to debug a panic scenario so I'm vouching more for improving the library usability. I already use a forked version with this change for my project. However I want to use the nova-3 and I would prefer to use the official version instead. If you think the review process can take long, I can add keep using the forked version @jpvajda |
|
@onur-yildirim-infinitusai We like better DX 😄 ! Any suggestions on how to best test these changes? |
naomi-lgbt
changed the title
Proposed changes
Currently, when a panic occurs in the SDK, the error messages are generic and don't provide enough context for debugging:
"Panic triggered"
"ProcessError(fatal panic - attempt to recover) failed"
"listen: Fatal socket error"
This PR adds detailed stack traces to panic recovery scenarios to help developers better understand and troubleshoot the root cause of failures. The enhanced logging will show the full call stack where the panic originated, making it easier to identify issues.
Types of changes
What types of changes does your code introduce to the community Go SDK?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
Summary by CodeRabbit
Bug Fixes
Chores