Improved message logging #385
Conversation
📝 WalkthroughWalkthroughRefactors message logging across the message queue toolkit, relocating logging responsibility from publishers to consumers through a centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/lib/queues/AbstractQueueService.ts (1)
345-349: Fix the property name - critical bug!Line 348 accesses
message[this.messageDeduplicationId]but the property is namedthis.messageDeduplicationIdField(defined at line 112). The "Field" suffix is missing, causing this to always returnundefinedand breaking deduplication ID tracking.🔎 Apply this diff to fix the property name:
const messageDeduplicationId = message && this.messageDeduplicationIdField in message ? // @ts-ignore - message[this.messageDeduplicationId] + message[this.messageDeduplicationIdField] : undefined
🧹 Nitpick comments (1)
packages/core/lib/events/DomainEventEmitter.spec.ts (1)
207-207: Consider removing the non-null assertion for consistency.The combination of optional chaining (
?.) and non-null assertion (!) is contradictory: optional chaining handles undefined gracefully, while!asserts the value is definitely not undefined. Line 407 uses the cleaner patternmetadata?.correlationIdwithout the trailing!.🔎 Apply this diff for consistency:
- correlationId: createdEventPayload.metadata?.correlationId!, + correlationId: createdEventPayload.metadata?.correlationId,
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/amqp/lib/AbstractAmqpConsumer.ts(2 hunks)packages/amqp/lib/AbstractAmqpPublisher.ts(0 hunks)packages/amqp/test/consumers/AmqpPermissionConsumer.spec.ts(3 hunks)packages/amqp/test/publishers/AmqpPermissionPublisher.spec.ts(1 hunks)packages/core/lib/events/DomainEventEmitter.spec.ts(1 hunks)packages/core/lib/queues/AbstractQueueService.ts(7 hunks)packages/core/lib/queues/HandlerContainer.ts(2 hunks)packages/core/lib/types/queueOptionsTypes.ts(2 hunks)packages/core/test/queues/HandlerContainer.spec.ts(0 hunks)packages/gcp-pubsub/lib/pubsub/AbstractPubSubConsumer.ts(2 hunks)packages/gcp-pubsub/lib/pubsub/AbstractPubSubPublisher.ts(0 hunks)packages/sns/lib/sns/AbstractSnsPublisher.ts(0 hunks)packages/sns/lib/utils/snsInitter.ts(1 hunks)packages/sqs/lib/sqs/AbstractSqsConsumer.ts(3 hunks)packages/sqs/lib/sqs/AbstractSqsPublisher.ts(0 hunks)packages/sqs/test/consumers/SqsPermissionConsumer.spec.ts(2 hunks)
💤 Files with no reviewable changes (5)
- packages/core/test/queues/HandlerContainer.spec.ts
- packages/gcp-pubsub/lib/pubsub/AbstractPubSubPublisher.ts
- packages/sns/lib/sns/AbstractSnsPublisher.ts
- packages/sqs/lib/sqs/AbstractSqsPublisher.ts
- packages/amqp/lib/AbstractAmqpPublisher.ts
🧰 Additional context used
🧬 Code graph analysis (7)
packages/gcp-pubsub/lib/pubsub/AbstractPubSubConsumer.ts (1)
packages/core/lib/types/queueOptionsTypes.ts (1)
ProcessedMessageMetadata(19-70)
packages/core/lib/queues/AbstractQueueService.ts (1)
packages/core/lib/types/queueOptionsTypes.ts (1)
ProcessedMessageMetadata(19-70)
packages/amqp/lib/AbstractAmqpConsumer.ts (1)
packages/core/lib/types/queueOptionsTypes.ts (1)
ProcessedMessageMetadata(19-70)
packages/amqp/test/consumers/AmqpPermissionConsumer.spec.ts (1)
examples/sns-sqs/lib/common/Dependencies.ts (1)
logger(22-22)
packages/amqp/test/publishers/AmqpPermissionPublisher.spec.ts (1)
examples/sns-sqs/lib/common/Dependencies.ts (1)
logger(22-22)
packages/sqs/test/consumers/SqsPermissionConsumer.spec.ts (1)
examples/sns-sqs/lib/common/Dependencies.ts (1)
logger(22-22)
packages/sqs/lib/sqs/AbstractSqsConsumer.ts (1)
packages/core/lib/types/queueOptionsTypes.ts (1)
ProcessedMessageMetadata(19-70)
⏰ 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). (13)
- GitHub Check: general (20.x, @message-queue-toolkit/sns) / build
- GitHub Check: general (22.x, @message-queue-toolkit/gcp-pubsub) / build
- GitHub Check: general (24.x, @message-queue-toolkit/sqs) / build
- GitHub Check: general (22.x, @message-queue-toolkit/core) / build
- GitHub Check: general (20.x, @message-queue-toolkit/s3-payload-store) / build
- GitHub Check: general (22.x, @message-queue-toolkit/sns) / build
- GitHub Check: general (22.x, @message-queue-toolkit/sqs) / build
- GitHub Check: general (24.x, @message-queue-toolkit/sns) / build
- GitHub Check: general (20.x, @message-queue-toolkit/sqs) / build
- GitHub Check: general (24.x, @message-queue-toolkit/s3-payload-store) / build
- GitHub Check: general (20.x, @message-queue-toolkit/gcp-pubsub) / build
- GitHub Check: kafka (24.x) / build
- GitHub Check: kafka (22.x) / build
🔇 Additional comments (18)
packages/core/lib/types/queueOptionsTypes.ts (1)
65-69: LGTM!The new optional fields
messageMetadataandmessageMetadataFieldare well-documented and properly typed to support the metadata-driven logging refactor described in the PR objectives.Also applies to: 121-121
packages/sqs/test/consumers/SqsPermissionConsumer.spec.ts (2)
446-458: LGTM!Test expectations correctly updated to reflect the new consolidated logging structure using
processedMessageMetadata. The reduction from 2 log entries to 1 aligns with the PR's goal of centralizing message logging.
515-517: LGTM!Metrics test correctly validates that
messageMetadatais captured and includes theschemaVersionsfield from the published message's metadata.packages/amqp/test/publishers/AmqpPermissionPublisher.spec.ts (1)
50-64: LGTM!Test expectations correctly updated to validate the new metadata-based logging structure for publishers, including the
processedMessageMetadataobject withprocessingResult: { status: 'published' }.packages/sqs/lib/sqs/AbstractSqsConsumer.ts (1)
835-846: LGTM!The
resolveMessageLogmethod has been correctly refactored to acceptProcessedMessageMetadataand includes appropriate null guards. The implementation properly returnsnullwhen the message log formatter is not configured, which aligns with the change to makemessageLogFormatteroptional inHandlerContainer.packages/amqp/test/consumers/AmqpPermissionConsumer.spec.ts (3)
98-115: LGTM!Test expectations correctly updated to reflect the consolidated logging structure with
processedMessageMetadataobjects. The reduction from 6 to 4 log entries aligns with the centralized logging approach.
166-166: LGTM!Correctly validates that
messageMetadataisundefinedwhen no metadata is provided in the message.
408-408: LGTM!Good defensive programming with optional chaining to safely access potentially undefined nested properties.
packages/core/lib/queues/HandlerContainer.ts (1)
99-99: LGTM - Important behavioral change!The
messageLogFormatteris now optional and no longer has a default value. This ensures that message payloads are not logged by default, which aligns with the PR's objective to avoid exposing potentially private data. Clients must now explicitly provide amessageLogFormatterif they want message logging.This is a semantic change that improves privacy by default.
Also applies to: 127-127
packages/gcp-pubsub/lib/pubsub/AbstractPubSubConsumer.ts (1)
865-876: LGTM!The
resolveMessageLogmethod has been correctly refactored to use the metadata-based approach, consistent with similar changes in other consumer implementations (SQS, AMQP). The implementation includes proper guards and correctly handles the optionalmessageLogFormatter.packages/amqp/lib/AbstractAmqpConsumer.ts (2)
9-9: LGTM!The import is necessary for the updated
resolveMessageLogsignature and is correctly placed.
266-277: LGTM!The refactored
resolveMessageLogmethod correctly:
- Accepts the new
ProcessedMessageMetadataparameter- Guards against missing message or messageType
- Returns null when no formatter is configured
- Invokes the formatter only when appropriate
This aligns well with the PR's centralized logging approach.
packages/core/lib/queues/AbstractQueueService.ts (6)
117-120: LGTM!The new
messageMetadataFieldproperty is well-documented and its purpose aligns with the PR's goal of structured, privacy-conscious logging.
164-164: LGTM!The initialization follows the established pattern and provides a sensible default value.
247-251: LGTM!The signature change to accept
ProcessedMessageMetadataaligns with the centralized metadata-driven logging approach described in the PR objectives.
253-277: LGTM!The new
logMessageProcessedmethod provides comprehensive, structured logging:
- Safely serializes
messageMetadatausingstringValueSerializer- Includes all relevant processing metadata
- Allows optional custom message formatting via
resolveMessageLogThis implementation aligns well with the PR's privacy and centralized logging objectives.
313-326: LGTM!The renamed
debugMessageLoggingEnabledvariable is more descriptive, and the call tologMessageProcessedcorrectly uses the new logging API.
350-366: LGTM!The
messageMetadataextraction correctly usesthis.messageMetadataFieldand follows the same pattern as other field extractions. The metadata is properly included in the returnedProcessedMessageMetadataobject.
| return handler.messageLogFormatter(message) | ||
| protected override resolveMessageLog( | ||
| processedMessageMetadata: ProcessedMessageMetadata<MessagePayloadType>, | ||
| ): unknown { |
There was a problem hiding this comment.
we can make no assumptions about result here? it may be a record or it can be a string?
There was a problem hiding this comment.
It returns messageLogFormatter result which is declaring unknown as a returning value. I can change the signature to Record<string, unknown> | string but it would be potentially breaking change
…o remove_payload_from_logs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/core/lib/queues/AbstractQueueService.ts:
- Around line 347-349: The code references a non-existent property
this.messageDeduplicationId in AbstractQueueService; replace that with the
correct property name this.messageDeduplicationIdField (the field declared on
the class) wherever used (e.g., the ternary expression at the message access),
and ensure any related ts-expect-error/comment reflects the corrected property
name so the code compiles against the class property declaration.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/lib/queues/AbstractQueueService.tspackages/core/lib/types/queueOptionsTypes.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/lib/types/queueOptionsTypes.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/lib/queues/AbstractQueueService.ts (1)
packages/core/lib/types/queueOptionsTypes.ts (1)
ProcessedMessageMetadata(19-70)
⏰ 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). (19)
- GitHub Check: general (20.x, @message-queue-toolkit/sqs) / build
- GitHub Check: general (24.x, @message-queue-toolkit/outbox-core) / build
- GitHub Check: general (24.x, @message-queue-toolkit/s3-payload-store) / build
- GitHub Check: general (20.x, @message-queue-toolkit/sns) / build
- GitHub Check: general (22.x, @message-queue-toolkit/sqs) / build
- GitHub Check: general (22.x, @message-queue-toolkit/gcp-pubsub) / build
- GitHub Check: general (24.x, @message-queue-toolkit/sqs) / build
- GitHub Check: general (20.x, @message-queue-toolkit/outbox-core) / build
- GitHub Check: general (22.x, @message-queue-toolkit/gcs-payload-store) / build
- GitHub Check: general (22.x, @message-queue-toolkit/sns) / build
- GitHub Check: general (24.x, @message-queue-toolkit/redis-message-deduplication-store) / build
- GitHub Check: general (20.x, @message-queue-toolkit/gcp-pubsub) / build
- GitHub Check: general (24.x, @message-queue-toolkit/gcp-pubsub) / build
- GitHub Check: general (24.x, @message-queue-toolkit/gcs-payload-store) / build
- GitHub Check: general (24.x, @message-queue-toolkit/sns) / build
- GitHub Check: general (24.x, @message-queue-toolkit/metrics) / build
- GitHub Check: general (20.x, @message-queue-toolkit/metrics) / build
- GitHub Check: kafka (22.x) / build
- GitHub Check: kafka (24.x) / build
🔇 Additional comments (6)
packages/core/lib/queues/AbstractQueueService.ts (6)
117-120: LGTM!The property declaration follows the established pattern for field identifiers in this class and includes clear documentation.
164-164: LGTM!The initialization follows the established pattern with a sensible default value.
247-251: LGTM!The signature change correctly supports the new logging approach where custom message formatting is optional and receives processed metadata instead of raw message payloads.
253-277: LGTM!The new logging method properly structures metadata for debug logs while avoiding exposure of full message payloads. The use of
stringValueSerializerfor metadata and conditional inclusion of custom logs is appropriate.
313-326: LGTM!The control flow correctly gates debug logging and the variable rename to
debugMessageLoggingEnabledimproves code clarity.
350-354: LGTM!The metadata extraction follows the correct pattern established for other dynamic field access in this class, and the extracted metadata is properly included in the returned object.
Also applies to: 366-366
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/lib/queues/AbstractQueueService.ts (1)
693-711: Preserve messageMetadata in offloaded payload pointer.The
OffloadedPayloadPointerPayloadpreserves several fields (messageIdField,messageTimestampField,messageDeduplicationIdField, etc.) but notably omitsmessageMetadataField. This creates an inconsistency with the PR objective to "always include parsed message properties (id, type, metadata, timestamps, etc.)" in logs.When an offloaded message pointer is logged before payload retrieval (e.g., during publishing), the metadata will be
undefinedin the logs, even though other similar fields are preserved and logged.📝 Proposed fix to preserve messageMetadata
const result: OffloadedPayloadPointerPayload = { // Extended payload reference format payloadRef: { id: payloadId, store: storeName, size: serializedPayload.size, }, // Legacy format for backward compatibility offloadedPayloadPointer: payloadId, offloadedPayloadSize: serializedPayload.size, // @ts-expect-error [this.messageIdField]: message[this.messageIdField], // @ts-expect-error [this.messageTimestampField]: message[this.messageTimestampField], // @ts-expect-error [this.messageDeduplicationIdField]: message[this.messageDeduplicationIdField], // @ts-expect-error [this.messageDeduplicationOptionsField]: message[this.messageDeduplicationOptionsField], + // @ts-expect-error + [this.messageMetadataField]: message[this.messageMetadataField], }
🤖 Fix all issues with AI agents
In @packages/core/lib/queues/AbstractQueueService.ts:
- Around line 247-251: The override in AbstractPubSubConsumer has a return type
mismatch with the base method; update the method signature of resolveMessageLog
in class AbstractPubSubConsumer (the implementation at the location currently
declared to return unknown) to return unknown | null to match
AbstractQueueService.resolveMessageLog, since the implementation returns null in
multiple branches; ensure the method declaration and any related type
annotations use unknown | null so the override conforms to the base contract.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/lib/queues/AbstractQueueService.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/lib/queues/AbstractQueueService.ts (1)
packages/core/lib/types/queueOptionsTypes.ts (1)
ProcessedMessageMetadata(19-70)
⏰ 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). (20)
- GitHub Check: general (22.x, @message-queue-toolkit/schemas) / build
- GitHub Check: general (20.x, @message-queue-toolkit/sns) / build
- GitHub Check: general (22.x, @message-queue-toolkit/core) / build
- GitHub Check: general (24.x, @message-queue-toolkit/redis-message-deduplication-store) / build
- GitHub Check: general (24.x, @message-queue-toolkit/core) / build
- GitHub Check: general (24.x, @message-queue-toolkit/gcs-payload-store) / build
- GitHub Check: general (24.x, @message-queue-toolkit/sns) / build
- GitHub Check: general (22.x, @message-queue-toolkit/gcp-pubsub) / build
- GitHub Check: general (20.x, @message-queue-toolkit/core) / build
- GitHub Check: general (22.x, @message-queue-toolkit/sns) / build
- GitHub Check: general (22.x, @message-queue-toolkit/redis-message-deduplication-store) / build
- GitHub Check: general (20.x, @message-queue-toolkit/gcp-pubsub) / build
- GitHub Check: general (24.x, @message-queue-toolkit/amqp) / build
- GitHub Check: general (20.x, @message-queue-toolkit/sqs) / build
- GitHub Check: general (24.x, @message-queue-toolkit/metrics) / build
- GitHub Check: general (24.x, @message-queue-toolkit/sqs) / build
- GitHub Check: general (24.x, @message-queue-toolkit/gcp-pubsub) / build
- GitHub Check: general (22.x, @message-queue-toolkit/sqs) / build
- GitHub Check: kafka (22.x) / build
- GitHub Check: kafka (24.x) / build
🔇 Additional comments (4)
packages/core/lib/queues/AbstractQueueService.ts (4)
117-120: LGTM! Consistent field property pattern.The new
messageMetadataFieldfollows the established pattern for configurable field names and appropriately defaults to'metadata'. The JSDoc clearly indicates its purpose for debug logging.Also applies to: 164-164
313-326: Excellent refactoring to centralize logging.The change from inline logging to the centralized
logMessageProcessedmethod aligns perfectly with the PR objective to remove redundant logs. ThedebugMessageLoggingEnabledguard is clear and appropriately scoped.
347-354: LGTM! Metadata extraction follows established patterns.The extraction of
messageMetadatausing the configurablemessageMetadataFieldfollows the same pattern as other field extractions in this method. Thets-expect-errorannotation is appropriate for dynamic property access on the generic message type.Note: The past review comment about a typo in
messageDeduplicationIdextraction (line 348) has been correctly addressed - the code now properly referencesthis.messageDeduplicationIdField.Also applies to: 366-366
253-277: VerifystringValueSerializerhandles optional metadata gracefully.The code at line 265 passes
processedMessageMetadata.messageMetadatatostringValueSerializerwithout a guard, butmessageMetadatais an optional field (messageMetadata?: Record<string, unknown>). Confirm thatstringValueSerializerfrom@lokalise/node-corehandlesundefinedinput gracefully, or add a null check before calling it.
…o remove_payload_from_logs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core/vitest.config.ts`:
- Around line 18-22: The branch coverage threshold was lowered from 48 to 47 in
the thresholds object (keys: lines, functions, branches, statements); either
revert the branches value back to 48 or add tests to cover the missing branching
logic so you can keep branches at 48—locate the thresholds object in
vitest.config.ts and either restore branches: 48 or add unit tests targeting the
uncovered branches to justify the lower threshold.
3 participants
Context:
The reason for the change is that we want to avoid logging full message payload in debug logs as it can potentially contain private data. The aim is to log only some metadata that is generally useful for debugging and gathering metrics about the processing + give client an option to provide custom formatter if more logs are needed
Summary of the changes:
handleMessageProcessedmethod and we have single log there:Summary by CodeRabbit
Release Notes
New Features
logMessagesto enable logging,messageMetadataFieldto specify metadata location, and optionalmessageLogFormatterfor custom message formatting.Documentation