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

chore: Proper logs with error level and exception trace #1217

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jul 18, 2024

While testing a PR i noticed error logging is quite odd with just the message being logged without an exception trace.

Mostly moving from warning to error for 500 status codes in logging and adding the exception trace to it.

This might cause duplicate logging for cases where an InternalError is thrown somewhere deeper but that seems better for now then missing an exception trace.

@blizzz We probably want to cleanup the way exceptions are handled there, I feel the generic InternalError is not very helpful there and could just be a generic Exception. Then we can catch and log all non-specific exceptions in the middleware or in one unified controller method. WDYT?

@juliusknorr juliusknorr added bug Something isn't working 3. to review Waiting for reviews labels Jul 18, 2024
@juliusknorr juliusknorr self-assigned this Jul 19, 2024
@blizzz
Copy link
Member

blizzz commented Jul 22, 2024

@blizzz We probably want to cleanup the way exceptions are handled there, I feel the generic InternalError is not very helpful there and could just be a generic Exception. Then we can catch and log all non-specific exceptions in the middleware or in one unified controller method. WDYT?

The InternalError is also disturbing me, and I think commonly I experienced it in the past when it was not an internal issue actually. So, yes, I am all for improving the situation. Whether it should be a generic one, I am not sure yet, but having them dealt with in a Middleware sounds nice. Maybe it is a bit hidden, if you are not aware about it, but it is least in your way when working with Controllers and removes a lot of boiler plate.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr merged commit e236ba0 into main Aug 7, 2024
59 checks passed
@juliusknorr juliusknorr deleted the chore/exception-logging branch August 7, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants