nextcloud / tables

🍱 Nextcloud tables app
https://apps.nextcloud.com/apps/tables
GNU Affero General Public License v3.0
143 stars 24 forks source link

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

Closed juliushaertl closed 1 month ago

juliushaertl commented 2 months ago

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?

blizzz commented 2 months ago

@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.