diff --git a/buildSrc/src/main/java/Dependencies.kt b/buildSrc/src/main/java/Dependencies.kt index 8243ebbd2c6..cc78e85df05 100644 --- 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/components/service/glean/src/main/java/mozilla/components/service/glean/net/ConceptFetchHttpUploader.kt b/components/service/glean/src/main/java/mozilla/components/service/glean/net/ConceptFetchHttpUploader.kt index f59c061ed1e..05a68f2d7fe 100644 --- a/components/service/glean/src/main/java/mozilla/components/service/glean/net/ConceptFetchHttpUploader.kt +++ b/components/service/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/components/service/glean/src/test/java/mozilla/components/service/glean/net/ConceptFetchHttpUploaderTest.kt b/components/service/glean/src/test/java/mozilla/components/service/glean/net/ConceptFetchHttpUploaderTest.kt index 290584b0849..b75526a9858 100644 --- a/components/service/glean/src/test/java/mozilla/components/service/glean/net/ConceptFetchHttpUploaderTest.kt +++ b/components/service/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(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() verify(mockClient).fetch(requestCaptor.capture()) @@ -102,7 +104,7 @@ class ConceptFetchHttpUploaderTest { val uploader = spy(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(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(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(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(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 index 66e6539964a..85b26904df4 100644 --- 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)