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

ref(trimming): Keep frames from both ends of the trace #3905

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

Dav1dde
Copy link
Member

@Dav1dde Dav1dde commented Aug 8, 2024

Currently we trim frames from only one end of the stacktrace, this works decently well except for recursion errors where you would also be interested in what triggered the recursion initially.

Adjust the trimming to keep some frames from the beginning of the trace, the majority is still taken from the end.

@Dav1dde Dav1dde requested a review from a team as a code owner August 8, 2024 08:38
@Dav1dde Dav1dde self-assigned this Aug 8, 2024
Copy link
Contributor

@olksdr olksdr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM!

But I'm mostly curious if this change makes sense product-wise.
Let's involved the requester of this change add more info on why exactly this needed and if this doesn't break the current product experience.

@realkosty
Copy link

But I'm mostly curious if this change makes sense product-wise. Let's involved the requester of this change add more info on why exactly this needed and if this doesn't break the current product experience.

@olksdr you're right, we should involve product as this has potential to impact grouping of existing issues.
@rachrwang @lobsterkatie
Here is the context and customer request: https://sentry.slack.com/archives/C019637C760/p1723069888501309
First step might be get a sense of what %-age of events has between 200 and 250 frames -> the ones potentially affected.
On the other hand, newly added "bottom" 50 frames that were previously trimmed starting to match existing stack trace/fingerprint/ownership rules probably more of a feature than bug.

@lobsterkatie
Copy link
Member

IDK that I have any expertise here, but naively I guess my question would be, what would this look like in the product? (I actually don't know what it looks like when we trim only from one end, either.) Will it be clear to the user what's happened and why?

@realkosty
Copy link

Currently product shows no indication that frames were trimmed. The only way to know that is guessing that whatever ends up as the first frame doesn't look like thread base or main entry point.
This change without product support (some kind of "..." in UI) might make in in some sense more confusing as it may be hard to understand where the splice point is

@SurenNihalani
Copy link

Can we get traction on this? This is making life hard while debugging recursion bugs

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

G2G. I would recommend to show in the user interface that frames were trimmed.

@Dav1dde Dav1dde enabled auto-merge (squash) August 26, 2024 08:58
@Dav1dde Dav1dde merged commit 6f94662 into master Aug 26, 2024
24 checks passed
@Dav1dde Dav1dde deleted the dav1d/trim-frames branch August 26, 2024 09:10
@SurenNihalani
Copy link

Thanks for merging this. when do we expect this to deployed onto sentry?

Stretch ask: It'd be nice to just deduplicate duplicate frames instead of trimming

@realkosty
Copy link

@SurenNihalani Most likely deployed already.

Related: getsentry/sentry#61034

@Dav1dde
Copy link
Member Author

Dav1dde commented Aug 28, 2024

Thanks for merging this. when do we expect this to deployed onto sentry?

Should be out already!

Stretch ask: It'd be nice to just deduplicate duplicate frames instead of trimming

No, that's definitely a good idea, opened an issue #3956, feel free to leave a comment if you think I missed something in the description!

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

Successfully merging this pull request may close these issues.

6 participants