Improve Error Tracking for complex errors#85
Improve Error Tracking for complex errors#85martosaur wants to merge 5 commits intoPostHog:masterfrom
Conversation
|
Hey @martosaur! Hopefully PostHog/posthog#48689 will help solve the newlines problem. |
rafaeelaudibert
left a comment
rafaeelaudibert
left a comment
There was a problem hiding this comment.
Tysm! This is good, as always. Left some comments for possible future improvements.
| defp do_type(%{msg: {:report, %{label: {:gen_server, :terminate}}}}) do | ||
| "GenServer terminating" | ||
| end | ||
|
|
||
| defp do_type(%{msg: {:report, %{label: {Task.Supervisor, :terminating}}}}) do | ||
| "Task terminating" | ||
| end | ||
|
|
||
| defp do_type(%{msg: {:report, %{label: {:gen_statem, :terminate}}}}) do | ||
| ":gen_statem terminating" | ||
| end | ||
|
|
||
| defp do_type(%{msg: {:report, %{label: {:proc_lib, :crash}}}}) do | ||
| "Process terminating" | ||
| end |
There was a problem hiding this comment.
This means we'll group all of this under the same issue. I assume this is not a problem because this is the underlying reason and we'll group under the actual error, but we might wanna change this in the future.
There was a problem hiding this comment.
I think this shouldn't be a problem, because by default fingerprinting uses not just type but also the latest application frame, which should be different 🤔
|
|
||
| defp stacktrace(_event, _), do: %{} | ||
|
|
||
| defp do_stacktrace([_ | _] = stacktrace, in_app_modules) do |
There was a problem hiding this comment.
Feels like an extra indirection for no reason, could've inlined this under stacktrace/2 above
There was a problem hiding this comment.
But stacktrace/2 has two heads at this point? We could use case, sure, but that kind of goes against the grain of all other functions in the module, esp. type + do_type. Or am I misunderstanding you comment?
|
@martosaur tests are failing, mind checking? |
|
Oof, I forgot that this whole thing will only be working nicely with Elixir 1.18+, previous versions will behave as before. I'll have to add test variants for older versions, I'll do this over the weekend! |
|
@martosaur all good! just remember to re-request my review once you've updated it. I've merged PostHog/posthog#48689 to hopefully make this more readable too |
2 participants
Error Tracking supports chained exceptions. This actually maps fairly well on OTP errors that are usually caused by some other underlying error. Let's take advantage of this!
In this PR:
crash_reasonin the logger metadata, we don't consider it the only important part, but rather use it as a sign of a chained error. We create a dedicated$exception_listentry for crash reason, and then add an additional one based on the rest of the log event. This also allows us to better support "handled" errors that addcrash_reason, e.g. `Logger.error("Unexpected error", crash_reason: {exception, STACKTRACE}). Previously, the string "unexpected error" would be simply ignored. Now it will be the first error in chain.enrich_metadatastage to logger handler that will extract some useful info from known OTP reports and put them into event properties.Before
After
It would be even better with newlines of course...