Skip to content
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

Set up Sentry for Android-native code #4996

Merged
merged 4 commits into from
Sep 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions android/app/src/main/AndroidManifest.xml
Original file line number Diff line number Diff line change
Expand Up @@ -87,5 +87,12 @@
<action android:name="com.google.firebase.MESSAGING_EVENT" />
</intent-filter>
</service>

Copy link
Contributor

@chrisbobbe chrisbobbe Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(IIUC)

I think it makes sense that getsentry/sentry-react-native#1102 would be the issue with TextCompressionModule#decompress, and that that report would be accurate about the problem being debug-mode-only. But I don't think we're 100% sure, right?

This should immediately mean that we start reporting to Sentry when
an exception propagates uncaught and crashes the app.

So I wonder if this deserves an asterisk on the word "should", just in case we find an uncaught exception not being reported to Sentry in the future? Maybe just a link to #4996 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure; added a note about that.

<!-- To make a build that reports errors to Sentry, fill in the Sentry DSN below
and uncomment. See src/sentryConfig.js for another site and more discussion. -->
<!--
<meta-data android:name="io.sentry.auto-init" android:value="true" tools:node="replace" />
<meta-data android:name="io.sentry.dsn" android:value="https://123@sentry.example/45" />
-->
</application>
</manifest>
26 changes: 26 additions & 0 deletions android/app/src/main/java/com/zulipmobile/SentryUtils.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package com.zulipmobile

import io.sentry.Sentry
import io.sentry.SentryLevel

/**
* A home for things that ought to be static extensions of `Sentry`.
*
* Extending Java classes with static members isn't currently a feature
* available in Kotlin:
* https://youtrack.jetbrains.com/issue/KT-11968
* so this is our substitute.
*/
class SentryX {
companion object {
/**
* Like `Sentry.captureException`, but at level `SentryLevel.WARNING`.
*/
public fun warnException(e: Throwable) {
Sentry.withScope { scope ->
scope.level = SentryLevel.WARNING
Sentry.captureException(e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I think I'd expect that you'd have to pass the scope param somewhere in the Sentry.captureException call. But I guess you don't: https://docs.sentry.io/platforms/java/enriching-events/scopes/#local-scopes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I think this API actually makes more sense than it would if you did have to pass it: basically the point is that there's always a current scope (there has to be, to handle uncaught exceptions where you don't explicitly invoke Sentry at all), and so Sentry.captureException uses that.

The withScope function is sort of doing two things at once: it pushes a new scope onto the scope stack (and undertakes to pop it at the end), and then it passes you the scope much like Sentry.configureScope does. I think that's a bit confusing, and if I were designing this API, I'd probably separate those, so that you'd write:

Sentry.pushScope {
  Sentry.configureScope { scope -> scope.level = SentryLevel.WARNING }
  Sentry.captureException(e)
}

(Well, and I'd probably make configureScope a bit less cumbersome too, like:

Sentry.pushScope {
  Sentry.currentScope.level = SentryLevel.WARNING
  Sentry.captureException(e)
}

)

}
}
}
}
31 changes: 31 additions & 0 deletions android/app/src/main/java/com/zulipmobile/ZLog.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package com.zulipmobile

import android.util.Log
import io.sentry.Sentry

/**
* Zulip-specific logging helpers.
*
* These mirror part of the interface of `android.util.Log`, but they log
* to Sentry as well as to the device console.
*
* We basically always want to use these instead of plain `Log.e` or `Log.w`.
*/
class ZLog {
companion object {
/** Log an error to both Sentry and the device log. */
public fun e(tag: String, e: Throwable) {
// Oddly there is no `Log.e` taking just a throwable, like there is for `Log.w`.
// Have a message that just repeats the first line of how the throwable prints.
val msg = "${e.javaClass.name}: ${e.message}"
Log.e(tag, msg, e)
Sentry.captureException(e)
}

/** Log a warning to both Sentry and the device log. */
public fun w(tag: String, e: Throwable) {
Log.w(tag, e)
SentryX.warnException(e)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import me.leolin.shortcutbadger.ShortcutBadger

import com.zulipmobile.BuildConfig
import com.zulipmobile.R
import com.zulipmobile.ZLog

private val CHANNEL_ID = "default"
private val NOTIFICATION_ID = 435
Expand Down Expand Up @@ -64,7 +65,7 @@ internal fun onReceived(context: Context, conversations: ConversationMap, mapDat
try {
fcmMessage = FcmMessage.fromFcmData(mapData)
} catch (e: FcmMessageParseException) {
Log.w(TAG, "Ignoring malformed FCM message: ${e.message}")
ZLog.w(TAG, e)
return
}

Expand Down Expand Up @@ -149,7 +150,7 @@ private fun getNotificationBuilder(
try {
ShortcutBadger.applyCount(context, totalMessagesCount)
} catch (e: Exception) {
Log.e(TAG, "BADGE ERROR: $e")
ZLog.e(TAG, e)
}

builder.setWhen(fcmMessage.timeMs)
Expand Down Expand Up @@ -181,7 +182,7 @@ internal fun onOpened(application: ReactApplication, conversations: Conversation
try {
ShortcutBadger.removeCount(application as Context)
} catch (e: Exception) {
Log.e(TAG, "BADGE ERROR: $e")
ZLog.e(TAG, e)
}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ import android.text.Spannable
import android.text.SpannableStringBuilder
import android.text.TextUtils
import android.text.style.StyleSpan
import android.util.Log
import android.util.TypedValue
import androidx.core.app.NotificationCompat
import com.zulipmobile.ZLog
import java.io.IOException
import java.io.InputStream
import java.net.URL
Expand All @@ -37,7 +37,7 @@ fun fetchBitmap(url: URL): Bitmap? {
(connection.content as? InputStream)
?.let { BitmapFactory.decodeStream(it) }
} catch (e: IOException) {
Log.e(TAG, "ERROR: $e")
ZLog.e(TAG, e)
null
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import android.util.Log
import com.facebook.react.ReactApplication
import com.facebook.react.bridge.Arguments
import com.facebook.react.bridge.WritableMap
import com.zulipmobile.ZLog
import com.zulipmobile.notifications.ReactAppStatus
import com.zulipmobile.notifications.appStatus
import com.zulipmobile.notifications.emit
Expand Down Expand Up @@ -40,7 +41,7 @@ private fun handleSend(intent: Intent, application: ReactApplication, contentRes
val params: WritableMap = try {
getParamsFromIntent(intent, contentResolver)
} catch (e: ShareParamsParseException) {
Log.w(TAG, "Ignoring malformed share Intent: ${e.message}")
ZLog.w(TAG, e)
return
}

Expand Down
3 changes: 3 additions & 0 deletions docs/howto/release.md
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,9 @@ script Sentry supplies.)
`sentryKey`. See the jsdoc and comment there for more about this
value.

Edit `android/app/src/main/AndroidManifest.xml` similarly,
following the comment around `io.sentry.dsn`.

You won't want to push that change, or to make builds with it
except when you're making a release build you intend to publish.
But you will want to have it handy to apply whenever you are making
Expand Down
1 change: 1 addition & 0 deletions src/sentryConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,5 @@
//
// If you're making your own builds and want to use Sentry with them, please
// create your own Sentry client key / DSN, and fill it in here.
// See also the comment in AndroidManifest.xml about `io.sentry.dsn`.
export const sentryKey: string | null = null;