feat: Add ensureNoCircularStructures experiment to help debug serialization bugs #3776
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report
|
kamilogorek
requested review from
a team,
lobsterkatie and
rhcarvalho
and removed request for
a team
5 tasks
rhcarvalho
reviewed
Jul 2, 2021
|
@rhcarvalho we already have 300LoC tests around it 😅 Nothing should(tm) go wrong, as this util is tested throughout, but it can just be costly with larger and more intensive payloads numbers. This will not be documented nor publicly exposed in any way, but we can ask customers/GH people to use it and get more insights at very little cost (bundle size wise) as it's optimized to a single LoC. |
lobsterkatie
added a commit
that referenced
this pull request
Jan 24, 2022
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://github.com/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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
ref: #2809
We don't want to expose this publicly, as it may be costly to normalize every event in its entirety (keep in mind that it defaults to infinite depth, but will break recursive cycles) but should be useful to expose what is triggering serialization issues in some cases.