-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Gutenberg] Add Sentry React native SDK #16700
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
Conversation
|
You can trigger an installable build for these changes by visiting CircleCI here. |
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
| DispatchQueue.main.async { | ||
| WordPressAppDelegate.crashLogging?.logEnvelope(envelope) | ||
| } |
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.
Calls to WordPressAppDelegate are required to be done in the main thread.
fastlane/Fastfile
Outdated
| FileUtils.cp(File.join(gutenberg_bundle, 'App.js'), File.join(sourcemaps_folder, 'main.jsbundle')) | ||
| FileUtils.cp(File.join(gutenberg_bundle, 'App.js.map'), File.join(sourcemaps_folder, 'main.jsbundle.map')) |
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.
It's important that the source map files have specific names, otherwise, Sentry doesn't un-minify the stack traces.
| # bundle exec fastlane upload_gutenberg_sourcemaps internal:true | ||
| ##################################################################################### | ||
| lane :upload_gutenberg_sourcemaps do | options | | ||
| build_version = ios_get_build_version(internal: options[:internal]) |
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.
# Conflicts: # Podfile # Podfile.lock # fastlane/Fastfile
antonis
left a comment
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.
Tested via WordPress/gutenberg#32768 (review)
The code changes LGTM too 🎉
mokagio
left a comment
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.
Thank you for jumping into adding logic to the Fastlane automation @fluiddot 👏 🙇♂️
I only left nitpick / minor improvement comments
fastlane/Fastfile
Outdated
| sourcemap: sourcemaps_folder | ||
| ) | ||
|
|
||
| FileUtils.rm_rf(sourcemaps_folder) |
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.
Ruby suggestion: there's a way to run this code that depends on a temporary directory in a block, so that even if something fails, the runtime will ensure the directory is removed.
It should be something like:
require 'tmpdir'
Dir.mktmpdir do |sourcemaps_folder|
# all the code should "just work" the way it's written right now
endThere 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.
Thanks for the suggestion 🙇 ! I'll update the code to use the temporary directory block.
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.
I applied your suggestion in this commit and run some tests, it worked like charm 🎊 .
fastlane/Fastfile
Outdated
| version: build_version, | ||
| dist: build_version, |
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.
Just to check, what's the difference between these two parameters and why is it okay for them to have the same value?
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.
They are documented here, the version value should match the one set here. However, I'm not sure about what to use for dist, when I checked Sentry events, I found that they had the same value (see attached examples) so I went ahead and use the same version value.
I've just checked from where the Sentry iOS SDK takes the dist value (code reference) and it's CFBundleVersion, so looks like that in the end it's actually the same value 😄 . One thing I could try is to remove the dist parameter from the action and check if it affects somehow 👍.
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.
I've done a test by removing the dist parameter and so far it works, however, this change also implies initializing the SDK in the RN side without that parameter (code reference).
To be honest, I don't have a strong opinion on whether include it or not, wdyt?
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.
@fluiddot I dug a bit into the documents and found a definition of what the dist value is meant to be here which matches your finding:
You will also need to pass a distribution to
distfor events to be attributed correctly to a release. Usually this value is the build number, for example 52. This value needs to be unique to only each release.
Given that, your original approach of passing the build number was spot on.
The snippet in that pages shows the release as something that looks more like a node module name that the build number
Sentry.init({
release: "my-project-name@2.3.12",
dist: "52",
});I guess they're making a bit of confusion on their end between release, dist, version, and build in the attempt of supporting different platforms? 🤷♂️
From what I see in your explanation and screenshots, this setup seems fine. 👍
Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>
Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>
Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>
Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>
Include details about the use of `rewrite` and `strip_common_prefix` params of `sentry_upload_sourcemap` Fastlane action. Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>
Generated by 🚫 dangerJS |
|
As posted in this comment, the Sentry native module has been moved to the WordPress-iOS project. |
# Conflicts: # Podfile # Podfile.lock
WordPress/Classes/ViewRelated/Gutenberg/GutenbergViewController.swift
Outdated
Show resolved
Hide resolved
…r.swift Co-authored-by: Gio Lodi <giovanni.lodi42@gmail.com>
|
👋 I'm going to close this PR due to lack of activity, but please reopen if this was an error. Thank you. |

gutenberg-mobilePR: wordpress-mobile/gutenberg-mobile#3629gutenbergPR: WordPress/gutenberg#32768Automattic-Tracks-iOSPR: Automattic/Automattic-Tracks-iOS#183To test:
Follow the testing instructions from
gutenbergPR: WordPress/gutenberg#32768Regression Notes
Potential unintended areas of impact
The lanes (
Fastfile) related to building and uploading the build as they're using a new lane I've added for uploading the source maps to Sentry.What I did to test those areas of impact (or what existing automated tests I relied on)
I manually tested the lane by running the command
bundle exec fastlane upload_gutenberg_sourcemaps internal:truebut I haven't tested the ones related to building and uploading builds.What automated tests I added (or what prevented me from doing so)
N/A
PR submission checklist:
RELEASE-NOTES.txtif necessary.