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

Add max_nesting of 10 to breadcrumbs data serialization #2583

Merged
merged 1 commit into from
Mar 17, 2025

Conversation

sl0thentr0py
Copy link
Member

@sl0thentr0py sl0thentr0py commented Mar 17, 2025

This is trimmed in relay at a depth of 5.
We allow upto 10 to have some leeway but if we have a really large data object we just drop it to avoid SystemStackError.

Closes #2393 and #2397

@sl0thentr0py sl0thentr0py requested a review from solnic March 17, 2025 15:09
@sl0thentr0py sl0thentr0py force-pushed the neel/limit-breadcrumb-json-recursion branch from 62bd21c to 240f514 Compare March 17, 2025 15:11
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

If I understand correctly, this change causes the entire breadcrumb data to be deleted if the nesting is at least 5. Why not just strip the portions which are too deeply nested?

I suppose if Relay is throwing away the entire data when there is too much nesting, this is okay, but if relay only removes the part that is too deeply nested, the SDK should have the same behavior.

@sl0thentr0py
Copy link
Member Author

sl0thentr0py commented Mar 17, 2025

valid, but that will require generalized databag trimming like python which is not implemented in ruby (and we don't plan to).
getting this in is more valuable since we're doing arbitrary recursion, we can revisit this separately.
If people have some large data in breadcrumbs, the source is some nonsense anyway that shouldn't have been there in the first place so this is fine for now.

@sl0thentr0py sl0thentr0py force-pushed the neel/limit-breadcrumb-json-recursion branch from 240f514 to b3300ea Compare March 17, 2025 15:28
@sl0thentr0py
Copy link
Member Author

alright, let's do this, I'll make the depth 10, so we allow some leeway instead of dropping the whole thing.

@sl0thentr0py sl0thentr0py force-pushed the neel/limit-breadcrumb-json-recursion branch from b3300ea to 70161d0 Compare March 17, 2025 15:34
This is [enforced in [relay](https://github.com/getsentry/relay/blob/bcbfa7e7db6e4a8f3f0383270fbb2b5cfe668770/relay-event-schema/src/protocol/breadcrumb.rs#L111) at a depth of 5.
We allow upto 10 to have some leeway but if we have a really large data
object we just drop it to avoid SystemStackError.

Closes #2393 and #2397
@sl0thentr0py sl0thentr0py force-pushed the neel/limit-breadcrumb-json-recursion branch from 70161d0 to 387f2a2 Compare March 17, 2025 15:35
@sl0thentr0py sl0thentr0py changed the title Add max_nesting of 5 to breadcrumbs data serialization Add max_nesting of 10 to breadcrumbs data serialization Mar 17, 2025
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

Nice! So if I understand correctly, the limit of 10 is so that if the depth of 5 is only slightly exceeded, we allow Relay's logic to trim down to a depth of 5 (preserving the portion that is less than 5 deep), but if we exceed the limit of 10, we drop the whole thing to avoid the error in Ruby?

I think that is reasonable. Although, if we can avoid the recursion error altogether, that would be the best solution (i.e. by replacing the recursive function which causes the error with an iterative one). Would this be feasible?

@sl0thentr0py
Copy link
Member Author

yes with 10 we will drop the whole thing
we will likely not replicate the python serializer mess here so there is no way to avoid this behavior for now, which is fine.

@sl0thentr0py sl0thentr0py merged commit ec34db9 into master Mar 17, 2025
137 of 138 checks passed
@sl0thentr0py sl0thentr0py deleted the neel/limit-breadcrumb-json-recursion branch March 17, 2025 17:38
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.

Restrict breadcrumb JSON generate/parse to depth 5
2 participants