-
Notifications
You must be signed in to change notification settings - Fork 220
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
Go 1.21 has changed function closures names and it disrupts grouping #712
Comments
Thanks for the detailed overview and a suggested fix, we'll look into this. |
Does #707 relate to this? |
@lawrencejones This only seems to happen specifically when calling a closured function correct? Or at least I can't see this behaviour otherwise. Could you share a deep sentry stack trace or two? I think I might have a simpler solution but I just need to see some more hairy/complex examples. :) What I'm trying to see is if the parent of the current frame's function name is always the unwanted prefix in the current frame's function name. Ie a little example I cooked up (other fields removed for clarity):
If that's the case then something like this might work: greywolve@b026f44 |
Summary
Nothing is going wrong wrong, but upgrading to go 1.21 almost entirely broke our ability to group errors.
Steps To Reproduce
This is because if you have a program like this:
It produces different function names depending on your go version:
Go 1.20
Go 1.21
Why is this a problem
The closured function returned by Logger is no longer consistently identifiable across the codebase using it's function name, because unlike in go 1.20 or lower where it is
Logger.func1
, with 1.21+ this has now acquired a prefix and isRun.Logger.func1
to acknowledge that it was constructed from withinRun
.While potentially useful for debugging or PGO, this significantly impacts event grouping for issues as you likely want to group on the stacktrace of the error returned in
Logger.func1
but those traces will no longer match if the prefix has been modified. The prefix will often be modified in standard Go apps because of the HTTP middleware pattern where you chain handlers like:Which means the prefix will be
Run.funcX
where X is the number of chains you have in yourRun
method, and any change to X – caused by moving your call around, or adding a new function – will change all stackframes of any errors produced byLogger.func1
for the purpose of any stacktrace grouping we have in Sentry.Expected Behavior
Errors generated from
Logger.func1
with otherwise similar frames should be grouped in Sentry, as they were with Go 1.20.Additional context
To fix this for our app, we have forked sentry-go and applied a patch here: incident-io#1
This seems to be working well. Where the problematic grouping for our app looked like this with Go 1.21 and no patch:
![image](https://private-user-images.githubusercontent.com/3518874/266070616-a8c877b0-c18c-435d-b4b6-409117467a16.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2MDQ3MDYsIm5iZiI6MTczOTYwNDQwNiwicGF0aCI6Ii8zNTE4ODc0LzI2NjA3MDYxNi1hOGM4NzdiMC1jMThjLTQzNWQtYjRiNi00MDkxMTc0NjdhMTYucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIxNSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMTVUMDcyNjQ2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9MzZlYTA5ZTg4N2IwZjlmMjc1ZTI4MDFiNWY1NDkwOWQxY2Q5OTcyZjFjMzZjYmM2OTZhNGJjODRlMzc0OTQ5NSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.KC7SmbDpSMcu75HYPuCFeueUDSFyqclC_3-2cpILLzY)
Where you can see
![image](https://private-user-images.githubusercontent.com/3518874/266070826-d02656f6-80d8-4ee3-819b-ab87041ee7af.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2MDQ3MDYsIm5iZiI6MTczOTYwNDQwNiwicGF0aCI6Ii8zNTE4ODc0LzI2NjA3MDgyNi1kMDI2NTZmNi04MGQ4LTRlZTMtODE5Yi1hYjg3MDQxZWU3YWYucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIxNSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMTVUMDcyNjQ2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9ZGY1MWNmYWVkNTc3OGQzMTc0ZGYwZjBhNzNiOTc3MjRiMzRiMzM5Yzk1ZGEyYjlmYmJlZjgzOTU1ZGY5M2FkNiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.LDH2oM-KZFYVnTrZxHRJPU3uf8mLaILGe0X5aLsTFyY)
Run.func61.Run.func61...
which caused all our grouping keys to bust, the patch now generates much cleaner grouping keys that look much closer to the grouping we had with Go 1.20:Some advice on whether you'd like us to submit this PR to upstream or if you'd like to handle this differently would be great, and you'll probably want to warn people that upgrading to Go 1.21 will cause their Sentry grouping to become much more volatile.
Thanks!
The text was updated successfully, but these errors were encountered: