JSON logging for multiclusterservice controllers #6546

Bhaumik10 · 2025-07-16T20:19:31Z

What type of PR is this?

JSON logging support. More details are highlighted in #6230.

Which issue(s) this PR fixes:
Fixes #
Partof #6230

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

karmada-bot · 2025-07-16T20:19:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign seanlaii for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/controllers/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gemini-code-assist

Summary of Changes

Hello @Bhaumik10, I'm Gemini Code Assist¹! 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 significantly enhances the logging capabilities of the multiclusterservice controllers by transitioning from traditional formatted strings to structured logging. By adopting klog.InfoS and klog.ErrorS, the logs now provide key-value pairs, making them machine-readable and much easier to query, filter, and analyze with logging tools. This change improves the overall observability and debugging experience for these critical components.

Highlights

Structured Logging Adoption: All klog.Infof and klog.Errorf calls within the multiclusterservice controllers have been migrated to use klog.InfoS and klog.ErrorS respectively. This change introduces structured logging (key-value pairs) for improved log parsing and analysis across endpointslice_collect_controller.go, endpointslice_dispatch_controller.go, and mcs_controller.go.
Logging Level Refinement: Several klog.Warningf calls have been adjusted to klog.V(4).InfoS. This indicates a re-evaluation of their severity from warnings to verbose informational messages, aligning with the new structured logging approach and ensuring more precise log categorization.

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.

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. ↩

gemini-code-assist

Code Review

This pull request migrates the logging in multiclusterservice controllers to use structured JSON format via klog. I've identified areas for improvement, mainly around ensuring log messages and keys are clear and consistent, and fixing a couple of minor bugs in the new logging statements. Addressing these points will help maximize the benefits of structured logging.

pkg/controllers/multiclusterservice/endpointslice_dispatch_controller.go

pkg/controllers/multiclusterservice/endpointslice_collect_controller.go

gemini-code-assist · 2025-07-16T20:22:02Z

pkg/controllers/multiclusterservice/endpointslice_collect_controller.go

@@ -245,7 +245,7 @@
 		curObj := obj.(runtime.Object)
 		key, err := keys.FederatedKeyFunc(clusterName, curObj)
 		if err != nil {
-			klog.Warningf("Failed to generate key for obj: %s", curObj.GetObjectKind().GroupVersionKind())
+			klog.V(4).InfoS("Failed to generate key for obj", "gvk", curObj.GetObjectKind().GroupVersionKind())


This change demotes a klog.Warningf to a klog.V(4).InfoS. Use klog.WarningS to maintain the same log level and log the err object for context.

Suggested change

klog.V(4).InfoS("Failed to generate key for obj", "gvk", curObj.GetObjectKind().GroupVersionKind())

klog.WarningS(err, "Failed to generate key for obj", "gvk", curObj.GetObjectKind().GroupVersionKind())

I don't think it is warningS method in first-place.

@jabellard Do you know why Klog does not provide klog.WarningS?

@ Bhaumik10, this log shows an unusual situation; we should log it with a warning or error level.

@RainbowMango Seems it is not available in package. I can either move to Error or Info with higher level as I mentioned with V(4). Do you have preference to go with Error?

gemini-code-assist · 2025-07-16T20:22:02Z

pkg/controllers/multiclusterservice/endpointslice_collect_controller.go

@@ -258,7 +258,7 @@
 		if !reflect.DeepEqual(oldObj, newObj) {
 			key, err := keys.FederatedKeyFunc(clusterName, curObj)
 			if err != nil {
-				klog.Warningf("Failed to generate key for obj: %s", curObj.GetObjectKind().GroupVersionKind())
+				klog.V(4).InfoS("Failed to generate key for obj", "gvk", curObj.GetObjectKind().GroupVersionKind())


This change demotes a klog.Warningf to a klog.V(4).InfoS. Use klog.WarningS to maintain the same log level and log the err object for context.

Suggested change

klog.V(4).InfoS("Failed to generate key for obj", "gvk", curObj.GetObjectKind().GroupVersionKind())

klog.WarningS(err, "Failed to generate key for obj", "gvk", curObj.GetObjectKind().GroupVersionKind())

pkg/controllers/multiclusterservice/endpointslice_collect_controller.go

pkg/controllers/multiclusterservice/endpointslice_dispatch_controller.go

pkg/controllers/multiclusterservice/mcs_controller.go

codecov-commenter · 2025-07-16T20:35:27Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 6.74157% with 83 lines in your changes missing coverage. Please review.

Project coverage is 45.45%. Comparing base (c60a2e0) to head (05bda3f).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
.../controllers/multiclusterservice/mcs_controller.go	13.15%	33 Missing ⚠️
...clusterservice/endpointslice_collect_controller.go	0.00%	28 Missing ⚠️
...lusterservice/endpointslice_dispatch_controller.go	4.34%	22 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6546      +/-   ##
==========================================
+ Coverage   45.43%   45.45%   +0.01%     
==========================================
  Files         687      687              
  Lines       56334    56337       +3     
==========================================
+ Hits        25594    25606      +12     
+ Misses      29140    29134       -6     
+ Partials     1600     1597       -3

Flag	Coverage Δ
unittests	`45.45% <6.74%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jabellard · 2025-07-16T21:10:56Z

/assign

pkg/controllers/multiclusterservice/endpointslice_collect_controller.go

jabellard

Left a couple comments. Other than that, generally looks good!

jabellard · 2025-07-17T13:21:33Z

/cc @RainbowMango for another look.

jabellard · 2025-07-17T13:21:44Z

/retest

Signed-off-by: Bhaumik Patel <[email protected]> Address comments Signed-off-by: Bhaumik Patel <[email protected]> Address comments

RainbowMango

/assign

RainbowMango · 2025-07-18T01:35:59Z

pkg/controllers/multiclusterservice/endpointslice_collect_controller.go

@@ -84,7 +85,7 @@ const EndpointSliceCollectControllerName = "endpointslice-collect-controller"

 // Reconcile performs a full reconciliation for the object referred to by the Request.
 func (c *EndpointSliceCollectController) Reconcile(ctx context.Context, req controllerruntime.Request) (controllerruntime.Result, error) {
-	klog.V(4).Infof("Reconciling Work %s", req.NamespacedName.String())
+	klog.V(4).InfoS("Reconciling Work", "namespacedName", req.NamespacedName.String(), "namespace", req.Namespace, "name", req.Name)


Suggested change

klog.V(4).InfoS("Reconciling Work", "namespacedName", req.NamespacedName.String(), "namespace", req.Namespace, "name", req.Name)

klog.V(4).InfoS("Reconciling Work", "namespace", req.Namespace, "name", req.Name)

The namespaceName is redundant.

RainbowMango · 2025-07-18T01:41:56Z

pkg/controllers/multiclusterservice/endpointslice_collect_controller.go

@@ -245,7 +245,7 @@
 		curObj := obj.(runtime.Object)
 		key, err := keys.FederatedKeyFunc(clusterName, curObj)
 		if err != nil {
-			klog.Warningf("Failed to generate key for obj: %s", curObj.GetObjectKind().GroupVersionKind())
+			klog.V(4).InfoS("Failed to generate key for obj", "gvk", curObj.GetObjectKind().GroupVersionKind())


@jabellard Do you know why Klog does not provide klog.WarningS?

RainbowMango · 2025-07-18T01:44:15Z

pkg/controllers/multiclusterservice/endpointslice_collect_controller.go

@@ -245,7 +245,7 @@
 		curObj := obj.(runtime.Object)
 		key, err := keys.FederatedKeyFunc(clusterName, curObj)
 		if err != nil {
-			klog.Warningf("Failed to generate key for obj: %s", curObj.GetObjectKind().GroupVersionKind())
+			klog.V(4).InfoS("Failed to generate key for obj", "gvk", curObj.GetObjectKind().GroupVersionKind())


@ Bhaumik10, this log shows an unusual situation; we should log it with a warning or error level.

RainbowMango · 2025-07-18T01:44:28Z

pkg/controllers/multiclusterservice/endpointslice_collect_controller.go

@@ -258,7 +258,7 @@
 		if !reflect.DeepEqual(oldObj, newObj) {
 			key, err := keys.FederatedKeyFunc(clusterName, curObj)
 			if err != nil {
-				klog.Warningf("Failed to generate key for obj: %s", curObj.GetObjectKind().GroupVersionKind())
+				klog.V(4).InfoS("Failed to generate key for obj", "gvk", curObj.GetObjectKind().GroupVersionKind())


pkg/controllers/multiclusterservice/endpointslice_collect_controller.go

RainbowMango · 2025-07-18T01:48:59Z

pkg/controllers/multiclusterservice/endpointslice_dispatch_controller.go

@@ -93,7 +93,7 @@ func (c *EndpointsliceDispatchController) Reconcile(ctx context.Context, req con
 	mcs := &networkingv1alpha1.MultiClusterService{}
 	if err := c.Client.Get(ctx, types.NamespacedName{Namespace: mcsNS, Name: mcsName}, mcs); err != nil {
 		if apierrors.IsNotFound(err) {
-			klog.Warningf("MultiClusterService %s/%s is not found", mcsNS, mcsName)
+			klog.V(4).InfoS("MultiClusterService is not found", "namespace", mcsNS, "name", mcsName)


I guess it's a normal case that when reconciling Work, but found the MultiClusterService is gone.

@XiShanYongYe-Chang could you confirm that?

I will wait for @XiShanYongYe-Chang to respond. For other comments, I have move those from Warning to Error level.

karmada-bot requested review from jwcesign and pigletfly July 16, 2025 20:19

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 16, 2025

Bhaumik10 mentioned this pull request Jul 16, 2025

JSON logging for multiclusterservice controllers #6463

Closed

gemini-code-assist bot reviewed Jul 16, 2025

View reviewed changes

Bhaumik10 force-pushed the mcs-crtl-logs branch from f170cd6 to 228df98 Compare July 16, 2025 20:51

karmada-bot assigned jabellard Jul 16, 2025

jabellard reviewed Jul 17, 2025

View reviewed changes

pkg/controllers/multiclusterservice/endpointslice_collect_controller.go Outdated Show resolved Hide resolved

jabellard reviewed Jul 17, 2025

View reviewed changes

pkg/controllers/multiclusterservice/endpointslice_collect_controller.go Outdated Show resolved Hide resolved

jabellard reviewed Jul 17, 2025

View reviewed changes

pkg/controllers/multiclusterservice/endpointslice_collect_controller.go Outdated Show resolved Hide resolved

jabellard suggested changes Jul 17, 2025

View reviewed changes

JSON logging for multiclusterservice controllers

05bda3f

Signed-off-by: Bhaumik Patel <[email protected]> Address comments Signed-off-by: Bhaumik Patel <[email protected]> Address comments

Bhaumik10 force-pushed the mcs-crtl-logs branch from 228df98 to 05bda3f Compare July 17, 2025 13:38

Bhaumik10 requested a review from jabellard July 17, 2025 14:56

jabellard mentioned this pull request Jul 16, 2025

Structured Logging for Karmada Components #6230

Open

34 tasks

RainbowMango reviewed Jul 18, 2025

View reviewed changes

karmada-bot assigned RainbowMango Jul 18, 2025

RainbowMango reviewed Jul 18, 2025

View reviewed changes

	klog.V(4).InfoS("Failed to generate key for obj", "gvk", curObj.GetObjectKind().GroupVersionKind())
	klog.WarningS(err, "Failed to generate key for obj", "gvk", curObj.GetObjectKind().GroupVersionKind())

	klog.V(4).InfoS("Reconciling Work", "namespacedName", req.NamespacedName.String(), "namespace", req.Namespace, "name", req.Name)
	klog.V(4).InfoS("Reconciling Work", "namespace", req.Namespace, "name", req.Name)

JSON logging for multiclusterservice controllers #6546

Are you sure you want to change the base?

JSON logging for multiclusterservice controllers #6546

Uh oh!

Conversation

Bhaumik10 commented Jul 16, 2025

Uh oh!

karmada-bot commented Jul 16, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist bot Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jabellard commented Jul 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jabellard left a comment

Choose a reason for hiding this comment

Uh oh!

jabellard commented Jul 17, 2025

Uh oh!

jabellard commented Jul 17, 2025

Uh oh!

RainbowMango left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-commenter commented Jul 16, 2025 •

edited

Loading