Improved message logging #385
Conversation
📝 WalkthroughWalkthroughRefactors per-message logging: removes inline logging in publishers/consumers, centralizes logging around ProcessedMessageMetadata, changes resolveMessageLog signatures to accept metadata, makes messageLogFormatter optional, adds messageMetadataField and messageMetadata handling, and removes the default log formatter export. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)packages/gcp-pubsub/lib/pubsub/AbstractPubSubConsumer.ts (1)
⏰ 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)
🔇 Additional comments (1)
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.
| this.definition = eventDefinition | ||
| this.messageType = options?.messageType | ||
| this.handler = handler | ||
| this.messageLogFormatter = options?.messageLogFormatter ?? defaultLogFormatter |
There was a problem hiding this comment.
why remove default formatter? if not set, we prefer not logging at all?
There was a problem hiding this comment.
We are still always logging message metadata, and if custom formatter is provided, we are additionally including formatted message there. If it's not provided, we are skipping this additional property
| 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.
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
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.