Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
ref(core): Renormalize event only after stringification errors (#4425)
Since at least 5.19.1, we've had a bug[1] 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](https:/getsentry/sentry-javascript/blob/6abb46374763a49b60b883e1190d5dfda8eda842/packages/utils/src/object.ts#L338-L341). Nonetheless, the problem persists. There are three possibilities: 1) The normalization function is somehow not getting called for certain events. 2) Data containing a circular reference is being added to the event between normalization and serialization. 3) 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 in `event.extra`), rather than instead of it. - Events which go through the initial normalization are tagged internally, so that if the serializer encounters an event which _doesn't_ have the tag, 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 in `extra` 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. [1] #2809
- Loading branch information
@lobsterkatie I was wondering why all events in my project have the
skippedNormalization
tag although they are passed through the normalization function (I'm using@sentry/node
with the default options) and I think this line is the reason. Maybe I'm misinterpreting what this code does but shouldn't it benormalized.sdkProcessingMetadata = ...
so that thebaseClientNormalized
flag is set in the returned and normalized event?