-
Notifications
You must be signed in to change notification settings - Fork 150
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
bugfix: log errors from event handlers #740
Conversation
src/event-bus.ts
Outdated
.pipe(mergeMap((event) => from(handler.handle(event)))) | ||
.subscribe({ | ||
error: (error) => { | ||
this.logger.error(`${handler.constructor.name} produced an error.`); |
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.
@Sikora00 shouldn't it also apply for other *buses?
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.
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 could do this, but it is not that important there as
- It doesn't crash the app, because is used mostly in a scope of a request so will be handled by an exception filter and will be mapped to 500 or something.
- It is executed within the main flow, directly by the developer, so he can easily handle exceptions produced within CommandHandler, which is not possible with events.
The only place where that comes to my mind is s saga which can produce unhandable execution of the CommandBus 🤔. So because of that, it makes sense, but for QueryBus I don't see such a case.
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.
And maybe it is not a good idea to make a lot of logs from the framework level. It is reasonable here as there is no other way to recognize such an error, but if we add this to CommandBus often it will look like we are logging the most critical kind of logs which is later properly handled :/
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 agree that we could also add logging if the command was executed via Saga (and QueryBus doesn't make much sense as you stated)
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.
Done @kgajowy
Add log when event handler faced an error
before the app crash
Refs #409
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
When an event handler faces an uncaught error the app is crashing without any information of the source of the problem.
Issue Number: #409
What is the new behavior?
We get information that the crash is caused by a specific event handler.
Does this PR introduce a breaking change?
Other information