Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): Use correct version of event when tagging normalization #4780

Merged
merged 2 commits into from
Mar 25, 2022

Conversation

lobsterkatie
Copy link
Member

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. Thanks to @klaussner for pointing this out.

lobsterkatie referenced this pull request Mar 25, 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
@github-actions
Copy link
Contributor

github-actions bot commented Mar 25, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.61 KB (+0.02% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 62.76 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.22 KB (+0.04% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 56.25 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 22.74 KB (+0.01% 🔺)
@sentry/browser - Webpack (minified) 80.67 KB (0%)
@sentry/react - Webpack (gzipped + minified) 22.77 KB (+0.02% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.71 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 25.49 KB (+0.02% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 23.83 KB (+0.03% 🔺)

@lobsterkatie lobsterkatie force-pushed the kmclb-fix-baseClientNormalized-tagging branch from 904ffec to 6b1ae13 Compare March 25, 2022 04:20
@lobsterkatie lobsterkatie requested review from a team, Lms24 and AbhiPrasad and removed request for a team March 25, 2022 05:20
@lobsterkatie lobsterkatie merged commit 66ce092 into master Mar 25, 2022
@lobsterkatie lobsterkatie deleted the kmclb-fix-baseClientNormalized-tagging branch March 25, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants