Skip to content
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

Stack parsers use both ? and <anonymous> to denote anonymous functions #5523

Closed
Tracked by #9508
timfish opened this issue Aug 4, 2022 · 10 comments
Closed
Tracked by #9508
Milestone

Comments

@timfish
Copy link
Collaborator

timfish commented Aug 4, 2022

Chrome/v8 stack frames use ? to denote anonymous functions but we also use <anonymous> in a few places.

function: frame.function || '?',

const defaultFunctionName = '<anonymous>';

@AbhiPrasad
Copy link
Member

Hey - you're right that this is kind of a weird inconsistency. @kamilogorek any ideas on why this happens?

@timfish
Copy link
Collaborator Author

timfish commented Aug 5, 2022

If we change this, will it impact fingerprinting?

@AbhiPrasad
Copy link
Member

Mostly likely yes - we'll need to understand the grouping impact, but we can get folks more involved there (and run some queries in Sentry to determine possible impact).

@kamilogorek
Copy link
Contributor

kamilogorek commented Aug 8, 2022

@kamilogorek any ideas on why this happens?

Historical reasons. It was called <anonymous> (the same value as anonymous URLs/files in some old browsers) since raven SDK, and we kept it like that. ? comes directly from Tracekit, which was also not changed since the old SDK, as the grouping strategy back then did not group them together afair. Changing this would cause some new issues to pop up.

Now, this will not affect any grouping (at least for one version of it), due to those frames being skipped, see: https://github.com/getsentry/sentry/blob/46d9c2aa672b8abe2e26e52c60e9ffc32eba9c23/src/sentry/grouping/strategies/newstyle.py#L394-L398

Also original references:

data: {function: orig.name || '<anonymous>'}

@github-actions
Copy link
Contributor

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@timfish
Copy link
Collaborator Author

timfish commented Aug 31, 2022

I'm going to submit a PR to change everything to ?...

@AbhiPrasad
Copy link
Member

yeah that sounds good to me - will be more bundle size efficient. Maybe we should also write this in the developer docs somewhere.

@timfish
Copy link
Collaborator Author

timfish commented Aug 31, 2022

This could potentially be a breaking change since customers might be using RewriteFrames and expecting some specific anonymous frames?

Seems highly unlikely anyone is doing this but worth considering.

@AbhiPrasad
Copy link
Member

How about we just backlog for the next major bump then? I don't mind tabling this patch until then since we treat it identically for grouping strategies.

@AbhiPrasad
Copy link
Member

resolved with #10732

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants