-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
[RNMobile] Add crash logging options initial prop #32768
Conversation
Size Change: +25.9 kB (+2%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
let sentryModule = RNSentry() | ||
sentryModule.gutenbergBridge = bridgeModule | ||
sentryModule.dataSource = dataSource |
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.
The Sentry native module is explicitly instantiated here because it requires access to the bridge and data source.
@@ -233,6 +233,9 @@ public protocol GutenbergBridgeDelegate: class { | |||
func gutenbergDidRequestMediaFilesUploadCancelDialog(_ mediaFiles: [[String: Any]]) | |||
|
|||
func gutenbergDidRequestMediaFilesSaveCancelDialog(_ mediaFiles: [[String: Any]]) | |||
|
|||
// Crash logging - Sentry | |||
func gutenbergDidLogSentryEnvelope(_ envelope: [String: Any]) |
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.
The React Native SDK sends the events as envelopes, it's basically an object that contains all the data required to ingest an event into Sentry. This function will be used by the Sentry native module to send the events to the main apps (code reference).
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.
Great work @fluiddot 👍
Tested this and everything works as expected. The code changes also LGTM 🎉
Thank you for adding Sentry native support. This would make our work much easier 🙇
Looks like the app is trying to send events even when they're disabled, so I'll review that part and push a fix. Here is a log extracted from the iOS console:
Thanks @antonis for spotting it 🙇 ! |
After discussing this integration with @mchowning, we noticed that adding Sentry-specific code to the Gutenberg repo could violate the policy against adding data-gathering code. For this purpose, I decided to explore the option of moving the Sentry native module to the main apps to reduce as much as possible the references to Sentry in this repo. I made the changes in a different branch ( |
This issue has been fixed in this commit from GB-mobile. |
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.
Great work @fluiddot !
gutenberg-mobile
PR: wordpress-mobile/gutenberg-mobile#3629WordPress-iOS
PR: wordpress-mobile/WordPress-iOS#16700Automattic-Tracks-iOS
PR: Automattic/Automattic-Tracks-iOS#183Description
This PR adds the Sentry native module and includes new functions to the Gutenberg bridge that would allow JS exceptions. be logged to Sentry via the main apps.
How has this been tested?
Preparation:
gutenberg/add/sentry-react-native
from WordPress-iOS repo and install dependencies with the commandrake dependencies
throw new Error( 'Exception test' );
npm run bundle:ios && cp -rf bundle/ios ../WordPress-iOS/Pods/Gutenberg/bundle
gutenberg/add/sentry-react-native
branch should be17.8.0.20210716
), the version can be fetched from this file and the source maps can be checked by navigating to the URLhttps://sentry.io/settings/a8c/projects/wordpress-ios/source-maps/<VERSION>/
.The source maps will be automatically uploaded during the release process but in case we might need to manually upload them, we could run the command
bundle exec fastlane upload_gutenberg_sourcemaps internal:true
in the WordPress-iOS project. For this purpose, you would need to generate an API token in Sentry and include it in thewpios-env
file.NOTE: For version
17.8.0.20210716
I already uploaded source maps.Generate Sentry issue:
Me/App Settings/Debug/Crash Logging
.localDeveloper
.Sentry event example: https://sentry.io/organizations/a8c/issues/2524917124/?environment=localDeveloper&project=1438083&query=is%3Aunresolved&statsPeriod=1h
Screenshots
Native crash:
![stack-trace-native](https://cdn.statically.io/img/user-images.githubusercontent.com/14905380/123247282-03219b00-d4e7-11eb-820d-9736bc1994fa.png)
Types of changes
New feature
Checklist:
*.native.js
files for terms that need renaming or removal).