Dedupe should check exceptions against an internal dictionary of past exceptions

[redgetan]

• [x] Review the documentation: https://docs.sentry.io/
• [x] Search for existing issues: https://github.com/getsentry/sentry-javascript/issues
• [x] Use the latest release: https://github.com/getsentry/sentry-javascript/releases

Package + Version

• [ ] @sentry/browser
• [x] @sentry/node
• [ ] raven-js
• [ ] raven-node (raven for node)
• [ ] other:

Version:

4.3.4

Problem

Right now, dedupe compares the current exception against the most recent exception to check whether to drop it based on whether it has same fingerprint/stacktrace. The problem arises when two different exceptions are triggered sequentially, and repeatedly afterwards. To illustrate, if we have a scenario below where exception A occurred, followed by B, then A, then B again indefinitely, the dedupe would fail because it only checks against the most recent exception.

Exceptions

• Exception A - "undefined variable error"
• Exception B - "foo is not a function"

Scenario:

• A->B->A->B->A-B...... (repeated 10000x times)

Actual Example:

Proposed Solution

I think we can maintain two dictionaries to achieve O(1) lookup/comparison. One indexes exceptions by their fingerprint, and the other indexes exceptions by stacktrace-based unique hash.

If this solution makes sense, I'll go ahead and make a pull-request and add a test case in /packages/integrations/test/dedupe.test.ts

[kamilogorek]

We never had a request like this, thus it's not a very common scenario for us and I don't think it should be a default config.

You can achieve this behavior with a few lines of code though:

const errors = new Map()

Sentry.init({
  dsn: '...',
  beforeSend(event, hint) {
    if (hint.originalException instanceof Error) {
      let err = hint.originalException;
      if (errors.has(err.message) && errors.get(err.message)=== err.stack) return null;
      errors.set(err.message, err.stack)
    }
    return event;
  }
})

(check code is silly, but it's up to you how to handle this)

[redgetan]

I see. Unfortunately this scenario occurs frequently for my use case. I run a node.js game server that executes gameloop every 100ms, and multiple errors can happen in a single gameloop. I already had my account reach the max quota twice within a few days of using sentry. I've ended up just writing my own custom dedupe integration and its working well for me so far.

Since you're saying this is not a common scenario, and given that people might have different dedupe needs/use-cases, feel free to close this ticket.

[kamilogorek]

Since you're saying this is not a common scenario, and given that people might have different dedupe needs/use-cases, feel free to close this ticket.

Thanks. Also feel free to let me know if you have any issues implementing this and I can help :)

[odinho]

Not so sure this is that uncommon. We just keep writing it always.

Especially when you use the console integration you'll often both have a console.error and a throw somewhere. Or anything else that makes 2 issues come up.

I was searching for an issue to this, to see whether you'd accept a patch that allows you to specify whether Dedupe should look at n last sent events.

This is basically the code I've been using now at my second company and third time I use this (after it became clear it was needed):

beforeSend(event, hint) {
   // Don't actually send events we've already seen
  const isDupe = lastSentEvents.some((prevEvent) => _shouldDropEvent(event, prevEvent));

  if (isDupe) {
     return null;
  } else {
     lastSentEvents.unshift(event);
     // Keep 10 latest events to check for dupes
     lastSentEvents.length = 10;
     return event;
  }
}

So it'll check the last 10 sent events for dupes rather than just the last one. We could probably set this to 2 right now, but just sat this high to make dupes quite unlikely. We have rather the problem of too many events rather than too few ;)

---

Would it be worth doing a PR, or do you still think this use case is niche? (okay for me, I just thought solving it thrice meant it should probably go upstream).

[kamilogorek]

cc @getsentry/team-web-sdk-frontend

[AbhiPrasad]

I don't think we want to broaden the scope of dedupe, since repeated errors that are more sparse might be indicative of an actual problem. I don't think we'll accept a patch as a result, but thanks for sharing your code @odinho - hopefully others find it useful!

[odinho]

Sure.

Default would have to be 1, and docs saying to be careful. And maybe mentioning you usually shouldn't use this, mostly if you use the console integration and console.error quite freely around your app. (though better to :)

I'm fine with this product decision. I might work on more loquacious apps than most ;)