Skip to content

Conversation

@kayw-geek
Copy link
Contributor

@kayw-geek kayw-geek commented Dec 8, 2022

The current method to limiting the call stack is not suitable for all users of the Sentry SDK, for example, some users have added additional logic based on Sentry, which can cause the call stack to exceed the limit and result in an unexpected unhandled flag.

Copy link
Member

@cleptric cleptric left a comment

Choose a reason for hiding this comment

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

I think the current default is very reasonable.
Can you provide us with some concrete example of where this would not work?

@kayw-geek
Copy link
Contributor Author

kayw-geek commented Dec 8, 2022

I think the current default is very reasonable. Can you provide us with some concrete example of where this would not work?

OK, I have some custom extra logic in our own private library laravel-sentry, so I didn't get to report helper until the 11th call stack.

image
image

@kayw-geek kayw-geek requested a review from cleptric December 8, 2022 04:01
@kayw-geek
Copy link
Contributor Author

I don't think this is an individual case.
and I don't think limiting the number of call stack calls has much significance in terms of performance, because when we find this report helper in a loop, it will return and stop the loop.

@stayallive
Copy link
Collaborator

stayallive commented Dec 8, 2022

I don't think this is an individual case.

You are the first to report this, so unless we have a lot more people chip in, it might be. In a "normal" Laravel application we expect the stack to be around 5-6 long before hitting the report helper frame so you are definitly doing a lot of indirection.

Regardless, we limit the amout of stack frames to prevent doing work that is not needed, Sentry threads a fine line between reporting accurate errors and being performant enought to not cause more problems than it's reporting.

That said, I'd be willing to bump it to 20 because I can see 10 might be a little on the thin side, but making it unbounded is not a good idea since not all errors go through report and the loop is not "free" especially with your example where you have 80+ stackframes :)

20 seems to also cover your case and then some, sounds good?

@kayw-geek
Copy link
Contributor Author

Sure, I will change the limit count to 20.

@kayw-geek kayw-geek force-pushed the bugfix/fix-report-helper-be-mark-unhandled branch 2 times, most recently from f4a0adc to 2600663 Compare December 8, 2022 10:29
@kayw-geek kayw-geek changed the title Delete debug trace limit in makeAnEducatedGuessIfTheExceptionMaybeWasHandled method Change debug trace limit in makeAnEducatedGuessIfTheExceptionMaybeWasHandled method Dec 8, 2022
Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

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

Thanks!

@kayw-geek kayw-geek force-pushed the bugfix/fix-report-helper-be-mark-unhandled branch from 2600663 to 764c5df Compare December 8, 2022 11:09
@cleptric cleptric merged commit bffd417 into getsentry:master Dec 9, 2022
@kayw-geek kayw-geek deleted the bugfix/fix-report-helper-be-mark-unhandled branch December 9, 2022 02:05
@kayw-geek
Copy link
Contributor Author

Hi @cleptric, Is there any plan to release a new tag recently?

@cleptric
Copy link
Member

cleptric commented Jan 9, 2023

@kayw-geek We'll get something out this week.

@cleptric cleptric self-assigned this Jan 9, 2023
@kayw-geek
Copy link
Contributor Author

@kayw-geek We'll get something out this week.

Thanks!

@cleptric
Copy link
Member

cleptric commented Jan 12, 2023

@kayw-geek
Copy link
Contributor Author

Thanks, I see it!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants