Limit `normalize` edge nodes #4687

timfish · 2022-03-05T14:34:35Z

I have been reviewing normalize and walk as part of #4579.

Looking through the recent PRs from @lobsterkatie it looks like most of the issues have already been sorted!

The last remaining thing I wanted to improve was limiting the overall work normalisation/serialisation has to do if users inadvertently log large objects. The existing implementation only considers depth but this can still result in huge output.

For example, the following will result in a huge serialised event, even with a normalisation depth set to 3:

const smallArray = new Array(1_000).fill('a');
const bigArray = new Array(10_000).fill(smallArrar);
console.log('my data', bigArray)

This PR moves walk inside normalize which allows counting enge nodes and bailing out when the limit is reached.

This means this:

JSON.stringify(someObj, walk)

now becomes:

JSON.stringify(normalize(someObj))

Alternatives

This PR halts object/array properties output when the limit is reached which favours properties that are encountered earlier and this may not be desirable.

An alternative would be to limit individual objects/arrays to a maximum number of properties/elements.

AbhiPrasad · 2022-03-07T12:20:16Z

packages/utils/src/object.ts

@@ -390,7 +389,7 @@ export function normalize(input: any, depth?: number): any {
 * eg. `Non-error exception captured with keys: foo, bar, baz`
 */
 // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
-export function extractExceptionKeysForMessage(exception: any, maxLength: number = 40): string {
+export function extractExceptionKeysForMessage(exception: unknown, maxLength: number = 40): string {


Unrelated change, let's move into another PR?

AbhiPrasad · 2022-03-07T12:23:03Z

packages/utils/src/object.ts

-    return serializeValue(value);
-  }
+    // If value implements `toJSON` method, call it and return early
+    if (typeof value?.toJSON === 'function') {


unfortunately optional chaining is less bundle size efficient.

Suggested change

if (typeof value?.toJSON === 'function') {

if (value != null && typeof value.toJSON === 'function') {

Could we just early return if value is undefined/null?

AbhiPrasad · 2022-03-07T12:25:33Z

packages/utils/src/object.ts

-  // Create source that we will use for the next iteration. It will either be an objectified error object (`Error` type
-  // with extracted key:value pairs) or the input itself.
-  const source = getWalkSource(value);
+    // Create an accumulator that will act as a parent for all future itterations of that branch


Suggested change

// Create an accumulator that will act as a parent for all future itterations of that branch

// Create an accumulator that will act as a parent for all future iterations of that branch

AbhiPrasad · 2022-03-07T12:29:16Z

packages/utils/src/object.ts

-// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
-export function walk(key: string, value: any, depth: number = +Infinity, memo: MemoFunc = memoBuilder()): any {
-  const [memoize, unmemoize] = memo;
+export function normalize(input: unknown, maxDepth: number = +Infinity, maxEdges: number = 10_000): any {


wait 10_000 works :0

Does it get transpiled properly?

We'll probably have to make maxEdges configurable like maxDepth

Yep, all gets transpiled by tsc.

Make maxEdges configurable, or remove the default value?

we should keep the default, but add an option similar to

sentry-javascript/packages/types/src/options.ts

Line 102 in 845aada

normalizeDepth?: number;

Also I wonder if this change will be disruptive to existing customers in any way. Any reason to choose 10_000?

10k is just a rough guestimate for what will likely not be noticed by customers but still limit crazy large objects.

I think the alternative of limiting number of properties/elements per object/array might be less disruptive and output more useful serialised output. The issue I see with this edges approach is that it's hard to pick a maxEdges value and when that number is reached, nothing more is outputted. This includes skipping properties further up the object tree.

The issue I see with this edges approach is that it's hard to pick a maxEdges value and when that number is reached, nothing more is outputted

Yeah good observation. I think limiting number of properties/elements per object/array is also much more intuitive to understand - and adjusting it will be much more obvious.

kamilogorek · 2022-03-07T14:04:52Z

packages/utils/src/object.ts

-    if (!Object.prototype.hasOwnProperty.call(source, innerKey)) {
-      continue;
+      if (edges >= maxEdges) {
+        acc[innerKey] = '[Max Edges Reached...]';


Maybe lets call this [MaxEdges ~] to make it consistent with [Circular ~]?

kamilogorek

Just a small nitpick, but other than that looks good

timfish · 2022-03-07T15:35:13Z

Closing in favour of #4689

timfish added 2 commits March 4, 2022 21:37

normalize

a3cecda

tighter types

2e8239b

AbhiPrasad requested review from kamilogorek and AbhiPrasad March 7, 2022 12:29

AbhiPrasad reviewed Mar 7, 2022

View reviewed changes

kamilogorek reviewed Mar 7, 2022

View reviewed changes

timfish mentioned this pull request Mar 7, 2022

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

Merged

timfish closed this Mar 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit `normalize` edge nodes #4687

Limit `normalize` edge nodes #4687

timfish commented Mar 5, 2022

AbhiPrasad Mar 7, 2022

AbhiPrasad Mar 7, 2022

AbhiPrasad Mar 7, 2022

AbhiPrasad Mar 7, 2022

AbhiPrasad Mar 7, 2022

AbhiPrasad Mar 7, 2022

timfish Mar 7, 2022

AbhiPrasad Mar 7, 2022 •

edited

Loading

timfish Mar 7, 2022

AbhiPrasad Mar 7, 2022

kamilogorek Mar 7, 2022

kamilogorek left a comment

timfish commented Mar 7, 2022

	if (typeof value?.toJSON === 'function') {
	if (value != null && typeof value.toJSON === 'function') {

	// Create an accumulator that will act as a parent for all future itterations of that branch
	// Create an accumulator that will act as a parent for all future iterations of that branch

Limit normalize edge nodes #4687

Limit normalize edge nodes #4687

Conversation

timfish commented Mar 5, 2022

Alternatives

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamilogorek left a comment

Choose a reason for hiding this comment

timfish commented Mar 7, 2022

Limit `normalize` edge nodes #4687

Limit `normalize` edge nodes #4687

AbhiPrasad Mar 7, 2022 •

edited

Loading