🌱 Add contextual logging based recorder #170
Conversation
Signed-off-by: Jian Qiu <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16 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 |
WalkthroughThis pull request introduces a context-aware event recording system for controllers. It adds new recorder implementations for Kubernetes-native and logging-based event recording, updates the Recorder interface to accept context parameters directly (removing the previous WithContext mechanism), and integrates the recorder into the controller context. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
qiujian16
changed the title
![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 (1)
pkg/basecontroller/events/logging_recorder.go (1)
38-45: Consider removing the "INFO:" prefix for cleaner logs.The implementation correctly uses
klog.FromContextand structured logging with component and reason fields. However, the"INFO:"prefix in the message might be redundant since the log level already indicates this is an info-level message.If you'd like cleaner output, consider this diff:
func (r *ContextualLoggingEventRecorder) Event(ctx context.Context, reason, message string) { logger := klog.FromContext(ctx) - logger.Info(fmt.Sprintf("INFO: %s", message), "component", r.component, "reason", reason) + logger.Info(message, "component", r.component, "reason", reason) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
vendor/k8s.io/client-go/tools/events/OWNERSis excluded by!vendor/**vendor/k8s.io/client-go/tools/events/doc.gois excluded by!vendor/**vendor/k8s.io/client-go/tools/events/event_broadcaster.gois excluded by!vendor/**vendor/k8s.io/client-go/tools/events/event_recorder.gois excluded by!vendor/**vendor/k8s.io/client-go/tools/events/fake.gois excluded by!vendor/**vendor/k8s.io/client-go/tools/events/helper.gois excluded by!vendor/**vendor/k8s.io/client-go/tools/events/interfaces.gois excluded by!vendor/**vendor/k8s.io/client-go/tools/internal/events/interfaces.gois excluded by!vendor/**vendor/k8s.io/client-go/tools/record/OWNERSis excluded by!vendor/**vendor/k8s.io/client-go/tools/record/doc.gois excluded by!vendor/**vendor/k8s.io/client-go/tools/record/event.gois excluded by!vendor/**vendor/k8s.io/client-go/tools/record/events_cache.gois excluded by!vendor/**vendor/k8s.io/client-go/tools/record/fake.gois excluded by!vendor/**vendor/k8s.io/client-go/tools/record/util/util.gois excluded by!vendor/**vendor/k8s.io/utils/internal/third_party/forked/golang/golang-lru/lru.gois excluded by!vendor/**vendor/k8s.io/utils/lru/lru.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**
📒 Files selected for processing (5)
pkg/basecontroller/events/event_recorder.go(1 hunks)pkg/basecontroller/events/logging_recorder.go(1 hunks)pkg/basecontroller/events/recorder.go(2 hunks)pkg/basecontroller/factory/controller_context.go(2 hunks)pkg/basecontroller/factory/interfaces.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/basecontroller/events/event_recorder.go (1)
pkg/basecontroller/events/recorder.go (1)
NewRecorder(37-43)
pkg/basecontroller/events/logging_recorder.go (1)
pkg/basecontroller/events/recorder.go (1)
Recorder(15-34)
pkg/basecontroller/factory/controller_context.go (2)
pkg/basecontroller/events/recorder.go (1)
Recorder(15-34)pkg/basecontroller/events/logging_recorder.go (1)
NewContextualLoggingEventRecorder(16-20)
pkg/basecontroller/factory/interfaces.go (1)
pkg/basecontroller/events/recorder.go (1)
Recorder(15-34)
⏰ 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: unit
- GitHub Check: verify
🔇 Additional comments (12)
pkg/basecontroller/events/recorder.go (3)
69-75: LGTM!Both
EventfandWarningfcorrectly accept and forward the context parameter to their respective base methods with formatted messages.
79-92: LGTM!Both
EventandWarningmethods correctly use the provided context when creating events. Error handling appropriately logs failures without blocking the caller, which is consistent with the fire-and-forget pattern for event recording.
16-19: Breaking API change has been properly implemented across all implementations and call sites.All implementations of the
Recorderinterface have been correctly updated to requirecontext.Contextas the first parameter. The two implementations (recorderandContextualLoggingEventRecorder) both have matching signatures, internal method routing properly propagates context (e.g.,Eventf→Event,Warningf→Warning), and the public API exposed throughSyncContext.Recorder()provides the correct type. The factory instantiation uses the updated constructor signatures. No orphaned calls with the old signature pattern were found.pkg/basecontroller/factory/interfaces.go (2)
5-5: LGTM!The import of the events package is necessary to support the new
Recorder()method in theSyncContextinterface.
39-40: No issues found—all SyncContext implementations have been properly updated.The verification confirms that
syncContextis the sole implementation of theSyncContextinterface and both required methods are correctly implemented:
Queue()(line 32 of controller_context.go)Recorder()(line 36 of controller_context.go)The compile-time assertion (
var _ SyncContext = syncContext{}) at line 19 validates interface compliance and would fail to compile if any method were missing.pkg/basecontroller/factory/controller_context.go (4)
9-9: LGTM!The events package import is necessary to support the new recorder field and initialization.
15-16: LGTM!The recorder field is properly added to the
syncContextstruct with the correct type. The interface compliance assertion at line 19 confirms the struct implements the updatedSyncContextinterface.
28-28: LGTM!The recorder is appropriately initialized with
NewContextualLoggingEventRecorder, which provides a logging-based recorder that doesn't require Kubernetes API access. The controller name is correctly passed as the component identifier.
36-38: LGTM!The
Recorder()method correctly implements theSyncContextinterface requirement and returns the initialized recorder.pkg/basecontroller/events/logging_recorder.go (2)
10-20: LGTM!The
ContextualLoggingEventRecorderstruct and constructor are well-designed. The simple structure with a component field enables component-aware logging while maintaining theRecorderinterface contract.
22-36: LGTM!The component management methods are correctly implemented:
ForComponentsafely creates a new recorder instance with a different componentWithComponentSuffixproperly builds onForComponentfor component hierarchiesShutdownas a no-op is appropriate for a logging-only recorderpkg/basecontroller/events/event_recorder.go (1)
11-22: Based on verification of the codebase and k8s.io/client-go documentation showing that canceling the context will shut down the broadcaster, and Shutdown doesn't need to be called, I can now provide the corrected assessment:Broadcaster context cleanup is already handled correctly—remove the verification requirement.
The function properly leverages
StartRecordingToSinkWithContext(ctx), which ensures the broadcaster shuts down automatically when the provided context is cancelled. No additional verification is needed for context cleanup.However, the review's documentation suggestions remain valid:
- Return type clarity: The function returns
k8s.io/client-go/tools/events.EventRecorder, not the localevents.Recorderinterface defined in this package. This differs fromNewContextualLoggingEventRecorder(which returns the local interface).- Lifecycle documentation: Callers should understand that broadcaster cleanup is automatic via context cancellation and that no manual shutdown call is required.
Consider adding package-level documentation explaining:
- When to use
NewEventRecorder(returns k8s client-go type with full event sink/sink recording) vs.NewContextualLoggingEventRecorder(returns local Recorder interface with logging-only behavior)- That the returned recorder's lifetime is tied to the provided context
| func (r *ContextualLoggingEventRecorder) Warning(ctx context.Context, reason, message string) { | ||
| logger := klog.FromContext(ctx) | ||
| logger.Info(fmt.Sprintf("WARNING: %s", message), "component", r.component, "reason", reason) | ||
| } | ||
|
|
||
| func (r *ContextualLoggingEventRecorder) Warningf(ctx context.Context, reason, messageFmt string, args ...interface{}) { | ||
| r.Warning(ctx, reason, fmt.Sprintf(messageFmt, args...)) | ||
| } |
There was a problem hiding this comment.
Warning events should use a distinct log level.
The Warning method currently uses logger.Info, which logs warnings at the same severity as normal events. This prevents users from filtering warnings separately using log level flags (e.g., -v flag in klog).
Consider using a distinct log level for warnings:
func (r *ContextualLoggingEventRecorder) Warning(ctx context.Context, reason, message string) {
logger := klog.FromContext(ctx)
- logger.Info(fmt.Sprintf("WARNING: %s", message), "component", r.component, "reason", reason)
+ logger.V(1).Info(message, "component", r.component, "reason", reason, "eventType", "Warning")
}Or use the error level if these represent actual problems:
func (r *ContextualLoggingEventRecorder) Warning(ctx context.Context, reason, message string) {
logger := klog.FromContext(ctx)
- logger.Info(fmt.Sprintf("WARNING: %s", message), "component", r.component, "reason", reason)
+ logger.Error(nil, message, "component", r.component, "reason", reason)
}The choice depends on your logging strategy and how you want warnings filtered.
📝 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.
| func (r *ContextualLoggingEventRecorder) Warning(ctx context.Context, reason, message string) { | |
| logger := klog.FromContext(ctx) | |
| logger.Info(fmt.Sprintf("WARNING: %s", message), "component", r.component, "reason", reason) | |
| } | |
| func (r *ContextualLoggingEventRecorder) Warningf(ctx context.Context, reason, messageFmt string, args ...interface{}) { | |
| r.Warning(ctx, reason, fmt.Sprintf(messageFmt, args...)) | |
| } | |
| func (r *ContextualLoggingEventRecorder) Warning(ctx context.Context, reason, message string) { | |
| logger := klog.FromContext(ctx) | |
| logger.V(1).Info(message, "component", r.component, "reason", reason, "eventType", "Warning") | |
| } | |
| func (r *ContextualLoggingEventRecorder) Warningf(ctx context.Context, reason, messageFmt string, args ...interface{}) { | |
| r.Warning(ctx, reason, fmt.Sprintf(messageFmt, args...)) | |
| } |
| func (r *ContextualLoggingEventRecorder) Warning(ctx context.Context, reason, message string) { | |
| logger := klog.FromContext(ctx) | |
| logger.Info(fmt.Sprintf("WARNING: %s", message), "component", r.component, "reason", reason) | |
| } | |
| func (r *ContextualLoggingEventRecorder) Warningf(ctx context.Context, reason, messageFmt string, args ...interface{}) { | |
| r.Warning(ctx, reason, fmt.Sprintf(messageFmt, args...)) | |
| } | |
| func (r *ContextualLoggingEventRecorder) Warning(ctx context.Context, reason, message string) { | |
| logger := klog.FromContext(ctx) | |
| logger.Error(nil, message, "component", r.component, "reason", reason) | |
| } | |
| func (r *ContextualLoggingEventRecorder) Warningf(ctx context.Context, reason, messageFmt string, args ...interface{}) { | |
| r.Warning(ctx, reason, fmt.Sprintf(messageFmt, args...)) | |
| } |
🤖 Prompt for AI Agents
In pkg/basecontroller/events/logging_recorder.go around lines 47-54, the Warning
method currently uses logger.Info which does not distinguish warnings from
normal info; replace logger.Info with logger.Error(nil, ...) so warnings are
logged at the error/warning severity (and update Warningf to call the changed
Warning implementation) — ensure the Error call keeps the same message and
key/value pairs ("component", r.component, "reason", reason).
|
/lgtm |
openshift-merge-bot
bot
merged commit 8ba6d23
into
open-cluster-management-io:main
2 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.