Skip to content
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

feat(rabbitmq): add verbose logging #264

Closed

Conversation

arjenvdhave
Copy link
Contributor

Add verbose logging arround the message handling, this makes debugging issues easier

Add verbose logging arround the message handling, this makes debugging issues easier
@WonderPanda
Copy link
Collaborator

@arjenvdhave For some reason this is breaking the integration tests:
https://dev.azure.com/golevelup/nestjs/_build/results?buildId=395&view=logs&j=8f9c5ecd-7085-541f-b145-4b6a0a921008&t=77075074-6a08-5725-577a-0c8f3817f3b3

I'm not sure at a glance yet why this change would have that affect but we will need to figure it out before this can be merged through

@arjenvdhave
Copy link
Contributor Author

@WonderPanda i managed to reproduce the error local
It seems that for some reason just adding the this.logger.verbose('log message') to the channel.consume makes the event handling super slow.

With a console.log('log message') it is even slower.

I tried to set the log level in the NestJS testing module, but this doesn't seem to do anything.

Do you think i should add an option verboseLogging to the RabbitMQConfig to enable this debug logging?
I made this PR because it took me way to long to discover a stupid bug i made in my own project, so for dev purposes verbose logging can be useful

@WonderPanda
Copy link
Collaborator

@arjenvdhave I do think it is a really good idea for us to have some kind of official hook for people to be able to do verbose logging. Its super useful for diagnostics.

I'm open to adding an option to the module configuration to turn on logging but I'm very curious about why adding a log statement would produce this kind of issue and want to make sure that it doesn't indicate something else wrong going on

@arjenvdhave
Copy link
Contributor Author

@WonderPanda I will continue with it next week and make it an option.
Maybe it is slower because the logging is a sync action?

I will try putting it in an async method and see what will happen to the test speed then.

@underfisk
Copy link
Contributor

@arjenvdhave Adding a verbose logging option seems reasonable to me, having this by default can pollute existing applications that are happy with the current logging. If this is still relevant, could we have an update?

@arjenvdhave
Copy link
Contributor Author

@underfisk I think this is still a good addition to the lib.
However I was never able to fix te tests, for some reason the fail with the extra logging.

The product im working on has moved away from RabbitMQ, so i don't have a working setup now anymore.
Do you maybe have a clue why the tests are failing?

@underfisk
Copy link
Contributor

@underfisk I think this is still a good addition to the lib. However I was never able to fix te tests, for some reason the fail with the extra logging.

The product im working on has moved away from RabbitMQ, so i don't have a working setup now anymore. Do you maybe have a clue why the tests are failing?

I can investigate why they were failing, i just wanted to make sure this is something worth diving in

@underfisk
Copy link
Contributor

@arjenvdhave There's a new PR that has been merged recently that allows you to provide a custom logger therefore you can customize your logs

@underfisk underfisk closed this Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants