✨ Add log tracing support for contextual logging #172
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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds a log-tracing package and integrates tracing propagation into CloudEvents codecs, clients, and controller flows; changes base controller default queue key and logging fields; and adds extensive unit tests for tracing, codecs, clients, and controller behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧬 Code graph analysis (5)pkg/basecontroller/factory/base_controller.go (1)
pkg/cloudevents/clients/work/source/codec/manifestbundle.go (2)
pkg/cloudevents/clients/work/agent/codec/manifestbundle.go (1)
pkg/cloudevents/clients/work/source/client/manifestwork.go (1)
pkg/logging/logging_test.go (1)
🪛 Gitleaks (8.30.0)pkg/logging/logging_test.go[high] 139-139: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) [high] 155-155: Detected a Generic API Key, potentially exposing access to various services and sensitive operations. (generic-api-key) ⏰ 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)
🔇 Additional comments (17)
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: 2
🧹 Nitpick comments (3)
pkg/cloudevents/clients/work/source/codec/manifestbundle_test.go (1)
374-542: LGTM! Comprehensive test coverage for log tracing.The test suite thoroughly validates the bidirectional propagation of log tracing data between ManifestWork annotations and CloudEvent extensions. All edge cases are covered, including encode/decode with and without tracing data.
Optional: Consider extracting common test setup code (e.g., creating basic CloudEvents or ManifestWork objects) into helper functions to reduce repetition, though the current inline approach is acceptable for test clarity.
pkg/logging/logging_test.go (1)
190-194: Consider parallel test safety when modifying global state.The test temporarily modifies the global
DefaultContextTracingKeysvariable. While the defer correctly restores it, this pattern can cause race conditions if Go's parallel test execution is enabled (viat.Parallel()).Consider one of these approaches:
- Make
DefaultContextTracingKeysconfigurable per-call rather than global- Use a mutex to synchronize access during tests
- Document that these tests must not run in parallel
Apply this pattern if refactoring to pass keys as a parameter:
-func SetLogTracingFromContext(ctx context.Context, object metav1.Object) { +func SetLogTracingFromContext(ctx context.Context, object metav1.Object, keys []ContextTracingKey) { + if keys == nil { + keys = DefaultContextTracingKeys + } annotations := object.GetAnnotations() if annotations == nil { annotations = make(map[string]string) } - for _, key := range DefaultContextTracingKeys { + for _, key := range keys {pkg/logging/logging.go (1)
81-92: Simplify unnecessary type conversion.At line 86,
valueis already astring(fromlogTracingMapwhich ismap[string]string), so the call tocloudeventstypes.ToString(value)is unnecessary.Apply this diff to simplify:
for key, value := range logTracingMap { if !strings.HasPrefix(key, LogTracingPrefix) { continue } tracingKey := strings.TrimPrefix(key, LogTracingPrefix) - tracingValue, err := cloudeventstypes.ToString(value) - if err != nil { - logger.Error(err, "Failed to get log tracing value") - return logger - } - logger = logger.WithValues(tracingKey, tracingValue) + logger = logger.WithValues(tracingKey, value) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
pkg/basecontroller/factory/base_controller.go(2 hunks)pkg/basecontroller/factory/base_controller_test.go(1 hunks)pkg/basecontroller/factory/factory.go(1 hunks)pkg/cloudevents/clients/work/agent/codec/manifestbundle.go(3 hunks)pkg/cloudevents/clients/work/agent/codec/manifestbundle_test.go(1 hunks)pkg/cloudevents/clients/work/source/client/manifestwork.go(4 hunks)pkg/cloudevents/clients/work/source/codec/manifestbundle.go(3 hunks)pkg/cloudevents/clients/work/source/codec/manifestbundle_test.go(1 hunks)pkg/cloudevents/generic/clients/baseclient.go(3 hunks)pkg/logging/logging.go(1 hunks)pkg/logging/logging_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-11T13:27:36.331Z
Learnt from: morvencao
Repo: open-cluster-management-io/sdk-go PR: 162
File: pkg/cloudevents/generic/options/pubsub/options.go:157-174
Timestamp: 2025-11-11T13:27:36.331Z
Learning: For open-cluster-management PubSub transport in pkg/cloudevents/generic/options/pubsub: broadcast topics (SourceBroadcast, AgentBroadcast) and their corresponding subscriptions are always required, not optional. The omitempty tags on types.Topics broadcast fields exist because the struct is shared with MQTT (where broadcasts are optional), but PubSub requires all broadcast channels for resync functionality.
Applied to files:
pkg/cloudevents/generic/clients/baseclient.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/clients/baseclient.go
🧬 Code graph analysis (8)
pkg/cloudevents/clients/work/agent/codec/manifestbundle.go (1)
pkg/logging/logging.go (2)
LogTracingFromObjectToEvent(148-165)LogTracingFromEventToObject(116-146)
pkg/cloudevents/generic/clients/baseclient.go (1)
pkg/logging/logging.go (1)
SetLogTracingByCloudEvent(70-94)
pkg/cloudevents/clients/work/source/codec/manifestbundle.go (2)
pkg/cloudevents/generic/types/types.go (1)
ExtensionWorkMeta(83-83)pkg/logging/logging.go (2)
LogTracingFromObjectToEvent(148-165)LogTracingFromEventToObject(116-146)
pkg/basecontroller/factory/base_controller_test.go (3)
pkg/basecontroller/factory/interfaces.go (1)
SyncContext(34-41)pkg/basecontroller/factory/controller_context.go (1)
NewSyncContext(22-30)pkg/basecontroller/factory/factory.go (3)
DefaultQueueKey(13-13)New(59-61)DefaultQueueKeysFunc(16-18)
pkg/cloudevents/clients/work/source/client/manifestwork.go (1)
pkg/logging/logging.go (1)
SetLogTracingFromContext(98-114)
pkg/logging/logging_test.go (1)
pkg/logging/logging.go (10)
LogTracingPrefix(17-17)SetLogTracingByObject(25-38)ExtensionLogTracing(18-18)SetLogTracingByCloudEvent(70-94)ContextTracingOPIDKey(19-19)SetLogTracingFromContext(98-114)DefaultContextTracingKeys(23-23)ContextTracingKey(14-14)LogTracingFromEventToObject(116-146)LogTracingFromObjectToEvent(148-165)
pkg/basecontroller/factory/base_controller.go (1)
pkg/basecontroller/factory/factory.go (1)
DefaultQueueKey(13-13)
pkg/cloudevents/clients/work/source/codec/manifestbundle_test.go (5)
pkg/cloudevents/clients/work/agent/codec/manifestbundle.go (1)
NewManifestBundleCodec(37-39)pkg/cloudevents/clients/work/source/codec/manifestbundle.go (1)
NewManifestBundleCodec(23-25)pkg/cloudevents/clients/utils/utils.go (1)
UID(222-225)pkg/cloudevents/generic/types/types.go (3)
CloudEventsType(221-230)CloudEventsDataType(210-214)SubResourceSpec(30-30)pkg/cloudevents/clients/work/payload/manifestbundle.go (2)
ManifestBundleEventDataType(11-15)ManifestBundleStatus(34-52)
⏰ 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 (21)
pkg/basecontroller/factory/base_controller.go (2)
83-83: LGTM!The log field rename from
numberOfWorkerstoworker-IDis more descriptive and accurately reflects thatirepresents the individual worker's identifier, not the total worker count.
160-164: LGTM! The conditional logging reduces noise for periodic resync errors.When using
DefaultQueueKey(typically from periodic resync), errors are logged with a simpler message at lower verbosity levels. The key is included only whenV(4)is enabled or for custom queue keys, which helps with debugging specific resource failures without flooding logs during periodic resync issues.pkg/cloudevents/clients/work/source/client/manifestwork.go (4)
7-8: LGTM!Import added for the new logging package.
113-114: LGTM! Tracing propagation in Create.The tracing annotation is correctly set before validation and publishing, ensuring contextual logging metadata flows through the CloudEvents system to the agent.
166-167: LGTM! Tracing propagation in Delete.Tracing is set on
deletingWorkbefore publishing the delete event, maintaining observability through the deletion flow.
306-307: LGTM! Tracing propagation in Patch.Tracing is set on
newWorkbefore validation and publishing, consistent with the Create method pattern.pkg/cloudevents/clients/work/agent/codec/manifestbundle.go (3)
9-9: LGTM!Import added for the logging package.
72-75: LGTM! Tracing transfer from object to event in Encode.Transfers log tracing annotations from the ManifestWork to the CloudEvent extensions during status encoding. The placement after status hash computation and before metadata serialization is appropriate.
157-160: LGTM! Tracing transfer from event to object in Decode.Transfers log tracing from CloudEvent extensions to work object annotations during spec decoding. This enables the agent to receive and preserve tracing context from the source.
pkg/cloudevents/clients/work/source/codec/manifestbundle.go (4)
8-8: LGTM!Import added for the logging package.
44-44: LGTM!Minor comment improvement: "meta data" → "metadata".
51-54: LGTM! Tracing transfer from object to event in Encode.Transfers log tracing annotations from the ManifestWork to CloudEvent extensions during spec encoding for the source-to-agent flow.
128-131: LGTM! Tracing transfer from event to object in Decode.Transfers log tracing from CloudEvent extensions to work object annotations when decoding status updates from agents.
pkg/cloudevents/clients/work/agent/codec/manifestbundle_test.go (1)
459-724: LGTM! Comprehensive test coverage for log tracing.The test suite thoroughly covers the log tracing functionality:
- Encode preserves tracing annotations and filters non-tracing ones
- Decode transfers tracing extensions to object annotations
- Edge cases with no tracing data are handled correctly
- Round-trip test validates end-to-end tracing preservation
The tests align well with the logging utilities in
pkg/logging/logging.go.pkg/cloudevents/generic/clients/baseclient.go (3)
6-6: LGTM!Import added for the logging package.
123-123: LGTM! Log enrichment in publish.The logger is now enriched with tracing information from the CloudEvent being published, improving observability for outgoing events.
205-213: LGTM! Log enrichment and context propagation in receive path.Key improvements:
- Creates a per-event logger enriched with tracing data via
SetLogTracingByCloudEvent- Updates the context with this logger so downstream handlers (the
receivecallback) can access it viaklog.FromContext(ctx)- Error handling uses
HandleErrorWithContextwith a clearer messageThis ensures tracing metadata flows through the entire receive → decode → handler chain.
pkg/basecontroller/factory/base_controller_test.go (1)
1-363: LGTM! Comprehensive test coverage for the base controller.The test file provides thorough coverage including:
- Controller run lifecycle with various worker counts
- Cache sync and periodic resync behavior
- Work item processing with success/error paths
- Queue requeue behavior on failures
- Accessor methods (Name, SyncContext, Sync)
- DefaultQueueKeysFunc behavior
The use of
sync.WaitGroupfor async synchronization and proper queue cleanup viaShutDown()demonstrates good test hygiene.pkg/basecontroller/factory/factory.go (1)
12-13: More descriptive default queue key, but verify external consumer impact.The rename from
"key"to"basecontroller-default-key"is clearer and reduces collision risk with actual resource keys. However, this is a breaking change if external consumers depend on the exact string value ofDefaultQueueKey. Confirm that:
- No external consumers rely on the hardcoded
"key"value- Public API documentation is updated if this constant is exported
- Integration tests cover controllers using this default queue key
pkg/logging/logging_test.go (1)
245-566: LGTM! Thorough test coverage for tracing propagation.The remaining test functions comprehensively validate:
- Event-to-object and object-to-event tracing transfers
- Nil input handling
- Edge cases with missing/invalid data
- Round-trip scenarios via extension helpers
Testing unexported helper functions (
getTracingMapFromExtenstion,setTracingMapToExtenstion) in the same package is appropriate for verifying internal behavior.pkg/logging/logging.go (1)
116-165: LGTM! Clean bidirectional tracing propagation.Both
LogTracingFromEventToObjectandLogTracingFromObjectToEventcorrectly:
- Handle nil inputs gracefully
- Filter keys by the tracing prefix
- Initialize annotation maps when needed
- Propagate tracing data between CloudEvents and Kubernetes objects
The symmetry between these functions ensures consistent behavior in both directions.
qiujian16
force-pushed
the
add-log-tracing-support
branch
2 times, most recently
from
24d17f6 to
556f433
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/basecontroller/factory/base_controller_test.go (2)
179-182: Consider more robust synchronization for rate-limited requeue.The 10ms sleep is used to wait for rate-limited items to be requeued. While this pattern is common in controller tests, it can be flaky in heavily loaded environments.
Consider using a retry loop with a reasonable timeout to check queue length, which would be more resilient than a fixed sleep.
199-240: Clarify test purpose or rename.The test name "TestBaseControllerDefaultQueueKey" and the comment "tests the error logging behavior with DefaultQueueKey" suggest it's testing logging functionality. However, the test only verifies requeue behavior on sync errors, which is already covered by
TestBaseControllerProcessNextWorkItem.Either enhance this test to actually verify error logging (e.g., using a custom log sink to capture log output) or rename it to better reflect what it tests, such as
TestBaseControllerRequeueBehavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
pkg/basecontroller/factory/base_controller.go(2 hunks)pkg/basecontroller/factory/base_controller_test.go(1 hunks)pkg/basecontroller/factory/factory.go(1 hunks)pkg/cloudevents/clients/work/agent/codec/manifestbundle.go(3 hunks)pkg/cloudevents/clients/work/agent/codec/manifestbundle_test.go(1 hunks)pkg/cloudevents/clients/work/source/client/manifestwork.go(4 hunks)pkg/cloudevents/clients/work/source/codec/manifestbundle.go(3 hunks)pkg/cloudevents/clients/work/source/codec/manifestbundle_test.go(1 hunks)pkg/cloudevents/generic/clients/baseclient.go(3 hunks)pkg/logging/logging.go(1 hunks)pkg/logging/logging_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/basecontroller/factory/base_controller.go
- pkg/cloudevents/clients/work/source/codec/manifestbundle.go
- pkg/cloudevents/clients/work/agent/codec/manifestbundle_test.go
- pkg/cloudevents/generic/clients/baseclient.go
- pkg/basecontroller/factory/factory.go
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/cloudevents/clients/work/source/client/manifestwork.go (1)
pkg/logging/logging.go (1)
SetLogTracingFromContext(93-109)
pkg/cloudevents/clients/work/agent/codec/manifestbundle.go (1)
pkg/logging/logging.go (2)
LogTracingFromObjectToEvent(143-160)LogTracingFromEventToObject(111-141)
pkg/logging/logging_test.go (1)
pkg/logging/logging.go (9)
LogTracingPrefix(17-17)SetLogTracingByObject(25-38)ExtensionLogTracing(18-18)SetLogTracingByCloudEvent(70-89)ContextTracingOPIDKey(19-19)DefaultContextTracingKeys(23-23)ContextTracingKey(14-14)LogTracingFromEventToObject(111-141)LogTracingFromObjectToEvent(143-160)
pkg/basecontroller/factory/base_controller_test.go (2)
pkg/basecontroller/factory/interfaces.go (1)
SyncContext(34-41)pkg/basecontroller/factory/controller_context.go (1)
NewSyncContext(22-30)
🪛 Gitleaks (8.30.0)
pkg/logging/logging_test.go
[high] 139-139: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 155-155: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (1)
- GitHub Check: integration
🔇 Additional comments (18)
pkg/basecontroller/factory/base_controller_test.go (7)
15-28: LGTM! Mock is sufficient for current test coverage.The
mockInformerprovides a minimal implementation that satisfies the test requirements. The handlers are tracked but never invoked, which is acceptable since the current tests only verify cache synchronization viaHasSynced.
54-60: Review the timing assumptions for periodic resync test.The test expects sync to be called via periodic resync (100ms interval) without explicitly adding items to the queue. While this should work, the timing is tight: the controller needs to start, wait for cache sync (up to 1s), start workers, trigger periodic resync, and process the item—all within the 1500ms wait timeout.
Consider increasing the timeout slightly or using a shorter resync interval to make the test more resilient in slow CI environments.
242-251: LGTM! Clean accessor test.The test correctly verifies the
Name()method returns the controller name.
253-263: LGTM! Clean accessor test.The test correctly verifies the
SyncContext()method returns the sync context.
265-292: LGTM! Thorough test of Sync method.The test correctly verifies that the
Sync()method calls the sync function with the expected key and propagates errors correctly.
294-329: LGTM! Well-designed periodic resync test.The test correctly verifies periodic resync functionality. The conservative check for at least 1 item (rather than exactly 8) makes the test resilient to timing variations while still confirming the periodic resync is working.
331-363: LGTM! Comprehensive test of DefaultQueueKeysFunc.The test correctly verifies that
DefaultQueueKeysFuncreturnsDefaultQueueKeyfor both nil and non-nil objects, matching the expected behavior from the function definition in factory.go.pkg/cloudevents/clients/work/agent/codec/manifestbundle.go (2)
72-75: LGTM! Clean integration of log tracing in encode path.The placement of the tracing call after setting the status hash is appropriate, and error handling correctly propagates failures.
157-160: LGTM! Clean integration of log tracing in decode path.The tracing call is well-positioned after metadata population, and error handling is appropriate.
pkg/cloudevents/clients/work/source/client/manifestwork.go (3)
113-114: LGTM! Context tracing correctly added to Create path.The placement after manifest encoding and before validation is appropriate.
166-167: LGTM! Context tracing correctly added to Delete path.Consistent with other operations and well-placed before event publishing.
306-307: LGTM! Context tracing correctly added to Patch path.The placement after resource version computation and before validation is appropriate.
pkg/cloudevents/clients/work/source/codec/manifestbundle_test.go (1)
374-542: Excellent test coverage for log tracing functionality.The test suite comprehensively covers:
- Encoding with tracing annotations (including prefix filtering)
- Decoding with tracing extensions
- Edge cases with missing tracing data
The assertions correctly verify that only annotations with the
LogTracingPrefixare propagated.pkg/logging/logging_test.go (1)
1-566: Comprehensive and well-structured test suite.The tests provide excellent coverage of all logging tracing functions, including:
- Nil handling
- Error cases (invalid JSON)
- Edge cases (missing extensions, no annotations)
- Context value propagation
Note: The Gitleaks static analysis warnings at lines 139 and 155 are false positives—these are test values ("operation-123", "operation-456"), not actual API keys.
pkg/logging/logging.go (4)
25-38: LGTM! Well-implemented logger augmentation.The function correctly:
- Handles nil objects
- Filters annotations by
LogTracingPrefix- Returns an augmented logger with tracing values
70-89: LGTM! Robust error handling in CloudEvent logger augmentation.The function appropriately:
- Handles nil events
- Logs errors when tracing map extraction fails (rather than failing silently)
- Returns a valid logger in all cases
111-141: LGTM! Clean implementation of event-to-object tracing transfer.The function correctly:
- Handles nil inputs
- Propagates errors from extension parsing
- Filters by
LogTracingPrefix- Preserves existing annotations
143-160: LGTM! Clean implementation of object-to-event tracing transfer.The function correctly filters annotations by
LogTracingPrefixand propagates them to the CloudEvent extension.
| evt.SetExtension(types.ExtensionStatusHash, statusHash) | ||
|
|
||
| // Add log tracing extension | ||
| if err := logging.LogTracingFromObjectToEvent(work, &evt); err != nil { |
There was a problem hiding this comment.
we need add this in other resources codec (cluster/addon ...)?
There was a problem hiding this comment.
could be done as follow up. But the other resources can be actually tracked by resource name.
| const ( | ||
| LogTracingPrefix = "logging.open-cluster-management.io/" | ||
| ExtensionLogTracing = "logtracing" | ||
| ContextTracingOPIDKey ContextTracingKey = "op-id" |
There was a problem hiding this comment.
may use opid instead of op-id, @machi1990 is opid used in use in CS for now?
There was a problem hiding this comment.
Nope. We use an iota.
It should be okay from cs and we could adapt to the contract put here.
Also, let's also consider the LIST/GET endpoints which are restful APIs
This commit adds comprehensive log tracing support across the cloudevents subsystem and base controller. Key changes include: - Add logging package with tracing annotation utilities - Transfer log tracing annotations between CloudEvents and ManifestWork objects - Enhance base controller with improved logging context - Add comprehensive test coverage for log tracing functionality - Update base controller default queue key to be more descriptive The log tracing feature enables better observability by propagating contextual logging metadata through the CloudEvents flow. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Jian Qiu <[email protected]>
qiujian16
force-pushed
the
add-log-tracing-support
branch
from
556f433 to
7eb19e0
Compare
4 participants
This commit adds comprehensive log tracing support across the cloudevents subsystem and base controller. Key changes include:
The log tracing feature enables better observability by propagating contextual logging metadata through the CloudEvents flow.
🤖 Generated with Claude Code
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.