🌱 using context log #151
🌱 using context log #151
Conversation
WalkthroughReplaces klog calls with contextual loggers (klog.FromContext(ctx)) and updates logging across connect, publish, subscribe, and receiver flows. Adds a call to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-16T02:22:20.929ZApplied to files:
⏰ 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). (3)
🔇 Additional comments (8)
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 (3)
pkg/cloudevents/generic/baseclient.go (3)
51-51: Tag logs with client identity once.Use WithValues to attach clientID to all logs from this context (apply similarly in publish/subscribe).
- logger := klog.FromContext(ctx) + logger := klog.FromContext(ctx).WithValues("clientID", c.clientID)Apply analogous changes:
- logger := klog.FromContext(ctx) + logger := klog.FromContext(ctx).WithValues("clientID", c.clientID)at Line 113 and Line 150.
153-153: LGTM: duplicate subscription guard message at V(2).Optionally include clientID for quicker correlation.
- logger.V(2).Info("the subscription has already started") + logger.V(2).Info("the subscription has already started")(Will be auto-included if you adopt WithValues as suggested earlier.)
192-198: LGTM: receiver restart/stop logs at V(2).Consider mirroring the non-blocking signal pattern for sendReceiverSignal to avoid holding RLock while sending.
-func (c *baseClient) sendReceiverSignal(signal int) { - c.RLock() - defer c.RUnlock() - if c.receiverChan != nil { - c.receiverChan <- signal - } -} +func (c *baseClient) sendReceiverSignal(signal int) { + c.RLock() + ch := c.receiverChan + c.RUnlock() + if ch == nil { + return + } + select { + case ch <- signal: + default: + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/cloudevents/generic/baseclient.go(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T02:22:20.929Z
Learnt from: skeeey
PR: open-cluster-management-io/sdk-go#144
File: pkg/cloudevents/generic/options/grpc/protocol/protocol.go:200-213
Timestamp: 2025-09-16T02:22:20.929Z
Learning: In the GRPC CloudEvents protocol implementation, when startEventsReceiver encounters a stream error, it sends the error to reconnectErrorChan. The consumer of this channel handles the error by calling Close() on the protocol, which triggers close(p.closeChan), causing OpenInbound to unblock and call cancel() to properly terminate both the events receiver and heartbeat watcher goroutines.
Applied to files:
pkg/cloudevents/generic/baseclient.go
⏰ 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). (3)
- GitHub Check: integration
- GitHub Check: verify
- GitHub Check: unit
🔇 Additional comments (6)
pkg/cloudevents/generic/baseclient.go (6)
63-76: Contextual reconnect logs look good.Verbosity and messages make sense; structured logger is used correctly.
113-113: LGTM: contextual logger in publish.
137-139: Event logging OK; confirm no sensitive payload at V(5).Metadata at V(2) and full event at V(5) meets the stated objectives. Verify that evt.String() cannot leak secrets/PII in your deployment.
150-150: LGTM: contextual logger in subscribe.
168-170: LGTM: received-event logs with metadata (V2) and full event (V5).
75-80: Review comment is based on an incorrect premise and should be disregarded.The review expresses concern that
reconnectedChanmay be nil and panic, but verification shows:
reconnectedChanis always initialized in bothsourceclient.go:50andagentclient.go:50viamake(chan struct{})- The channel is never explicitly closed anywhere in the codebase
- Sending to a nil channel would never occur—there is no nil panic risk
While the code does hold an RLock during the channel send (which could theoretically be optimized for better concurrency on an unbuffered channel), this is not a panic risk and is a separate performance concern from the nil-related issue claimed in the review. The current implementation is safe and functional.
The suggested diff is unnecessary.
Likely an incorrect or invalid review comment.
Signed-off-by: Wei Liu <[email protected]>
|
/assign @qiujian16 |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, skeeey The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit a3f7625
into
open-cluster-management-io:main
2 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit