refactor(app): improve logging in graceful eviction controllers #6555
Conversation
…e structured logging Signed-off-by: Mohit Nagaraj <[email protected]>
karmada-bot
added
the
kind/cleanup
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Welcome @mohit-nagaraj! It looks like this is your first PR to karmada-io/karmada 🎉 |
karmada-bot
added
the
size/S
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @mohit-nagaraj, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the logging implementation within the graceful eviction controllers (CRBGracefulEvictionController and RBGracefulEvictionController) to adopt structured logging. The change aims to enhance the observability and debuggability of these components by making log messages more consistent and easier to parse programmatically.
Highlights
- Structured Logging Adoption: Converted
klog.Infofcalls toklog.InfoSfor structured logging within theCRBGracefulEvictionController. - Structured Logging Adoption: Converted
klog.Infofcalls toklog.InfoSfor structured logging within theRBGracefulEvictionController. - Code Cleanup: Improved log message clarity and machine readability by replacing printf-style formatting with key-value pairs in log statements.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR refactors logging statements in the graceful eviction controllers to use structured logging instead of formatted string logging. The changes improve log consistency and machine readability by replacing klog.Infof() calls with klog.InfoS() and converting formatted strings to key-value pairs.
- Converts formatted logging to structured logging using
klog.InfoS() - Replaces string interpolation with structured key-value pairs
- Updates log messages for better clarity and consistency
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/controllers/gracefuleviction/rb_graceful_eviction_controller.go | Updates ResourceBinding controller logging to use structured format |
| pkg/controllers/gracefuleviction/crb_graceful_eviction_controller.go | Updates ClusterResourceBinding controller logging to use structured format |
| @@ -71,7 +71,7 @@ func (c *RBGracefulEvictionController) Reconcile(ctx context.Context, req contro | |||
| return controllerruntime.Result{}, err | |||
| } | |||
| if retryDuration > 0 { | |||
| klog.V(4).Infof("Retry to evict task after %v minutes.", retryDuration.Minutes()) | |||
| klog.V(4).InfoS("Retry to evict task after minutes", "retryAfterMinutes", retryDuration.Minutes()) | |||
There was a problem hiding this comment.
The log message "Retry to evict task after minutes" is unclear without context. Consider changing it to "Retry to evict task after specified minutes" or "Will retry eviction task" to make it more descriptive.
| klog.V(4).InfoS("Retry to evict task after minutes", "retryAfterMinutes", retryDuration.Minutes()) | |
| klog.V(4).InfoS("Retrying eviction task after specified duration in minutes", "retryAfterMinutes", retryDuration.Minutes()) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Depends on the opinion of the maintainer I would prefer to wait for review once
| @@ -71,7 +71,7 @@ func (c *CRBGracefulEvictionController) Reconcile(ctx context.Context, req contr | |||
| return controllerruntime.Result{}, err | |||
| } | |||
| if retryDuration > 0 { | |||
| klog.V(4).Infof("Retry to evict task after %v minutes.", retryDuration.Minutes()) | |||
| klog.V(4).InfoS("Retry to evict task after minutes", "retryAfterMinutes", retryDuration.Minutes()) | |||
There was a problem hiding this comment.
The log message "Retry to evict task after minutes" is unclear without context. Consider changing it to "Retry to evict task after specified minutes" or "Will retry eviction task" to make it more descriptive.
| klog.V(4).InfoS("Retry to evict task after minutes", "retryAfterMinutes", retryDuration.Minutes()) | |
| klog.V(4).InfoS("Will retry eviction task after specified minutes", "retryAfterMinutes", retryDuration.Minutes()) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Code Review
This pull request refactors logging to use structured logging with klog.InfoS, which is a great improvement. My review focuses on enhancing the clarity and consistency of the new log statements. I've suggested changes to:
- Improve log message phrasing.
- Use more descriptive and less ambiguous keys for structured data.
- Ensure consistency in logging related entities within the same file.
These changes should make the logs more useful and easier to query.
| @@ -71,7 +71,7 @@ func (c *CRBGracefulEvictionController) Reconcile(ctx context.Context, req contr | |||
| return controllerruntime.Result{}, err | |||
| } | |||
| if retryDuration > 0 { | |||
| klog.V(4).Infof("Retry to evict task after %v minutes.", retryDuration.Minutes()) | |||
| klog.V(4).InfoS("Retry to evict task after minutes", "retryAfterMinutes", retryDuration.Minutes()) | |||
There was a problem hiding this comment.
The log message "Retry to evict task after minutes" is a bit awkward. A more descriptive message would be better. Also, it's idiomatic in Go to log time.Duration values directly and let the logging library format them, rather than converting to a specific unit like minutes manually.
| klog.V(4).InfoS("Retry to evict task after minutes", "retryAfterMinutes", retryDuration.Minutes()) | |
| klog.V(4).InfoS("Requeuing eviction task for retry", "retryAfter", retryDuration) |
| klog.V(2).InfoS("Evicted cluster from ClusterResourceBinding gracefulEvictionTasks", | ||
| "cluster", cluster, "name", binding.Name) |
There was a problem hiding this comment.
The key name is ambiguous. For clarity and consistency with the logging statement on line 55, please use clusterResourceBinding as the key for the binding's name.
| klog.V(2).InfoS("Evicted cluster from ClusterResourceBinding gracefulEvictionTasks", | |
| "cluster", cluster, "name", binding.Name) | |
| klog.V(2).InfoS("Evicted cluster from ClusterResourceBinding gracefulEvictionTasks", | |
| "cluster", cluster, "clusterResourceBinding", binding.Name) |
| @@ -52,7 +52,7 @@ type RBGracefulEvictionController struct { | |||
| // The Controller will requeue the Request to be processed again if an error is non-nil or | |||
| // Result.Requeue is true, otherwise upon completion it will remove the work from the queue. | |||
| func (c *RBGracefulEvictionController) Reconcile(ctx context.Context, req controllerruntime.Request) (controllerruntime.Result, error) { | |||
| klog.V(4).Infof("Reconciling ResourceBinding %s.", req.NamespacedName.String()) | |||
| klog.V(4).InfoS("Reconciling ResourceBinding", "resourceBinding", req.NamespacedName.String()) | |||
There was a problem hiding this comment.
For consistency with other logging statements in this file (e.g., lines 100-101) and for better queryability of structured logs, it's preferable to log namespace and name as separate key-value pairs. Additionally, using a more specific key like resourceBindingName instead of just name improves clarity.
| klog.V(4).InfoS("Reconciling ResourceBinding", "resourceBinding", req.NamespacedName.String()) | |
| klog.V(4).InfoS("Reconciling ResourceBinding", "namespace", req.Namespace, "resourceBindingName", req.Name) |
| @@ -71,7 +71,7 @@ func (c *RBGracefulEvictionController) Reconcile(ctx context.Context, req contro | |||
| return controllerruntime.Result{}, err | |||
| } | |||
| if retryDuration > 0 { | |||
| klog.V(4).Infof("Retry to evict task after %v minutes.", retryDuration.Minutes()) | |||
| klog.V(4).InfoS("Retry to evict task after minutes", "retryAfterMinutes", retryDuration.Minutes()) | |||
There was a problem hiding this comment.
The log message "Retry to evict task after minutes" is a bit awkward. A more descriptive message would be better. Also, it's idiomatic in Go to log time.Duration values directly and let the logging library format them, rather than converting to a specific unit like minutes manually.
| klog.V(4).InfoS("Retry to evict task after minutes", "retryAfterMinutes", retryDuration.Minutes()) | |
| klog.V(4).InfoS("Requeuing eviction task for retry", "retryAfter", retryDuration) |
| klog.V(2).InfoS("Evicted cluster from ResourceBinding gracefulEvictionTasks", "cluster", cluster, | ||
| "namespace", binding.Namespace, "name", binding.Name) |
There was a problem hiding this comment.
The key name is ambiguous. For better clarity and consistency, please use a more specific key like resourceBindingName.
| klog.V(2).InfoS("Evicted cluster from ResourceBinding gracefulEvictionTasks", "cluster", cluster, | |
| "namespace", binding.Namespace, "name", binding.Name) | |
| klog.V(2).InfoS("Evicted cluster from ResourceBinding gracefulEvictionTasks", "cluster", cluster, | |
| "namespace", binding.Namespace, "resourceBindingName", binding.Name) |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #6555 +/- ##
=======================================
Coverage 45.40% 45.41%
=======================================
Files 687 687
Lines 56397 56397
=======================================
+ Hits 25605 25610 +5
+ Misses 29195 29191 -4
+ Partials 1597 1596 -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:
|
What type of PR is this?
Add one of the following kinds:
/kind cleanup
What this PR does / why we need it:
Structured logging support
Which issue(s) this PR fixes:
Partially Fix #6230
Special notes for your reviewer:
Does this PR introduce a user-facing change?: