Merge #6777

6777: Issue #1705: Create separate interface for crash telemetry services. r=rocketsroger a=pocmo

In #1705 I am going to add more methods/properties to the `CrashReporterService` interface. Those will not make any sense for a telemetry service. So here I am going to create a separate interface for telemetry service and make glean and the crash reporter class use that instead.
Co-authored-by: Sebastian Kaspari <s.kaspari@gmail.com>

components/lib/crash/src/main/java/mozilla/components/lib/crash/CrashReporter.kt

@@ -19,6 +19,7 @@ import mozilla.components.lib.crash.handler.ExceptionHandler
 import mozilla.components.lib.crash.notification.CrashNotification
 import mozilla.components.lib.crash.prompt.CrashPrompt
 import mozilla.components.lib.crash.service.CrashReporterService
+import mozilla.components.lib.crash.service.CrashTelemetryService
 import mozilla.components.lib.crash.service.SendCrashReportService
 import mozilla.components.lib.crash.service.SendCrashTelemetryService
 import mozilla.components.support.base.crash.CrashReporting
@@ -54,7 +55,7 @@ import mozilla.components.support.base.log.logger.Logger
 @Suppress("TooManyFunctions")
 class CrashReporter(
     private val services: List<CrashReporterService> = emptyList(),
-    private val telemetryServices: List<CrashReporterService> = emptyList(),
+    private val telemetryServices: List<CrashTelemetryService> = emptyList(),
     private val shouldPrompt: Prompt = Prompt.NEVER,
     var enabled: Boolean = true,
     internal val promptConfiguration: PromptConfiguration = PromptConfiguration(),
@@ -109,8 +110,8 @@ class CrashReporter(
         return scope.launch {
             telemetryServices.forEach { telemetryService ->
                 when (crash) {
-                    is Crash.NativeCodeCrash -> telemetryService.report(crash)
-                    is Crash.UncaughtExceptionCrash -> telemetryService.report(crash)
+                    is Crash.NativeCodeCrash -> telemetryService.record(crash)
+                    is Crash.UncaughtExceptionCrash -> telemetryService.record(crash)
                 }
             }
 

components/lib/crash/src/main/java/mozilla/components/lib/crash/service/CrashTelemetryService.kt

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package mozilla.components.lib.crash.service
+
+import mozilla.components.lib.crash.Crash
+
+/**
+ * Interface to be implemented by external services that collect telemetry about crash reports.
+ */
+interface CrashTelemetryService {
+    /**
+     * Records telemetry for this [Crash.UncaughtExceptionCrash].
+     */
+    fun record(crash: Crash.UncaughtExceptionCrash)
+
+    /**
+     * Records telemetry for this [Crash.NativeCodeCrash].
+     */
+    fun record(crash: Crash.NativeCodeCrash)
+
+    /**
+     * Records telemetry for this caught [Throwable] (non-crash).
+     */
+    fun record(throwable: Throwable)
+}

components/lib/crash/src/main/java/mozilla/components/lib/crash/service/GleanCrashReporterService.kt

@@ -22,7 +22,7 @@ class GleanCrashReporterService(
     val context: Context,
     @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
     internal val file: File = File(context.applicationInfo.dataDir, CRASH_FILE_NAME)
-) : CrashReporterService {
+) : CrashTelemetryService {
     companion object {
         // This file is stored in the application's data directory, so it should be located in the
         // same location as the application.
@@ -158,7 +158,7 @@ class GleanCrashReporterService(
      * or [NONFATAL_NATIVE_CODE_CRASH_KEY]
      */
     @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
-    internal fun reportCrash(crash: String) {
+    internal fun recordCrash(crash: String) {
         // Persist the crash in a file so that it can be recorded on the next application start. We
         // cannot directly record to Glean here because CrashHandler process is not the same process
         // as Glean is initialized in.
@@ -173,22 +173,19 @@ class GleanCrashReporterService(
         }
     }
 
-    override fun report(crash: Crash.UncaughtExceptionCrash): String? {
-        reportCrash(UNCAUGHT_EXCEPTION_KEY)
-        return null
+    override fun record(crash: Crash.UncaughtExceptionCrash) {
+        recordCrash(UNCAUGHT_EXCEPTION_KEY)
     }
 
-    override fun report(crash: Crash.NativeCodeCrash): String? {
+    override fun record(crash: Crash.NativeCodeCrash) {
         if (crash.isFatal) {
-            reportCrash(FATAL_NATIVE_CODE_CRASH_KEY)
+            recordCrash(FATAL_NATIVE_CODE_CRASH_KEY)
         } else {
-            reportCrash(NONFATAL_NATIVE_CODE_CRASH_KEY)
+            recordCrash(NONFATAL_NATIVE_CODE_CRASH_KEY)
         }
-        return null
     }
 
-    override fun report(throwable: Throwable): String? {
-        reportCrash(CAUGHT_EXCEPTION_KEY)
-        return null
+    override fun record(throwable: Throwable) {
+        recordCrash(CAUGHT_EXCEPTION_KEY)
     }
 }

components/lib/crash/src/test/java/mozilla/components/lib/crash/CrashReporterTest.kt

@@ -12,6 +12,7 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.test.TestCoroutineDispatcher
 import kotlinx.coroutines.test.TestCoroutineScope
 import mozilla.components.lib.crash.service.CrashReporterService
+import mozilla.components.lib.crash.service.CrashTelemetryService
 import mozilla.components.support.test.any
 import mozilla.components.support.test.eq
 import mozilla.components.support.test.expectException
@@ -72,7 +73,7 @@ class CrashReporterTest {
     @Test
     fun `CrashReporter will submit report immediately if setup with Prompt-NEVER`() {
         val service: CrashReporterService = mock()
-        val telemetryService: CrashReporterService = mock()
+        val telemetryService: CrashTelemetryService = mock()
 
         val reporter = spy(CrashReporter(
             services = listOf(service),
@@ -93,7 +94,7 @@ class CrashReporterTest {
     @Test
     fun `CrashReporter will show prompt if setup with Prompt-ALWAYS`() {
         val service: CrashReporterService = mock()
-        val telemetryService: CrashReporterService = mock()
+        val telemetryService: CrashTelemetryService = mock()
 
         val reporter = spy(CrashReporter(
             services = listOf(service),
@@ -114,7 +115,7 @@ class CrashReporterTest {
     @Test
     fun `CrashReporter will submit report immediately for non native crash and with setup Prompt-ONLY_NATIVE_CRASH`() {
         val service: CrashReporterService = mock()
-        val telemetryService: CrashReporterService = mock()
+        val telemetryService: CrashTelemetryService = mock()
 
         val reporter = spy(CrashReporter(
             services = listOf(service),
@@ -135,7 +136,7 @@ class CrashReporterTest {
     @Test
     fun `CrashReporter will show prompt for fatal native crash and with setup Prompt-ONLY_NATIVE_CRASH`() {
         val service: CrashReporterService = mock()
-        val telemetryService: CrashReporterService = mock()
+        val telemetryService: CrashTelemetryService = mock()
 
         val reporter = spy(CrashReporter(
             services = listOf(service),
@@ -162,7 +163,7 @@ class CrashReporterTest {
     @Test
     fun `CrashReporter will submit crash telemetry even if crash report requires prompt`() {
         val service: CrashReporterService = mock()
-        val telemetryService: CrashReporterService = mock()
+        val telemetryService: CrashTelemetryService = mock()
 
         val reporter = spy(CrashReporter(
             services = listOf(service),
@@ -181,7 +182,7 @@ class CrashReporterTest {
 
     @Test
     fun `CrashReporter will not prompt the user if there is no crash services`() {
-        val telemetryService: CrashReporterService = mock()
+        val telemetryService: CrashTelemetryService = mock()
 
         val reporter = spy(CrashReporter(
             telemetryServices = listOf(telemetryService),
@@ -381,15 +382,14 @@ class CrashReporterTest {
     fun `CrashReporter forwards native crashes to telemetry service`() {
         var nativeCrash = false
 
-        val telemetryService = object : CrashReporterService {
-            override fun report(crash: Crash.UncaughtExceptionCrash): String? = null
+        val telemetryService = object : CrashTelemetryService {
+            override fun record(crash: Crash.UncaughtExceptionCrash) = Unit
 
-            override fun report(crash: Crash.NativeCodeCrash): String? {
+            override fun record(crash: Crash.NativeCodeCrash) {
                 nativeCrash = true
-                return null
             }
 
-            override fun report(throwable: Throwable): String? = null
+            override fun record(throwable: Throwable) = Unit
         }
 
         val reporter = spy(CrashReporter(
@@ -460,7 +460,7 @@ class CrashReporterTest {
     @Test
     fun `CrashReporter sends telemetry but don't send native crash if the crash is non-fatal and nonFatalPendingIntent is not null`() {
         val service: CrashReporterService = mock()
-        val telemetryService: CrashReporterService = mock()
+        val telemetryService: CrashTelemetryService = mock()
 
         val reporter = spy(CrashReporter(
             services = listOf(service),
@@ -486,7 +486,7 @@ class CrashReporterTest {
     @Test
     fun `CrashReporter sends telemetry and crash if the crash is non-fatal and nonFatalPendingIntent is null`() {
         val service: CrashReporterService = mock()
-        val telemetryService: CrashReporterService = mock()
+        val telemetryService: CrashTelemetryService = mock()
 
         val reporter = spy(CrashReporter(
             services = listOf(service),

components/lib/crash/src/test/java/mozilla/components/lib/crash/service/GleanCrashReporterServiceTest.kt

@@ -46,9 +46,9 @@ class GleanCrashReporterServiceTest {
             assertFalse("No previous persisted crashes must exist", service.file.exists())
 
             val crash = Crash.NativeCodeCrash("", true, "", true, arrayListOf())
-            service.report(crash)
+            service.record(crash)
 
-            verify(service).report(crash)
+            verify(service).record(crash)
 
             assertTrue("Persistence file must exist", service.file.exists())
             val lines = service.file.readLines()
@@ -85,9 +85,7 @@ class GleanCrashReporterServiceTest {
             assertFalse("No previous persisted crashes must exist", service.file.exists())
 
             val crash = Crash.NativeCodeCrash("", true, "", false, arrayListOf())
-            service.report(crash)
-
-            verify(service).report(crash)
+            service.record(crash)
 
             assertTrue("Persistence file must exist", service.file.exists())
             val lines = service.file.readLines()
@@ -124,8 +122,7 @@ class GleanCrashReporterServiceTest {
             assertFalse("No previous persisted crashes must exist", service.file.exists())
 
             val crash = Crash.UncaughtExceptionCrash(RuntimeException("Test"), arrayListOf())
-            service.report(crash)
-            verify(service).report(crash)
+            service.record(crash)
 
             assertTrue("Persistence file must exist", service.file.exists())
             val lines = service.file.readLines()
@@ -162,8 +159,7 @@ class GleanCrashReporterServiceTest {
             assertFalse("No previous persisted crashes must exist", service.file.exists())
 
             val throwable = RuntimeException("Test")
-            service.report(throwable)
-            verify(service).report(throwable)
+            service.record(throwable)
 
             assertTrue("Persistence file must exist", service.file.exists())
             val lines = service.file.readLines()
@@ -212,10 +208,10 @@ class GleanCrashReporterServiceTest {
             val nonfatalNativeCodeCrash = Crash.NativeCodeCrash("", true, "", false, arrayListOf())
 
             // Record some crashes
-            service.report(uncaughtExceptionCrash)
-            service.report(fatalNativeCodeCrash)
-            service.report(uncaughtExceptionCrash)
-            service.report(nonfatalNativeCodeCrash)
+            service.record(uncaughtExceptionCrash)
+            service.record(fatalNativeCodeCrash)
+            service.record(uncaughtExceptionCrash)
+            service.record(nonfatalNativeCodeCrash)
 
             // Make sure the file exists
             assertTrue("Persistence file must exist", service.file.exists())
@@ -260,8 +256,7 @@ class GleanCrashReporterServiceTest {
         assertFalse("No previous persisted crashes must exist", service.file.exists())
 
         val crash = Crash.UncaughtExceptionCrash(RuntimeException("Test"), arrayListOf())
-        service.report(crash)
-        verify(service).report(crash)
+        service.record(crash)
 
         assertTrue("Persistence file must exist", service.file.exists())
         val lines = service.file.readLines()
@@ -285,9 +280,7 @@ class GleanCrashReporterServiceTest {
             assertFalse("No previous persisted crashes must exist", service.file.exists())
 
             val crash = Crash.NativeCodeCrash("", true, "", true, arrayListOf())
-            service.report(crash)
-
-            verify(service).report(crash)
+            service.record(crash)
 
             assertTrue("Persistence file must exist", service.file.exists())
 

components/lib/crash/src/test/java/mozilla/components/lib/crash/service/SendCrashTelemetryServiceTest.kt

@@ -54,20 +54,17 @@ class SendCrashTelemetryServiceTest {
         var caughtCrash: Crash.NativeCodeCrash? = null
         val crashReporter = spy(CrashReporter(
             shouldPrompt = CrashReporter.Prompt.NEVER,
-            telemetryServices = listOf(object : CrashReporterService {
-                override fun report(crash: Crash.UncaughtExceptionCrash): String? {
+            telemetryServices = listOf(object : CrashTelemetryService {
+                override fun record(crash: Crash.UncaughtExceptionCrash) {
                     fail("Didn't expect uncaught exception crash")
-                    return null
                 }
 
-                override fun report(crash: Crash.NativeCodeCrash): String? {
+                override fun record(crash: Crash.NativeCodeCrash) {
                     caughtCrash = crash
-                    return null
                 }
 
-                override fun report(throwable: Throwable): String? {
+                override fun record(throwable: Throwable) {
                     fail("Didn't expect caught exception")
-                    return null
                 }
             }),
             scope = scope

samples/crash/src/main/java/org/mozilla/samples/crash/CrashApplication.kt

@@ -30,7 +30,8 @@ class CrashApplication : Application() {
         Log.addSink(AndroidLogSink())
 
         crashReporter = CrashReporter(
-            services = listOf(createDummyCrashService(this), GleanCrashReporterService(applicationContext)),
+            services = listOf(createDummyCrashService(this)),
+            telemetryServices = listOf(GleanCrashReporterService(applicationContext)),
             shouldPrompt = CrashReporter.Prompt.ALWAYS,
             promptConfiguration = CrashReporter.PromptConfiguration(
                 appName = "Sample App",