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(integration): Ensure dedupe integration ignores non-errors #7172
Conversation
@@ -23,6 +23,12 @@ export class Dedupe implements Integration { | |||
*/ | |||
public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void { | |||
const eventProcessor: EventProcessor = currentEvent => { | |||
// We want to ignore any non-error type events, e.g. transactions or replays | |||
// These should never be deduped, and also not be compared against as _previousEvent. | |||
if (currentEvent.type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In these cases it's so weird that error events don't have a type....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jup, backwards compatibility I guess 😅
size-limit report 📦
|
As a consequence, this might lower total event volume on the browser SDKs! |
Replay SDK metrics 🚀
develop |
Revision | LCP | CLS | CPU | JS heap avg | JS heap max | netTx | netRx | netCount | netTime |
---|---|---|---|---|---|---|---|---|---|
e9eec27 | +61.38 ms | -0.00 ms | +6.04 pp | +927.84 kB | +1.05 MB | +2.21 kB | +41 B | +1 | +88.58 ms |
d604022 | +58.83 ms | -0.00 ms | +7.65 pp | +930.16 kB | +1.05 MB | +2.21 kB | +41 B | +1 | +109.63 ms |
a961e57 | +54.75 ms | -0.00 ms | +6.50 pp | +929.18 kB | +1.07 MB | +2.21 kB | +41 B | +1 | +92.73 ms |
f7c0a2f | +46.14 ms | +0.00 ms | +6.37 pp | +921.47 kB | +1.06 MB | +2.23 kB | +41 B | +1 | +207.30 ms |
cb19818 | +57.16 ms | +0.00 ms | +11.95 pp | +1.07 MB | +2.21 MB | +2.52 kB | +41 B | +1 | +111.50 ms |
ee301c3 | +71.07 ms | -0.00 ms | +12.64 pp | +1.07 MB | +2.22 MB | +2.55 kB | +41 B | +1 | +94.67 ms |
93c4759 | +61.10 ms | -0.00 ms | +12.72 pp | +1.08 MB | +2.19 MB | +2.57 kB | +41 B | +1 | +116.75 ms |
274f489 | +63.60 ms | -0.00 ms | +11.56 pp | +1.08 MB | +2.2 MB | +2.56 kB | +41 B | +1 | +116.60 ms |
4827b60 | +58.67 ms | +0.00 ms | +18.38 pp | +1.07 MB | +2.22 MB | +2.6 kB | +41 B | +1 | +91.21 ms |
c3806eb | +79.85 ms | -0.00 ms | +12.10 pp | +1.05 MB | +2.16 MB | +2.54 kB | +41 B | +1 | +93.58 ms |
Last updated: Tue, 14 Feb 2023 10:06:42 GMT
Currently, the dedupe integration will never drop non-error events, but it will still consider them as
_previousEvent
. Thus, if we have e.g. a sequence of events like this:It will capture
error 1
twice.This PR fixes this so we only consider error events for deduping.
This may be related to #7147, although TBH I am not sure if this would really account for that many consecutive errors... 🤔