Skip to content

feat: Add thirdPartyErrorFilterIntegration #12267

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

Merged
merged 9 commits into from
Jun 6, 2024

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented May 28, 2024

Described here:
#11718 (comment)

I didn't want to use the word integrity since for me at least, it reminds me of Subresource Integrity hashes and this is nothing like that. It's more of an id. For the metadata property I used bundle_key although it might be a better idea to use something that is less likely to clash with users existing metadata like __bundler_key.

My only minor concern is the behaviour strings which use what feels like a double negative logic... drop-if-not-matched. It made it quite hard to reason about what each one did and I got the logic wrong myself a couple of times!

type Behaviour =
  | 'drop-if-some-frames-not-matched'
  | 'drop-if-every-frames-not-matched'
  | 'apply-tag-if-some-frames-not-matched'
  | 'apply-tag-if-every-frames-not-matched';

@timfish timfish requested a review from lforst May 28, 2024 18:07
@timfish
Copy link
Collaborator Author

timfish commented May 28, 2024

Ah, another issue here is that module_metadata is any. Users could set this to a string via the bundler plugins and then we can't include our key with any existing metadata. We could make module_metadata an object before we move it out of _experimental settings?

Copy link
Contributor

github-actions bot commented May 28, 2024

size-limit report 📦

Path Size
@sentry/browser 21.8 KB (+0.3% 🔺)
@sentry/browser (incl. Tracing) 32.85 KB (+0.23% 🔺)
@sentry/browser (incl. Tracing, Replay) 68.42 KB (+0.12% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 61.71 KB (+0.1% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas) 72.47 KB (+0.1% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback) 84.59 KB (+0.1% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 86.43 KB (+0.08% 🔺)
@sentry/browser (incl. metrics) 25.99 KB (+0.28% 🔺)
@sentry/browser (incl. Feedback) 37.96 KB (+0.18% 🔺)
@sentry/browser (incl. sendFeedback) 26.39 KB (+0.25% 🔺)
@sentry/browser (incl. FeedbackAsync) 30.94 KB (+0.26% 🔺)
@sentry/react 24.57 KB (+0.27% 🔺)
@sentry/react (incl. Tracing) 35.88 KB (+0.21% 🔺)
@sentry/vue 25.81 KB (+0.31% 🔺)
@sentry/vue (incl. Tracing) 34.68 KB (+0.21% 🔺)
@sentry/svelte 21.93 KB (+0.3% 🔺)
CDN Bundle 23.16 KB (+0.22% 🔺)
CDN Bundle (incl. Tracing) 34.56 KB (+0.17% 🔺)
CDN Bundle (incl. Tracing, Replay) 68.49 KB (+0.09% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) 73.65 KB (+0.06% 🔺)
CDN Bundle - uncompressed 68.16 KB (+0.25% 🔺)
CDN Bundle (incl. Tracing) - uncompressed 102.34 KB (+0.17% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed 212.23 KB (+0.08% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 224.7 KB (+0.08% 🔺)
@sentry/nextjs (client) 35.23 KB (+0.22% 🔺)
@sentry/sveltekit (client) 33.47 KB (+0.2% 🔺)
@sentry/node 115.24 KB (0%)
@sentry/node - without tracing 94.55 KB (0%)
@sentry/aws-serverless 103.72 KB (0%)

- Get rid of negation in behaviour modes
- Co-locate some variables with their checcks
- Extend getFramesFromEvent to get frames from all event-values
- Change tag to `third_party_code` to get rid of negation
| 'drop-error-if-exclusively-contains-third-party-frames'
| 'apply-tag-if-contains-third-party-frames'
| 'apply-tag-if-exclusively-contains-third-party-frames';
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are much better names!

Copy link
Member

Choose a reason for hiding this comment

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

Yup, I agree. Your comment was a wake-up call 😂

}),
} as unknown as Client;

describe('ThirdPartyErrorFilter', () => {
Copy link
Member

Choose a reason for hiding this comment

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

l: these tests lend themselves well to parameterization

Copy link
Member

Choose a reason for hiding this comment

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

True, but it's a bit annoying since we are checking for defined-ness and that is not so easy to do in parameterized tests.

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean expect(result).toBeDefined();? I think using expect.anything() should work for that.

@lforst lforst merged commit bc5d2e1 into develop Jun 6, 2024
110 checks passed
@lforst lforst deleted the timfish/thirdPartyErrorFilterIntegration branch June 6, 2024 09:26
billyvg pushed a commit that referenced this pull request Jun 10, 2024
Co-authored-by: Luca Forstner <luca.forstner@sentry.io>
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.

3 participants