ref(core): Renormalize event only after stringification errors #4425
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
|
lobsterkatie
force-pushed
the
kmclb-debug-circular-JSON-error-normalize-before-sentry-request
branch
from
e65d1b1
to
6adc783
Compare
kamilogorek
suggested changes
Jan 20, 2022
AbhiPrasad
reviewed
Jan 20, 2022
lobsterkatie
force-pushed
the
kmclb-debug-circular-JSON-error-normalize-before-sentry-request
branch
from
6adc783
to
b26c806
Compare
lobsterkatie
force-pushed
the
kmclb-debug-circular-JSON-error-normalize-before-sentry-request
branch
from
b26c806
to
e9ae54d
Compare
kamilogorek
approved these changes
Jan 24, 2022
lobsterkatie
deleted the
kmclb-debug-circular-JSON-error-normalize-before-sentry-request
branch
lobsterkatie
added a commit
that referenced
this pull request
Mar 25, 2022
…4780) In #4425, various hacks were added to the SDK to try to diagnose the root cause of #2809. Unfortunately, one of those hacks (tagging when events are normalized) was applied to the pre-normalized version of the event rather than the normalized version that continues on through the event processing pipeline. This led to false positives, where events got tagged as if they'd skipped the normalization process even when they hadn't. This fixes that by adding the flag to the normalized version of the event instead.
lobsterkatie
added a commit
that referenced
this pull request
Apr 6, 2022
…4870) This is a follow up to #4425 and #4574, all in aid of debugging #2809, wherein serializing events leads to a circular reference error. One of the possible culprits is that the event is somehow skipping our usual normalization process, and one of the reasons that might happen is if `normalizeDepth` is set to something other than a number. We're already logging its value in problematic cases, and this change makes it so that we'll now log its type as well.
lobsterkatie
added a commit
that referenced
this pull request
Jun 16, 2022
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.
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
JSONStringifyErrortag 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
skippedNormalizationtag 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 renormalizationevent logged to Sentry (again with the error inextradata) 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.