🌱 use SDK basecontroller for better logging. #1269
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 |
|
Important Review skippedMore than 25% of the files skipped due to max files limit. The review is being skipped to prevent a low-quality review. 72 files out of 180 files are above the max files limit of 100. Please upgrade to Pro plan to get higher limits. You can disable this status message by setting the ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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
force-pushed
the
sdk-basecontroller
branch
2 times, most recently
from
849c62d to
7ac84ee
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1269 +/- ##
=======================================
Coverage 62.19% 62.19%
=======================================
Files 210 209 -1
Lines 17084 16992 -92
=======================================
- Hits 10625 10568 -57
+ Misses 5342 5308 -34
+ Partials 1117 1116 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| clusterv1informers.NewSharedInformerFactory(spokeClusterClient, 10*time.Minute), | ||
| aboutinformers.NewSharedInformerFactory(aboutClusterClient, 10*time.Minute), | ||
| controllerContext.EventRecorder, | ||
| events.NewContextualLoggingEventRecorder("registration-agent"), |
There was a problem hiding this comment.
Same question here, with this recorder, only log, no k8s event created?
|
@qiujian16 I am a little confused about the title. From my understanding, most content in this PR is about |
added some explanation. The main reason is basecontroller in sdk-go has contextual logging implemented. |
| func (e *EventsRecorderWrapper) WithContext(ctx context.Context) librarygoevents.Recorder { | ||
| eCopy := *e | ||
| eCopy.ctx = ctx | ||
| return e |
There was a problem hiding this comment.
Should return eCopy?
func (e *EventsRecorderWrapper) WithContext(ctx context.Context) librarygoevents.Recorder {
eCopy := *e
eCopy.ctx = ctx
return &eCopy
}
7ac84ee to
9d54b98
Compare
| e.recorder.Warningf(e.ctx, reason, messageFmt, args...) | ||
| } | ||
|
|
||
| func (e *EventsRecorderWrapper) ForComponent(componentName string) librarygoevents.Recorder { |
There was a problem hiding this comment.
Seems here we have similar issues for these 2 functions, expected to change to the following way:
func (e *EventsRecorderWrapper) ForComponent(componentName string) librarygoevents.Recorder {
return &EventsRecorderWrapper{
recorder: e.recorder.ForComponent(componentName),
ctx: e.ctx,
}
}
func (e *EventsRecorderWrapper) WithComponentSuffix(suffix string) librarygoevents.Recorder {
return &EventsRecorderWrapper{
recorder: e.recorder.WithComponentSuffix(suffix),
ctx: e.ctx,
}
}
Based on the interface defination, e.recorder.ForComponent(componentName) and e.recorder.WithComponentSuffix(suffix) returns new recorder instead of modifying the old recorder. So the old way will make new recorder not used.
There was a problem hiding this comment.
interface defination:
// ForComponent allows to fiddle the component name before sending the event to sink.
// Making more unique components will prevent the spam filter in upstream event sink from dropping
// events.
ForComponent(componentName string) Recorder
// WithComponentSuffix is similar to ForComponent except it just suffix the current component name instead of overriding.
WithComponentSuffix(componentNameSuffix string) Recorder
There was a problem hiding this comment.
this is intentional, we need this to return a new one to avoid race if multiple components call at the same time.
See implementation here https://github.com/openshift/library-go/blob/master/pkg/operator/events/recorder_logging.go#L35-L38
There was a problem hiding this comment.
But the current implementation doesn't use the new recorder, because:
func (e *EventsRecorderWrapper) ForComponent(componentName string) librarygoevents.Recorder {
e.recorder.ForComponent(componentName)
return e
}
equals to
func (e *EventsRecorderWrapper) ForComponent(componentName string) librarygoevents.Recorder {
newRecorder := e.recorder.ForComponent(componentName)
return e // if we return `e` here, the above newRecorder is throwed.
}
|
LGTM |
Signed-off-by: Jian Qiu <[email protected]>
qiujian16
force-pushed
the
sdk-basecontroller
branch
from
9d54b98 to
d81d810
Compare
|
Also look good to me! |
|
/lgtm |
openshift-merge-bot
bot
merged commit 3331061
into
open-cluster-management-io:main
5 participants
Summary
basecontroller in library-go does not support contextual logging. The basecontroller in sdk-go has implemented contextual logging.
Related issue(s)
Fixes #