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(core): Exclude Dev Server and Sentry Dsn request from Breadcrumbs #4240

Merged
merged 62 commits into from
Nov 18, 2024

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Nov 6, 2024

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 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 but http://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.

⚠️ This PR was initially based on #4124 to avoid conflicts. Since the implementation changed direction this is no longer needed.

💡 Motivation and Context

Fixes #3105

💚 How did you test it?

CI, Manual testing

Capture exception Before After
iOS event event
Android event event

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • All tests passing
  • No breaking changes

🔮 Next steps

# Conflicts:
#	android/src/main/java/io/sentry/react/RNSentryModuleImpl.java
# Conflicts:
#	packages/core/android/src/main/java/io/sentry/react/RNSentryBreadcrumb.java
# Conflicts:
#	CHANGELOG.md
#	packages/core/RNSentry.podspec
@antonis antonis changed the title Don't add Dev Server and Sentry Dsn request to BreadcrumsAntonis/3105 exclude dsn and devserver Exclude Dev Server and Sentry Dsn request from Breadcrums Nov 6, 2024
Co-authored-by: LucasZF <[email protected]>
Copy link
Collaborator

@lucas-zimerman lucas-zimerman left a 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!

@antonis
Copy link
Collaborator Author

antonis commented Nov 15, 2024

Thank you for all the help on this PR @lucas-zimerman 🙇

Base automatically changed from antonis/add-breadcrumb-origin to main November 18, 2024 11:50
@krystofwoldrich
Copy link
Member

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

Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

LG 🚀 Thank you!

@antonis antonis merged commit 112c4f8 into main Nov 18, 2024
62 checks passed
@antonis antonis deleted the antonis/3105-exclude-dsn-and-devserver branch November 18, 2024 14:23
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.

Don't add Dev Server and Sentry Dsn request to Breadcrums
3 participants