Skip to content

feat(core): Remove health check transaction filters #10818

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 2 commits into from
Feb 28, 2024

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Feb 27, 2024

This PR removes the ignoreTransactions default values which were all patterns trying to identify health check transactions.

We remove these because:

  • We now have server-side inbound filters that are configurable in the Sentry UI
  • Having two filters (SDK and Sentry backend) is more than unintuitive for users
  • The patterns between the SDK and the Sentry backend filters were slightly different
  • There's no other SDK that has client-side health check transaction filtering
  • There simply is no safe way to filter such transactions without dropping false positives. This ofc also applies to the server-side filter but it's better to have this questionable heuristic only in one place than two.

Implementation note: Decided to remove the entire defaults array b/c there's no point in keeping around an empty array. Therefore, I also removed the ignoreTransactionDefaults option. We can re-add all of this if we ever want to filter out something by default again but for now, I'd argue the bundle size reduction is worth removing this code.

@Lms24 Lms24 requested review from HazAT and mydea February 27, 2024 07:58
Copy link
Contributor

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 77.24 KB (-0.04% 🔽)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 68.51 KB (-0.05% 🔽)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped) 72.42 KB (-0.05% 🔽)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 62.02 KB (-0.06% 🔽)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 32.69 KB (-0.09% 🔽)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped) 32.69 KB (-0.09% 🔽)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 30.86 KB (-0.16% 🔽)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped) 30.86 KB (-0.16% 🔽)
@sentry/browser - Webpack (gzipped) 22.15 KB (-0.22% 🔽)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 75.67 KB (-0.11% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 67.41 KB (-0.13% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 33.2 KB (-0.26% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped) 24.69 KB (-0.16% 🔽)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 210.92 KB (-0.17% 🔽)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 99.64 KB (-0.36% 🔽)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 73.8 KB (+0.04% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 36.26 KB (-0.28% 🔽)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 68.8 KB (-0.04% 🔽)
@sentry/react - Webpack (gzipped) 22.17 KB (-0.21% 🔽)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 85.25 KB (+0.01% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 49.62 KB (-0.09% 🔽)
@sentry-internal/feedback - Webpack (gzipped) 17.14 KB (0%)

## Removal of Client-Side health check transaction filters

The SDK no longer filters out health check transactions by default. Instead, they are sent to Sentry but still dropped
by the Sentry backend by default. You can disable dropping them in your Sentry project settings. If you still want to
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a note here that they are dropped by default for projects created after X? As I guess that is the case here...? And add a note like "If your project was created before X, you can enable the inbound filter for this at Y"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the server side inbound filter was turned on for all projects 🤔 I checked for projects in Sentry that were created way before we introduced the filter and there it's also turned on.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually, I'm not too sure anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, so even the projects in my private org have the filter turned on and I definitely didn't flip any switches there. I didn't find additional PRs that would have changed the defaults in older project but I asked in slack (#discuss-issues) to be sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Talked to the issues team; the health check transaction filter is on by default for all (old and new) projects. So we should be good to go @mydea 👍

@Lms24 Lms24 merged commit 34e7233 into develop Feb 28, 2024
@Lms24 Lms24 deleted the lms/feat-remove-health-check-txn-filters branch February 28, 2024 10:03
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