feat_: add Sentry panic reporting #6054
Open
+589
−7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (79)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6054 +/- ##
===========================================
- Coverage 60.90% 60.86% -0.04%
===========================================
Files 819 823 +4
Lines 109444 109569 +125
===========================================
+ Hits 66656 66694 +38
- Misses 34950 35050 +100
+ Partials 7838 7825 -13
Flags with carried forward coverage won't be shown. Click here to find out more.
|
igor-sirotin
force-pushed
the
feat/sentry
branch
from
3a4d917
to
fcedb01
Compare
igor-sirotin
changed the title
igor-sirotin
force-pushed
the
feat/sentry
branch
from
fcedb01
to
08ee67a
Compare
|
@igor-sirotin one point to consider, which @sunleos already mentioned, is the desire to keep usage data separate from everything else. We already have two separate flags, one for Waku usage data (telemetry) and general usage data (analytics/MixPanel). It would be better if we could have a new setting just for app monitoring, that way a user will be able to toggle it independently of the other two. One other reason is that we don't want internal CCs enabling the centralized metrics toggle because that skews MixPanel reports due to the small number of active (external) users. |
igor-sirotin
force-pushed
the
feat/sentry
branch
from
16d7c02
to
8e9547a
Compare
Settings planWe're discussing to make 3 separate toggles for analytics, but all under a single main toggle as well. Something like this:
What we haveSo far we have this controls in the database:
What we needMost probably we need 4 boolean flags. Might work with 3 as well, depending on what the design will be. PS. I will keep this PR without these controls and bind to |
igor-sirotin
force-pushed
the
feat/sentry
branch
2 times, most recently
from
5acde49
to
72d55a2
Compare
igor-sirotin
force-pushed
the
feat/sentry
branch
from
72d55a2
to
b1846a9
Compare
✔️ status-go/prs/macos/x86_64/main/PR-6054#12 🔹 ~5 min 31 sec 🔹 b1846a9 🔹 📦 macos package |
This was referenced Nov 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requires:
Iterates:
Desktop PR:
Note
TODO:
Share usage data with StatusInitialize Sentry for unit testsWon't be done: go tests run in goroutines, which is created internally in
testingpackage.The only way to workaround this is to add
defer common.LogOnPanic()to EACH TEST. This doesn't seem to be that easy right now. And the benefit is not that big: we only need this for develop/nightly runs.Consider case when running client e2e tests with no metrics reporting (we still want crash reports)I guess we won't this do this now as well. Not high priority to have reports from in-test crashes.
Next steps:
Description
This PR iterates integration of Sentry into Status project. So far:
🛬 Where
We use self-hosted Sentry: https://sentry.infra.status.im/
🕐 When
When a panic (crash) happens in status-go in one of those cases:
status-desktop/status-mobile:/mobile/status.go/services/**/api.gostatus-backend:📦 What
Here's some highlights:
Device info (OS, architecture, number of cpu cores)
Error stack trace:
Stacktrace contains paths to source files. When built locally, this might contain sensitive information, e.g. user name.
This will not be a problem for us, as we will only enable Sentry for builds made on our CI, so paths will only contain our CI-related paths, which is safe.
Trace ID (so far will be unique for each event)
More details in sentry docs.
Examples
Example of a status-go panic, while running as part of status-desktop:
Sentry event link or full JSON.
Example of a status-go panic , while running as part of status-backend:
Sentry event link or full JSON
Re: Privacy policy
Only enabled for users that both:
The feature is opt-in only:
status-go/mobile/status.go
Lines 107 to 115 in 3a4d917
I assume this should be fine within the existing Privacy policy. We don't report any more privacy-enclosing information than to the opt-in metrics.
Please check Sentry security and compliance for more details.
Configuration
I added 2 main tags to identify the error. The configuration is a bit complicated, but provides full information.
Questionproductioncan only be set at build time to prevent users from hacking the environment- All others can be set at runtime, because on CI we sometimes use same build for multiple environments
productiondevelopmentci-prci-mainci-nightlydevelopmentandci-prare dropped, because we only want to consider panics from stable codestatus-desktopstatus-mobilestatus-backendcmd/status-backendCan be other
cmd/*as well.matterbridgestatus-go-testsTo cover this requirements, I added these environment variables:
SENTRY_DSNsentry.Init- At runtime with
InitializeApplicationendpointSENTRY_CONTEXT_NAMESENTRY_CONTEXT_VERSIONSENTRY_PRODUCTIONtrueor1:-Defines if this is a production build
-Sets environment to
production-Has precedence over runtime
SENTRY_ENVIRONMENTSENTRY_ENVIRONMENTSENTRY_ENVIRONMENTis setClient instructions
Set
SENTRY_CONTEXT_NAMEandSENTRY_CONTEXT_VERSIONat status-go build timeProvide
sentryDSNto theInitializeApplicationcall.DSN must be kept private and will be provided by CI. Expect a
STATUS_GO_SENTRY_DSNenvironment variable.Why can't we consume `STATUS_GO_SENTRY_DSN` directly in status-go build?
Implementation details
internal/sentrypackage that encapsulates all custom details, configurations, etc.status-go/common/utils.go
Line 102 in fcedb01
status-go/mobile/callog/status_request_log.go
Line 69 in fcedb01
status-go/cmd/status-backend/main.go
Line 40 in fcedb01
This covers all goroutines, because we have a linter to check that all goroutines have
defer common.LogOnPanic.InitializeApplication- covers desktop/mobile clientsstatus-go/mobile/status.go
Lines 105 to 108 in fcedb01
status-backend- covers functional tests:status-go/cmd/status-backend/main.go
Lines 36 to 39 in fcedb01