Skip to content

fix: Don't hang when capturing long stacktrace #4191

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 28, 2025
Merged

fix: Don't hang when capturing long stacktrace #4191

merged 4 commits into from
Mar 28, 2025
+85 −8

Conversation

szokeasaurusrex
Copy link
Member

Fixes #2764

@szokeasaurusrex szokeasaurusrex requested a review from a team as a code owner March 24, 2025 14:49
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/recursion-depth branch 2 times, most recently from 47f7812 to 09b2f95 Compare March 24, 2025 15:02
@codecov Codecov
Copy link

codecov bot commented Mar 24, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.55%. Comparing base (7406113) to head (335e9a3).
Report is 4 commits behind head on master.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
sentry_sdk/client.py 0.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4191      +/-   ##
==========================================
+ Coverage   79.50%   79.55%   +0.05%     
==========================================
  Files         141      141              
  Lines       15738    15748      +10     
  Branches     2675     2679       +4     
==========================================
+ Hits        12512    12528      +16     
+ Misses       2378     2372       -6     
  Partials      848      848
Files with missing lines Coverage Δ
sentry_sdk/utils.py 84.14% <100.00%> (+0.15%) ⬆️
sentry_sdk/client.py 78.86% <0.00%> (-0.31%) ⬇️

... and 3 files with indirect coverage changes

@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/recursion-depth branch 2 times, most recently from 7e31f86 to 7642fc2 Compare March 25, 2025 08:54
@szokeasaurusrex
fix: Don't hang when capturing long stacktrace
Loading
Loading status checks…
2fa2d46 
Fixes #2764
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/recursion-depth branch from 7642fc2 to 2fa2d46 Compare March 25, 2025 09:07
szokeasaurusrex and others added 2 commits March 25, 2025 10:21
@szokeasaurusrex
try reverting annotated stuff

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
Loading
Loading status checks…
a547e17
@antonpirker
Merge branch 'master' into szokeasaurusrex/recursion-depth

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
Loading
Loading status checks…
309e6ef
sentrivana
sentrivana approved these changes Mar 26, 2025
View reviewed changes
Copy link
Contributor

@sentrivana sentrivana left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this looks ok, left a minor comment. I assume you've verified that relay/Sentry has no issues with a None stacktrace -- if not please do before merging.

@szokeasaurusrex
Copy link
Member Author

I think this looks ok, left a minor comment. I assume you've verified that relay/Sentry has no issues with a None stacktrace -- if not please do before merging.

Yep, here is an example event with None stack trace. Interestingly, although the _meta is set correctly from the AnnotatedValue in the event JSON, this information is not shown in the Sentry UI. I am assuming this would need to be fixed/implemented in the Sentry UI though, don't think it's an SDK problem

@sentrivana
Copy link
Contributor

Interestingly, although the _meta is set correctly from the AnnotatedValue in the event JSON, this information is not shown in the Sentry UI. I am assuming this would need to be fixed/implemented in the Sentry UI though, don't think it's an SDK problem

Also don't think so, afaik Sentry has been mostly ignoring the _meta field except for some cases (dropped spans come to mind as a recent example).

@szokeasaurusrex
add explanation comments

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
Loading
Loading status checks…
335e9a3
szokeasaurusrex
szokeasaurusrex commented Mar 26, 2025
View reviewed changes
sentry_sdk/utils.py
Comment on lines +744 to +758
# Process at most MAX_STACK_FRAMES + 1 frames, to avoid hanging on
# processing a super-long stacktrace.
for tb, _ in zip(iter_stacks(tb), range(MAX_STACK_FRAMES + 1))
] # type: List[Dict[str, Any]]

if frames:
if len(frames) > MAX_STACK_FRAMES:
# If we have more frames than the limit, we remove the stacktrace completely.
# We don't trim the stacktrace here because we have not processed the whole
# thing (see above, we stop at MAX_STACK_FRAMES + 1). Normally, Relay would
# intelligently trim by removing frames in the middle of the stacktrace, but
# since we don't have the whole stacktrace, we can't do that. Instead, we
# drop the entire stacktrace.
exception_value["stacktrace"] = AnnotatedValue.removed_because_over_size_limit(
value=None
)
Copy link
Member Author

Choose a reason for hiding this comment

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

@sentrivana what do you think about these comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm

@szokeasaurusrex szokeasaurusrex merged commit e432fb4 into master Mar 28, 2025
139 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/recursion-depth branch March 28, 2025 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sentrivana sentrivana

@antonpirker antonpirker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SDK stuck in processing RecursionError:maximum recursion depth exceeded events
3 participants
@szokeasaurusrex @sentrivana @antonpirker