Skip to content

🌱 Use contextual logger in base controller - #165

Merged
openshift-merge-bot[bot] merged 1 commit into
open-cluster-management-io:mainfrom
qiujian16:controller-logger
Nov 10, 2025
Merged

🌱 Use contextual logger in base controller#165
openshift-merge-bot[bot] merged 1 commit into
open-cluster-management-io:mainfrom
qiujian16:controller-logger

Conversation

@qiujian16

@qiujian16 qiujian16 commented Nov 10, 2025

Copy link
Copy Markdown
Member

Summary

Related issue(s)

Fixes #

Summary by CodeRabbit

  • Refactor
    • Improved observability by implementing context-aware, structured logging across controller initialization, worker management, and shutdown phases.
    • Enhanced error handling with contextual information, providing richer details for debugging and troubleshooting controller issues.
    • Refined logging consistency for lifecycle events and worker operations.

Signed-off-by: Jian Qiu <jqiu@redhat.com>
@openshift-ci
openshift-ci Bot requested a review from deads2k November 10, 2025 05:49
@openshift-ci

openshift-ci Bot commented Nov 10, 2025

Copy link
Copy Markdown

[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

Details Needs approval from an approver in each of these files:

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

@coderabbitai

coderabbitai Bot commented Nov 10, 2025

Copy link
Copy Markdown

Walkthrough

The PR refactors logging throughout the base controller to be context-aware, replacing direct klog calls with context-bound logger calls. It updates error handling to use utilruntime.HandleErrorWithContext with contextual information and modifies queue cancellation to derive from parent context instead of a TODO context.

Changes

Cohort / File(s) Summary
Context-aware logging refactor
pkg/basecontroller/factory/base_controller.go
Introduces ctx parameter to waitForNamedCacheSync, derives logger from context in Run function, and replaces klog.Info calls with context-bound logger.Info throughout. Updates worker lifecycle event logging and shutdown messaging to use structured, context-aware logging. Modifies queue cancellation to use parent context.
Error handling enhancement
pkg/basecontroller/factory/base_controller.go
Updates processNextWorkItem to initialize logger from queue context and refactors error reporting to use utilruntime.HandleErrorWithContext with richer contextual information (queue item key). Changes error messages to reflect reconciliation context.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Extra attention areas:
    • Verify context propagation flow through waitForNamedCacheSync and worker lifecycle methods
    • Confirm that utilruntime.HandleErrorWithContext maintains backward compatibility with existing error handling behavior
    • Review the conditional logging logic in error paths to ensure no error reporting is skipped unintentionally
    • Check that logger initialization from context is consistent across all affected functions

Suggested labels

lgtm

Suggested reviewers

  • deads2k

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is entirely empty beyond the template boilerplate, with no actual summary or issue references provided by the author. Add a substantive summary describing the changes made, the rationale for using contextual loggers, and reference any related issues by filling in the template sections.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding contextual logger usage to the base controller, which aligns with the detailed summary showing comprehensive logger refactoring throughout the codebase.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@skeeey

skeeey commented Nov 10, 2025

Copy link
Copy Markdown
Member

/lgtm

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
pkg/basecontroller/factory/base_controller.go (1)

83-84: Clarify worker log field names

numberOfWorkers suggests the total worker count, yet the value here is the loop index (1, 2, …). That makes the structured log misleading when inspecting metrics. Rename the field to reflect it’s an identifier (and optionally log the total worker count) to keep the log semantics straight.

Apply this diff:

-		logger.Info("Starting worker of controller ...", "numberOfWorkers", i)
+		logger.Info("Starting worker of controller ...", "workerID", i, "workerCount", workers)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e1b074a and 83b3dec.

📒 Files selected for processing (1)
  • pkg/basecontroller/factory/base_controller.go (6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/basecontroller/factory/base_controller.go (1)
vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go (1)
  • HandleErrorWithContext (202-204)
⏰ 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

@openshift-merge-bot
openshift-merge-bot Bot merged commit 105041a into open-cluster-management-io:main Nov 10, 2025
12 checks passed
@qiujian16
qiujian16 deleted the controller-logger branch November 10, 2025 06:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants