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

feat: Make dedupe integration default for browser #3730

Merged
merged 2 commits into from
Jun 25, 2021

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented Jun 22, 2021

At one point in the past remove the dedupe integration from being default for both Browser & Node and made it an optional integration.
Since we extended our SDKs capabilities for unhandled promises rejections error can get noisy. Also sometimes browser extensions create weird error loops that produce a lot of unnecessary events that burn through users' quota.

Code Changes:
I am aware that copy-pasting the Integration is not elegant but there is no other way to do it without breaking people.

@HazAT HazAT self-assigned this Jun 22, 2021
@HazAT HazAT requested a review from kamilogorek as a code owner June 22, 2021 13:50
@github-actions
Copy link
Contributor

github-actions bot commented Jun 22, 2021

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 21.34 KB (+1.55% 🔺)
@sentry/browser - Webpack 22.36 KB (+1.83% 🔺)
@sentry/react - Webpack 22.39 KB (+1.82% 🔺)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 28.78 KB (+1.26% 🔺)

Copy link
Contributor

@kamilogorek kamilogorek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move it to core, but your call. We can leave it here for now as well.

@HazAT HazAT merged commit bf52938 into master Jun 25, 2021
@HazAT HazAT deleted the feat/dedupe-browser-default branch June 25, 2021 07:20
1999 added a commit to 1999/sentry-javascript that referenced this pull request Jun 25, 2021
…transport

* upstream/master: (29 commits)
  ref: Always use lowercase files (getsentry#3742)
  feat: Make dedupe integration default for browser (getsentry#3730)
  ref(ember): Allow initing Ember without config entry (getsentry#3745)
  fix(serverless): wrapEventFunction does not await for async code (getsentry#3740)
  Metrics: Tag CLS elements (getsentry#3734)
  feat: Add Next.js 11 to supported peer dependencies list (getsentry#3711)
  test: Run integration tests for Next 10/11 and Webpack 4/5 matrix (getsentry#3741)
  fix: Correctly limit Buffer requests (getsentry#3736)
  Whoops. Remove pinned node version from package.json
  ref: Introduce test runner for node session health tests (getsentry#3728)
  fix: Prevent circular structure serialization in events (getsentry#3727)
  ref(node): Update Node manual tests and test for sessionCount (getsentry#3726)
  ref(ember): Update scenarios and remove a few to speed up tests (getsentry#3720)
  docs: Fix typos (getsentry#3716)
  fix(ember): Fix ember test flake (getsentry#3719)
  release: 6.7.2
  ci: fix ember flaky test (getsentry#3718)
  misc: changelog for release 6.7.2 (getsentry#3717)
  fix(release-health): Prevent sending terminal status session updates (getsentry#3701)
  ref: Make beforeSend more strict (getsentry#3713)
  ...
@preciselywilliam
Copy link

How can I follow when this will be released? From what I could see it has not been included in recent releases, right?

@HazAT
Copy link
Member Author

HazAT commented Jun 28, 2021

This will be released today

@winzig
Copy link

winzig commented Jul 7, 2021

Any chance this explains the 89K errors I received in the last 21 days, described in this discussion: https://forum.sentry.io/t/unhandledrejection-non-error-promise-rejection-captured-with-value/14062/19

@kamilogorek
Copy link
Contributor

No, not really, they are completely separate things. This change would lower the number if anything.

@winzig
Copy link

winzig commented Jul 8, 2021

@kamilogorek Sorry, what I meant was I'm wondering if this feature will fix the problem that caused Sentry to log 89K events when most of them appear to be dupes or due to some loop at Sentry.

@kamilogorek
Copy link
Contributor

Oh, then yes, it can definitely lower the amount of data sent. It all depends on whether they come from the same sessions (eg same browser tab) or are very granularly spread across all users.

fwiw you can check this without updating the SDK with:

import * as Sentry from "@sentry/browser";
import { Dedupe as DedupeIntegration } from "@sentry/integrations";

Sentry.init({
  dsn: "https://[email protected]/0",
  integrations: [new DedupeIntegration()],
});

kemenaran added a commit to demarches-simplifiees/demarches-simplifiees.fr that referenced this pull request Jul 13, 2021
Helps with de-duplicating issues being trigerred in a loop.

See getsentry/sentry-javascript#3730
kemenaran added a commit to demarches-simplifiees/demarches-simplifiees.fr that referenced this pull request Jul 13, 2021
Helps with de-duplicating issues being trigerred in a loop.

See getsentry/sentry-javascript#3730
kemenaran added a commit to demarches-simplifiees/demarches-simplifiees.fr that referenced this pull request Jul 20, 2021
Helps with de-duplicating issues being trigerred in a loop.

See getsentry/sentry-javascript#3730
lydiasan pushed a commit to demarches-simplifiees/demarches-simplifiees.fr that referenced this pull request Jul 20, 2021
Helps with de-duplicating issues being trigerred in a loop.

See getsentry/sentry-javascript#3730
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.

4 participants