Merge #4696

4696: 1566488: Persist start_times to avoid 0 between start and end r=mdboom a=mdboom
Co-authored-by: Michael Droettboom <mdboom@gmail.com>

components/service/glean/src/main/java/mozilla/components/service/glean/Glean.kt

@@ -283,7 +283,7 @@ open class GleanInternalAPI internal constructor () {
 
         pingStorageEngine.clearPendingPings()
         storageEngineManager.clearAllStores()
-        pingMaker.resetPingSequenceNumbers()
+        pingMaker.resetPingMakerStorage()
 
         // This does not clear the experiments store (which isn't managed by the
         // StorageEngineManager), since doing so would mean we would have to have the

components/service/glean/src/main/java/mozilla/components/service/glean/ping/PingMaker.kt

@@ -22,7 +22,6 @@ internal class PingMaker(
     private val applicationContext: Context
 ) {
     private val logger = Logger("glean/PingMaker")
-    private val pingStartTimes: MutableMap<String, String> = mutableMapOf()
     private val objectStartTime = getISOTimeString()
     internal val sharedPreferences: SharedPreferences? by lazy {
         applicationContext.getSharedPreferences(
@@ -56,14 +55,53 @@ internal class PingMaker(
     }
 
     /**
-     * Reset all ping sequence numbers.
+     * Reset all ping sequence numbers and start times.
      */
-    internal fun resetPingSequenceNumbers() {
+    internal fun resetPingMakerStorage() {
         sharedPreferences?.let {
             it.edit().clear().apply()
         }
     }
 
+    /**
+     * Get the start time for a given ping.
+     * This is always equal to the end time of the last time the ping was sent.
+     *
+     * @param pingName The name of the ping
+     * @return start time
+     */
+    internal fun getPingStartTime(pingName: String): String {
+        sharedPreferences?.let {
+            val key = "${pingName}_start_time"
+            val currentValue = it.getString(key, objectStartTime)!!
+            return currentValue
+        }
+
+        // This clause should happen in testing only, where a sharedPreferences object
+        // isn't guaranteed to exist if using a mocked ApplicationContext
+        logger.error("Couldn't get SharedPreferences object for ping start times")
+        return objectStartTime
+    }
+
+    /**
+     * Set the start time for a given ping.
+     *
+     * @param pingName The name of the ping
+     * @param startTime The start time to set for the ping
+     */
+    internal fun setPingStartTime(pingName: String, startTime: String) {
+        sharedPreferences?.let {
+            val key = "${pingName}_start_time"
+            val editor = it.edit()
+            editor.putString(key, startTime)
+            editor.apply()
+        }
+
+        // This clause should happen in testing only, where a sharedPreferences object
+        // isn't guaranteed to exist if using a mocked ApplicationContext
+        logger.error("Couldn't get SharedPreferences object for ping start times")
+    }
+
     /**
      * Return the object containing the "ping_info" section of a ping.
      *
@@ -83,12 +121,12 @@ internal class PingMaker(
         // This needs to be a bit more involved for start-end times. "start_time" is
         // the time the ping was generated the last time. If not available, we use the
         // date the object was initialized.
-        val startTime = if (pingName in pingStartTimes) pingStartTimes[pingName] else objectStartTime
+        val startTime = getPingStartTime(pingName)
         pingInfo.put("start_time", startTime)
         val endTime = getISOTimeString()
         pingInfo.put("end_time", endTime)
         // Update the start time with the current time.
-        pingStartTimes[pingName] = endTime
+        setPingStartTime(pingName, endTime)
         return pingInfo
     }
 

components/service/glean/src/test/java/mozilla/components/service/glean/ping/PingMakerTest.kt

@@ -72,6 +72,48 @@ class PingMakerTest {
             <= OffsetDateTime.parse(pingInfo.getString("end_time")))
     }
 
+    @Test
+    fun `ping_info must persist start_time`() {
+        val applicationContext = ApplicationProvider.getApplicationContext<Context>()
+        val maker = PingMaker(
+            StorageEngineManager(
+                storageEngines = mapOf(
+                    "engine2" to MockStorageEngine(JSONObject(mapOf("a.b" to "foo")))
+                ),
+                applicationContext = applicationContext
+            ),
+            applicationContext
+        )
+
+        // Insert a dummy value into the first maker to make sure it's picked up by
+        // the second maker
+        maker.setPingStartTime(customPing.name, "2100-01-01")
+
+        val maker2 = PingMaker(
+            StorageEngineManager(
+                storageEngines = mapOf(
+                    "engine2" to MockStorageEngine(JSONObject(mapOf("a.b" to "foo")))
+                ),
+                applicationContext = applicationContext
+            ),
+            applicationContext
+        )
+
+        // Gather the data. We expect an empty ping with the "ping_info" information
+        val data = maker2.collect(customPing).orEmpty()
+        assertTrue("We expect a non-empty JSON blob", "{}" != data)
+
+        // Parse the data so that we can easily check the other fields
+        val jsonData = JSONObject(data)
+        val pingInfo = jsonData["ping_info"] as JSONObject
+        assertNotNull(pingInfo)
+
+        // "start_time" and "end_time" must be valid ISO8601 dates. DateTimeFormatter would
+        // throw otherwise.
+        assertEquals("2100-01-01", pingInfo.getString("start_time"))
+        DateTimeFormatter.ISO_OFFSET_DATE_TIME.parse(pingInfo.getString("end_time"))
+    }
+
     @Test
     fun `getPingInfo() must report all the required fields`() {
         val maker = PingMaker(