Add instrumentation for contextual logging to include service provider keys - #7412
Add instrumentation for contextual logging to include service provider keys#7412deads2k wants to merge 2 commits into
Conversation
WalkthroughAdds annotation-driven logging context across multiple controllers and webhooks by extracting service-provider annotations and injecting them into controller loggers and namespaces; introduces new support/logcontext utilities to build and propagate this annotation-based context. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/hypershift-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (1)
337-346: Consider validating that the extracted log key is not empty.After removing the prefix from the annotation key, if the result is an empty string (e.g., if someone sets an annotation with key exactly
"context.serviceprovider.hypershift.openshift.io/"), you would add an empty key to the logger. While this is an unlikely edge case, adding a defensive check would improve robustness.🔎 Proposed defensive check
func addAnnotationContext(log logr.Logger, annotations map[string]string) logr.Logger { for k, v := range annotations { if !strings.HasPrefix(k, "context.serviceprovider.hypershift.openshift.io/") { continue } logKey, _ := strings.CutPrefix(k, "context.serviceprovider.hypershift.openshift.io/") - + if logKey == "" { + continue + } log = log.WithValues(logKey, v) } return log }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
🔇 Additional comments (1)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (1)
364-364: LGTM! Good placement for contextual logging enrichment.The logger is enriched with service provider context early in the reconciliation flow, which will ensure all subsequent log entries include this context for better correlation and debugging across teams.
| // This is useful when the service provider has keys like resourceGroupName, resourceName, hcpClusterName, clusterServiceID | ||
| // and wants to be able to select all log lines that contain those keys. | ||
| // We use annotations because they can hold more values and are applicable to all resource types. | ||
| func addAnnotationContext(log logr.Logger, annotations map[string]string) logr.Logger { |
There was a problem hiding this comment.
Per Cesar, move to support/logcontext
…r keys We will use this to correlate our logs and have more efficient debugging capability between teams
Adding annotation from the service provider to the namespaces created for particular requested clusters makes auto-labelling easier for various aggregation concerns.
53153ca to
5a8335f
Compare
openshift-ci
Bot
added
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: deads2k The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
support/logcontext/service_provider_logging.go (1)
28-38: Consider adding a defensive nil check for targetAnnotations.The function mutates
targetAnnotationsin-place (line 36), which will panic iftargetAnnotationsis nil. The current caller properly initializes the map before calling (hostedcluster_controller.go:1310-1311), so there's no immediate issue. However, adding a guard would make the function more robust for future callers.🔎 Optional defensive check
func AddServiceProviderAnnotations(targetAnnotations map[string]string, hostedClusterAnnotations map[string]string) { + if targetAnnotations == nil { + return + } for k, v := range hostedClusterAnnotations { if !strings.HasPrefix(k, "context.serviceprovider.hypershift.openshift.io/") { continue } if _, exists := targetAnnotations[k]; exists { continue } targetAnnotations[k] = v } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (10)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go(4 hunks)hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go(2 hunks)hypershift-operator/controllers/hostedclustersizing/hostedclustersizing_controller.go(2 hunks)hypershift-operator/controllers/nodepool/nodepool_controller.go(2 hunks)hypershift-operator/controllers/platform/aws/controller.go(2 hunks)hypershift-operator/controllers/resourcebasedcpautoscaler/controller.go(2 hunks)hypershift-operator/controllers/scheduler/aws/autoscaler.go(2 hunks)hypershift-operator/controllers/scheduler/aws/dedicated_request_serving_nodes.go(3 hunks)hypershift-operator/controllers/scheduler/azure/controller.go(2 hunks)support/logcontext/service_provider_logging.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
hypershift-operator/controllers/hostedclustersizing/hostedclustersizing_controller.gohypershift-operator/controllers/scheduler/azure/controller.gohypershift-operator/controllers/scheduler/aws/dedicated_request_serving_nodes.gohypershift-operator/controllers/nodepool/nodepool_controller.gosupport/logcontext/service_provider_logging.gohypershift-operator/controllers/scheduler/aws/autoscaler.gohypershift-operator/controllers/resourcebasedcpautoscaler/controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/platform/aws/controller.go
🧬 Code graph analysis (9)
hypershift-operator/controllers/hostedclustersizing/hostedclustersizing_controller.go (1)
support/logcontext/service_provider_logging.go (1)
AddAnnotationContext(13-23)
hypershift-operator/controllers/scheduler/azure/controller.go (1)
support/logcontext/service_provider_logging.go (1)
AddAnnotationContext(13-23)
hypershift-operator/controllers/scheduler/aws/dedicated_request_serving_nodes.go (1)
support/logcontext/service_provider_logging.go (1)
AddAnnotationContext(13-23)
hypershift-operator/controllers/nodepool/nodepool_controller.go (1)
support/logcontext/service_provider_logging.go (1)
AddAnnotationContext(13-23)
hypershift-operator/controllers/scheduler/aws/autoscaler.go (1)
support/logcontext/service_provider_logging.go (1)
AddAnnotationContext(13-23)
hypershift-operator/controllers/resourcebasedcpautoscaler/controller.go (1)
support/logcontext/service_provider_logging.go (1)
AddAnnotationContext(13-23)
hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (1)
support/logcontext/service_provider_logging.go (2)
AddAnnotationContext(13-23)AddServiceProviderAnnotations(28-38)
hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go (1)
support/logcontext/service_provider_logging.go (1)
AddAnnotationContext(13-23)
hypershift-operator/controllers/platform/aws/controller.go (1)
support/logcontext/service_provider_logging.go (1)
AddAnnotationContext(13-23)
🔇 Additional comments (13)
hypershift-operator/controllers/scheduler/aws/autoscaler.go (1)
172-173: LGTM!The logging context enrichment is correctly implemented. The logger is augmented with HostedCluster annotations and properly re-injected into the context for downstream use, enabling better log correlation across teams.
hypershift-operator/controllers/hostedclustersizing/hostedclustersizing_controller.go (1)
138-139: LGTM!The annotation-based logging context is properly set up after fetching the HostedCluster, ensuring enriched log output for the reconciliation flow.
hypershift-operator/controllers/resourcebasedcpautoscaler/controller.go (1)
123-125: LGTM!The logging context is correctly enriched with HostedCluster annotations early in the reconciliation flow, providing useful context even during deletion scenarios.
hypershift-operator/controllers/nodepool/nodepool_controller.go (1)
222-223: LGTM!The logging context enrichment is properly implemented after retrieving the HostedCluster, ensuring annotation-based context is available throughout the NodePool reconciliation.
hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go (1)
38-41: LGTM!The logging context enrichment is well-implemented with a clear explanatory comment. This ensures annotation-based context propagates to any code retrieving the logger from the context during defaulting operations.
hypershift-operator/controllers/platform/aws/controller.go (1)
260-261: LGTM!The logging context is correctly augmented with HostedCluster annotations after fetching the cluster, providing enriched context for AWS endpoint service reconciliation.
hypershift-operator/controllers/scheduler/azure/controller.go (1)
69-70: LGTM!The annotation-based logging context is properly established after fetching the HostedCluster, ensuring enriched log output for Azure scheduling operations.
hypershift-operator/controllers/scheduler/aws/dedicated_request_serving_nodes.go (2)
155-156: LGTM!The logging context enrichment in
DedicatedServingComponentScheduler.Reconcileis correctly implemented, providing annotation-based context for dedicated serving component scheduling operations.
434-435: LGTM!The logging context enrichment in
DedicatedServingComponentSchedulerAndSizer.Reconcilefollows the same correct pattern, ensuring consistent annotation-based logging context across both scheduler implementations.hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go (3)
61-61: LGTM!The new import is required for the annotation-based logging context functionality introduced in this PR.
348-349: LGTM!The logger enrichment pattern is correct:
- Extracts service-provider annotations from HostedCluster
- Adds them as log context via
AddAnnotationContext- Rebinds the enriched logger into the context for downstream reconciliation steps
This ensures log correlation across teams as intended by the PR objectives.
1310-1314: LGTM!The namespace annotation propagation correctly:
- Initializes the annotations map if needed (defensive nil check)
- Propagates service-provider annotations from HostedCluster to the HostedControlPlane namespace
- Enables consistent log context across namespaces for correlation
This aligns with the PR objective of enabling log correlation between teams.
support/logcontext/service_provider_logging.go (1)
13-23: LGTM!The function correctly:
- Filters annotations by the service-provider prefix
- Strips the prefix to create clean log keys
- Enriches the logger with key-value pairs using
WithValues- Returns the augmented logger for caller rebinding
Handles nil annotations safely (range over nil map is a no-op in Go).
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3 similar comments
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@deads2k: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
|
Stale PRs rot after 14d of inactivity. Mark the PR as fresh by commenting If this PR is safe to close now please do so with /lifecycle rotten |
openshift-ci
Bot
added
lifecycle/rotten
|
Now I have the complete picture across all four jobs. Let me compile the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryAll four failing CI jobs share the same root cause: PR #7412's branch is severely out of date with the Root CausePrimary cause: Stale PR branch with merge conflicts (all jobs) PR #7412 modifies 10 files across multiple HyperShift controllers to add contextual logging instrumentation via a new The PR touches controller
The conflict count has grown over time:
Secondary cause for e2e-aks (transient infrastructure flake) The e2e-aks job artifacts were garbage-collected, but the PR discussion indicates test Tide error is derivative: Tide cannot merge the PR because required status checks are failing. Recommendations
Evidence
|
|
Rotten PRs close after 7d of inactivity. Reopen the PR by commenting /close |
|
@openshift-ci[bot]: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3 participants
We will use this to correlate our logs and have more efficient debugging capability between teams
/assign @sjenning
if this is agreeable, I'll search out all controllers and update them. There will likely be a followup later to log using JSON so we can more easily extract the keys.