-
Notifications
You must be signed in to change notification settings - Fork 19
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
Change exceptions logging: Add logging of exceptions' causes #38
Conversation
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.
Hi @dominh and thank you for the contribution.
I really like the idea of exposing the cause
from the exception as that can sometimes provide useful information about what happened. I have a few of concerns with the change as-is right now though:
- The output structure is changed in a backwards-incompatible way: we're moving from a flat, 3-fields structure to a multi-level one. People might be relying on the older structure and have their observability platforms configured with monitors and alerts, so we can't just change it like that unfortunately. A few options on how we could address this:
- Keep both the old and new structure, add a config setting to pick which one to use (defaults to OLD)
- Add a new
error_cause
field to the payload alongside the 3 existing ones. Since this is an entirely new field, this CAN be multi-level so you could have the same recursiveerror_cause: { class:, message:, backtrace:, cause: }
- The recursion is uncapped; this could have various unintended side-effects. Some observability systems may struggle with too many levels of data, and the log line size could go over the transfer/ingestion limit (e.g. Datadog has a hard limit of 1MB, but a suggested soft limit of 25KB).
- My suggestion here would be to cap the recursion to 1-2 levels deep. I doubt you'll need more than that under normal circumstances. This could also be made into a setting later if we need to provide more flexibility.
…auses_logging_max_depth` configs so the new incompatible error logging format has to be turn on manually
…h_causes` `causes_logging_max_depth` Settings
9b1f18b
to
5dcb397
Compare
Hi @iMacTia |
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.
Thank you for addressing my comments @dominh 🙏
I also prefer the approach of having 2 separate but configurable behaviours.
I believe adding the exception_cause
to the legacy behaviour would have also been a nice addition, but appreciate this is not a priority for you so I'll do it myself in a follow-up PR 🙂
Inspired by this change, I've opened #39 🙌 |
This pull request adds logging for
Exception#cause