Inconsistent normalization of Transaction/Span data

[rhcarvalho]

Summary

• Transaction.data is too easily truncated
• Span.data is never normalized

Details

Introduction

The JS SDKs have a feature to normalize events before serialization that prevents serializing very deeply nested objects.

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

Lines 97 to 106 in 4e552ec

	/**
	* Maximum number of levels that normalization algorithm will traverse in objects and arrays.
	* Used when normalizing an event before sending, on all of the listed attributes:
	* - `breadcrumbs.data`
	* - `user`
	* - `contexts`
	* - `extra`
	* Defaults to `3`. Set to `0` to disable.
	*/
	normalizeDepth?: number;

This is a great feature, however it does not apply consistently to Transaction/Span data.

Problem

• Transaction data is stored in Event.contexts.trace.data.
• Span data is stored in Event.spans[].data.

Unlike for breadcrumbs that normalization applies only to the .data property of each breadcrumb, normalization is applied to Event.contexts as a whole:

sentry-javascript/packages/core/src/baseclient.ts

Lines 303 to 323 in 4e552ec

	return {
	...event,
	...(event.breadcrumbs && {
	breadcrumbs: event.breadcrumbs.map(b => ({
	...b,
	...(b.data && {
	data: normalize(b.data, depth),
	}),
	})),
	}),
	...(event.user && {
	user: normalize(event.user, depth),
	}),
	...(event.contexts && {
	contexts: normalize(event.contexts, depth),
	}),
	...(event.extra && {
	extra: normalize(event.extra, depth),
	}),
	};
	}

What that means in practice is that, without changes to normalizeDepth, it is impossible to store objects in Transaction.data and have them sent to Sentry. Transaction.data is indirectly forced to be a flat object.

Users unaware of normalizeDepth can hack it and JSON.stringify the values they send, but that gives worse user experience 2 times:

1. Seeing [Object] where the expect some data; fix the frustration with JSON.stringify and then
2. Having a string instead of proper JSON means we cannot pretty print the value in the UI:

Manually stringified object

Regular JSON object

Possible solutions

Considering that all documented context types are flat objects, it is possible that we could relax the way how normalization applies to Event.contexts.

The new contexts.trace is the only key under contexts that may have nested objects under contexts.trace.data.

Option 1: liberal

An obvious option to consistently handle Transaction/Span data is to remove normalization from contexts altogether.

Option 2: restrictive

Alternatively, normalization should:

• not apply to contexts as a whole; and
• apply only to contexts.trace.data; and
• apply to spans[].data.

Option 3: Improve Contexts normalization

• not apply to contexts as a whole; and
• Instead, walk through all the keys in contexts and apply it to that values of the key

edit: this is not an option to fix the inconsistency, it would still treat Transaction.data and Span.data differently.

[HazAT]

Option 3️⃣
Reasoning:
Option 1
changes the whole behavior of the SDK again -> We can drop whole normalization once we decided what to do with the payload issue.

Option 2
I don't want to apply this now to span.data since it wasn't there in the first place

Option 3
It fixes the current problem while leaving most of the stuff the same as it was before

[dashed]

This shouldn't affect how we should display this on product side if any of the 3 options are implemented, right?

[HazAT]

@dashed nope

[rhcarvalho]

@HazAT

Option 3 would not fix the inconsistency. Just delays the time until someone will hit the [Object] result on the transaction level, where it would otherwise not happen for the very same content stored in a span.

[kamilogorek]

Related: #2539

[rhcarvalho]

In light of hearing that Event.contexts may also be used to store arbitrary data similar to Event.extra, here comes a slightly more convoluted solution scoped only to transactions and spans:

Option 4

• remember the original contexts.trace; and
• apply normalization to contexts as a whole; and
• overwrite contexts.trace with the original value prior to normalization.

The end result is normalization applies to every part of Event as before, except for contexts.trace.

The serialization of Transactions and Spans is then consistent, and neither Transaction.data nor Span.data get normalized.

[rhcarvalho]

FTR, we merged an implementation of Option 4.

[dashed]

@rhcarvalho as an optimization, would it be better to delete event.contexts.trace after saving the original value, and before the normalization?

[rhcarvalho]

@rhcarvalho as an optimization, would it be better to delete event.contexts.trace after saving the original value, and before the normalization?

Sounds good. I bet if we invest time on profiling the SDK we may find other/better optimizations in the hot path of events.