🌱 Use contextual logging for cloudevents - #164
Conversation
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Warning Rate limit exceeded@qiujian16 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (29)
WalkthroughAdds context.Context propagation and replaces global/unstructured logging with context-derived structured klog loggers across CloudEvents clients, stores, work processors, protocol options, cert rotation, heartbeat, MQTT logging, and tests. Several Handler/HandleReceivedResource signatures and worker-run signatures were extended to accept context.Context. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cloudevents/clients/work/store/local.go (1)
165-233: processLoop ignores ctx cancellation
processLoopnow receives acontext.Context, but the loop never observesctx.Done(). Once the goroutine is insideeventQueue.Pop, cancellation does not unblock it, so the store leaks the goroutine and never shuts down cleanly. You need to break out when the context is canceled, e.g. close the FIFO as soon as<-ctx.Done()fires soPopreturnscache.ErrFIFOClosed.func (s *SourceLocalWatcherStore) processLoop(ctx context.Context) { logger := klog.FromContext(ctx) + + go func() { + <-ctx.Done() + s.eventQueue.Close() + }() for { // this will be blocked until the event queue has events
🧹 Nitpick comments (1)
pkg/cloudevents/generic/options/cert/rotation.go (1)
63-69: Inject a real logger intoctxbefore callingrun.
Right nowStartClientCertRotatingstill invokesrun(context.Background()), soklog.FromContext(ctx)just falls back to the TODO logger and we miss out on any request/controller fields we hoped to propagate. Please either let the caller hand in its context or wrap the background context withklog.NewContext(..., logger.WithValues(...))so the worker actually emits contextual logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
pkg/cloudevents/clients/options/generic.go(6 hunks)pkg/cloudevents/clients/store/informer.go(3 hunks)pkg/cloudevents/clients/store/informer_test.go(2 hunks)pkg/cloudevents/clients/store/interface.go(3 hunks)pkg/cloudevents/clients/store/simplestore.go(3 hunks)pkg/cloudevents/clients/store/simplestore_test.go(2 hunks)pkg/cloudevents/clients/work/agent/client/manifestwork.go(4 hunks)pkg/cloudevents/clients/work/source/client/manifestwork.go(3 hunks)pkg/cloudevents/clients/work/store/base.go(8 hunks)pkg/cloudevents/clients/work/store/informer.go(3 hunks)pkg/cloudevents/clients/work/store/local.go(5 hunks)pkg/cloudevents/generic/clients/agentclient.go(4 hunks)pkg/cloudevents/generic/clients/agentclient_test.go(1 hunks)pkg/cloudevents/generic/clients/baseclient.go(3 hunks)pkg/cloudevents/generic/clients/clients_metrics_test.go(1 hunks)pkg/cloudevents/generic/clients/sourceclient.go(5 hunks)pkg/cloudevents/generic/clients/sourceclient_test.go(1 hunks)pkg/cloudevents/generic/interface.go(1 hunks)pkg/cloudevents/generic/options/cert/rotation.go(4 hunks)pkg/cloudevents/generic/options/grpc/agentoptions.go(2 hunks)pkg/cloudevents/generic/options/grpc/options.go(2 hunks)pkg/cloudevents/generic/options/grpc/protocol/protocol.go(5 hunks)pkg/cloudevents/generic/options/grpc/sourceoptions.go(2 hunks)pkg/cloudevents/generic/options/mqtt/agentoptions.go(2 hunks)pkg/cloudevents/generic/options/mqtt/logger.go(1 hunks)pkg/cloudevents/generic/options/mqtt/options.go(3 hunks)pkg/cloudevents/server/grpc/heartbeat/healthcheck.go(4 hunks)pkg/cloudevents/server/grpc/heartbeat/heartbeat.go(3 hunks)test/integration/cloudevents/source/client.go(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-12T02:55:39.701Z
Learnt from: qiujian16
Repo: open-cluster-management-io/sdk-go PR: 140
File: pkg/cloudevents/generic/options/grpc/protocol/protocol.go:36-39
Timestamp: 2025-09-12T02:55:39.701Z
Learning: In the GRPC protocol health check implementation, WithReconnectErrorOption already validates the interval parameter and sets a default of 20 seconds if interval <= 0, preventing time.NewTicker panics.
Applied to files:
pkg/cloudevents/generic/options/grpc/options.gopkg/cloudevents/server/grpc/heartbeat/healthcheck.go
📚 Learning: 2025-09-16T02:22:20.929Z
Learnt from: skeeey
Repo: open-cluster-management-io/sdk-go PR: 144
File: pkg/cloudevents/generic/options/grpc/protocol/protocol.go:200-213
Timestamp: 2025-09-16T02:22:20.929Z
Learning: In the GRPC CloudEvents protocol implementation, when startEventsReceiver encounters a stream error, it sends the error to reconnectErrorChan. The consumer of this channel handles the error by calling Close() on the protocol, which triggers close(p.closeChan), causing OpenInbound to unblock and call cancel() to properly terminate both the events receiver and heartbeat watcher goroutines.
Applied to files:
pkg/cloudevents/generic/options/grpc/options.gopkg/cloudevents/generic/options/grpc/protocol/protocol.gopkg/cloudevents/clients/options/generic.gopkg/cloudevents/generic/clients/baseclient.go
📚 Learning: 2025-09-17T13:29:00.675Z
Learnt from: qiujian16
Repo: open-cluster-management-io/sdk-go PR: 147
File: pkg/cloudevents/generic/options/grpc/protocol/heartbeat_integration_test.go:66-68
Timestamp: 2025-09-17T13:29:00.675Z
Learning: In the pkg/cloudevents/generic/options/grpc/protocol package, bufSize is already defined as a constant in protocal_test.go and is accessible to other test files in the same package, including heartbeat_integration_test.go.
Applied to files:
pkg/cloudevents/generic/options/grpc/protocol/protocol.go
📚 Learning: 2025-09-16T06:03:45.232Z
Learnt from: skeeey
Repo: open-cluster-management-io/sdk-go PR: 144
File: pkg/cloudevents/server/grpc/broker.go:169-194
Timestamp: 2025-09-16T06:03:45.232Z
Learning: In the Open Cluster Management gRPC CloudEvents implementation, client reconnections due to heartbeat timeout are considered acceptable behavior by the maintainer skeeey, rather than something that needs optimization to prevent.
Applied to files:
pkg/cloudevents/clients/options/generic.gopkg/cloudevents/generic/clients/baseclient.go
🧬 Code graph analysis (16)
pkg/cloudevents/generic/interface.go (1)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)
pkg/cloudevents/generic/clients/agentclient.go (2)
pkg/cloudevents/generic/types/types.go (6)
ParseCloudEventsType(209-230)CloudEventsDataType(165-169)ResyncRequestAction(41-41)SubResourceStatus(33-33)SubResourceSpec(30-30)ExtensionClusterName(74-74)vendor/github.com/cloudevents/sdk-go/v2/types/value.go (1)
ToString(230-241)
pkg/cloudevents/clients/store/informer.go (2)
pkg/cloudevents/clients/work/store/informer.go (1)
AgentInformerWatcherStore(89-91)pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)
pkg/cloudevents/generic/options/mqtt/options.go (2)
vendor/github.com/cloudevents/sdk-go/protocol/mqtt_paho/v2/option.go (2)
WithDebugLogger(53-58)WithErrorLogger(62-67)pkg/cloudevents/generic/options/mqtt/logger.go (2)
PahoDebugLogger(13-15)PahoErrorLogger(9-11)
pkg/cloudevents/clients/store/simplestore.go (1)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)
pkg/cloudevents/generic/clients/clients_metrics_test.go (2)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)pkg/cloudevents/generic/testing/resource.go (1)
MockResource(22-29)
pkg/cloudevents/clients/work/store/base.go (2)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)vendor/open-cluster-management.io/api/work/v1/types.go (1)
ManifestWork(18-28)
pkg/cloudevents/clients/store/informer_test.go (2)
pkg/cloudevents/generic/types/types.go (3)
Added(91-91)Modified(94-94)Deleted(100-100)vendor/open-cluster-management.io/api/cluster/v1/types.go (1)
ManagedCluster(34-44)
pkg/cloudevents/generic/clients/agentclient_test.go (2)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)pkg/cloudevents/generic/testing/resource.go (1)
MockResource(22-29)
test/integration/cloudevents/source/client.go (2)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)test/integration/cloudevents/store/resource.go (1)
Resource(19-27)
pkg/cloudevents/generic/clients/sourceclient_test.go (2)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)pkg/cloudevents/generic/testing/resource.go (1)
MockResource(22-29)
pkg/cloudevents/clients/options/generic.go (1)
pkg/cloudevents/generic/types/types.go (2)
SourceAll(22-22)ClusterAll(18-18)
pkg/cloudevents/clients/work/store/local.go (1)
vendor/open-cluster-management.io/api/work/v1/types.go (1)
ManifestWork(18-28)
pkg/cloudevents/clients/work/store/informer.go (3)
pkg/cloudevents/clients/store/informer.go (1)
AgentInformerWatcherStore(25-30)pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)vendor/open-cluster-management.io/api/work/v1/types.go (1)
ManifestWork(18-28)
pkg/cloudevents/generic/clients/sourceclient.go (2)
pkg/cloudevents/generic/types/types.go (5)
ParseCloudEventsType(209-230)ExtensionClusterName(74-74)CloudEventsDataType(165-169)SubResourceStatus(33-33)StatusModified(97-97)pkg/cloudevents/generic/metrics/metrics_collector.go (1)
UpdateResourceSpecResyncDurationMetric(323-331)
pkg/cloudevents/generic/clients/baseclient.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: unit
- GitHub Check: integration
- GitHub Check: verify
🔇 Additional comments (44)
pkg/cloudevents/server/grpc/heartbeat/heartbeat.go (1)
6-6: LGTM! Clean migration to klog contextual logging.The migration from
cecontext.LoggerFrom(ctx)toklog.FromContext(ctx)aligns with the PR objectives and follows Kubernetes logging best practices for context-aware structured logging.Also applies to: 26-26
pkg/cloudevents/server/grpc/heartbeat/healthcheck.go (2)
6-6: LGTM: Import updated for klog migration.The import change correctly switches from the CloudEvents SDK logger to klog/v2 for structured, contextual logging.
48-57: LGTM: Contextual logging implemented correctly.The logger is properly initialized from context, and the structured logging with
V(4)verbosity is appropriate for debug-level heartbeat messages.pkg/cloudevents/clients/work/source/client/manifestwork.go (2)
188-189: LGTM: Clean contextual logging implementation.The transition to context-aware logging is correctly implemented with proper structured fields.
207-208: LGTM: Clean contextual logging implementation.The List method correctly uses context-aware structured logging.
pkg/cloudevents/generic/options/grpc/agentoptions.go (1)
38-61: LGTM: Proper context-aware error logging.The refactor correctly replaces global
klog.Errorfwith context-derived structured logging.pkg/cloudevents/generic/options/grpc/sourceoptions.go (1)
37-56: LGTM: Consistent context-aware error logging.Matches the pattern in agentoptions.go - correctly uses context-derived logger.
pkg/cloudevents/generic/clients/clients_metrics_test.go (1)
102-105: LGTM: Test updated for new handler signature.Correctly reflects the API change to include
context.Contextin the Subscribe callback.pkg/cloudevents/clients/options/generic.go (3)
102-102: LGTM: Context-aware logging initialization.Properly initializes the logger from context for use throughout the method.
145-166: LGTM: Consistent structured logging in reconnect handler.All resync-related logging correctly uses the context-derived logger with structured fields.
175-238: LGTM: Consistent structured logging in SourceClient.Mirrors the AgentClient pattern - correctly uses context-aware logging throughout.
pkg/cloudevents/generic/clients/baseclient.go (3)
83-83: LGTM: Context-aware error handling.Correctly propagates context to error handling.
106-113: LGTM: Consistent context-aware error handling.Both error cases properly use
HandleErrorWithContext.
208-208: LGTM: Context-aware error handling with structured fields.Properly uses context and includes the signal value as a structured field.
pkg/cloudevents/clients/work/agent/client/manifestwork.go (3)
79-81: LGTM: Proper structured logging with explicit fields.The Get method correctly uses context-aware logging with structured fields.
118-119: LGTM: Clean structured logging in Watch.Correctly implements context-aware logging with structured fields.
132-133: LGTM: Clean structured logging in Patch.Properly uses context-derived logger with explicit fields for namespace and name.
pkg/cloudevents/generic/options/mqtt/logger.go (1)
9-34: Logger field is properly initialized in actual usage—no action required.The script results show that while type assertions at lines 17-18 use empty initialization (which is expected for compile-time interface verification), the actual instantiations in
pkg/cloudevents/generic/options/mqtt/options.goat lines 238-239 correctly initialize the logger field:cloudeventsmqtt.WithDebugLogger(&PahoDebugLogger{logger: logger}), cloudeventsmqtt.WithErrorLogger(&PahoErrorLogger{logger: logger}),The logger parameter is properly passed and assigned. No nil pointer dereferences will occur.
Likely an incorrect or invalid review comment.
pkg/cloudevents/generic/options/mqtt/agentoptions.go (2)
43-44: LGTM! Contextual logging correctly implemented.The logger is properly derived from the context, aligning with the PR's objective to introduce context-aware logging throughout the codebase.
67-67: Verify the log level change from Warning to Info.The log level was changed from
klog.Warningftologger.Info. While the contextual logging pattern is correct, changing from Warning to Info may affect observability of this fallback scenario. Please confirm this log level change is intentional.pkg/cloudevents/generic/options/grpc/options.go (1)
263-264: LGTM! Contextual logging correctly implemented.The logger is properly derived from the context and used consistently throughout the connection state monitoring goroutine. The log levels remain unchanged (Info and Error), maintaining the existing observability characteristics.
Also applies to: 271-271, 289-289
pkg/cloudevents/generic/clients/sourceclient_test.go (1)
454-454: LGTM! Test correctly updated for new API signature.The callback signature has been properly updated to include the context parameter, aligning with the ResourceHandler API change. Using the blank identifier for the unused context is appropriate for this test.
test/integration/cloudevents/source/client.go (1)
40-40: LGTM! Integration test correctly updated for new API signature.The Subscribe callback signature has been properly updated to include the context parameter, aligning with the ResourceHandler API change.
pkg/cloudevents/generic/options/mqtt/options.go (1)
223-224: LGTM! Contextual logging correctly wired into MQTT protocol loggers.The logger is properly derived from the context and passed to both PahoDebugLogger and PahoErrorLogger, ensuring that MQTT protocol-level logging benefits from contextual logging throughout the stack.
Also applies to: 238-239
pkg/cloudevents/clients/store/informer_test.go (1)
192-204: LGTM! Test correctly updated for new API signature.All HandleReceivedResource invocations have been properly updated to include the context parameter as the first argument, aligning with the API change.
pkg/cloudevents/generic/interface.go (1)
15-15: LGTM! API change enables contextual logging and follows Go best practices.The addition of
context.Contextas the first parameter to ResourceHandler is a breaking change but follows Go conventions and enables:
- Context-aware logging with structured fields
- Proper cancellation and timeout propagation
- Request-scoped values for tracing and debugging
All call sites in the codebase have been consistently updated to match this new signature.
pkg/cloudevents/clients/store/simplestore_test.go (1)
4-4: LGTM! Test correctly updated for new API signature.The test properly imports the context package and passes
context.Background()to HandleReceivedResource, aligning with the API change. Using a background context is appropriate for this unit test.Also applies to: 112-112
pkg/cloudevents/generic/clients/agentclient_test.go (1)
542-546: LGTM! Test adapted to new handler signature.The callback signature correctly includes the context parameter to match the updated interface, even though the test doesn't currently use it.
pkg/cloudevents/clients/store/interface.go (2)
37-37: LGTM! Public API updated for context propagation.The interface method signature correctly adds
context.Contextas the first parameter, enabling context-aware logging throughout the resource handling pipeline.
65-86: LGTM! Context-aware logging properly implemented.The function correctly derives a logger from the context and uses structured logging for the error case.
pkg/cloudevents/clients/store/simplestore.go (1)
54-83: LGTM! Context-aware logging properly implemented.The method signature correctly accepts
context.Contextas the first parameter, and the logger is properly derived from the context. The deletion guard now uses structured logging with descriptive field names.Note: The log level changed from Warning to Info for the deletion guard. This seems appropriate as it's informational rather than a problem condition.
pkg/cloudevents/clients/store/informer.go (1)
54-83: LGTM! Consistent context-aware logging implementation.The implementation mirrors the approach in
simplestore.go, properly deriving the logger from context and using structured fields for logging. The deletion guard message is clear and appropriately logged at Info level.pkg/cloudevents/generic/options/grpc/protocol/protocol.go (4)
7-7: LGTM! Logging dependency updated.The import correctly switches from CloudEvents context-based logging to
k8s.io/klog/v2for structured logging.
92-93: LGTM! Structured logging for event publishing.The logging correctly uses the context-derived logger with a descriptive field name for the message ID.
115-129: LGTM! Comprehensive subscription logging.The logging provides clear structured context for both source-based and cluster-based subscriptions, with appropriate field names for filtering and debugging.
149-149: LGTM! Connection closure logging.Simple and clear logging for the connection closure event.
pkg/cloudevents/clients/work/store/informer.go (2)
46-46: LGTM! Context propagation to worker processor.The worker processor now receives the context, enabling proper cancellation and context-aware logging throughout the processing pipeline.
104-122: LGTM! Context-aware resource handling.The method signature correctly accepts context as the first parameter, and the deletion guard uses structured logging with appropriate field names for ManifestWork resources.
pkg/cloudevents/generic/clients/sourceclient.go (3)
168-197: LGTM! Comprehensive context-aware logging for event handling.The logging statements properly use structured fields and appropriate log levels (Info for ignorable events, Error for failures).
206-209: LGTM! Context propagated to resource handler.The handler invocation correctly passes the context as the first parameter, enabling context-aware operations throughout the handler chain.
223-224: LGTM! Context-aware logging in resync response.The logger is properly initialized with the eventDataType for contextual logging throughout the resync operation.
pkg/cloudevents/generic/clients/agentclient.go (3)
163-199: LGTM! Comprehensive context-aware logging.The logging statements properly use structured fields with appropriate keys (eventClusterName, agentClusterName, eventDataType) and levels (Info for ignorable events, Error for failures).
218-221: LGTM! Context propagated to resource handler.The handler correctly receives the context as the first parameter, enabling context-aware operations.
233-267: LGTM! Structured logging in resync response.The logger is properly initialized with eventDataType, and the resource not found message includes the uid field for debugging.
| case h.output <- heartbeat: | ||
| default: | ||
| logger.Warn("send channel is full, dropping heartbeat") | ||
| logger.Info("send channel is full, dropping heartbeat") |
There was a problem hiding this comment.
Reconsider downgrading log level from Warn to Info.
Dropping heartbeats because the send channel is full typically indicates backpressure or that the consumer isn't keeping up, which may signal an operational issue. Downgrading from Warn to Info reduces the visibility of this condition and could cause operators to miss early warning signs of system degradation in production environments where Info logs might be filtered or less prominent.
Consider keeping this at Warning level or provide justification for the downgrade.
🤖 Prompt for AI Agents
In pkg/cloudevents/server/grpc/heartbeat/heartbeat.go around line 43, the log
that reports "send channel is full, dropping heartbeat" was downgraded to Info
but should remain or be escalated to Warn to surface backpressure/consumer lag;
change the logger call back to logger.Warn (or logger.Warnf) with the same
message and consider including contextual fields (e.g., channel name, heartbeat
id or queue length) to aid operational troubleshooting.
52781b7 to
f302cbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/cloudevents/clients/work/source/client/manifestwork.go (1)
238-239: Structured log message shouldn't use printf placeholderPlease drop the
%s; structured logging treats subsequent args as key/value pairs, so the placeholder will never be substituted. Revert to a plain message string.Apply this diff:
- logger.V(4).Info("patching manifestwork %s", "manifestworkName", name) + logger.V(4).Info("patching manifestwork", "manifestworkName", name)pkg/cloudevents/generic/clients/sourceclient.go (1)
150-157: Critical: WithValues result not assigned (unresolved from previous review).This issue was previously flagged but remains unresolved. At line 157,
logger.WithValues("eventType", evt.Type())returns a new logger instance, but the result is not assigned back to theloggervariable. This means theeventTypefield will not be included in any subsequent log statements within this function.Apply this fix:
- logger.WithValues("eventType", evt.Type()) + logger = logger.WithValues("eventType", evt.Type())
🧹 Nitpick comments (2)
pkg/cloudevents/generic/options/mqtt/logger.go (1)
21-21: Simplify error formatting.The error message construction uses awkward wording and redundant formatting. The phrase "get err" is non-idiomatic, and wrapping
fmt.Sprint(v...)insidefmt.Errorf("get err %s", ...)is unnecessarily complex.Apply this diff to simplify:
- l.logger.Error(fmt.Errorf("get err %s", fmt.Sprint(v...)), "MQTT error message") + l.logger.Error(fmt.Errorf("%s", fmt.Sprint(v...)), "MQTT error message")Or, even simpler, create the error directly from the Sprint result:
- l.logger.Error(fmt.Errorf("get err %s", fmt.Sprint(v...)), "MQTT error message") + l.logger.Error(fmt.Errorf(fmt.Sprint(v...)), "MQTT error message")pkg/cloudevents/clients/work/store/base.go (1)
96-96: Fix structured logging key typoLine 96 currently records
"manifestsworkName"(extra “s”). Please rename the key to"manifestworkName"so downstream log consumers don’t end up with fragmented field names.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
pkg/cloudevents/clients/options/generic.go(6 hunks)pkg/cloudevents/clients/store/informer.go(3 hunks)pkg/cloudevents/clients/store/informer_test.go(2 hunks)pkg/cloudevents/clients/store/interface.go(3 hunks)pkg/cloudevents/clients/store/simplestore.go(3 hunks)pkg/cloudevents/clients/store/simplestore_test.go(2 hunks)pkg/cloudevents/clients/work/agent/client/manifestwork.go(4 hunks)pkg/cloudevents/clients/work/source/client/manifestwork.go(3 hunks)pkg/cloudevents/clients/work/store/base.go(8 hunks)pkg/cloudevents/clients/work/store/informer.go(3 hunks)pkg/cloudevents/clients/work/store/local.go(5 hunks)pkg/cloudevents/generic/clients/agentclient.go(4 hunks)pkg/cloudevents/generic/clients/agentclient_test.go(1 hunks)pkg/cloudevents/generic/clients/baseclient.go(3 hunks)pkg/cloudevents/generic/clients/clients_metrics_test.go(1 hunks)pkg/cloudevents/generic/clients/sourceclient.go(5 hunks)pkg/cloudevents/generic/clients/sourceclient_test.go(1 hunks)pkg/cloudevents/generic/interface.go(1 hunks)pkg/cloudevents/generic/options/cert/rotation.go(4 hunks)pkg/cloudevents/generic/options/grpc/agentoptions.go(2 hunks)pkg/cloudevents/generic/options/grpc/options.go(2 hunks)pkg/cloudevents/generic/options/grpc/protocol/protocol.go(5 hunks)pkg/cloudevents/generic/options/grpc/sourceoptions.go(2 hunks)pkg/cloudevents/generic/options/mqtt/agentoptions.go(2 hunks)pkg/cloudevents/generic/options/mqtt/logger.go(1 hunks)pkg/cloudevents/generic/options/mqtt/options.go(3 hunks)pkg/cloudevents/server/grpc/heartbeat/healthcheck.go(4 hunks)pkg/cloudevents/server/grpc/heartbeat/heartbeat.go(3 hunks)test/integration/cloudevents/source/client.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- pkg/cloudevents/generic/options/grpc/sourceoptions.go
- pkg/cloudevents/generic/options/mqtt/agentoptions.go
- pkg/cloudevents/server/grpc/heartbeat/heartbeat.go
- pkg/cloudevents/clients/store/simplestore.go
- pkg/cloudevents/generic/options/grpc/protocol/protocol.go
- test/integration/cloudevents/source/client.go
- pkg/cloudevents/generic/options/mqtt/options.go
- pkg/cloudevents/clients/store/informer.go
- pkg/cloudevents/generic/clients/baseclient.go
- pkg/cloudevents/server/grpc/heartbeat/healthcheck.go
- pkg/cloudevents/clients/work/agent/client/manifestwork.go
- pkg/cloudevents/clients/options/generic.go
- pkg/cloudevents/generic/clients/sourceclient_test.go
- pkg/cloudevents/generic/clients/agentclient.go
- pkg/cloudevents/clients/store/informer_test.go
- pkg/cloudevents/clients/store/simplestore_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-12T02:55:39.701Z
Learnt from: qiujian16
Repo: open-cluster-management-io/sdk-go PR: 140
File: pkg/cloudevents/generic/options/grpc/protocol/protocol.go:36-39
Timestamp: 2025-09-12T02:55:39.701Z
Learning: In the GRPC protocol health check implementation, WithReconnectErrorOption already validates the interval parameter and sets a default of 20 seconds if interval <= 0, preventing time.NewTicker panics.
Applied to files:
pkg/cloudevents/generic/options/grpc/options.go
📚 Learning: 2025-09-16T02:22:20.929Z
Learnt from: skeeey
Repo: open-cluster-management-io/sdk-go PR: 144
File: pkg/cloudevents/generic/options/grpc/protocol/protocol.go:200-213
Timestamp: 2025-09-16T02:22:20.929Z
Learning: In the GRPC CloudEvents protocol implementation, when startEventsReceiver encounters a stream error, it sends the error to reconnectErrorChan. The consumer of this channel handles the error by calling Close() on the protocol, which triggers close(p.closeChan), causing OpenInbound to unblock and call cancel() to properly terminate both the events receiver and heartbeat watcher goroutines.
Applied to files:
pkg/cloudevents/generic/options/grpc/options.go
🧬 Code graph analysis (7)
pkg/cloudevents/generic/interface.go (1)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)
pkg/cloudevents/generic/clients/clients_metrics_test.go (2)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)pkg/cloudevents/generic/testing/resource.go (1)
MockResource(22-29)
pkg/cloudevents/clients/store/interface.go (1)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)
pkg/cloudevents/generic/clients/agentclient_test.go (2)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)pkg/cloudevents/generic/testing/resource.go (1)
MockResource(22-29)
pkg/cloudevents/clients/work/store/base.go (2)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)vendor/open-cluster-management.io/api/work/v1/types.go (1)
ManifestWork(18-28)
pkg/cloudevents/generic/clients/sourceclient.go (2)
pkg/cloudevents/generic/types/types.go (5)
ParseCloudEventsType(209-230)ExtensionClusterName(74-74)CloudEventsDataType(165-169)SubResourceStatus(33-33)StatusModified(97-97)pkg/cloudevents/generic/metrics/metrics_collector.go (1)
UpdateResourceSpecResyncDurationMetric(323-331)
pkg/cloudevents/clients/work/store/informer.go (3)
pkg/cloudevents/clients/store/informer.go (1)
AgentInformerWatcherStore(25-30)pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)vendor/open-cluster-management.io/api/work/v1/types.go (1)
ManifestWork(18-28)
⏰ 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: unit
- GitHub Check: integration
- GitHub Check: verify
🔇 Additional comments (18)
pkg/cloudevents/generic/options/grpc/options.go (1)
263-289: LGTM! Clean adoption of contextual logging.The changes correctly introduce context-aware logging:
- Logger is properly obtained from context using
klog.FromContext(ctx)- Info and error logging follow the structured logging patterns
- The goroutine safely uses the captured logger
The implementation aligns with the PR objective of adopting contextual logging throughout the CloudEvents codebase.
pkg/cloudevents/generic/options/grpc/agentoptions.go (2)
38-38: LGTM! Contextual logger properly extracted.The use of
klog.FromContext(ctx)aligns with klog v2 best practices for contextual logging and ensures that any contextual information (e.g., request IDs, trace IDs) attached to the context will be included in log entries.
61-61: LGTM! Structured logging migration complete.The migration from
klog.Errorftologger.Errorcorrectly uses the structured logging API while preserving the original error-handling behavior (report through channel, log if channel full).pkg/cloudevents/generic/options/cert/rotation.go (4)
63-68: LGTM! Context-aware logging properly implemented.The logger is correctly obtained from the context, and the transition from
wait.Untiltowait.UntilWithContextproperly enables context-based cancellation for the worker loop.
87-88: LGTM! Context properly propagated through the worker loop.The context parameter is correctly threaded through to
processNextWorkItem, maintaining the context chain for logging and cancellation.
92-99: LGTM! Context correctly propagated to work item processing.The context is properly passed through to
loadClientCert, enabling contextual logging for certificate loading operations.
112-113: LGTM! Contextual logging correctly implemented for certificate operations.The logger is properly derived from the context and used for rotation detection logging, aligning with structured logging best practices.
Also applies to: 141-141
pkg/cloudevents/generic/options/mqtt/logger.go (3)
4-4: LGTM: Required import for formatting.The
fmtimport is necessary for the error formatting and string conversion functions used in the updated logging methods.
28-34: LGTM: Clean debug logging implementation.The debug logger methods correctly use verbose level 4 for debug messages, with appropriate string formatting via
fmt.Sprintandfmt.Sprintf.
10-10: ****The
loggerfield is properly initialized. Inpkg/cloudevents/generic/options/mqtt/options.go(lines 238-239), bothPahoErrorLoggerandPahoDebugLoggerare instantiated with theloggerfield assigned:logger := klog.FromContext(ctx) // line 223 opts := []cloudeventsmqtt.Option{ cloudeventsmqtt.WithDebugLogger(&PahoDebugLogger{logger: logger}), // line 238 cloudeventsmqtt.WithErrorLogger(&PahoErrorLogger{logger: logger}), // line 239 }The
loggervariable is guaranteed non-nil:klog.FromContext()never returns nil—it falls back to the global klog logger if no logger is found in the context. No nil pointer dereference risk exists.Likely an incorrect or invalid review comment.
pkg/cloudevents/clients/store/interface.go (2)
37-37: LGTM: Context parameter added for contextual logging.The addition of
ctx context.Contextas the first parameter aligns with the PR's objective to enable context-derived logging throughout the codebase. This is a breaking interface change, but it's consistent with the broader refactoring effort.
66-81: LGTM: Contextual logging properly implemented.The
WaitForStoreInitfunction correctly derives the logger from the context and uses it for error logging, following standard klog patterns.pkg/cloudevents/generic/clients/agentclient_test.go (1)
542-546: LGTM: Test callback updated for new signature.The test callback correctly includes the
context.Contextparameter to match the updatedResourceHandlersignature. Using_for the unused context parameter is appropriate for this test scenario.pkg/cloudevents/clients/work/store/informer.go (3)
46-46: LGTM: Context propagation to worker goroutine.The worker goroutine now receives the context directly, enabling proper cancellation and contextual logging throughout the processing pipeline.
104-105: LGTM: Context parameter and logger initialization.The
HandleReceivedResourcemethod signature correctly adds the context parameter, and the logger is properly initialized usingklog.FromContext(ctx).
120-122: Unable to verify log level change—manual confirmation needed.The sandbox environment cannot access the repository structure to verify the log level downgrade from
klog.Warningftologger.Info. To confirm this change is intentional and appropriate:
- Verify the log message context: "the work is deleting, ignore the update" describes expected, normal behavior during deletion, not an error condition
- Check if similar deletion-related logs elsewhere in the codebase use
Infolevel for expected events- Confirm this aligns with your logging conventions (warnings typically reserved for unexpected/abnormal conditions)
pkg/cloudevents/generic/clients/sourceclient.go (2)
154-210: LGTM: Contextual logging properly implemented for error and info paths.The error handling and informational logging throughout the
receivemethod correctly use the context-derived logger. The structured logging with fields (e.g., "eventDataType", "event") enhances observability.
223-270: LGTM: Contextual logging in respondResyncSpecRequest.The
respondResyncSpecRequestmethod correctly initializes and uses the context-derived logger throughout, including for V-level debug logs with appropriate structured fields.
f302cbb to
fa1cf4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
pkg/cloudevents/server/grpc/heartbeat/heartbeat.go (1)
43-43: Reconsider log level downgrade from Warn to Info.Dropping heartbeats because the send channel is full typically indicates backpressure or that the consumer isn't keeping up, which may signal an operational issue. Downgrading from
WarntoInforeduces the visibility of this condition and could cause operators to miss early warning signs of system degradation.pkg/cloudevents/generic/clients/sourceclient.go (1)
149-210: LGTM! Context-aware logging correctly implemented.The receive method now properly initializes a logger from context and enriches it with eventType. The WithValues result is correctly assigned back to the logger variable (line 157), addressing the previous review concern. All subsequent logging uses the context-derived logger with appropriate structured fields.
pkg/cloudevents/generic/clients/agentclient.go (1)
151-222: LGTM! Context-aware logging correctly implemented.The receive method properly initializes a logger from context and enriches it with eventType. The WithValues result is correctly assigned back to the logger variable (line 159), addressing the previous review concern. All subsequent logging uses the context-derived logger with appropriate structured fields throughout the event handling flow.
pkg/cloudevents/clients/work/store/base.go (1)
133-135: Clarify log message and fix typo.The log message is confusing and contains a typo:
- "great" should be "greater"
- "resource version is great than its generation" doesn't accurately describe the condition being checked
The condition checks whether the agent's resource version is greater than the source's resource version, not anything about generation.
Apply this diff to improve clarity:
- logger.Info("the work resource version is great than its generation, ignore", + logger.Info("the agent resource version is greater than the source resource version, ignore", "agentResourceVersion", resourceVersion, "sourceResourceVersion", lastResourceVersion)
🧹 Nitpick comments (1)
pkg/cloudevents/clients/work/store/base.go (1)
150-152: Clarify sequenceID comparison message.The message "the work current sequenceID is less than its last" could be clearer. Consider rephrasing to make it more explicit that the incoming update is being ignored because it's out of order.
Apply this diff:
- logger.Info("the work current sequenceID is less than its last, ignore", + logger.Info("the incoming work sequenceID is not greater than the current sequenceID, ignore", "currentSequenceID", sequenceID, "lastSequenceID", lastSequenceID)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
pkg/cloudevents/clients/options/generic.go(6 hunks)pkg/cloudevents/clients/store/informer.go(3 hunks)pkg/cloudevents/clients/store/informer_test.go(2 hunks)pkg/cloudevents/clients/store/interface.go(3 hunks)pkg/cloudevents/clients/store/simplestore.go(3 hunks)pkg/cloudevents/clients/store/simplestore_test.go(2 hunks)pkg/cloudevents/clients/work/agent/client/manifestwork.go(4 hunks)pkg/cloudevents/clients/work/source/client/manifestwork.go(3 hunks)pkg/cloudevents/clients/work/store/base.go(8 hunks)pkg/cloudevents/clients/work/store/informer.go(3 hunks)pkg/cloudevents/clients/work/store/local.go(5 hunks)pkg/cloudevents/generic/clients/agentclient.go(4 hunks)pkg/cloudevents/generic/clients/agentclient_test.go(1 hunks)pkg/cloudevents/generic/clients/baseclient.go(3 hunks)pkg/cloudevents/generic/clients/clients_metrics_test.go(1 hunks)pkg/cloudevents/generic/clients/sourceclient.go(5 hunks)pkg/cloudevents/generic/clients/sourceclient_test.go(1 hunks)pkg/cloudevents/generic/interface.go(1 hunks)pkg/cloudevents/generic/options/cert/rotation.go(4 hunks)pkg/cloudevents/generic/options/grpc/agentoptions.go(2 hunks)pkg/cloudevents/generic/options/grpc/options.go(2 hunks)pkg/cloudevents/generic/options/grpc/protocol/protocol.go(5 hunks)pkg/cloudevents/generic/options/grpc/sourceoptions.go(2 hunks)pkg/cloudevents/generic/options/mqtt/agentoptions.go(2 hunks)pkg/cloudevents/generic/options/mqtt/logger.go(1 hunks)pkg/cloudevents/generic/options/mqtt/options.go(3 hunks)pkg/cloudevents/server/grpc/heartbeat/healthcheck.go(4 hunks)pkg/cloudevents/server/grpc/heartbeat/heartbeat.go(3 hunks)test/integration/cloudevents/source/client.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/cloudevents/clients/work/agent/client/manifestwork.go
- pkg/cloudevents/clients/options/generic.go
- pkg/cloudevents/generic/options/grpc/sourceoptions.go
- pkg/cloudevents/clients/work/source/client/manifestwork.go
- pkg/cloudevents/generic/options/grpc/agentoptions.go
- pkg/cloudevents/generic/clients/sourceclient_test.go
- pkg/cloudevents/generic/options/mqtt/agentoptions.go
- pkg/cloudevents/generic/options/cert/rotation.go
- pkg/cloudevents/generic/options/mqtt/options.go
- test/integration/cloudevents/source/client.go
- pkg/cloudevents/clients/store/informer.go
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-17T13:29:00.675Z
Learnt from: qiujian16
Repo: open-cluster-management-io/sdk-go PR: 147
File: pkg/cloudevents/generic/options/grpc/protocol/heartbeat_integration_test.go:66-68
Timestamp: 2025-09-17T13:29:00.675Z
Learning: In the pkg/cloudevents/generic/options/grpc/protocol package, bufSize is already defined as a constant in protocal_test.go and is accessible to other test files in the same package, including heartbeat_integration_test.go.
Applied to files:
pkg/cloudevents/generic/options/grpc/protocol/protocol.go
📚 Learning: 2025-09-16T02:22:20.929Z
Learnt from: skeeey
Repo: open-cluster-management-io/sdk-go PR: 144
File: pkg/cloudevents/generic/options/grpc/protocol/protocol.go:200-213
Timestamp: 2025-09-16T02:22:20.929Z
Learning: In the GRPC CloudEvents protocol implementation, when startEventsReceiver encounters a stream error, it sends the error to reconnectErrorChan. The consumer of this channel handles the error by calling Close() on the protocol, which triggers close(p.closeChan), causing OpenInbound to unblock and call cancel() to properly terminate both the events receiver and heartbeat watcher goroutines.
Applied to files:
pkg/cloudevents/generic/options/grpc/protocol/protocol.gopkg/cloudevents/generic/clients/baseclient.gopkg/cloudevents/generic/options/grpc/options.go
📚 Learning: 2025-09-12T02:55:39.701Z
Learnt from: qiujian16
Repo: open-cluster-management-io/sdk-go PR: 140
File: pkg/cloudevents/generic/options/grpc/protocol/protocol.go:36-39
Timestamp: 2025-09-12T02:55:39.701Z
Learning: In the GRPC protocol health check implementation, WithReconnectErrorOption already validates the interval parameter and sets a default of 20 seconds if interval <= 0, preventing time.NewTicker panics.
Applied to files:
pkg/cloudevents/server/grpc/heartbeat/healthcheck.gopkg/cloudevents/generic/options/grpc/options.go
📚 Learning: 2025-09-16T06:03:45.232Z
Learnt from: skeeey
Repo: open-cluster-management-io/sdk-go PR: 144
File: pkg/cloudevents/server/grpc/broker.go:169-194
Timestamp: 2025-09-16T06:03:45.232Z
Learning: In the Open Cluster Management gRPC CloudEvents implementation, client reconnections due to heartbeat timeout are considered acceptable behavior by the maintainer skeeey, rather than something that needs optimization to prevent.
Applied to files:
pkg/cloudevents/generic/clients/baseclient.go
🧬 Code graph analysis (9)
pkg/cloudevents/generic/interface.go (1)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)
pkg/cloudevents/generic/clients/baseclient.go (1)
vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go (1)
HandleErrorWithContext(202-204)
pkg/cloudevents/clients/store/interface.go (1)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)
pkg/cloudevents/generic/clients/sourceclient.go (2)
pkg/cloudevents/generic/types/types.go (5)
ParseCloudEventsType(209-230)ExtensionClusterName(74-74)CloudEventsDataType(165-169)SubResourceStatus(33-33)StatusModified(97-97)pkg/cloudevents/generic/metrics/metrics_collector.go (1)
UpdateResourceSpecResyncDurationMetric(323-331)
pkg/cloudevents/clients/work/store/base.go (2)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)vendor/open-cluster-management.io/api/work/v1/types.go (1)
ManifestWork(18-28)
pkg/cloudevents/generic/clients/clients_metrics_test.go (2)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)pkg/cloudevents/generic/testing/resource.go (1)
MockResource(22-29)
pkg/cloudevents/generic/clients/agentclient.go (2)
pkg/cloudevents/generic/types/types.go (6)
ParseCloudEventsType(209-230)CloudEventsDataType(165-169)ResyncRequestAction(41-41)SubResourceStatus(33-33)SubResourceSpec(30-30)ExtensionClusterName(74-74)vendor/github.com/cloudevents/sdk-go/v2/types/value.go (1)
ToString(230-241)
pkg/cloudevents/clients/work/store/informer.go (3)
pkg/cloudevents/clients/store/informer.go (1)
AgentInformerWatcherStore(25-30)pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)vendor/open-cluster-management.io/api/work/v1/types.go (1)
ManifestWork(18-28)
pkg/cloudevents/generic/clients/agentclient_test.go (2)
pkg/cloudevents/generic/types/types.go (1)
ResourceAction(87-87)pkg/cloudevents/generic/testing/resource.go (1)
MockResource(22-29)
⏰ 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: unit
- GitHub Check: integration
- GitHub Check: verify
🔇 Additional comments (27)
pkg/cloudevents/generic/options/grpc/options.go (2)
263-263: LGTM: Context-derived logger initialized correctly.The logger is properly initialized from the context using
klog.FromContext(ctx), enabling structured, context-aware logging throughout the connection state monitoring goroutine.
271-271: LGTM: Structured logging applied consistently.The logging calls have been correctly updated to use structured logging methods (
InfoandError) with the context-derived logger, replacing the previous global logging calls.Also applies to: 289-289
pkg/cloudevents/generic/clients/clients_metrics_test.go (1)
102-105: LGTM: Test updated to match new context-aware callback signature.The Subscribe callback has been correctly updated to accept
context.Contextas the first parameter, aligning with the API changes across the PR. The context parameter is appropriately ignored with_since this is a no-op test callback.pkg/cloudevents/generic/options/grpc/protocol/protocol.go (3)
92-93: LGTM: Publishing path uses structured logging.The logger is correctly initialized from context and the log message has been updated to use structured logging with a descriptive key-value pair for the message ID.
115-129: LGTM: Inbound subscription path uses structured logging.The logging implementation correctly uses context-derived logger and structured fields (
source,cluster,eventDataType) to provide clear operational visibility for both source and cluster subscription scenarios.
149-149: LGTM: Connection closing logged appropriately.The structured logging call correctly logs the connection closure event.
pkg/cloudevents/generic/options/mqtt/logger.go (3)
10-10: LGTM: Logger fields added to route Paho logging through klog.The addition of
logger klog.Loggerfields to bothPahoErrorLoggerandPahoDebugLoggerenables proper integration with context-aware structured logging. These loggers should be initialized with a context-derived logger in the calling code.Also applies to: 14-14
20-26: LGTM: Error logging correctly routes through structured logger.The Paho error logging methods now delegate to the structured logger with appropriate error formatting. The use of
l.logger.Errorensures errors are logged with proper structure and context.
28-34: LGTM: Debug logging uses appropriate verbosity level.Debug messages are logged at verbosity level 4 (
V(4)), which is an appropriate level for debug/trace-level MQTT protocol details, preventing log spam in production while still being available when needed.pkg/cloudevents/generic/clients/baseclient.go (2)
83-83: LGTM: Error handling updated to use context-aware variants.The error handling calls have been correctly updated to use
runtime.HandleErrorWithContext, which enables context-aware error logging. The error messages are clear and descriptive.Also applies to: 106-106, 113-113
208-208: LGTM: Structured error handling with key-value context.The unknown receiver signal error correctly uses
HandleErrorWithContextwith an empty message string and provides the signal value as a structured key-value pair, enabling better debugging while maintaining consistent error handling patterns.pkg/cloudevents/clients/store/informer_test.go (1)
192-211: LGTM: Test updated to pass context to HandleReceivedResource.The test correctly passes the context as the first parameter to all
HandleReceivedResourcecalls, aligning with the updated API signature that enables context-aware logging and processing.pkg/cloudevents/clients/store/simplestore_test.go (1)
112-112: LGTM: Test correctly passes context to HandleReceivedResource.The test has been properly updated to pass
context.Background()as the first parameter toHandleReceivedResource, aligning with the new context-aware API signature.pkg/cloudevents/server/grpc/heartbeat/heartbeat.go (1)
26-26: LGTM: Context-derived logger initialized correctly.The logger is properly initialized from the context using
klog.FromContext(ctx), enabling structured, context-aware logging in the heartbeat mechanism.pkg/cloudevents/server/grpc/heartbeat/healthcheck.go (1)
48-58: Consistent heartbeat logging looks good.Thanks for switching to the context-derived logger and aligning both call sites on the
"heartbeat"field—this keeps the logs uniform and easier to search.Also applies to: 80-84
pkg/cloudevents/generic/interface.go (1)
15-15: Context parameter addition aligns with the refactor.Carrying
context.ContextintoResourceHandlerkeeps handlers consistent with the rest of the pipeline—looks good to me.pkg/cloudevents/generic/clients/agentclient_test.go (1)
542-546: Test callback update is spot on.Good call adding the context parameter and discarding it where unused; keeps the tests aligned with the new handler signature.
pkg/cloudevents/clients/store/interface.go (1)
37-37: Context-aware store interface change looks solid.Extending
HandleReceivedResourcewithctxand switchingWaitForStoreInittoklog.FromContextkeeps the store path in sync with the rest of the contextual logging work—no issues spotted.Also applies to: 66-82
pkg/cloudevents/clients/store/simplestore.go (1)
4-4: SimpleStore logging refactor looks good.Using
klog.FromContextand structured fields keeps the deletion guard messaging intact while aligning with the new logging pattern.Also applies to: 54-83
pkg/cloudevents/clients/work/store/informer.go (1)
104-122: LGTM! Context-aware logging implemented correctly.The function signature now properly accepts a context parameter, initializes a structured logger from it, and uses appropriate structured fields (manifestworkNamespace, manifestworkName) for the deletion-guard log message.
pkg/cloudevents/clients/work/store/local.go (2)
92-95: LGTM! Context propagation improved.Both the work processor and the processLoop now properly receive and use the context, replacing the previous
ctx.Done()pattern with directctxpassing and usingwait.UntilWithContextfor context-aware execution.
165-232: LGTM! Context-aware logging with appropriate structured fields.The processLoop now initializes a logger from the context and consistently uses it throughout with appropriate structured fields for debugging (object, key, uid). Error handling is clear and structured.
pkg/cloudevents/generic/clients/sourceclient.go (1)
220-304: LGTM! Consistent context-aware logging in resync handler.The respondResyncSpecRequest method correctly initializes a logger from context and uses it consistently throughout with appropriate structured fields (object, error) and V-level logging where appropriate.
pkg/cloudevents/generic/clients/agentclient.go (1)
230-287: LGTM! Consistent context-aware logging in resync handler.The respondResyncStatusRequest method correctly initializes a logger from context with the eventDataType field and uses it consistently for logging resource-not-found scenarios with the uid field.
pkg/cloudevents/clients/work/store/base.go (3)
54-70: LGTM! Context propagation through worker pipeline.The run, runWorker, and processNextWork methods now properly thread context through the worker execution pipeline, using
wait.UntilWithContextandctx.Done()for context-aware lifecycle management.
95-166: Context-aware logging successfully integrated.The handleWork method properly initializes a logger from context with structured fields and uses it consistently throughout the work validation and update flow. Good use of structured fields for debugging.
168-183: LGTM! Context-aware logging in helper method.The getWork method correctly initializes a logger from context and uses it for error logging when listing works fails.
|
/hold |
fa1cf4c to
639f14d
Compare
| case <-cloudEventsClient.ReconnectedChan(): | ||
| if !o.resync { | ||
| klog.V(4).Infof("resync is disabled, do nothing") | ||
| logger.V(4).Info("resync is disabled, do nothing") |
There was a problem hiding this comment.
| logger.V(4).Info("resync is disabled, do nothing") | |
| logger.Info("resync is disabled, do nothing") |
| case <-cloudEventsClient.ReconnectedChan(): | ||
| if !o.resync { | ||
| klog.V(4).Infof("resync is disabled, do nothing") | ||
| logger.V(4).Info("resync is disabled, do nothing") |
There was a problem hiding this comment.
| logger.V(4).Info("resync is disabled, do nothing") | |
| logger.Info("resync is disabled, do nothing") |
Signed-off-by: Jian Qiu <jqiu@redhat.com>
639f14d to
eca9910
Compare
|
LGTM |
|
/unhold |
|
/lgtm |
e1b074a
into
open-cluster-management-io:main
2 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit