-
-
Notifications
You must be signed in to change notification settings - Fork 338
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(core): Exclude Dev Server and Sentry Dsn request from Breadcrumbs #4240
Conversation
# Conflicts: # RNSentry.podspec
# Conflicts: # android/src/main/java/io/sentry/react/RNSentryModuleImpl.java
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # packages/core/android/src/main/java/io/sentry/react/RNSentryBreadcrumb.java
# Conflicts: # CHANGELOG.md # packages/core/RNSentry.podspec
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
Co-authored-by: LucasZF <[email protected]>
Co-authored-by: LucasZF <[email protected]>
Co-authored-by: LucasZF <[email protected]>
packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java
Outdated
Show resolved
Hide resolved
packages/core/RNSentryCocoaTester/RNSentryCocoaTesterTests/RNSentryTests.mm
Outdated
Show resolved
Hide resolved
Co-authored-by: LucasZF <[email protected]>
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.
That's all from my side, once the comments are resolved, we could merge this PR!
Thank you for your work!
Thank you for all the help on this PR @lucas-zimerman 🙇 |
One non blocking note, the native breadcrumbs filter will not work for users who manually initialize the native SDKs. This is acceptable now, as there are other features which are not enabled when using manual init. But it will need update when implementing #3608 |
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.
LG 🚀 Thank you!
📢 Type of change
📜 Description
Filters out DevServer and DSN related breadcrumbs using the
beforeBreadcrumb
option on iOS, Android and JS:Note: The current implementation excludes breadcrumbs with the dev server including the port. For example
http://localhost:8081
breadcrumbs are excluded buthttp://localhost:8969/stream
are not. A possible enhancement would be to get the host name from the dev server url and exclude all requests. This might have the side effect of missing other requests from a local development environment.💡 Motivation and Context
Fixes #3105
💚 How did you test it?
CI, Manual testing
📝 Checklist
sendDefaultPII
is enabled🔮 Next steps