Improved message logging #385
Conversation
WalkthroughThe PR refactors message logging across queue implementations (AMQP, GCP PubSub, SNS, SQS). It removes inline logging from publisher/consumer methods, centralizes logging through Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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)
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.
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.