Normalize Elixir anonymous function stack frames#119
Conversation
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
test/posthog/handler_test.exs:122-145
**Prefer parameterised test for index normalisation**
The team convention is to always use parameterised tests. This test only exercises index `250`, so the regex coverage for other common values (e.g. `0`, `1`, `9`, `99`) is untested. A parameterised variant using `ExUnit`'s `for` comprehension or table-driven setup would let you assert that any numeric index is stripped without duplicating the test body.
Reviews (1): Last reviewed commit: "Normalize anonymous function stack frame..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
noozo
force-pushed
the
normalize-elixir-anonymous-functions
branch
from
a64b739 to
26c07b8
Compare
|
Updated the test to use a parameterized Verified:
|
|
Hi @noozo, I'm not part of PostHog, but I wrote the original implementation for this part. Could you please explain a bit what this normalization is and what's it goal? |
@martosaur The issue we see on our usage is that everytime there is a stacktrace that contains anonymous functions (for instance "no function clause matching in anonymous fn/1") what ends up happening is that Posthog does not group those under the same error, because the fingerprint is different. Elixir's stacktrace will have something like "anonymous fn(250) in" where the 250 is a number attributed at runtime to that particular anonymous function. However, the error is the same; it is failing by not being able to pattern match on that anonymous function, but posthog sees it as a different error in error tracking. which means dismissing the fix will never dismiss all the other reports that will still remain in Posthog as errors. This fix attempts to squash those unique fingerprints so that posthog correctly groups the traces under the same error. |
|
@noozo Ah I see, this makes a lot of sense. Yeah grouping is probably the hardest part of error tracking and a lot of it is happening server side, so it's probably not the last time we'll have to look into it. Looking at anonymous function example, the number in If that's indeed what you're seeing, I think the proper fix would be to just always replace arguments with arity for formatting purposes. I am pretty deep into the weeds with this, so I can take it from here, but you can also give it a shot if you want, just let me know. |
Go for it, i just thought of submitting this mostly for the conversation :) thx!!! |
…es (#122) ## 💡 Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here. --> This PR replaces #119 The problem is that PostHog Error Tracking uses resolved function name as part of the fingerprint for issue grouping. This is fine, but in Elixir SDK function name comes from the stacktrace frames, which are formatted using `Exception.format_mfa(module, function, arity_or_args)`. `arity_or_args` is exactly that: arity or args. When it's arity, it's a simple integer that is stable across all calls. But when it's args, it's a dynamic value. So when included in the fingerprint, it prevents errors from grouping. The solution in this PR is to always use arity for formatting, even if we are passed arguments. ## 💚 How did you test it? Unit and integration tests ## 📝 Checklist <!--- Put an `x` in the boxes that apply --> - [x] I reviewed the submitted code. - [x] I added tests to verify the changes. - [x] I updated the docs if needed. - [x] No breaking change or entry added to the changelog. ### If releasing new changes - [x] Ran `sampo add` to generate a changeset file <!-- For more details check RELEASING.md --> Closes #119
|
Thank you for your help, guys! I've closed this because #122 by @martosaur is a better solution. That PR has been merged, a new release is happening soon. |
|
@noozo please yell at me if it still doesn't work. I'm currently not using this SDK in production, so have limited lived experience with it :( |
3 participants
Summary
Tests
Disclaimer: generated with Codex ;)