ref(core): Renormalize event only after stringification errors #4425
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since at least 5.19.1, we've had a bug which causes events not to be able to be serialized for transmission, because they contain circular references. In theory, this should never happen, because all events go through a normalization process which removes circular references. Nonetheless, the problem persists.
There are three possibilities:
The normalization function is somehow not getting called for certain events.
Data containing a circular reference is being added to the event between normalization and serialization.
The normalization function doesn't catch (and fix) all cases in which circular references exist.
In #3776, a debug flag was added which causes normalization to run twice, as a way to protect against possibility 1. As described in the above-linked issue, though, this hasn't solved the problem, at least not completely, as the serialization error is still occurring for some people.
This PR aims to improve on that initial debugging step, by making the following changes:
The initial attempt at serialization is wrapped in a try-catch, so that any errors that arise can be collected alongside the real event (as a
JSONStringifyError
tag and as a stacktrace inevent.extra
), rather than instead of it.Events which go through the initial normalization are flagged internally, so that if the serializer encounters an event which doesn't have the flag, it can note that (as a
skippedNormalization
tag on the event). In theory this should never show up, but if it does, it points to possibility 1.Renormalization has been moved so that it's part of the serialization process itself. If this fixes the problem, that points to possibility 2.
Renormalization has been wrapped in a try-catch, with a
JSON.stringify error after renormalization
event logged to Sentry (again with the error inextra
data) in cases where it fails. This is another situation which should never happen, but if it does, it points to possibility 3.Also, this is not specifically for debugging, but a bonus side effect of moving the renormalization to be part of serialization is that it allows us to only renormalize if theres's a problem, which eliminates a relatively computationally expensive operation in cases where it's not needed and therefore lets us ditch the debug flag.
P.S. Disclaimer: I know this isn't all that pretty, but my assumption is that this will stay in the SDK for a release or two while we (hopefully) finally solve the mystery, and then be pulled back out before we ship v7.