-
Notifications
You must be signed in to change notification settings - Fork 498
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
Track non-fatal issues via analytics / Sentry if consent provided #6308
Conversation
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.
One inline comment, otherwise LGTM!
|
||
func stop() { | ||
MXLog.debug("[SentryMonitoringClient] Stopped") | ||
SentrySDK.close() |
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 wonder if SentrySDK.endSession
makes more sense here. Not sure but feels like close
is an unrecoverable action.
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.
endSession
only really rotates session IDs etc, but keeps the Sentry service running. Having tested this out close
is not in fact unrecoverable and SentrySDK can be properly restarted again.
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.
lgtm
📱 Scan the QR code below to install the build for this PR. If you can't scan the QR code you can install the build via this link: https://i.diawi.com/viHKYU |
Kudos, SonarCloud Quality Gate passed! |
Codecov Report
@@ Coverage Diff @@
## develop #6308 +/- ##
========================================
Coverage 6.23% 6.23%
========================================
Files 1443 1444 +1
Lines 155488 155533 +45
Branches 62484 62502 +18
========================================
+ Hits 9695 9699 +4
- Misses 145386 145427 +41
Partials 407 407
Continue to review full report at Codecov.
|
Adds Sentry as a monitoring tool to track app health metrics such as crashes and non-fatal issues logged via
MXLog.failure
. Using Sentry is subject to the same user consent as other crash and analytics data and is configurable from user settings.Sentry in this PR is integrated directly into an existing
Analytics
class, which has some pros:The downside of this approach is giving
Analytics
more responsibilities than it was originally indented to have. I tried to remedy this by changing the documentation of the class, but it is still overwhelmingly aimed at trackingAnalyticEvents
. Further refactors could make the split between the use cases more explicit.Related SDK change