-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Concurrent consumers support #232
base: main
Are you sure you want to change the base?
Conversation
processingResult: MessageProcessingResult, | ||
messageId?: string, | ||
) { | ||
const messageTimestamp = message ? this.tryToExtractTimestamp(message) : undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would do the check on the log level for this.logger first and skip it entirely if it's below debug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see Logger is a generic interface and it does not expose any property for log level. We could check level
property as it's available in pino
, but it won't apply all of the time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should switch to CommonLogger interface from the node-core, it exposes the isLevelEnabled method from pino
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we should probably change it everywhere and since logger is provided from outside that will be breaking change. Should I address it in separate PR then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the draft PR with logger update: #233
I will add level
condition there after this PR is merged
Co-authored-by: Igor Savin <[email protected]>
…essage-queue-toolkit into concurrent_consumers_support
Changes:
consumer
field inAbstractSqsConsumer
, we useconsumers
array nowconcurrentConsumersAmount
optionmessageProcessingTime
andmessageType
to the log for better observability