Merge #7209

7209: Update Glean to include new upload mechanism (& gzip) r=brizental a=badboy Co-authored-by: Jan-Erik Rediger <[email protected]>
mozilla-mobile · Jun 3, 2020 · fc17511 · fc17511
2 parents bec28a5 + 9383083
commit fc17511
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 56 deletions.
diff --git a/buildSrc/src/main/java/Dependencies.kt b/buildSrc/src/main/java/Dependencies.kt
@@ -29,7 +29,7 @@ object Versions {
 
     const val mozilla_appservices = "60.0.0"
 
-    const val mozilla_glean = "29.1.1"
+    const val mozilla_glean = "31.0.2"
 
     const val material = "1.1.0"
     const val nearby = "17.0.0"

diff --git a/...vice/glean/src/main/java/mozilla/components/service/glean/net/ConceptFetchHttpUploader.kt b/...vice/glean/src/main/java/mozilla/components/service/glean/net/ConceptFetchHttpUploader.kt
@@ -10,11 +10,12 @@ import mozilla.components.concept.fetch.Client
 import mozilla.components.concept.fetch.Header
 import mozilla.components.concept.fetch.MutableHeaders
 import mozilla.components.concept.fetch.Request
-import mozilla.components.concept.fetch.isClientError
-import mozilla.components.concept.fetch.isSuccess
 import mozilla.components.support.base.log.logger.Logger
 import mozilla.telemetry.glean.net.HeadersList
 import mozilla.telemetry.glean.net.PingUploader as CorePingUploader
+import mozilla.telemetry.glean.net.UploadResult
+import mozilla.telemetry.glean.net.HttpResponse
+import mozilla.telemetry.glean.net.RecoverableFailure
 import java.io.IOException
 import java.util.concurrent.TimeUnit
 
@@ -59,21 +60,21 @@ class ConceptFetchHttpUploader(
      *         or faced an unrecoverable error), false if there was a recoverable
      *         error callers can deal with.
      */
-    override fun upload(url: String, data: String, headers: HeadersList): Boolean {
+    override fun upload(url: String, data: ByteArray, headers: HeadersList): UploadResult {
         val request = buildRequest(url, data, headers)
 
         return try {
             performUpload(client.value, request)
         } catch (e: IOException) {
             logger.warn("IOException while uploading ping", e)
-            false
+            RecoverableFailure
         }
     }
 
     @VisibleForTesting(otherwise = PRIVATE)
     internal fun buildRequest(
         url: String,
-        data: String,
+        data: ByteArray,
         headers: HeadersList
     ): Request {
         val conceptHeaders = MutableHeaders(headers.map { Header(it.first, it.second) })
@@ -88,49 +89,15 @@ class ConceptFetchHttpUploader(
             // offer a better API to do that, so we nuke all cookies going to our telemetry
             // endpoint.
             cookiePolicy = Request.CookiePolicy.OMIT,
-            body = Request.Body.fromString(data)
+            body = Request.Body(data.inputStream())
         )
     }
 
     @Throws(IOException::class)
-    internal fun performUpload(client: Client, request: Request): Boolean {
+    internal fun performUpload(client: Client, request: Request): UploadResult {
         logger.debug("Submitting ping to: ${request.url}")
         client.fetch(request).use { response ->
-            when {
-                response.isSuccess -> {
-                    // Known success errors (2xx):
-                    // 200 - OK. Request accepted into the pipeline.
-
-                    // We treat all success codes as successful upload even though we only expect 200.
-                    logger.debug("Ping successfully sent (${response.status})")
-                    return true
-                }
-
-                response.isClientError -> {
-                    // Known client (4xx) errors:
-                    // 404 - not found - POST/PUT to an unknown namespace
-                    // 405 - wrong request type (anything other than POST/PUT)
-                    // 411 - missing content-length header
-                    // 413 - request body too large (Note that if we have badly-behaved clients that
-                    //       retry on 4XX, we should send back 202 on body/path too long).
-                    // 414 - request path too long (See above)
-
-                    // Something our client did is not correct. It's unlikely that the client is going
-                    // to recover from this by re-trying again, so we just log and error and report a
-                    // successful upload to the service.
-                    logger.error("Server returned client error code ${response.status} for ${request.url}")
-                    return true
-                }
-
-                else -> {
-                    // Known other errors:
-                    // 500 - internal error
-
-                    // For all other errors we log a warning an try again at a later time.
-                    logger.warn("Server returned response code ${response.status} for ${request.url}")
-                    return false
-                }
-            }
+            return HttpResponse(response.status)
         }
     }
 }
diff --git a/.../glean/src/test/java/mozilla/components/service/glean/net/ConceptFetchHttpUploaderTest.kt b/.../glean/src/test/java/mozilla/components/service/glean/net/ConceptFetchHttpUploaderTest.kt
@@ -13,6 +13,8 @@ import mozilla.components.support.test.any
 import mozilla.components.support.test.argumentCaptor
 import mozilla.components.support.test.mock
 import mozilla.telemetry.glean.config.Configuration
+import mozilla.telemetry.glean.net.HttpResponse
+import mozilla.telemetry.glean.net.RecoverableFailure
 import okhttp3.mockwebserver.Dispatcher
 import okhttp3.mockwebserver.MockResponse
 import okhttp3.mockwebserver.MockWebServer
@@ -60,7 +62,7 @@ class ConceptFetchHttpUploaderTest {
         val uploader =
             spy<ConceptFetchHttpUploader>(ConceptFetchHttpUploader(lazy { HttpURLConnectionClient() }))
 
-        val request = uploader.buildRequest(testPath, testPing, emptyList())
+        val request = uploader.buildRequest(testPath, testPing.toByteArray(), emptyList())
 
         assertEquals(
             Pair(ConceptFetchHttpUploader.DEFAULT_READ_TIMEOUT, TimeUnit.MILLISECONDS),
@@ -85,7 +87,7 @@ class ConceptFetchHttpUploaderTest {
         )
 
         val uploader = ConceptFetchHttpUploader(lazy { mockClient })
-        uploader.upload(testPath, testPing, expectedHeaders.toList())
+        uploader.upload(testPath, testPing.toByteArray(), expectedHeaders.toList())
         val requestCaptor = argumentCaptor<Request>()
         verify(mockClient).fetch(requestCaptor.capture())
 
@@ -102,7 +104,7 @@ class ConceptFetchHttpUploaderTest {
         val uploader =
             spy<ConceptFetchHttpUploader>(ConceptFetchHttpUploader(lazy { HttpURLConnectionClient() }))
 
-        val request = uploader.buildRequest(testPath, testPing, emptyList())
+        val request = uploader.buildRequest(testPath, testPing.toByteArray(), emptyList())
 
         assertEquals(request.cookiePolicy, Request.CookiePolicy.OMIT)
     }
@@ -115,7 +117,7 @@ class ConceptFetchHttpUploaderTest {
 
         val uploader = spy<ConceptFetchHttpUploader>(ConceptFetchHttpUploader(lazy { mockClient }))
 
-        assertTrue(uploader.upload(testPath, testPing, emptyList()))
+        assertEquals(HttpResponse(200), uploader.upload(testPath, testPing.toByteArray(), emptyList()))
     }
     @Test
     fun `upload() returns false for server errors (5xx)`() {
@@ -126,7 +128,7 @@ class ConceptFetchHttpUploaderTest {
 
             val uploader = spy<ConceptFetchHttpUploader>(ConceptFetchHttpUploader(lazy { mockClient }))
 
-            assertFalse(uploader.upload(testPath, testPing, emptyList()))
+            assertEquals(HttpResponse(responseCode), uploader.upload(testPath, testPing.toByteArray(), emptyList()))
         }
     }
 
@@ -139,7 +141,7 @@ class ConceptFetchHttpUploaderTest {
 
             val uploader = spy<ConceptFetchHttpUploader>(ConceptFetchHttpUploader(lazy { mockClient }))
 
-            assertTrue(uploader.upload(testPath, testPing, emptyList()))
+            assertEquals(HttpResponse(responseCode), uploader.upload(testPath, testPing.toByteArray(), emptyList()))
         }
     }
 
@@ -152,7 +154,7 @@ class ConceptFetchHttpUploaderTest {
 
             val uploader = spy<ConceptFetchHttpUploader>(ConceptFetchHttpUploader(lazy { mockClient }))
 
-            assertTrue(uploader.upload(testPath, testPing, emptyList()))
+            assertEquals(HttpResponse(responseCode), uploader.upload(testPath, testPing.toByteArray(), emptyList()))
         }
     }
 
@@ -163,7 +165,7 @@ class ConceptFetchHttpUploaderTest {
         val client = ConceptFetchHttpUploader(lazy { HttpURLConnectionClient() })
 
         val submissionUrl = "http://" + server.hostName + ":" + server.port + testPath
-        assertTrue(client.upload(submissionUrl, testPing, listOf(Pair("test", "header"))))
+        assertEquals(HttpResponse(200), client.upload(submissionUrl, testPing.toByteArray(), listOf(Pair("test", "header"))))
 
         val request = server.takeRequest()
         assertEquals(testPath, request.path)
@@ -181,7 +183,7 @@ class ConceptFetchHttpUploaderTest {
         val client = ConceptFetchHttpUploader(lazy { HttpURLConnectionClient() })
 
         val submissionUrl = "http://" + server.hostName + ":" + server.port + testPath
-        assertTrue(client.upload(submissionUrl, testPing, listOf(Pair("test", "header"))))
+        assertEquals(HttpResponse(200), client.upload(submissionUrl, testPing.toByteArray(), listOf(Pair("test", "header"))))
 
         val request = server.takeRequest()
         assertEquals(testPath, request.path)
@@ -200,7 +202,7 @@ class ConceptFetchHttpUploaderTest {
         val client = ConceptFetchHttpUploader(lazy { OkHttpClient() })
 
         val submissionUrl = "http://" + server.hostName + ":" + server.port + testPath
-        assertTrue(client.upload(submissionUrl, testPing, listOf(Pair("test", "header"))))
+        assertEquals(HttpResponse(200), client.upload(submissionUrl, testPing.toByteArray(), listOf(Pair("test", "header"))))
 
         val request = server.takeRequest()
         assertEquals(testPath, request.path)
@@ -250,7 +252,7 @@ class ConceptFetchHttpUploaderTest {
         // Trigger the connection.
         val client = ConceptFetchHttpUploader(lazy { HttpURLConnectionClient() })
         val submissionUrl = testConfig.serverEndpoint + testPath
-        assertTrue(client.upload(submissionUrl, testPing, emptyList()))
+        assertEquals(HttpResponse(200), client.upload(submissionUrl, testPing.toByteArray(), emptyList()))
 
         val request = server.takeRequest()
         assertEquals(testPath, request.path)
@@ -274,7 +276,7 @@ class ConceptFetchHttpUploaderTest {
 
         // And IOException during upload is a failed upload that we should retry. The client should
         // return false in this case.
-        assertFalse(uploader.upload("path", "ping", emptyList()))
+        assertEquals(RecoverableFailure, uploader.upload("path", "ping".toByteArray(), emptyList()))
     }
 
     @Test
@@ -286,7 +288,7 @@ class ConceptFetchHttpUploaderTest {
         assertFalse(uploader.client.isInitialized())
 
         // After calling upload, the client must get instantiated.
-        uploader.upload("path", "ping", emptyList())
+        uploader.upload("path", "ping".toByteArray(), emptyList())
         assertTrue(uploader.client.isInitialized())
     }
 }
diff --git a/docs/changelog.md b/docs/changelog.md
@@ -51,6 +51,12 @@ permalink: /changelog/
 * **support-ktx**
   * `String.isUrlStrict`, deprecated in 40.0.0, was now removed due to performance issues. Use the less strict `isURL` instead or customize based on `:lib-publicsuffixlist`.
 
+* **service-glean**
+  * Glean was updated to v31.0.2
+    * Provide a new upload mechanism, now driven by internals. This has no impact to consumers of service-glean.
+    * Automatically Gzip-compress ping payloads before upload
+    * Upgrade `glean_parser` to v1.22.0
+
 # 43.0.0
 
 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v42.0.0...v43.0.0)