Use secret Sentry DSN value #4210
Conversation
We realised our DSN entry has been shipped along with the code and it was being used in several forks as is, resulting in wrong bug reports coming into our Sentry dashboard and making it very hard to debug actual issues in the app.
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
| @@ -9,7 +9,7 @@ package io.element.android.services.analyticsproviders.sentry | |||
|
|
|||
| object SentryConfig { | |||
| const val NAME = "Sentry" | |||
| const val DNS = "https://[email protected]/59" | |||
| const val DNS = BuildConfig.SENTRY_DSN | |||
There was a problem hiding this comment.
Maybe take the opportunity to fix the typo DNS -> DSN
There was a problem hiding this comment.
Oops, I completely missed it 😅 .
| value = (System.getenv("ELEMENT_ANDROID_SENTRY_DSN") | ||
| ?: readLocalProperty("services.analyticsproviders.sentry.dsn") | ||
| ?: "" | ||
| ).let { "\"$it\"" } |
There was a problem hiding this comment.
According to the doc:
So it's maybe safer to set default value to null, or do not call options.dsn = SentryConfig.DNS if SentryConfig.DNS is empty (maybe the latter is simpler, and we may Timber a warning in this case).
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4210 +/- ##
===========================================
- Coverage 80.19% 80.19% -0.01%
===========================================
Files 2022 2022
Lines 53271 53273 +2
Branches 6443 6444 +1
===========================================
Hits 42723 42723
- Misses 8330 8332 +2
Partials 2218 2218 ☔ View full report in Codecov by Sentry. |
|
| } else { | ||
| Timber.w("No Sentry DSN provided, Sentry will not be initialized") | ||
| return | ||
| } |
There was a problem hiding this comment.
I was thinking of something like
val dsn = SentryConfig.DSN.takeIf { it.isNotBlank() }
if (dsn == null)
Timber.w("No Sentry DSN provided, Sentry will not report anything")
}but this is probably equivalent.
Content
Motivation and context
We realised our DSN entry has been shipped along with the code and it was being used in several forks as is, resulting in wrong bug reports coming into our Sentry dashboard and making it very hard to debug actual issues in the app.
Tests
It's kind of difficult to test: Sentry will silently do nothing AFAICT if the DSN is not provided.
One possible test is to make the app crash and check the issue is not reported, then add the local property or env variable, make it crash again and check the new crash is there.
Tested devices
Checklist