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

feat(utils): Limit normalize maximum properties/elements #4689

Merged
merged 5 commits into from
Mar 16, 2022

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Mar 7, 2022

Limits number of properties/elements serialised for an object/array. An improvement over #4687 because it doesn't bail out of normalisation when the limit is reached.

This protects against huge objects being serialised if users inadvertently log huge objects.

I chose a limit of 1000 properties/elements. If this is deemed too low for a minor release, we could increase this and reduce it again for v7?

NOTE
This adds normalizeMaxBreadth to init options (documented in getsentry/sentry-docs#4844)

Ref: https://getsentry.atlassian.net/browse/WEB-711

@timfish timfish marked this pull request as ready for review March 7, 2022 15:34
@timfish timfish changed the title Optionally limit normalize maximum properties/elements Limit normalize maximum properties/elements Mar 7, 2022
Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

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

I left a few language edits on the current implementation, but I have a more fundamental question.

I get why to limit the breadth of any given object/array, but why do we have to move walk inside of normalize to do it? That feels like a change which a) should happen in a separate PR if it happens, and b) makes the code harder to read and which I'd therefore try to talk you out of. 😛

packages/types/src/options.ts Outdated Show resolved Hide resolved
packages/utils/src/object.ts Outdated Show resolved Hide resolved
packages/utils/src/object.ts Outdated Show resolved Hide resolved
packages/utils/src/object.ts Outdated Show resolved Hide resolved
@timfish
Copy link
Collaborator Author

timfish commented Mar 7, 2022

but why do we have to move walk inside of normalize to do it?

Yep, completely unnecessary and copied from the previous PR where it made more sense!

@timfish
Copy link
Collaborator Author

timfish commented Mar 8, 2022

I moved walk back out of normalize and the PR diff is now much more sensible 😳

Need to do a docs PR to add this option.

@lobsterkatie
Copy link
Member

lobsterkatie commented Mar 8, 2022

I moved walk back out of normalize and the PR diff is now much more sensible 😳

Nice. 🙂

This looks good! My only other question is naming. Given that objects have properties but arrays have elements, and this applies to both, what would you think of something more generic like normalizeMaxBreadth?

@timfish
Copy link
Collaborator Author

timfish commented Mar 15, 2022

I've changed it to normalizeMaxBreadth

Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

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

LGTM!

@lobsterkatie
Copy link
Member

@timfish - Not sure if you have merge permissions, so I'm going to go ahead and pull this in. LMK if that's something you can do, and if so, I'll leave it for you in future.

@timfish
Copy link
Collaborator Author

timfish commented Mar 16, 2022

I don't have merge permissions. Merge away!

@lobsterkatie lobsterkatie changed the title Limit normalize maximum properties/elements feat(utils): Limit normalize maximum properties/elements Mar 16, 2022
@lobsterkatie lobsterkatie merged commit 72aed62 into getsentry:master Mar 16, 2022
lobsterkatie added a commit to getsentry/sentry-docs that referenced this pull request Mar 22, 2022
This documents the new `normalizeMaxBreadth` option added to JS-based SDKs in getsentry/sentry-javascript#4689, which controls how many properties or entries any given object or array will contain after normalization.
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.

2 participants