From 86b8a133f3e805c5bb495afa32226ad6c669df42 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 12 May 2020 12:20:51 +0200 Subject: [PATCH 01/63] Upgrade mdbook-dtmo [doc only] --- .circleci/config.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 6bb458dce3..fc62b88f58 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -346,10 +346,10 @@ jobs: - run: name: Install mdbook-dtmo command: | - MDBOOK_VERSION=0.5.0 + MDBOOK_VERSION=0.6.0 MDBOOK="mdbook-dtmo-${MDBOOK_VERSION}-x86_64-unknown-linux-gnu.tar.gz" - MDBOOK_SHA256=ce96727a7d066dc69b9148db46b737f45f79bec738446177806e645673ed8a4e - curl -sfSL --retry 5 --retry-delay 10 -O "https://github.com/badboy/mdbook-dtmo/releases/download/${MDBOOK_VERSION}/${MDBOOK}" + MDBOOK_SHA256=a399a4a478290d1d889b4acefc3aecb8cd1ea051728aed276a9b169c03f8d375 + curl -sfSL --retry 5 -O "https://github.com/badboy/mdbook-dtmo/releases/download/${MDBOOK_VERSION}/${MDBOOK}" echo "${MDBOOK_SHA256} *${MDBOOK}" | shasum -a 256 -c - tar -xvf "${MDBOOK}" # We rename it to mdbook here, so other tools keep working as expected From 8592dd55c8b63e0ec3a76910a0df4830e6aabfbe Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 12 May 2020 12:42:33 +0200 Subject: [PATCH 02/63] Fix Java API examples to be valid Java [doc only] --- docs/user/metrics/boolean.md | 10 ++++----- docs/user/metrics/counter.md | 14 ++++++------- docs/user/metrics/datetime.md | 14 ++++++------- docs/user/metrics/string.md | 14 ++++++------- docs/user/metrics/timespan.md | 22 ++++++++++---------- docs/user/metrics/timing_distribution.md | 26 ++++++++++++------------ docs/user/metrics/uuid.md | 12 +++++------ 7 files changed, 56 insertions(+), 56 deletions(-) diff --git a/docs/user/metrics/boolean.md b/docs/user/metrics/boolean.md index c1ed53e62e..69d4a92a2c 100644 --- a/docs/user/metrics/boolean.md +++ b/docs/user/metrics/boolean.md @@ -44,20 +44,20 @@ assertTrue(Flags.a11yEnabled.testGetValue())
```Java -import org.mozilla.yourApplication.GleanMetrics.Flags +import org.mozilla.yourApplication.GleanMetrics.Flags; -Flags.INSTANCE.a11yEnabled.set(System.isAccessibilityEnabled()) +Flags.INSTANCE.a11yEnabled.set(System.isAccessibilityEnabled()); ``` There are test APIs available too: ```Java -import org.mozilla.yourApplication.GleanMetrics.Flags +import org.mozilla.yourApplication.GleanMetrics.Flags; // Was anything recorded? -assertTrue(Flags.INSTANCE.a11yEnabled.testHasValue()) +assertTrue(Flags.INSTANCE.a11yEnabled.testHasValue()); // Does it have the expected value? -assertTrue(Flags.INSTANCE.a11yEnabled.testGetValue()) +assertTrue(Flags.INSTANCE.a11yEnabled.testGetValue()); ```
diff --git a/docs/user/metrics/counter.md b/docs/user/metrics/counter.md index 491372e973..de8566a3d4 100644 --- a/docs/user/metrics/counter.md +++ b/docs/user/metrics/counter.md @@ -50,25 +50,25 @@ assertEquals(
```Java -import org.mozilla.yourApplication.GleanMetrics.Controls +import org.mozilla.yourApplication.GleanMetrics.Controls; -Controls.INSTANCE.refreshPressed.add() // Adds 1 to the counter. -Controls.INSTANCE.refreshPressed.add(5) // Adds 5 to the counter. +Controls.INSTANCE.refreshPressed.add(); // Adds 1 to the counter. +Controls.INSTANCE.refreshPressed.add(5); // Adds 5 to the counter. ``` There are test APIs available too: ```Java -import org.mozilla.yourApplication.GleanMetrics.Controls +import org.mozilla.yourApplication.GleanMetrics.Controls; // Was anything recorded? -assertTrue(Controls.INSTANCE.refreshPressed.testHasValue()) +assertTrue(Controls.INSTANCE.refreshPressed.testHasValue()); // Does the counter have the expected value? -assertEquals(6, Controls.INSTANCE.refreshPressed.testGetValue()) +assertEquals(6, Controls.INSTANCE.refreshPressed.testGetValue()); // Did the counter record an negative value? assertEquals( 1, Controls.INSTANCE.refreshPressed.testGetNumRecordedErrors(ErrorType.InvalidValue) -) +); ```
diff --git a/docs/user/metrics/datetime.md b/docs/user/metrics/datetime.md index d72df604e7..b8845c8d59 100644 --- a/docs/user/metrics/datetime.md +++ b/docs/user/metrics/datetime.md @@ -66,25 +66,25 @@ assertEquals(1, Install.firstRun.testGetNumRecordedErrors(ErrorType.InvalidValue
```Java -import org.mozilla.yourApplication.GleanMetrics.Install +import org.mozilla.yourApplication.GleanMetrics.Install; -Install.INSTANCE.firstRun.set() // Records "now" -Install.INSTANCE.firstRun.set(Calendar(2019, 3, 25)) // Records a custom datetime +Install.INSTANCE.firstRun.set(); // Records "now" +Install.INSTANCE.firstRun.set(Calendar(2019, 3, 25)); // Records a custom datetime ``` There are test APIs available too: ```Java -import org.mozilla.yourApplication.GleanMetrics.Install +import org.mozilla.yourApplication.GleanMetrics.Install; // Was anything recorded? -assertTrue(Install.INSTANCE.firstRun.testHasValue()) +assertTrue(Install.INSTANCE.firstRun.testHasValue()); // Was it the expected value? // NOTE: Datetimes always include a timezone offset from UTC, hence the // "-05:00" suffix. -assertEquals("2019-03-25-05:00", Install.INSTANCE.firstRun.testGetValueAsString()) +assertEquals("2019-03-25-05:00", Install.INSTANCE.firstRun.testGetValueAsString()); // Was the value invalid? -assertEquals(1, Install.INSTANCE.firstRun.testGetNumRecordedErrors(ErrorType.InvalidValue)) +assertEquals(1, Install.INSTANCE.firstRun.testGetNumRecordedErrors(ErrorType.InvalidValue)); ```
diff --git a/docs/user/metrics/string.md b/docs/user/metrics/string.md index d9080629da..3ae5e29e63 100644 --- a/docs/user/metrics/string.md +++ b/docs/user/metrics/string.md @@ -54,12 +54,12 @@ assertEquals(1, SearchDefault.name.testGetNumRecordedErrors(ErrorType.InvalidVal
```Java -import org.mozilla.yourApplication.GleanMetrics.SearchDefault +import org.mozilla.yourApplication.GleanMetrics.SearchDefault; // Record a value into the metric. -SearchDefault.INSTANCE.name.set("duck duck go") +SearchDefault.INSTANCE.name.set("duck duck go"); // If it changed later, you can record the new value: -SearchDefault.INSTANCE.name.set("wikipedia") +SearchDefault.INSTANCE.name.set("wikipedia"); ``` There are test APIs available too: @@ -68,17 +68,17 @@ There are test APIs available too: import org.mozilla.yourApplication.GleanMetrics.SearchDefault // Was anything recorded? -assertTrue(SearchDefault.INSTANCE.name.testHasValue()) +assertTrue(SearchDefault.INSTANCE.name.testHasValue()); // Does the string metric have the expected value? // IMPORTANT: It may have been truncated -- see "Limits" below -assertEquals("wikipedia", SearchDefault.INSTANCE.name.testGetValue()) +assertEquals("wikipedia", SearchDefault.INSTANCE.name.testGetValue()); // Was the string truncated, and an error reported? assertEquals( - 1, + 1, SearchDefault.INSTANCE.name.testGetNumRecordedErrors( ErrorType.InvalidValue ) -) +); ```
diff --git a/docs/user/metrics/timespan.md b/docs/user/metrics/timespan.md index aef360df0f..fa03e973f0 100644 --- a/docs/user/metrics/timespan.md +++ b/docs/user/metrics/timespan.md @@ -77,20 +77,20 @@ assertEquals(1, Auth.loginTime.testGetNumRecordedErrors(ErrorType.InvalidValue))
```Java -import org.mozilla.yourApplication.GleanMetrics.Auth +import org.mozilla.yourApplication.GleanMetrics.Auth; -fun onShowLogin() { - Auth.INSTANCE.loginTime.start() +void onShowLogin() { + Auth.INSTANCE.loginTime.start(); // ... } -fun onLogin() { - Auth.INSTANCE.loginTime.stop() +void onLogin() { + Auth.INSTANCE.loginTime.stop(); // ... } -fun onLoginCancel() { - Auth.INSTANCE.loginTime.cancel() +void onLoginCancel() { + Auth.INSTANCE.loginTime.cancel(); // ... } ``` @@ -100,19 +100,19 @@ The time reported in the telemetry ping will be timespan recorded during the lif There are test APIs available too: ```Java -import org.mozilla.yourApplication.GleanMetrics.Auth +import org.mozilla.yourApplication.GleanMetrics.Auth; // Was anything recorded? -assertTrue(Auth.INSTANCE.loginTime.testHasValue()) +assertTrue(Auth.INSTANCE.loginTime.testHasValue()); // Does the timer have the expected value -assertTrue(Auth.INSTANCE.loginTime.testGetValue() > 0) +assertTrue(Auth.INSTANCE.loginTime.testGetValue() > 0); // Was the timing recorded incorrectly? assertEquals( 1, Auth.INSTANCE.loginTime.testGetNumRecordedErrors( ErrorType.InvalidValue ) -) +); ```
diff --git a/docs/user/metrics/timing_distribution.md b/docs/user/metrics/timing_distribution.md index b6a7a57f19..aba32197a4 100644 --- a/docs/user/metrics/timing_distribution.md +++ b/docs/user/metrics/timing_distribution.md @@ -95,17 +95,17 @@ assertEquals(1, pages.pageLoad.testGetNumRecordedErrors(ErrorType.InvalidValue))
```Java -import mozilla.components.service.glean.GleanTimerId -import org.mozilla.yourApplication.GleanMetrics.Pages +import mozilla.components.service.glean.GleanTimerId; +import org.mozilla.yourApplication.GleanMetrics.Pages; -val timerId : GleanTimerId +GleanTimerId timerId; -fun onPageStart(e: Event) { - timerId = Pages.INSTANCE.pageLoad.start() +void onPageStart(Event e) { + timerId = Pages.INSTANCE.pageLoad.start(); } -fun onPageLoaded(e: Event) { - Pages.INSTANCE.pageLoad.stopAndAccumulate(timerId) +void onPageLoaded(Event e) { + Pages.INSTANCE.pageLoad.stopAndAccumulate(timerId); } ``` @@ -114,19 +114,19 @@ There are test APIs available too. For convenience, properties `sum` and `count Continuing the `pageLoad` example above, at this point the metric should have a `sum == 11` and a `count == 2`: ```Java -import org.mozilla.yourApplication.GleanMetrics.Pages +import org.mozilla.yourApplication.GleanMetrics.Pages; // Was anything recorded? -assertTrue(pages.INSTANCE.pageLoad.testHasValue()) +assertTrue(pages.INSTANCE.pageLoad.testHasValue()); // Get snapshot. -val snapshot = pages.INSTANCE.pageLoad.testGetValue() +DistributionData snapshot = pages.INSTANCE.pageLoad.testGetValue(); // Does the sum have the expected value? -assertEquals(11, snapshot.getSum) +assertEquals(11, snapshot.getSum); // Usually you don't know the exact timing values, but how many should have been recorded. -assertEquals(2L, snapshot.getCount) +assertEquals(2L, snapshot.getCount); // Was an error recorded? assertEquals( @@ -134,7 +134,7 @@ assertEquals( pages.INSTANCE.pageLoad.testGetNumRecordedErrors( ErrorType.InvalidValue ) -) +); ```
diff --git a/docs/user/metrics/uuid.md b/docs/user/metrics/uuid.md index baa3483ad1..1725b3e8a2 100644 --- a/docs/user/metrics/uuid.md +++ b/docs/user/metrics/uuid.md @@ -47,21 +47,21 @@ assertEquals(uuid, User.clientId.testGetValue())
```Java -import org.mozilla.yourApplication.GleanMetrics.User +import org.mozilla.yourApplication.GleanMetrics.User; -User.INSTANCE.clientId.generateAndSet() // Generate a new UUID and record it -User.INSTANCE.clientId.set(UUID.randomUUID()) // Set a UUID explicitly +User.INSTANCE.clientId.generateAndSet(); // Generate a new UUID and record it +User.INSTANCE.clientId.set(UUID.randomUUID()); // Set a UUID explicitly ``` There are test APIs available too: ```Java -import org.mozilla.yourApplication.GleanMetrics.User +import org.mozilla.yourApplication.GleanMetrics.User; // Was anything recorded? -assertTrue(User.INSTANCE.clientId.testHasValue()) +assertTrue(User.INSTANCE.clientId.testHasValue()); // Was it the expected value? -assertEquals(uuid, User.INSTANCE.clientId.testGetValue()) +assertEquals(uuid, User.INSTANCE.clientId.testGetValue()); ```
From baad9eb1844a526ddb54d34e9658410aee58911e Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 12 May 2020 12:42:54 +0200 Subject: [PATCH 03/63] Adjust notes about geckoview-only metric types [doc only] --- docs/user/metrics/custom_distribution.md | 2 +- docs/user/metrics/quantity.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/user/metrics/custom_distribution.md b/docs/user/metrics/custom_distribution.md index 864390ad2b..173bbf343f 100644 --- a/docs/user/metrics/custom_distribution.md +++ b/docs/user/metrics/custom_distribution.md @@ -8,7 +8,7 @@ Otherwise, look at the standard distribution metric types: * [Timing Distributions](timing_distribution.md) * [Memory Distributions](memory_distribution.md) -> Note: Custom distributions are currently only allowed for GeckoView metrics (the `gecko_datapoint` parameter is present). +> **Note**: Custom distributions are currently only allowed for GeckoView metrics (the `gecko_datapoint` parameter is present) and thus have only a Kotlin API. ## Configuration diff --git a/docs/user/metrics/quantity.md b/docs/user/metrics/quantity.md index d7cdec6027..0e59edc99d 100644 --- a/docs/user/metrics/quantity.md +++ b/docs/user/metrics/quantity.md @@ -3,7 +3,7 @@ Used to record a single non-negative integer value. For example, the width of the display in pixels. -> Quantities are only available for metrics that come from Gecko. +> **Note**: Quantities are currently only allowed for GeckoView metrics (the `gecko_datapoint` parameter is present) and thus have only a Kotlin API. ## Configuration From bd2fc2e6545c02c81c8fd9ea4af99978e276a798 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 12 May 2020 12:45:45 +0200 Subject: [PATCH 04/63] Include note about time spent in sleep on Android [doc only] --- docs/user/metrics/timespan.md | 6 +++++- docs/user/metrics/timing_distribution.md | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/user/metrics/timespan.md b/docs/user/metrics/timespan.md index fa03e973f0..c42241c25b 100644 --- a/docs/user/metrics/timespan.md +++ b/docs/user/metrics/timespan.md @@ -236,7 +236,11 @@ HistorySync.setRawNanos(duration) ## Limits -* None. +* Timings are recorded in nanoseconds. + + * On Android, the [`SystemClock.elapsedRealtimeNanos()`](https://developer.android.com/reference/android/os/SystemClock.html#elapsedRealtimeNanos()) function is used, so it is limited by the accuracy and performance of that timer. The time measurement includes time spent in sleep. + + * On Python 3.7 and later, [`time.monotonic_ns()`](https://docs.python.org/3/library/time.html#time.monotonic_ns) is used. On earlier versions of Python, [`time.monotonics()`](https://docs.python.org/3/library/time.html#time.monotonic) is used, which is not guaranteed to have nanosecond resolution. ## Examples diff --git a/docs/user/metrics/timing_distribution.md b/docs/user/metrics/timing_distribution.md index aba32197a4..60bd17f430 100644 --- a/docs/user/metrics/timing_distribution.md +++ b/docs/user/metrics/timing_distribution.md @@ -231,7 +231,7 @@ assert 1 == metrics.pages.page_load.test_get_num_recorded_errors( * Timings are recorded in nanoseconds. - * On Android, the [`SystemClock.elapsedRealtimeNanos()`](https://developer.android.com/reference/android/os/SystemClock.html#elapsedRealtimeNanos()) function is used, so it is limited by the accuracy and performance of that timer. + * On Android, the [`SystemClock.elapsedRealtimeNanos()`](https://developer.android.com/reference/android/os/SystemClock.html#elapsedRealtimeNanos()) function is used, so it is limited by the accuracy and performance of that timer. The time measurement includes time spent in sleep. * On Python 3.7 and later, [`time.monotonic_ns()`](https://docs.python.org/3/library/time.html#time.monotonic_ns) is used. On earlier versions of Python, [`time.monotonics()`](https://docs.python.org/3/library/time.html#time.monotonic) is used, which is not guaranteed to have nanosecond resolution. From 77ec4b241fd33093bf04e810214c4af53a2632fb Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 12 May 2020 12:55:31 +0200 Subject: [PATCH 05/63] Include note about sleep causing timer to pause on iOS [doc only] --- docs/user/metrics/timespan.md | 4 ++++ docs/user/metrics/timing_distribution.md | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/docs/user/metrics/timespan.md b/docs/user/metrics/timespan.md index c42241c25b..b268c5a2a9 100644 --- a/docs/user/metrics/timespan.md +++ b/docs/user/metrics/timespan.md @@ -240,6 +240,10 @@ HistorySync.setRawNanos(duration) * On Android, the [`SystemClock.elapsedRealtimeNanos()`](https://developer.android.com/reference/android/os/SystemClock.html#elapsedRealtimeNanos()) function is used, so it is limited by the accuracy and performance of that timer. The time measurement includes time spent in sleep. + * On iOS, the [`mach_absolute_time`](https://developer.apple.com/library/archive/documentation/Darwin/Conceptual/KernelProgramming/services/services.html) function is used, + so it is limited by the accuracy and performance of that timer. + The time measurement does not include time spent in sleep. + * On Python 3.7 and later, [`time.monotonic_ns()`](https://docs.python.org/3/library/time.html#time.monotonic_ns) is used. On earlier versions of Python, [`time.monotonics()`](https://docs.python.org/3/library/time.html#time.monotonic) is used, which is not guaranteed to have nanosecond resolution. ## Examples diff --git a/docs/user/metrics/timing_distribution.md b/docs/user/metrics/timing_distribution.md index 60bd17f430..c9ba5e45c1 100644 --- a/docs/user/metrics/timing_distribution.md +++ b/docs/user/metrics/timing_distribution.md @@ -233,6 +233,10 @@ assert 1 == metrics.pages.page_load.test_get_num_recorded_errors( * On Android, the [`SystemClock.elapsedRealtimeNanos()`](https://developer.android.com/reference/android/os/SystemClock.html#elapsedRealtimeNanos()) function is used, so it is limited by the accuracy and performance of that timer. The time measurement includes time spent in sleep. + * On iOS, the [`mach_absolute_time`](https://developer.apple.com/library/archive/documentation/Darwin/Conceptual/KernelProgramming/services/services.html) function is used, + so it is limited by the accuracy and performance of that timer. + The time measurement does not include time spent in sleep. + * On Python 3.7 and later, [`time.monotonic_ns()`](https://docs.python.org/3/library/time.html#time.monotonic_ns) is used. On earlier versions of Python, [`time.monotonics()`](https://docs.python.org/3/library/time.html#time.monotonic) is used, which is not guaranteed to have nanosecond resolution. * The maximum timing value that will be recorded depends on the `time_unit` parameter: From 68c67f2a1e923483680f446630ff594f17027958 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 13 May 2020 11:46:55 +0200 Subject: [PATCH 06/63] Regenerate dependency summary before a release [doc only] --- bin/prepare-release.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/bin/prepare-release.sh b/bin/prepare-release.sh index efa3a51fac..4173d16e1a 100755 --- a/bin/prepare-release.sh +++ b/bin/prepare-release.sh @@ -144,6 +144,11 @@ ${CHANGELOG} EOL fi +### Dependency summary ### + +echo "Updating dependency summary" +run "${WORKSPACE_ROOT}"/bin/dependency-summary.sh + echo "Everything prepared for v${NEW_VERSION}" echo echo "Changed files:" From 72b490affcd351a50c16e791c4dee0a64209784d Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 13 May 2020 14:11:27 +0200 Subject: [PATCH 07/63] Always fetch the "latest" schema from the master branch [doc only] --- .circleci/config.yml | 2 +- bin/update-schema.sh | 32 ++------------------------------ 2 files changed, 3 insertions(+), 31 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 6bb458dce3..3b6a481733 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -232,7 +232,7 @@ jobs: - run: name: Check vendored schema for upstream updates command: | - bin/update-schema.sh latest + bin/update-schema.sh master if ! git diff --exit-code HEAD -- glean-core/preview/tests/glean.1.schema.json; then echo "====================================" echo "Latest schema from upstream changed." diff --git a/bin/update-schema.sh b/bin/update-schema.sh index 8caddc2429..fbd5f33da8 100755 --- a/bin/update-schema.sh +++ b/bin/update-schema.sh @@ -16,18 +16,13 @@ # # Environment: # -# DRY_RUN - Do not modify files or run destructive commands when set. # VERB - Log commands that are run when set. set -eo pipefail run() { [ "${VERB:-0}" != 0 ] && echo "+ $*" - if [ "$DOIT" = y ]; then - "$@" - else - true - fi + "$@" } update() { @@ -39,38 +34,15 @@ update() { run curl --silent --fail --show-error --location --retry 5 --retry-delay 10 "$FULL_URL" --output "$SCHEMA_PATH" } -get_latest() { - API_URL="https://api.github.com/repos/mozilla-services/mozilla-pipeline-schemas/commits?path=schemas%2Fglean%2Fglean%2Fglean.1.schema.json&page=1&per_page=1" - SHA="$(curl --silent --fail --show-error --location --retry 5 --retry-delay 10 "$API_URL" | grep --max-count=1 sha)" - - echo "$SHA" | $SED -E -e 's/.+: "([^"]+)".*/\1/' -} - -SED=sed -if command -v gsed >/dev/null; then - SED=gsed -fi - -DOIT=y -if [[ -n "$DRY_RUN" ]]; then - echo "Dry-run. Not modifying files." - DOIT=n -fi - WORKSPACE_ROOT="$( cd "$(dirname "$0")/.." ; pwd -P )" SCHEMA_URL="https://raw.githubusercontent.com/mozilla-services/mozilla-pipeline-schemas/%s/schemas/glean/glean/glean.1.schema.json" if [ -z "$1" ]; then - echo "Usage: $(basename $0) " + echo "Usage: $(basename $0) " echo echo "Update schema version to test" exit 1 fi COMMIT_HASH="$1" - -if [ "$COMMIT_HASH" = "latest" ]; then - COMMIT_HASH="$(get_latest)" -fi - update "$COMMIT_HASH" From e94f571132a5c9083e5b105848806ee5b9c283ab Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 13 May 2020 17:34:16 +0200 Subject: [PATCH 08/63] Use newer Android Gradle Plugin This is required to be able for local composite builds with a-c. a-c upgraded a while back already. Last change here: https://github.com/mozilla-mobile/android-components/commit/61d56b74c2 --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index d14e15acc1..e063007d84 100644 --- a/build.gradle +++ b/build.gradle @@ -12,7 +12,7 @@ buildscript { // versions below must match the ones in that repository. ext.versions = [ android_components: '15.0.0', - android_gradle_plugin: '3.4.1', + android_gradle_plugin: '3.5.3', android_maven_publish_plugin: '3.6.2', coroutines: '1.3.0', dokka: '0.9.17', From 2745269dc311818ad159555829cfc199694ce26a Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Wed, 13 May 2020 17:45:31 +0200 Subject: [PATCH 09/63] Upgrade Gradle in use A new version is required the Android Gradle plugin. This is the same as a-c currently uses. --- gradle/wrapper/gradle-wrapper.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gradle/wrapper/gradle-wrapper.properties b/gradle/wrapper/gradle-wrapper.properties index ea13fdfd19..5028f28f8e 100644 --- a/gradle/wrapper/gradle-wrapper.properties +++ b/gradle/wrapper/gradle-wrapper.properties @@ -1,5 +1,5 @@ distributionBase=GRADLE_USER_HOME distributionPath=wrapper/dists -distributionUrl=https\://services.gradle.org/distributions/gradle-5.3.1-bin.zip +distributionUrl=https\://services.gradle.org/distributions/gradle-5.6.4-bin.zip zipStoreBase=GRADLE_USER_HOME zipStorePath=wrapper/dists From 0dc78c625e668fb6d0b456b752c397ed019d25da Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Thu, 14 May 2020 10:57:14 +0200 Subject: [PATCH 10/63] Enable Glean Python support outside of MinGW Currently, Glean fails to install on Windows, outside of MinGW. This enables that support again. --- glean-core/python/setup.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/glean-core/python/setup.py b/glean-core/python/setup.py index 8cdeb85d9f..e073830529 100644 --- a/glean-core/python/setup.py +++ b/glean-core/python/setup.py @@ -66,7 +66,9 @@ shared_object = "libglean_ffi.so" elif platform == "darwin": shared_object = "libglean_ffi.dylib" -elif platform == "windows": +elif platform.startswith("win"): + # `platform` can be both "windows" (if running within MinGW) or "win32" + # if running in a standard Python environment. Account for both. shared_object = "glean_ffi.dll" else: raise ValueError("The platform {} is not supported.".format(sys.platform)) From bab6cd1cc3c62b023e56dbf53e06aea991ebd158 Mon Sep 17 00:00:00 2001 From: Piotr Zalewa Date: Thu, 14 May 2020 12:58:24 +0200 Subject: [PATCH 11/63] Add keyword-only arguments: 'application_id' and 'application_version' to `reset_glean` method. --- docs/user/testing-metrics.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/user/testing-metrics.md b/docs/user/testing-metrics.md index b81dd2f0e2..a9dc3c8e4a 100644 --- a/docs/user/testing-metrics.md +++ b/docs/user/testing-metrics.md @@ -117,7 +117,7 @@ from glean import testing @pytest.fixture(name="reset_glean", scope="function", autouse=True) def fixture_reset_glean(): - testing.reset_glean("my-app-id", "0.1.0") + testing.reset_glean(application_id="my-app-id", application_version="0.1.0") ``` To check if a value exists (i.e. it has been recorded), there is a `test_has_value()` function on each of the metric instances: From 1beedb78fef3589083f651793f37e4a2419f4fe9 Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Tue, 12 May 2020 16:25:22 +0200 Subject: [PATCH 12/63] Enable ping gzipping in glean-core This adds the 'flate2' dependency with a version that matches what's currently in mozilla-central. --- Cargo.lock | 37 +++++++++++++++++++++++++++++++ glean-core/Cargo.toml | 1 + glean-core/ffi/src/upload.rs | 17 ++++++++------ glean-core/src/upload/request.rs | 38 ++++++++++++++++++++++++++++---- 4 files changed, 82 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0a5a2538b8..ad60b576f0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1,5 +1,10 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. +[[package]] +name = "adler32" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" + [[package]] name = "android_log-sys" version = "0.1.2" @@ -127,6 +132,14 @@ dependencies = [ "unicode-width 0.1.7 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "crc32fast" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "cfg-if 0.1.10 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "criterion" version = "0.3.1" @@ -276,6 +289,17 @@ dependencies = [ "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "flate2" +version = "1.0.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "cfg-if 0.1.10 (registry+https://github.com/rust-lang/crates.io-index)", + "crc32fast 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)", + "libc 0.2.67 (registry+https://github.com/rust-lang/crates.io-index)", + "miniz_oxide 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "getrandom" version = "0.1.14" @@ -295,6 +319,7 @@ dependencies = [ "ctor 0.1.13 (registry+https://github.com/rust-lang/crates.io-index)", "env_logger 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)", "ffi-support 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", + "flate2 1.0.14 (registry+https://github.com/rust-lang/crates.io-index)", "iso8601 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", "once_cell 1.3.1 (registry+https://github.com/rust-lang/crates.io-index)", @@ -483,6 +508,14 @@ dependencies = [ "rustc_version 0.2.3 (registry+https://github.com/rust-lang/crates.io-index)", ] +[[package]] +name = "miniz_oxide" +version = "0.3.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "adler32 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)", +] + [[package]] name = "nom" version = "5.1.1" @@ -986,6 +1019,7 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" [metadata] +"checksum adler32 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)" = "5d2e7343e7fc9de883d1b0341e0b13970f764c14101234857d2ddafa1cb1cac2" "checksum android_log-sys 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)" = "b8052e2d8aabbb8d556d6abbcce2a22b9590996c5f849b9c7ce4544a2e3b984e" "checksum android_logger 0.8.6 (registry+https://github.com/rust-lang/crates.io-index)" = "8cbd542dd180566fad88fd2729a53a62a734843c626638006a9d63ec0688484e" "checksum arrayref 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)" = "a4c527152e37cf757a3f78aae5a06fbeefdb07ccc535c980a3208ee3060dd544" @@ -1002,6 +1036,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" "checksum cfg-if 0.1.10 (registry+https://github.com/rust-lang/crates.io-index)" = "4785bdd1c96b2a846b2bd7cc02e86b6b3dbf14e7e53446c4f54c92a361040822" "checksum chrono 0.4.10 (registry+https://github.com/rust-lang/crates.io-index)" = "31850b4a4d6bae316f7a09e691c944c28299298837edc0a03f755618c23cbc01" "checksum clap 2.33.0 (registry+https://github.com/rust-lang/crates.io-index)" = "5067f5bb2d80ef5d68b4c87db81601f0b75bca627bc2ef76b141d7b846a3c6d9" +"checksum crc32fast 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "ba125de2af0df55319f41944744ad91c71113bf74a4646efff39afe1f6842db1" "checksum criterion 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)" = "1fc755679c12bda8e5523a71e4d654b6bf2e14bd838dfc48cde6559a05caf7d1" "checksum criterion-plot 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)" = "a01e15e0ea58e8234f96146b1f91fa9d0e4dd7a38da93ff7a75d42c0b9d3a545" "checksum crossbeam-deque 0.7.3 (registry+https://github.com/rust-lang/crates.io-index)" = "9f02af974daeee82218205558e51ec8768b48cf524bd01d550abe5573a608285" @@ -1016,6 +1051,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" "checksum failure 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)" = "f8273f13c977665c5db7eb2b99ae520952fe5ac831ae4cd09d80c4c7042b5ed9" "checksum failure_derive 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)" = "0bc225b78e0391e4b8683440bf2e63c2deeeb2ce5189eab46e2b68c6d3725d08" "checksum ffi-support 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)" = "087be066eb6e85d7150f0c5400018a32802f99d688b2d3868c526f7bbfe17960" +"checksum flate2 1.0.14 (registry+https://github.com/rust-lang/crates.io-index)" = "2cfff41391129e0a856d6d822600b8d71179d46879e310417eb9c762eb178b42" "checksum getrandom 0.1.14 (registry+https://github.com/rust-lang/crates.io-index)" = "7abc8dd8451921606d809ba32e95b6111925cd2906060d2dcc29c070220503eb" "checksum hermit-abi 0.1.8 (registry+https://github.com/rust-lang/crates.io-index)" = "1010591b26bbfe835e9faeabeb11866061cc7dcebffd56ad7d0942d0e61aefd8" "checksum humantime 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)" = "df004cfca50ef23c36850aaaa59ad52cc70d0e90243c3c7737a4dd32dc7a3c4f" @@ -1036,6 +1072,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" "checksum maybe-uninit 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "60302e4db3a61da70c0cb7991976248362f30319e88850c487b9b95bbf059e00" "checksum memchr 2.3.3 (registry+https://github.com/rust-lang/crates.io-index)" = "3728d817d99e5ac407411fa471ff9800a778d88a24685968b36824eaf4bee400" "checksum memoffset 0.5.3 (registry+https://github.com/rust-lang/crates.io-index)" = "75189eb85871ea5c2e2c15abbdd541185f63b408415e5051f5cac122d8c774b9" +"checksum miniz_oxide 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)" = "aa679ff6578b1cddee93d7e82e263b94a575e0bfced07284eb0c037c1d2416a5" "checksum nom 5.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "0b471253da97532da4b61552249c521e01e736071f71c1a4f7ebbfbf0a06aad6" "checksum num-integer 0.1.42 (registry+https://github.com/rust-lang/crates.io-index)" = "3f6ea62e9d81a77cd3ee9a2a5b9b609447857f3d358704331e4ef39eb247fcba" "checksum num-traits 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)" = "c62be47e61d1842b9170f0fdeec8eba98e60e90e5446449a0545e5152acd7096" diff --git a/glean-core/Cargo.toml b/glean-core/Cargo.toml index 2e327a4eab..6aeb73808d 100644 --- a/glean-core/Cargo.toml +++ b/glean-core/Cargo.toml @@ -32,6 +32,7 @@ ffi-support = "0.4.0" regex = { version = "1.3.3", default-features = false, features = ["std"] } chrono = { version = "0.4.10", features = ["serde"] } once_cell = "1.2.0" +flate2 = {version = "1.0.11", default-features = false, features = ["miniz_oxide"] } [dev-dependencies] env_logger = { version = "0.7.1", default-features = false, features = ["termcolor", "atty", "humantime"] } diff --git a/glean-core/ffi/src/upload.rs b/glean-core/ffi/src/upload.rs index 527079755c..2640fbbd08 100644 --- a/glean-core/ffi/src/upload.rs +++ b/glean-core/ffi/src/upload.rs @@ -10,7 +10,7 @@ use std::ffi::CString; use std::os::raw::c_char; -use ffi_support::IntoFfi; +use ffi_support::{ByteBuffer, IntoFfi}; use crate::glean_str_free; use glean_core::upload::PingUploadTask; @@ -101,7 +101,7 @@ pub enum FfiPingUploadTask { Upload { document_id: *mut c_char, path: *mut c_char, - body: *mut c_char, + body: ByteBuffer, headers: *mut c_char, }, Wait, @@ -114,17 +114,15 @@ impl From for FfiPingUploadTask { PingUploadTask::Upload(request) => { // Safe unwraps: // 1. CString::new(..) should not fail as we are the ones that created the strings being transformed; - // 2. serde_json::to_string(&request.body) should not fail as request.body is a JsonValue; - // 3. serde_json::to_string(&request.headers) should not fail as request.headers is a HashMap of Strings. + // 2. serde_json::to_string(&request.headers) should not fail as request.headers is a HashMap of Strings. let document_id = CString::new(request.document_id.to_owned()).unwrap(); let path = CString::new(request.path.to_owned()).unwrap(); - let body = CString::new(serde_json::to_string(&request.body).unwrap()).unwrap(); let headers = CString::new(serde_json::to_string(&request.headers).unwrap()).unwrap(); FfiPingUploadTask::Upload { document_id: document_id.into_raw(), path: path.into_raw(), - body: body.into_raw(), + body: ByteBuffer::from_vec(request.body), headers: headers.into_raw(), } } @@ -147,9 +145,14 @@ impl Drop for FfiPingUploadTask { unsafe { glean_str_free(*document_id); glean_str_free(*path); - glean_str_free(*body); glean_str_free(*headers); } + // Unfortunately, we cannot directly call `body.destroy();` as + // we're behind a mutable reference, so we have to manually take the + // ownership and drop. Moreover, `ByteBuffer::new_with_size(0)` + // does not allocate, so we are not leaking memory. + let body = std::mem::replace(body, ByteBuffer::new_with_size(0)); + body.destroy(); } } } diff --git a/glean-core/src/upload/request.rs b/glean-core/src/upload/request.rs index 998c82c150..e745e2cd17 100644 --- a/glean-core/src/upload/request.rs +++ b/glean-core/src/upload/request.rs @@ -7,7 +7,10 @@ use std::collections::HashMap; use chrono::prelude::{DateTime, Utc}; +use flate2::write::GzEncoder; +use flate2::Compression; use serde_json::{self, Value as JsonValue}; +use std::io::prelude::*; /// Represents a request to upload a ping. #[derive(PartialEq, Debug, Clone)] @@ -17,8 +20,10 @@ pub struct PingRequest { pub document_id: String, /// The path for the server to upload the ping to. pub path: String, - /// The body of the request. - pub body: JsonValue, + /// The body of the request, as a byte array. If gzip encoded, then + /// the `headers` list will contain a `Content-Encoding` header with + /// the value `gzip`. + pub body: Vec, /// A map with all the headers to be sent with the request. pub headers: HashMap<&'static str, String>, } @@ -29,11 +34,19 @@ impl PingRequest { /// Automatically creates the default request headers. /// Clients may add more headers such as `userAgent` to this list. pub fn new(document_id: &str, path: &str, body: JsonValue) -> Self { + // We want uploads to be gzip'd. Instead of doing this for each platform + // we have language bindings for, apply compression here. + let original_as_string = body.to_string(); + let gzipped_content = Self::gzip_content(path, original_as_string.as_bytes()); + let add_gzip_header = gzipped_content.is_some(); + let body = gzipped_content.unwrap_or_else(|| original_as_string.into_bytes()); + let body_len = body.len(); + Self { document_id: document_id.into(), path: path.into(), body, - headers: Self::create_request_headers(), + headers: Self::create_request_headers(add_gzip_header, body_len), } } @@ -47,8 +60,21 @@ impl PingRequest { .unwrap_or(false) } + /// Attempt to gzip the provided ping content. + fn gzip_content(path: &str, content: &[u8]) -> Option> { + let mut gzipper = GzEncoder::new(Vec::new(), Compression::default()); + + // Attempt to add the content to the gzipper. + if let Err(e) = gzipper.write_all(content) { + log::error!("Failed to write to the gzipper: {} - {:?}", path, e); + return None; + } + + gzipper.finish().ok() + } + /// Creates the default request headers. - fn create_request_headers() -> HashMap<&'static str, String> { + fn create_request_headers(is_gzipped: bool, body_len: usize) -> HashMap<&'static str, String> { let mut headers = HashMap::new(); let date: DateTime = Utc::now(); headers.insert("Date", date.to_string()); @@ -57,6 +83,10 @@ impl PingRequest { "Content-Type", "application/json; charset=utf-8".to_string(), ); + headers.insert("Content-Length", body_len.to_string()); + if is_gzipped { + headers.insert("Content-Encoding", "gzip".to_string()); + } headers.insert("X-Client-Version", crate::GLEAN_VERSION.to_string()); headers } From 427504e9555d8df4b07c6cde8492d33958b24402 Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Tue, 12 May 2020 16:26:59 +0200 Subject: [PATCH 13/63] BONUS: remove unused getCalendarInstance in BaseUploader --- .../main/java/mozilla/telemetry/glean/net/BaseUploader.kt | 7 ------- 1 file changed, 7 deletions(-) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/net/BaseUploader.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/net/BaseUploader.kt index 016ffa395b..69a146f481 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/net/BaseUploader.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/net/BaseUploader.kt @@ -9,7 +9,6 @@ import androidx.annotation.VisibleForTesting import mozilla.telemetry.glean.config.Configuration import org.json.JSONException import org.json.JSONObject -import java.util.Calendar /** * The logic for uploading pings: this leaves the actual upload implementation @@ -74,12 +73,6 @@ class BaseUploader(d: PingUploader) : PingUploader by d { } } - /** - * TEST-ONLY. Allows to set specific dates for testing. - */ - @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) - internal fun getCalendarInstance(): Calendar { return Calendar.getInstance() } - /** * This function triggers the actual upload: logs the ping and calls the implementation * specific upload function. From 135dd870dda4f733b8ab4160c6ae78f199e7dd34 Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Tue, 12 May 2020 18:05:49 +0200 Subject: [PATCH 14/63] BONUS: Add the DateUtilsTest.kt to the right package This removes a warning. --- .../test/java/mozilla/telemetry/glean/utils/DateUtilsTest.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/utils/DateUtilsTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/utils/DateUtilsTest.kt index 29da908067..4662fa2ece 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/utils/DateUtilsTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/utils/DateUtilsTest.kt @@ -3,10 +3,10 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +package mozilla.telemetry.glean.utils + import androidx.test.ext.junit.runners.AndroidJUnit4 import mozilla.telemetry.glean.private.TimeUnit -import mozilla.telemetry.glean.utils.getISOTimeString -import mozilla.telemetry.glean.utils.parseISOTimeString import org.junit.Test import org.junit.runner.RunWith import org.junit.Assert.assertEquals From 80adfc72deaf86688d468acadee7d349796d67a3 Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Tue, 12 May 2020 18:09:16 +0200 Subject: [PATCH 15/63] Add an helper function to decompress gzip payloads in Android --- .../telemetry/glean/utils/GzipUtils.kt | 19 +++++++++++++ .../telemetry/glean/utils/GzipUtilsTest.kt | 27 +++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 glean-core/android/src/main/java/mozilla/telemetry/glean/utils/GzipUtils.kt create mode 100644 glean-core/android/src/test/java/mozilla/telemetry/glean/utils/GzipUtilsTest.kt diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/utils/GzipUtils.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/utils/GzipUtils.kt new file mode 100644 index 0000000000..0efc8464c3 --- /dev/null +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/utils/GzipUtils.kt @@ -0,0 +1,19 @@ +/* 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.telemetry.glean.utils + +import java.io.BufferedReader +import java.io.ByteArrayInputStream +import java.util.zip.GZIPInputStream + +/** + * Decompress the GZIP returned by the glean-core layer. + * + * @param data the gzipped [ByteArray] to decompress + * @return a [String] containing the uncompressed data. + */ +fun decompressGZIP(data: ByteArray): String { + return GZIPInputStream(ByteArrayInputStream(data)).bufferedReader().use(BufferedReader::readText) +} diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/utils/GzipUtilsTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/utils/GzipUtilsTest.kt new file mode 100644 index 0000000000..46bfc87886 --- /dev/null +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/utils/GzipUtilsTest.kt @@ -0,0 +1,27 @@ +/* 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.telemetry.glean.utils + +import org.junit.Test +import org.junit.Assert.assertEquals +import java.io.ByteArrayOutputStream +import java.util.zip.GZIPOutputStream + +class GzipUtilsTest { + private val testPing: String = "{ 'ping': 'test' }" + + @Test + fun `gzip must be correctly decompressed`() { + // Compress the test ping. + val byteOutputStream = ByteArrayOutputStream() + GZIPOutputStream(byteOutputStream).bufferedWriter(Charsets.UTF_8).use { it.write(testPing) } + val compressedTestPing = byteOutputStream.toByteArray() + + // Decompress the result and check if it's valid. + val decompressedPing = decompressGZIP(compressedTestPing) + assertEquals(testPing, decompressedPing) + } +} From 47ef1c8c6c6e9fd85cd3cd215efc0785fb1f4e03 Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Tue, 12 May 2020 18:10:45 +0200 Subject: [PATCH 16/63] Add support for uploading gzip ping content on Android Note that this also supports handling non gzipped contents if the glean-core library transfers a non gzipped ping. --- .../mozilla/telemetry/glean/net/BaseUploader.kt | 6 ++++-- .../glean/net/HttpURLConnectionUploader.kt | 14 +++++++++----- .../mozilla/telemetry/glean/net/PingUploader.kt | 2 +- .../java/mozilla/telemetry/glean/net/Upload.kt | 13 +++++++++---- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/net/BaseUploader.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/net/BaseUploader.kt index 69a146f481..ca43aff84c 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/net/BaseUploader.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/net/BaseUploader.kt @@ -7,6 +7,7 @@ package mozilla.telemetry.glean.net import android.util.Log import androidx.annotation.VisibleForTesting import mozilla.telemetry.glean.config.Configuration +import mozilla.telemetry.glean.utils.decompressGZIP import org.json.JSONException import org.json.JSONObject @@ -84,9 +85,10 @@ class BaseUploader(d: PingUploader) : PingUploader by d { * * @return return the status code of the upload response or null in case unable to upload. */ - internal fun doUpload(path: String, data: String, headers: HeadersList, config: Configuration): UploadResult { + internal fun doUpload(path: String, data: ByteArray, headers: HeadersList, config: Configuration): UploadResult { + val isGzip = !headers.none { (it.first == "Content-Encoding") && (it.second == "gzip") } if (config.logPings) { - logPing(path, data) + logPing(path, if (isGzip) decompressGZIP(data) else data.toString(Charsets.UTF_8)) } return upload( diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/net/HttpURLConnectionUploader.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/net/HttpURLConnectionUploader.kt index 015904653b..d9a0d3b412 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/net/HttpURLConnectionUploader.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/net/HttpURLConnectionUploader.kt @@ -6,6 +6,7 @@ package mozilla.telemetry.glean.net import android.util.Log import androidx.annotation.VisibleForTesting +import java.io.ByteArrayOutputStream import java.io.IOException import java.net.CookieHandler import java.net.CookieManager @@ -37,12 +38,13 @@ class HttpURLConnectionUploader : PingUploader { * @param data the serialized text data to send * @param headers a [HeadersList] containing String to String [Pair] with * the first entry being the header name and the second its value. + * @param isGzipped whether or not the payload is gzipped * * @return return the status code of the upload response, * or null in case unable to upload. */ @Suppress("ReturnCount", "MagicNumber") - override fun upload(url: String, data: String, headers: HeadersList): UploadResult { + override fun upload(url: String, data: ByteArray, headers: HeadersList): UploadResult { var connection: HttpURLConnection? = null try { connection = openConnection(url) @@ -61,7 +63,7 @@ class HttpURLConnectionUploader : PingUploader { removeCookies(url) // Finally upload. - var statusCode = doUpload(connection, data) + val statusCode = doUpload(connection, data) return HttpResponse(statusCode) } catch (e: MalformedURLException) { // There's nothing we can do to recover from this here. So let's just log an error and @@ -104,9 +106,11 @@ class HttpURLConnectionUploader : PingUploader { } @Throws(IOException::class) - internal fun doUpload(connection: HttpURLConnection, data: String): Int { - connection.outputStream.bufferedWriter().use { - it.write(data) + internal fun doUpload(connection: HttpURLConnection, data: ByteArray): Int { + connection.outputStream.use { + val byteOutputStream = ByteArrayOutputStream() + byteOutputStream.write(data) + byteOutputStream.writeTo(it) it.flush() } diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/net/PingUploader.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/net/PingUploader.kt index 1ef54db183..f99fd8c6fd 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/net/PingUploader.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/net/PingUploader.kt @@ -73,5 +73,5 @@ interface PingUploader { * @return return the status code of the upload response, * or null in case upload could not be attempted at all. */ - fun upload(url: String, data: String, headers: HeadersList): UploadResult + fun upload(url: String, data: ByteArray, headers: HeadersList): UploadResult } diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/net/Upload.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/net/Upload.kt index 0f20eec750..93b9fa027c 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/net/Upload.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/net/Upload.kt @@ -24,20 +24,25 @@ enum class UploadTaskTag { Done } -@Structure.FieldOrder("tag", "documentId", "path", "body", "headers") +@Structure.FieldOrder("tag", "documentId", "path", "bodyLen", "body", "headers") internal class UploadBody( // NOTE: We need to provide defaults here, so that JNA can create this object. @JvmField val tag: Byte = UploadTaskTag.Done.ordinal.toByte(), @JvmField val documentId: Pointer? = null, @JvmField val path: Pointer? = null, - @JvmField val body: Pointer? = null, + // Note that the next two fields (`bodyLen` and `body`) are defined as a single + // structure, on the Rust side, called `ByteBuffer`. While the ideal would be to + // use something like `RustBuffer` (as provided by application-services), this + // does not work in the context of JNA unions out of the box. + @JvmField var bodyLen: Long = 0, + @JvmField var body: Pointer? = null, @JvmField val headers: Pointer? = null ) : Structure() { fun toPingRequest(): PingRequest { return PingRequest( this.documentId!!.getRustString(), this.path!!.getRustString(), - this.body!!.getRustString(), + this.body!!.getByteArray(0, this.bodyLen.toInt()), this.headers!!.getRustString() ) } @@ -73,7 +78,7 @@ internal open class FfiPingUploadTask( internal class PingRequest( private val documentId: String, val path: String, - val body: String, + val body: ByteArray, headers: String ) { val headers: HeadersList = headersFromJSONString(headers) From 6309f6af4efd2d42e9d0000ff3c2b1f70ce9c02c Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Tue, 12 May 2020 18:11:25 +0200 Subject: [PATCH 17/63] Fix the tests --- .../java/mozilla/telemetry/glean/GleanTest.kt | 10 ++++---- .../java/mozilla/telemetry/glean/TestUtil.kt | 18 ++++++++++++++ .../glean/debug/GleanDebugActivityTest.kt | 2 +- .../telemetry/glean/net/BaseUploaderTest.kt | 4 ++-- .../net/HttpURLConnectionUploaderTest.kt | 24 +++++++++++-------- .../telemetry/glean/pings/CustomPingTest.kt | 5 ++-- .../glean/private/EventMetricTypeTest.kt | 13 +++++----- .../telemetry/glean/private/PingTypeTest.kt | 11 +++++---- .../scheduler/MetricsPingSchedulerTest.kt | 22 ++++++++--------- .../gleancore/pings/SharedTestUtils.kt | 9 ++++++- 10 files changed, 75 insertions(+), 43 deletions(-) diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt index ee5008be48..93ab25f080 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt @@ -239,7 +239,7 @@ class GleanTest { for (i in 0..3) { val request = server.takeRequest(20L, TimeUnit.SECONDS) val docType = request.path.split("/")[3] - val json = JSONObject(request.body.readUtf8()) + val json = JSONObject(request.getPlainBody()) checkPingSchema(json) if (docType == "events") { assertEquals(1, json.getJSONArray("events").length()) @@ -294,7 +294,7 @@ class GleanTest { val docType = request.path.split("/")[3] assertEquals("The received ping must be a 'baseline' ping", "baseline", docType) - val baselineJson = JSONObject(request.body.readUtf8()) + val baselineJson = JSONObject(request.getPlainBody()) assertEquals("dirty_startup", baselineJson.getJSONObject("ping_info")["reason"]) checkPingSchema(baselineJson) @@ -517,7 +517,7 @@ class GleanTest { val docType = request.path.split("/")[3] assertEquals(pingName, docType) - val pingJson = JSONObject(request.body.readUtf8()) + val pingJson = JSONObject(request.getPlainBody()) checkPingSchema(pingJson) val pingMetricsObject = pingJson.getJSONObject("metrics") @@ -647,7 +647,7 @@ class GleanTest { triggerWorkManager(context) val request = server.takeRequest(20L, TimeUnit.SECONDS) - val jsonContent = JSONObject(request.body.readUtf8()) + val jsonContent = JSONObject(request.getPlainBody()) assertEquals( 110, jsonContent @@ -744,7 +744,7 @@ class GleanTest { val docType = request.path.split("/")[3] assertEquals("The received ping must be a 'baseline' ping", "baseline", docType) - val baselineJson = JSONObject(request.body.readUtf8()) + val baselineJson = JSONObject(request.getPlainBody()) assertEquals("dirty_startup", baselineJson.getJSONObject("ping_info")["reason"]) checkPingSchema(baselineJson) diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt index f0caa70394..65a7c9c85d 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt @@ -22,12 +22,14 @@ import org.mockito.Mockito import mozilla.telemetry.glean.config.Configuration import mozilla.telemetry.glean.scheduler.PingUploadWorker import mozilla.telemetry.glean.private.PingTypeBase +import mozilla.telemetry.glean.utils.decompressGZIP import okhttp3.mockwebserver.Dispatcher import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import okhttp3.mockwebserver.RecordedRequest import org.junit.Assert import org.robolectric.shadows.ShadowLog +import java.io.ByteArrayInputStream import java.util.UUID import java.util.concurrent.ExecutionException @@ -236,3 +238,19 @@ internal fun getMockWebServer(): MockWebServer { }) return server } + +/** + * Convenience method to get the body of a request as a String. + * The UTF8 representation of the request body will be returned. + * If the request body is gzipped, it will be decompressed first. + * + * @return a [String] containing the body of the request. + */ +fun RecordedRequest.getPlainBody(): String { + return if (this.getHeader("Content-Encoding") == "gzip") { + val bodyInBytes = ByteArrayInputStream(this.body.readByteArray()).readBytes() + decompressGZIP(bodyInBytes) + } else { + this.body.readUtf8() + } +} diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/debug/GleanDebugActivityTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/debug/GleanDebugActivityTest.kt index 9868948958..206019e52a 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/debug/GleanDebugActivityTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/debug/GleanDebugActivityTest.kt @@ -41,7 +41,7 @@ private class TestPingTagClient( private val responseUrl: String = Configuration.DEFAULT_TELEMETRY_ENDPOINT, private val debugHeaderValue: String? = null ) : PingUploader { - override fun upload(url: String, data: String, headers: HeadersList): UploadResult { + override fun upload(url: String, data: ByteArray, headers: HeadersList): UploadResult { assertTrue("URL must be redirected for tagged pings", url.startsWith(responseUrl)) assertEquals("Debug headers must match what the ping tag was set to", diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/net/BaseUploaderTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/net/BaseUploaderTest.kt index c0a0964d55..d7f6a57701 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/net/BaseUploaderTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/net/BaseUploaderTest.kt @@ -28,7 +28,7 @@ class BaseUploaderTest { * A stub uploader class that does not upload anything. */ private class TestUploader : PingUploader { - override fun upload(url: String, data: String, headers: HeadersList): UploadResult { + override fun upload(url: String, data: ByteArray, headers: HeadersList): UploadResult { return UnrecoverableFailure } } @@ -38,7 +38,7 @@ class BaseUploaderTest { val uploader = spy(BaseUploader(TestUploader())) val expectedUrl = testDefaultConfig.serverEndpoint + testPath - uploader.doUpload(testPath, testPing, testHeaders, testDefaultConfig) + uploader.doUpload(testPath, testPing.toByteArray(Charsets.UTF_8), testHeaders, testDefaultConfig) verify(uploader).upload(eq(expectedUrl), any(), any()) } diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/net/HttpURLConnectionUploaderTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/net/HttpURLConnectionUploaderTest.kt index 588867bc7d..dd8881d949 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/net/HttpURLConnectionUploaderTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/net/HttpURLConnectionUploaderTest.kt @@ -7,6 +7,7 @@ package mozilla.telemetry.glean.net import androidx.test.ext.junit.runners.AndroidJUnit4 import mozilla.components.support.test.argumentCaptor import mozilla.telemetry.glean.config.Configuration +import mozilla.telemetry.glean.getPlainBody import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import org.junit.Assert.assertEquals @@ -45,9 +46,9 @@ class HttpURLConnectionUploaderTest { val client = spy(HttpURLConnectionUploader()) doReturn(connection).`when`(client).openConnection(anyString()) - doReturn(200).`when`(client).doUpload(connection, testPing) + doReturn(200).`when`(client).doUpload(connection, testPing.toByteArray(Charsets.UTF_8)) - client.upload(testPath, testPing, emptyList()) + client.upload(testPath, testPing.toByteArray(Charsets.UTF_8), emptyList()) verify(connection).readTimeout = HttpURLConnectionUploader.DEFAULT_READ_TIMEOUT verify(connection).connectTimeout = HttpURLConnectionUploader.DEFAULT_CONNECTION_TIMEOUT @@ -59,14 +60,14 @@ class HttpURLConnectionUploaderTest { val uploader = spy(HttpURLConnectionUploader()) val connection = mock(HttpURLConnection::class.java) doReturn(connection).`when`(uploader).openConnection(anyString()) - doReturn(200).`when`(uploader).doUpload(connection, testPing) + doReturn(200).`when`(uploader).doUpload(connection, testPing.toByteArray(Charsets.UTF_8)) val expectedHeaders = mapOf( "Content-Type" to "application/json; charset=utf-8", "Test-header" to "SomeValue", "OtherHeader" to "Glean/Test 25.0.2" ) - uploader.upload(testPath, testPing, expectedHeaders.toList()) + uploader.upload(testPath, testPing.toByteArray(Charsets.UTF_8), expectedHeaders.toList()) val headerNameCaptor = argumentCaptor() val headerValueCaptor = argumentCaptor() @@ -100,7 +101,10 @@ class HttpURLConnectionUploaderTest { val client = spy(HttpURLConnectionUploader()) doReturn(connection).`when`(client).openConnection(anyString()) - assertEquals(client.upload(testPath, testPing, emptyList()), HttpResponse(200)) + assertEquals( + client.upload(testPath, testPing.toByteArray(Charsets.UTF_8), emptyList()), + HttpResponse(200) + ) verify(connection, times(1)).disconnect() } @@ -116,12 +120,12 @@ class HttpURLConnectionUploaderTest { val client = HttpURLConnectionUploader() val url = testConfig.serverEndpoint + testPath - assertNotNull(client.upload(url, testPing, emptyList())) + assertNotNull(client.upload(url, testPing.toByteArray(Charsets.UTF_8), emptyList())) val request = server.takeRequest() assertEquals(testPath, request.path) assertEquals("POST", request.method) - assertEquals(testPing, request.body.readUtf8()) + assertEquals(testPing, request.getPlainBody()) server.shutdown() } @@ -172,12 +176,12 @@ class HttpURLConnectionUploaderTest { // Trigger the connection. val url = testConfig.serverEndpoint + testPath val client = HttpURLConnectionUploader() - assertNotNull(client.upload(url, testPing, emptyList())) + assertNotNull(client.upload(url, testPing.toByteArray(Charsets.UTF_8), emptyList())) val request = server.takeRequest() assertEquals(testPath, request.path) assertEquals("POST", request.method) - assertEquals(testPing, request.body.readUtf8()) + assertEquals(testPing, request.getPlainBody()) assertTrue(request.headers.values("Cookie").isEmpty()) // Check that we still have a cookie. @@ -191,6 +195,6 @@ class HttpURLConnectionUploaderTest { fun `upload() discards pings on malformed URLs`() { val client = spy(HttpURLConnectionUploader()) doThrow(MalformedURLException()).`when`(client).openConnection(anyString()) - assertEquals(UnrecoverableFailure, client.upload("path", "ping", emptyList())) + assertEquals(UnrecoverableFailure, client.upload("path", "ping".toByteArray(Charsets.UTF_8), emptyList())) } } diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt index f70b584860..791b1f1839 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/pings/CustomPingTest.kt @@ -6,6 +6,7 @@ package mozilla.telemetry.glean.scheduler import androidx.test.ext.junit.runners.AndroidJUnit4 import mozilla.telemetry.glean.Glean +import mozilla.telemetry.glean.getPlainBody import mozilla.telemetry.glean.getContextWithMockedInfo import mozilla.telemetry.glean.getMockWebServer import mozilla.telemetry.glean.private.NoReasonCodes @@ -96,7 +97,7 @@ class CustomPingTest { // Not much data in these pings, // but order should be preserved, so we can check the sequence number. - var pingJson = JSONObject(request.body.readUtf8()) + var pingJson = JSONObject(request.getPlainBody()) var pingInfo = pingJson.getJSONObject("ping_info") assertEquals(0L, pingInfo.tryGetLong("seq")) @@ -105,7 +106,7 @@ class CustomPingTest { docType = request.path.split("/")[3] assertEquals("custom-ping", docType) - pingJson = JSONObject(request.body.readUtf8()) + pingJson = JSONObject(request.getPlainBody()) pingInfo = pingJson.getJSONObject("ping_info") assertEquals(1L, pingInfo.tryGetLong("seq")!!) } diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt index 46e1578d89..66e28993b8 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/EventMetricTypeTest.kt @@ -13,14 +13,15 @@ package mozilla.telemetry.glean.private import android.os.SystemClock import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 -import java.lang.NullPointerException -import java.util.concurrent.TimeUnit -import mozilla.telemetry.glean.Dispatchers import mozilla.telemetry.glean.Glean +import mozilla.telemetry.glean.getPlainBody import mozilla.telemetry.glean.checkPingSchema +import mozilla.telemetry.glean.Dispatchers import mozilla.telemetry.glean.getContextWithMockedInfo import mozilla.telemetry.glean.getMockWebServer import mozilla.telemetry.glean.resetGlean +import java.lang.NullPointerException +import java.util.concurrent.TimeUnit import mozilla.telemetry.glean.testing.ErrorType import mozilla.telemetry.glean.testing.GleanTestRule import mozilla.telemetry.glean.triggerWorkManager @@ -273,7 +274,7 @@ class EventMetricTypeTest { assert( request.path.startsWith("/submit/$applicationId/events/") ) - val pingJsonData = request.body.readUtf8() + val pingJsonData = request.getPlainBody() val pingJson = JSONObject(pingJsonData) checkPingSchema(pingJson) assertNotNull(pingJson.opt("events")) @@ -334,7 +335,7 @@ class EventMetricTypeTest { triggerWorkManager(context) var request = server.takeRequest(20L, TimeUnit.SECONDS) - var pingJsonData = request.body.readUtf8() + var pingJsonData = request.getPlainBody() var pingJson = JSONObject(pingJsonData) checkPingSchema(pingJson) assertNotNull(pingJson.opt("events")) @@ -359,7 +360,7 @@ class EventMetricTypeTest { triggerWorkManager(context) request = server.takeRequest(20L, TimeUnit.SECONDS) - pingJsonData = request.body.readUtf8() + pingJsonData = request.getPlainBody() pingJson = JSONObject(pingJsonData) checkPingSchema(pingJson) assertNotNull(pingJson.opt("events")) diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt index 34fc7f73fb..07441ea174 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/PingTypeTest.kt @@ -7,10 +7,11 @@ package mozilla.telemetry.glean.private import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import mozilla.telemetry.glean.Glean -import mozilla.telemetry.glean.getWorkerStatus import mozilla.telemetry.glean.checkPingSchema +import mozilla.telemetry.glean.getPlainBody import mozilla.telemetry.glean.getContextWithMockedInfo import mozilla.telemetry.glean.getMockWebServer +import mozilla.telemetry.glean.getWorkerStatus import mozilla.telemetry.glean.resetGlean import mozilla.telemetry.glean.scheduler.PingUploadWorker import mozilla.telemetry.glean.testing.GleanTestRule @@ -68,7 +69,7 @@ class PingTypeTest { val docType = request.path.split("/")[3] assertEquals("custom", docType) - val pingJson = JSONObject(request.body.readUtf8()) + val pingJson = JSONObject(request.getPlainBody()) assertNotNull(pingJson.getJSONObject("client_info")["client_id"]) checkPingSchema(pingJson) } @@ -109,7 +110,7 @@ class PingTypeTest { val docType = request.path.split("/")[3] assertEquals("custom_ping", docType) - val pingJson = JSONObject(request.body.readUtf8()) + val pingJson = JSONObject(request.getPlainBody()) assertNotNull(pingJson.getJSONObject("client_info")["client_id"]) checkPingSchema(pingJson) } @@ -150,7 +151,7 @@ class PingTypeTest { val docType = request.path.split("/")[3] assertEquals("custom-ping", docType) - val pingJson = JSONObject(request.body.readUtf8()) + val pingJson = JSONObject(request.getPlainBody()) assertNotNull(pingJson.getJSONObject("client_info")["client_id"]) checkPingSchema(pingJson) } @@ -191,7 +192,7 @@ class PingTypeTest { val docType = request.path.split("/")[3] assertEquals("custom", docType) - val pingJson = JSONObject(request.body.readUtf8()) + val pingJson = JSONObject(request.getPlainBody()) assertNull(pingJson.getJSONObject("client_info").opt("client_id")) checkPingSchema(pingJson) } diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/MetricsPingSchedulerTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/MetricsPingSchedulerTest.kt index 7f9b92bfb6..028aa75dbb 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/MetricsPingSchedulerTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/MetricsPingSchedulerTest.kt @@ -11,18 +11,19 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.work.testing.WorkManagerTestInitHelper import mozilla.components.support.test.any import mozilla.components.support.test.eq -import mozilla.telemetry.glean.Dispatchers -import mozilla.telemetry.glean.getContextWithMockedInfo import mozilla.telemetry.glean.Glean import mozilla.telemetry.glean.GleanMetrics.Pings +import mozilla.telemetry.glean.checkPingSchema import mozilla.telemetry.glean.private.Lifetime -import mozilla.telemetry.glean.resetGlean import mozilla.telemetry.glean.private.StringMetricType import mozilla.telemetry.glean.private.TimeUnit -import mozilla.telemetry.glean.checkPingSchema -import mozilla.telemetry.glean.triggerWorkManager import mozilla.telemetry.glean.config.Configuration +import mozilla.telemetry.glean.getContextWithMockedInfo import mozilla.telemetry.glean.getMockWebServer +import mozilla.telemetry.glean.getPlainBody +import mozilla.telemetry.glean.Dispatchers +import mozilla.telemetry.glean.resetGlean +import mozilla.telemetry.glean.triggerWorkManager import mozilla.telemetry.glean.utils.getISOTimeString import mozilla.telemetry.glean.utils.parseISOTimeString import org.json.JSONObject @@ -42,7 +43,6 @@ import org.mockito.Mockito.never import org.mockito.Mockito.spy import org.mockito.Mockito.times import org.mockito.Mockito.verify -import org.mockito.Mockito.`when` import java.util.Calendar import java.util.concurrent.TimeUnit as AndroidTimeUnit @@ -291,7 +291,7 @@ class MetricsPingSchedulerTest { val docType = request.path.split("/")[3] assertEquals("The received ping must be a 'metrics' ping", "metrics", docType) - val metricsJsonData = request.body.readUtf8() + val metricsJsonData = request.getPlainBody() val metricsJson = JSONObject(metricsJsonData) // Validate the received data. @@ -532,7 +532,7 @@ class MetricsPingSchedulerTest { val docType = request.path.split("/")[3] assertEquals("The received ping must be a 'metrics' ping", "metrics", docType) - val metricsJsonData = request.body.readUtf8() + val metricsJsonData = request.getPlainBody() val pingJson = JSONObject(metricsJsonData) assertEquals("The received ping must contain the old version", @@ -709,7 +709,7 @@ class MetricsPingSchedulerTest { val docType = request.path.split("/")[3] assertEquals("The received ping must be a 'metrics' ping", "metrics", docType) - val metricsJsonData = request.body.readUtf8() + val metricsJsonData = request.getPlainBody() assertFalse("The canary metric must not be present in this ping", metricsJsonData.contains("must-not-be-in-the-first-ping")) @@ -788,7 +788,7 @@ class MetricsPingSchedulerTest { val docType = request.path.split("/")[3] assertEquals("The received ping must be a 'metrics' ping", "metrics", docType) - val metricsJsonData = request.body.readUtf8() + val metricsJsonData = request.getPlainBody() assertTrue("The expected metric must be in this ping", metricsJsonData.contains(expectedString)) @@ -875,7 +875,7 @@ class MetricsPingSchedulerTest { // } // // Parse the received ping payload to a JSON object. - // val metricsJsonData = request.body.readUtf8() + // val metricsJsonData = request.getPlainBody() // val metricsJson = JSONObject(metricsJsonData) // // Validate the received data. diff --git a/samples/android/app/src/androidTest/java/org/mozilla/samples/gleancore/pings/SharedTestUtils.kt b/samples/android/app/src/androidTest/java/org/mozilla/samples/gleancore/pings/SharedTestUtils.kt index dd91fe2d15..7f42d36e47 100644 --- a/samples/android/app/src/androidTest/java/org/mozilla/samples/gleancore/pings/SharedTestUtils.kt +++ b/samples/android/app/src/androidTest/java/org/mozilla/samples/gleancore/pings/SharedTestUtils.kt @@ -4,11 +4,13 @@ package org.mozilla.samples.gleancore.pings +import mozilla.telemetry.glean.utils.decompressGZIP import okhttp3.mockwebserver.Dispatcher import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import okhttp3.mockwebserver.RecordedRequest import org.json.JSONObject +import java.io.ByteArrayInputStream import java.util.concurrent.TimeUnit /** @@ -44,7 +46,12 @@ fun waitForPingContent( val request = server.takeRequest(20L, TimeUnit.SECONDS) val docType = request.path.split("/")[3] if (pingName == docType) { - return JSONObject(request.body.readUtf8()) + return if (request.getHeader("Content-Encoding") == "gzip") { + val bodyInBytes = ByteArrayInputStream(request.body.readByteArray()).readBytes() + JSONObject(decompressGZIP(bodyInBytes)) + } else { + JSONObject(request.body.readUtf8()) + } } } while (attempts < maxAttempts) From 9ea04fb5af3fb0b2c3b2e0b38d8c5ad6c8cd1338 Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Wed, 13 May 2020 12:09:35 +0200 Subject: [PATCH 18/63] Enable the Zlib license Zlib is compatible with MPL, see https://docs.google.com/document/d/1Oguqp43W4_ChyroJ9AJAzG1jSwkUWfKvBKVvrDxVsMg/edit Zlib is required by flate2's dependency adler32. --- deny.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/deny.toml b/deny.toml index 53cbcf336c..1d93c43f39 100644 --- a/deny.toml +++ b/deny.toml @@ -6,4 +6,5 @@ allow = [ "Apache-2.0", "MIT", "BSD-2-Clause", + "Zlib", ] From 30cd7acf4720a98a31c38fe850f51f86d479a870 Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Wed, 13 May 2020 12:13:42 +0200 Subject: [PATCH 19/63] Update cbindgen --- glean-core/ffi/glean.h | 60 ++++++++++++++++++++++++++++++++- glean-core/ios/Glean/GleanFfi.h | 60 ++++++++++++++++++++++++++++++++- 2 files changed, 118 insertions(+), 2 deletions(-) diff --git a/glean-core/ffi/glean.h b/glean-core/ffi/glean.h index 142b288c9a..6e142ca926 100644 --- a/glean-core/ffi/glean.h +++ b/glean-core/ffi/glean.h @@ -86,6 +86,64 @@ typedef const int32_t *RawIntArray; typedef const char *const *RawStringArray; +/** + * ByteBuffer is a struct that represents an array of bytes to be sent over the FFI boundaries. + * There are several cases when you might want to use this, but the primary one for us + * is for returning protobuf-encoded data to Swift and Java. The type is currently rather + * limited (implementing almost no functionality), however in the future it may be + * more expanded. + * + * ## Caveats + * + * Note that the order of the fields is `len` (an i64) then `data` (a `*mut u8`), getting + * this wrong on the other side of the FFI will cause memory corruption and crashes. + * `i64` is used for the length instead of `u64` and `usize` because JNA has interop + * issues with both these types. + * + * ByteBuffer does not implement Drop. This is intentional. Memory passed into it will + * be leaked if it is not explicitly destroyed by calling [`ByteBuffer::destroy`]. This + * is because in the future, we may allow it's use for passing data into Rust code. + * ByteBuffer assuming ownership of the data would make this a problem. + * + * Note that alling `destroy` manually is not typically needed or recommended, + * and instead you should use [`define_bytebuffer_destructor!`]. + * + * ## Layout/fields + * + * This struct's field are not `pub` (mostly so that we can soundly implement `Send`, but also so + * that we can verify rust users are constructing them appropriately), the fields, their types, and + * their order are *very much* a part of the public API of this type. Consumers on the other side + * of the FFI will need to know its layout. + * + * If this were a C struct, it would look like + * + * ```c,no_run + * struct ByteBuffer { + * int64_t len; + * uint8_t *data; // note: nullable + * }; + * ``` + * + * In rust, there are two fields, in this order: `len: i64`, and `data: *mut u8`. + * + * ### Description of fields + * + * `data` is a pointer to an array of `len` bytes. Not that data can be a null pointer and therefore + * should be checked. + * + * The bytes array is allocated on the heap and must be freed on it as well. Critically, if there + * are multiple rust packages using being used in the same application, it *must be freed on the + * same heap that allocated it*, or you will corrupt both heaps. + * + * Typically, this object is managed on the other side of the FFI (on the "FFI consumer"), which + * means you must expose a function to release the resources of `data` which can be done easily + * using the [`define_bytebuffer_destructor!`] macro provided by this crate. + */ +typedef struct { + int64_t len; + uint8_t *data; +} ByteBuffer; + /** * A FFI-compatible representation for the PingUploadTask. * @@ -143,7 +201,7 @@ typedef struct { FfiPingUploadTask_Tag tag; char *document_id; char *path; - char *body; + ByteBuffer body; char *headers; } FfiPingUploadTask_Upload_Body; diff --git a/glean-core/ios/Glean/GleanFfi.h b/glean-core/ios/Glean/GleanFfi.h index 142b288c9a..6e142ca926 100644 --- a/glean-core/ios/Glean/GleanFfi.h +++ b/glean-core/ios/Glean/GleanFfi.h @@ -86,6 +86,64 @@ typedef const int32_t *RawIntArray; typedef const char *const *RawStringArray; +/** + * ByteBuffer is a struct that represents an array of bytes to be sent over the FFI boundaries. + * There are several cases when you might want to use this, but the primary one for us + * is for returning protobuf-encoded data to Swift and Java. The type is currently rather + * limited (implementing almost no functionality), however in the future it may be + * more expanded. + * + * ## Caveats + * + * Note that the order of the fields is `len` (an i64) then `data` (a `*mut u8`), getting + * this wrong on the other side of the FFI will cause memory corruption and crashes. + * `i64` is used for the length instead of `u64` and `usize` because JNA has interop + * issues with both these types. + * + * ByteBuffer does not implement Drop. This is intentional. Memory passed into it will + * be leaked if it is not explicitly destroyed by calling [`ByteBuffer::destroy`]. This + * is because in the future, we may allow it's use for passing data into Rust code. + * ByteBuffer assuming ownership of the data would make this a problem. + * + * Note that alling `destroy` manually is not typically needed or recommended, + * and instead you should use [`define_bytebuffer_destructor!`]. + * + * ## Layout/fields + * + * This struct's field are not `pub` (mostly so that we can soundly implement `Send`, but also so + * that we can verify rust users are constructing them appropriately), the fields, their types, and + * their order are *very much* a part of the public API of this type. Consumers on the other side + * of the FFI will need to know its layout. + * + * If this were a C struct, it would look like + * + * ```c,no_run + * struct ByteBuffer { + * int64_t len; + * uint8_t *data; // note: nullable + * }; + * ``` + * + * In rust, there are two fields, in this order: `len: i64`, and `data: *mut u8`. + * + * ### Description of fields + * + * `data` is a pointer to an array of `len` bytes. Not that data can be a null pointer and therefore + * should be checked. + * + * The bytes array is allocated on the heap and must be freed on it as well. Critically, if there + * are multiple rust packages using being used in the same application, it *must be freed on the + * same heap that allocated it*, or you will corrupt both heaps. + * + * Typically, this object is managed on the other side of the FFI (on the "FFI consumer"), which + * means you must expose a function to release the resources of `data` which can be done easily + * using the [`define_bytebuffer_destructor!`] macro provided by this crate. + */ +typedef struct { + int64_t len; + uint8_t *data; +} ByteBuffer; + /** * A FFI-compatible representation for the PingUploadTask. * @@ -143,7 +201,7 @@ typedef struct { FfiPingUploadTask_Tag tag; char *document_id; char *path; - char *body; + ByteBuffer body; char *headers; } FfiPingUploadTask_Upload_Body; From f236504f76460d740ec6fa4cdd974998eb1f7cb9 Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Wed, 13 May 2020 17:27:03 +0200 Subject: [PATCH 20/63] Add the changelog entry --- .dictionary | 1 + CHANGELOG.md | 3 +++ 2 files changed, 4 insertions(+) diff --git a/.dictionary b/.dictionary index f675227462..a34bfa61d3 100644 --- a/.dictionary +++ b/.dictionary @@ -105,6 +105,7 @@ gfritzsche glinter gradle grcov +gzip hotfix html init diff --git a/CHANGELOG.md b/CHANGELOG.md index 898dfc13f4..0428de939c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ [Full changelog](https://github.com/mozilla/glean/compare/v30.0.0...master) +* Android + * Ping payloads are now compressed using gzip. + # v30.0.0 (2020-05-13) [Full changelog](https://github.com/mozilla/glean/compare/v29.1.0...v30.0.0) From e36e4daf145ec635ea699b0dadff595da7e44e12 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 15 May 2020 11:48:42 +0200 Subject: [PATCH 21/63] iOS (cleanup): Don't copy frameworks to output. We're not an application. See Step 7 in https://github.com/Carthage/Carthage#quick-start --- .../ios/Glean.xcodeproj/project.pbxproj | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/glean-core/ios/Glean.xcodeproj/project.pbxproj b/glean-core/ios/Glean.xcodeproj/project.pbxproj index cba332511e..c287633b83 100644 --- a/glean-core/ios/Glean.xcodeproj/project.pbxproj +++ b/glean-core/ios/Glean.xcodeproj/project.pbxproj @@ -410,7 +410,6 @@ buildPhases = ( BF6F2DA5224BF2E000394062 /* Run Script */, BFB59A9723429FC000F40CA8 /* Run Glean SDK generator */, - AC0C2B5E2328090D00A50CC8 /* Copy Carthage Dependencies */, BF3DE38C2243A2F20018E23F /* Headers */, BF3DE38D2243A2F20018E23F /* Sources */, BF3DE38E2243A2F20018E23F /* Frameworks */, @@ -520,24 +519,6 @@ shellPath = /bin/sh; shellScript = "/usr/local/bin/carthage copy-frameworks\n"; }; - AC0C2B5E2328090D00A50CC8 /* Copy Carthage Dependencies */ = { - isa = PBXShellScriptBuildPhase; - buildActionMask = 2147483647; - files = ( - ); - inputFileListPaths = ( - ); - inputPaths = ( - ); - name = "Copy Carthage Dependencies"; - outputFileListPaths = ( - ); - outputPaths = ( - ); - runOnlyForDeploymentPostprocessing = 0; - shellPath = /bin/sh; - shellScript = "/usr/local/bin/carthage copy-frameworks\n"; - }; BF6F2DA5224BF2E000394062 /* Run Script */ = { isa = PBXShellScriptBuildPhase; buildActionMask = 2147483647; From 9d6d2bef1df1cc4fe5ce52b02ec87b3b4f72506f Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 15 May 2020 12:45:38 +0200 Subject: [PATCH 22/63] iOS (cleanup): Define missing output file --- glean-core/ios/Glean.xcodeproj/project.pbxproj | 1 + 1 file changed, 1 insertion(+) diff --git a/glean-core/ios/Glean.xcodeproj/project.pbxproj b/glean-core/ios/Glean.xcodeproj/project.pbxproj index c287633b83..b2a23d142c 100644 --- a/glean-core/ios/Glean.xcodeproj/project.pbxproj +++ b/glean-core/ios/Glean.xcodeproj/project.pbxproj @@ -514,6 +514,7 @@ outputFileListPaths = ( ); outputPaths = ( + "$(BUILT_PRODUCTS_DIR)/$(FRAMEWORKS_FOLDER_PATH)/OHHTTPStubs.framework", ); runOnlyForDeploymentPostprocessing = 0; shellPath = /bin/sh; From 8edd3fbc28cb3be50bcd2ce94523e85e60ff9df5 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Fri, 15 May 2020 11:42:21 +0200 Subject: [PATCH 23/63] iOS (cleanup): Add UI tests to main testing scheme of sample app For some reason this was missing. With this added one can run the UI tests from the Xcode GUI --- bin/run-ios-sample-app-test.sh | 2 +- .../xcshareddata/xcschemes/glean-sample-app.xcscheme | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/bin/run-ios-sample-app-test.sh b/bin/run-ios-sample-app-test.sh index ca331d219d..c419f510c3 100755 --- a/bin/run-ios-sample-app-test.sh +++ b/bin/run-ios-sample-app-test.sh @@ -9,7 +9,7 @@ set -euvx set -o pipefail && \ xcodebuild \ -workspace ./samples/ios/app/glean-sample-app.xcodeproj/project.xcworkspace \ - -scheme glean-sample-appUITests \ + -scheme glean-sample-app \ -sdk iphonesimulator \ -destination 'platform=iOS Simulator,name=iPhone 11' \ test | \ diff --git a/samples/ios/app/glean-sample-app.xcodeproj/xcshareddata/xcschemes/glean-sample-app.xcscheme b/samples/ios/app/glean-sample-app.xcodeproj/xcshareddata/xcschemes/glean-sample-app.xcscheme index 09f49931dc..83c2889c67 100644 --- a/samples/ios/app/glean-sample-app.xcodeproj/xcshareddata/xcschemes/glean-sample-app.xcscheme +++ b/samples/ios/app/glean-sample-app.xcodeproj/xcshareddata/xcschemes/glean-sample-app.xcscheme @@ -28,6 +28,16 @@ selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB" shouldUseLaunchSchemeArgsEnv = "YES"> + + + + Date: Thu, 14 May 2020 12:02:36 +0200 Subject: [PATCH 24/63] iOS: Use a bytebuffer instead of a string for the ping payload --- .../ios/Glean/Net/HttpPingUploader.swift | 24 +++++++++++++++---- glean-core/ios/Glean/Net/Upload.swift | 11 +++++---- .../Net/HttpPingUploaderTests.swift | 4 ++-- 3 files changed, 28 insertions(+), 11 deletions(-) diff --git a/glean-core/ios/Glean/Net/HttpPingUploader.swift b/glean-core/ios/Glean/Net/HttpPingUploader.swift index bffaf32db0..bc680109d4 100644 --- a/glean-core/ios/Glean/Net/HttpPingUploader.swift +++ b/glean-core/ios/Glean/Net/HttpPingUploader.swift @@ -52,15 +52,16 @@ public class HttpPingUploader { /// Note that the `X-Client-Type`: `Glean` and `X-Client-Version`: /// headers are added to the HTTP request in addition to the UserAgent. This allows /// us to easily handle pings coming from Glean on the legacy Mozilla pipeline. - func upload(path: String, data: String, headers: [String: String], callback: @escaping (UploadResult) -> Void) { + func upload(path: String, data: Data, headers: [String: String], callback: @escaping (UploadResult) -> Void) { if config.logPings { - logPing(path: path, data: data) + // FIXME: ungzip data if it is gzipped. + ///logPing(path: path, data: data) } // Build the request and create an async upload operation and launch it through the // Dispatchers if let request = buildRequest(path: path, data: data, headers: headers) { - let uploadOperation = PingUploadOperation(request: request, data: Data(data.utf8), callback: callback) + let uploadOperation = PingUploadOperation(request: request, data: data, callback: callback) Dispatchers.shared.launchConcurrent(operation: uploadOperation) } } @@ -73,7 +74,7 @@ public class HttpPingUploader { /// * callback: A callback to return the success/failure of the upload /// /// - returns: Optional `URLRequest` object with the configured headings set. - func buildRequest(path: String, data: String, headers: [String: String]) -> URLRequest? { + func buildRequest(path: String, data: Data, headers: [String: String]) -> URLRequest? { if let url = URL(string: config.serverEndpoint + path) { var request = URLRequest(url: url) for (field, value) in headers { @@ -81,9 +82,22 @@ public class HttpPingUploader { } request.timeoutInterval = TimeInterval(Constants.connectionTimeout) request.httpMethod = "POST" - request.httpBody = Data(data.utf8) request.httpShouldHandleCookies = false + // NOTE: We're using `URLSession.uploadTask` in `PingUploadOperation`, + // which ignores the `httpBody` and instead takes the body payload as a parameter + // to add to the request. + // However in tests we're using OHHTTPStubs to stub out the HTTP upload. + // It has the known limitation that it doesn't simulate data upload, + // because the underlying protocol doesn't expose a hook for that. + // By setting `httpBody` here the data is still attached to the request, + // so OHHTTPStubs sees it. + // It shouldn't be too bad memory-wise and not duplicate the data in memory. + // This should only be a reference and Swift keeps track of all the places it's needed. + // + // See https://github.com/AliSoftware/OHHTTPStubs#known-limitations. + request.httpBody = data + if let tag = config.pingTag { request.addValue(tag, forHTTPHeaderField: "X-Debug-ID") } diff --git a/glean-core/ios/Glean/Net/Upload.swift b/glean-core/ios/Glean/Net/Upload.swift index db164009bd..3fd06393de 100644 --- a/glean-core/ios/Glean/Net/Upload.swift +++ b/glean-core/ios/Glean/Net/Upload.swift @@ -47,12 +47,15 @@ struct PingRequest { let documentId: String /// The path to upload this ping to. let path: String - /// The JSON-encoded payload of the ping. - let body: String + /// The body of the request. + /// + /// If gzip encoded, then the `headers` list will + /// contain a `Content-Encoding` header with the value `gzip`. + let body: Data /// A map of headers for the HTTP request to send. let headers: [String: String] - init(documentId: String, path: String, body: String, headers: [String: String]) { + init(documentId: String, path: String, body: Data, headers: [String: String]) { self.documentId = documentId self.path = path self.body = body @@ -117,7 +120,7 @@ extension FfiPingUploadTask_Upload_Body { func toPingRequest() -> PingRequest { let documentId = String(cString: self.document_id) let path = String(cString: self.path) - let body = String(cString: self.body) + let body = Data(bytes: self.body.data, count: Int(self.body.len)) // Decode the header object from JSON let json = String(cString: self.headers) diff --git a/glean-core/ios/GleanTests/Net/HttpPingUploaderTests.swift b/glean-core/ios/GleanTests/Net/HttpPingUploaderTests.swift index 3c7013a60f..3a5884659b 100644 --- a/glean-core/ios/GleanTests/Net/HttpPingUploaderTests.swift +++ b/glean-core/ios/GleanTests/Net/HttpPingUploaderTests.swift @@ -88,7 +88,7 @@ class HttpPingUploaderTests: XCTestCase { expectation = expectation(description: "Completed upload") let httpPingUploader = HttpPingUploader(configuration: testConfig) - httpPingUploader.upload(path: testPath, data: testPing, headers: [:]) { result in + httpPingUploader.upload(path: testPath, data: Data(testPing.utf8), headers: [:]) { result in testValue = result self.expectation?.fulfill() } @@ -109,7 +109,7 @@ class HttpPingUploaderTests: XCTestCase { // We specify a single additional header here. // In usual code they are generated on the Rust side. let request = HttpPingUploader(configuration: testConfig) - .buildRequest(path: testPath, data: testPing, headers: ["X-Client-Type": "Glean"]) + .buildRequest(path: testPath, data: Data(testPing.utf8), headers: ["X-Client-Type": "Glean"]) XCTAssertEqual( request?.url?.path, From d9eec3bbffa385d70a73c56b40fc23abc7d8b232 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 18 May 2020 13:07:39 +0200 Subject: [PATCH 25/63] Streamline Android dev setup documentation --- .../setup-android-build-environment.md | 96 +++++++++++-------- 1 file changed, 55 insertions(+), 41 deletions(-) diff --git a/docs/dev/android/setup-android-build-environment.md b/docs/dev/android/setup-android-build-environment.md index 5cf91836b1..0e0d3fc4a5 100644 --- a/docs/dev/android/setup-android-build-environment.md +++ b/docs/dev/android/setup-android-build-environment.md @@ -1,28 +1,23 @@ # Setup the Android Build Environment -## Doing a local build of the Android Components: +## Doing a local build of the Glean SDK: -This document describes how to make local builds of the Android bindings in this -repository. Most consumers of these bindings *do not* need to follow this -process, but will instead [use pre-built -bindings](../../user/adding-glean-to-your-project.html). +This document describes how to make local builds of the Android bindings in this repository. +Most consumers of these bindings *do not* need to follow this process, +but will instead [use pre-built bindings](../../user/adding-glean-to-your-project.html). ## Prepare your build environment Typically, this process only needs to be run once, although periodically you -may need to repeat some steps (e.g., rust updates should be done periodically) +may need to repeat some steps (e.g., Rust updates should be done periodically). ### Setting up Android dependencies -At the end of this process you should have the following environment variables set up. - -- `ANDROID_HOME` -- `ANDROID_NDK_ROOT` -- `JAVA_HOME` +#### With Android Studio The easiest way to install all the dependencies (and automatically handle updates), is by using [Android Studio](https://developer.android.com/studio/index.html). -Once this is installed, it must be run and the Glean project opened to complete initial setup. +Once this is installed, start Android Studio and open the Glean project. If Android Studio asks you to upgrade the version of Gradle, decline. The following dependencies can be installed in Android Studio through `Tools > SDK Manager > SDK Tools`: @@ -32,58 +27,77 @@ The following dependencies can be installed in Android Studio through `Tools > S - CMake - LLDB -With the dependencies installed, note down the `Android SDK Location` in `Tools > SDK Manager`. +You should be set to build Glean from within Android Studio now. + +#### Manually + +Set `JAVA_HOME` to be the location of Android Studio's JDK which can be found in Android Studio's "Project Structure" menu (you may need to escape spaces in the path). + +Note down the location of your SDK. +If installed through Android Studio you will find the `Android SDK Location` in `Tools > SDK Manager`. + Set the `ANDROID_HOME` environment variable to that path. -The `ANDROID_NDK_ROOT` can be set to `ANDROID_NDK_ROOT=$ANDROID_HOME/ndk-bundle`. -Set `JAVA_HOME` to be the location of Android Studio's JDK which can be found in Glean's "Project Structure" menu. (You may need to escape spaces in the path). +Alternatively add the following line to the `local.properties` file in the root of the Glean checkout (create the file if it does not exist): + +``` +sdk.dir=/path/to/sdk +``` -If you want to install the NDK manually: +For the Android NDK: 1. Download NDK r20 from . 2. Extract it and put it somewhere (`$HOME/.android-ndk-r20` is a reasonable choice, but it doesn't matter). -3. Set `ANDROID_NDK_ROOT` to this path. - * Set `ANDROID_NDK_HOME` to match `ANDROID_NDK_ROOT`, for compatibility with some android Gradle plugins. +3. Add the following line to the `local.properties` file in the root of the Glean checkout (create the file if it does not exist): + ``` + ndk.dir=/path/to/.android-ndk-r20 + ``` ### Setting up Rust Rust can be installed using `rustup`, with the following commands: -- `curl https://sh.rustup.rs -sSf | sh` -- `rustup update` +``` +curl https://sh.rustup.rs -sSf | sh +rustup update +``` -Platform specific toolchains need to be installed for Rust. This can be -done using the `rustup target` command. In order to enable building to real -devices and Android emulators, the following targets need to be installed: +Platform specific toolchains need to be installed for Rust. +This can be done using the `rustup target` command. +In order to enable building for real devices and Android emulators, +the following targets need to be installed: -- `rustup target add aarch64-linux-android` -- `rustup target add armv7-linux-androideabi` -- `rustup target add i686-linux-android` -- `rustup target add x86_64-linux-android` +``` +rustup target add aarch64-linux-android +rustup target add armv7-linux-androideabi +rustup target add i686-linux-android +rustup target add x86_64-linux-android +``` ## Building -This should be relatively straightforward and painless: +Before building: -1. Ensure your repository is up-to-date. +* Ensure your repository is up-to-date. +* Ensure Rust is up-to-date by running `rustup update`. -2. Ensure Rust is up-to-date by running `rustup update`. +### With Android Studio -3. The builds are all performed by `./gradlew` and the general syntax used is - `./gradlew project:task` +After importing the Glean project into Android Studio it should be all set up and you can build the project using `Build > Make Project` - You can see a list of projects by executing `./gradlew projects` and a list - of tasks by executing `./gradlew tasks`. +### Manually -The above can be skipped if using `Android Studio`. The project directory can be imported -and all the build details can be left to the IDE. +The builds are all performed by `./gradlew` and the general syntax used is `./gradlew project:task` + +Build the whole project and run tests: + +``` +./gradlew build +``` + +You can see a list of projects by executing `./gradlew projects` and a list of tasks by executing `./gradlew tasks`. ## FAQ - **Q: Android Studio complains about Python not being found when building.** - A: Make sure that the `python` binary is on your `PATH`. On Windows, in addition to that, it might be required to add its full path to the `rust.pythonCommand` entry in `$project_root/local.properties`. - -- **Q: Android Studio complains about `Toolchain for X does not exist`.** -- A: On Windows, make sure that the `ndk.dir` property in `$project_root/local.properties` points to an existing -directory containing the Android NDK. - From 7d61e7e25702b8cd295a2b153ce7d30d3bae34e8 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 18 May 2020 13:13:01 +0200 Subject: [PATCH 26/63] Upgrade to NDK r21 This is the only officially supported version (at least for macOS, due to signing). There's no harm in updating, it is compatible will older SDKs. --- docs/dev/android/setup-android-build-environment.md | 8 ++++---- docs/dev/core/internal/sdk-ndk-versions.md | 6 +++--- taskcluster/docker/linux/Dockerfile | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/dev/android/setup-android-build-environment.md b/docs/dev/android/setup-android-build-environment.md index 0e0d3fc4a5..16db7818a7 100644 --- a/docs/dev/android/setup-android-build-environment.md +++ b/docs/dev/android/setup-android-build-environment.md @@ -23,7 +23,7 @@ If Android Studio asks you to upgrade the version of Gradle, decline. The following dependencies can be installed in Android Studio through `Tools > SDK Manager > SDK Tools`: - Android SDK Tools (may already be selected) -- NDK r20 +- NDK r21 - CMake - LLDB @@ -45,11 +45,11 @@ sdk.dir=/path/to/sdk For the Android NDK: -1. Download NDK r20 from . -2. Extract it and put it somewhere (`$HOME/.android-ndk-r20` is a reasonable choice, but it doesn't matter). +1. Download NDK r21 from . +2. Extract it and put it somewhere (`$HOME/.android-ndk-r21` is a reasonable choice, but it doesn't matter). 3. Add the following line to the `local.properties` file in the root of the Glean checkout (create the file if it does not exist): ``` - ndk.dir=/path/to/.android-ndk-r20 + ndk.dir=/path/to/.android-ndk-r21 ``` ### Setting up Rust diff --git a/docs/dev/core/internal/sdk-ndk-versions.md b/docs/dev/core/internal/sdk-ndk-versions.md index 09e267e5e2..559d3e7a80 100644 --- a/docs/dev/core/internal/sdk-ndk-versions.md +++ b/docs/dev/core/internal/sdk-ndk-versions.md @@ -7,8 +7,8 @@ The Glean SDK implementation is currently build against the following versions: * or install with: `sdkmanager --verbose "platforms;android-28"` * Android build tools 28.0.3 * Download link: -* NDK r20 - * Download link: +* NDK r21 + * Download link: For the full setup see [Setup the Android Build Environment](../../android/setup-android-build-environment.html). @@ -26,4 +26,4 @@ All locations need to be updated on upgrades: * `ENV ANDROID_BUILD_TOOLS "28.0.3"` * `ENV ANDROID_SDK_VERSION "3859397"` * `ENV ANDROID_PLATFORM_VERSION "28"` - * `ENV ANDROID_NDK_VERSION "r20"` + * `ENV ANDROID_NDK_VERSION "r21"` diff --git a/taskcluster/docker/linux/Dockerfile b/taskcluster/docker/linux/Dockerfile index cf967e44f6..11f60c97fc 100644 --- a/taskcluster/docker/linux/Dockerfile +++ b/taskcluster/docker/linux/Dockerfile @@ -122,7 +122,7 @@ RUN chown -R worker:worker /builds/worker/android-sdk # Android NDK -ENV ANDROID_NDK_VERSION "r20" +ENV ANDROID_NDK_VERSION "r21" # $ANDROID_NDK_ROOT is the preferred name, but the android gradle plugin uses $ANDROID_NDK_HOME. ENV ANDROID_NDK_ROOT /builds/worker/android-ndk @@ -133,7 +133,7 @@ RUN curl -sfSL --retry 5 --retry-delay 10 https://dl.google.com/android/reposito && rm ndk.zip \ && mv /builds/worker/android-ndk-${ANDROID_NDK_VERSION} ${ANDROID_NDK_ROOT} -ENV ANDROID_NDK_TOOLCHAIN_DIR /builds/worker/.android-ndk-r20-toolchain +ENV ANDROID_NDK_TOOLCHAIN_DIR /builds/worker/.android-ndk-r21-toolchain ENV ANDROID_NDK_API_VERSION 21 # sccache From 8501158411167ad4cb417be801b888fd6d0bd240 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 18 May 2020 13:16:45 +0200 Subject: [PATCH 27/63] Move SDK/NDK docs into Android section --- docs/SUMMARY.md | 2 +- docs/dev/{core/internal => android}/sdk-ndk-versions.md | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename docs/dev/{core/internal => android}/sdk-ndk-versions.md (100%) diff --git a/docs/SUMMARY.md b/docs/SUMMARY.md index 476c0aa82f..656516a9c6 100644 --- a/docs/SUMMARY.md +++ b/docs/SUMMARY.md @@ -46,6 +46,7 @@ - [Developing documentation](dev/docs.md) - [Android bindings](dev/android/index.md) - [Setup Build Environment](dev/android/setup-android-build-environment.md) + - [Android SDK/NDK versions](dev/android/sdk-ndk-versions.md) - [iOS bindings](dev/ios/index.md) - [Setup Build Environment](dev/ios/setup-ios-build-environment.md) - [Debugging Different Versions of Glean](dev/ios/debug-glean-on-ios.md) @@ -71,7 +72,6 @@ - [Debug Pings](dev/core/internal/debug-pings.md) - [Upload mechanism](dev/core/internal/upload.md) - [Implementations](dev/core/internal/implementations.md) - - [Android SDK/NDK versions](dev/core/internal/sdk-ndk-versions.md) - [Howtos](dev/howtos/index.md) - [Development with android-components](dev/howtos/development-with-android-components.md) - [Locally-published components in Fenix](dev/howtos/locally-published-components-in-fenix.md) diff --git a/docs/dev/core/internal/sdk-ndk-versions.md b/docs/dev/android/sdk-ndk-versions.md similarity index 100% rename from docs/dev/core/internal/sdk-ndk-versions.md rename to docs/dev/android/sdk-ndk-versions.md From 927d77c8edc6bf03422efedbb1d9803be30fee6c Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 18 May 2020 13:17:08 +0200 Subject: [PATCH 28/63] Move a-c/fenix development tips into Android section --- bin/build-rust-docs.sh | 2 +- docs/SUMMARY.md | 5 ++--- .../development-with-android-components.md | 0 .../locally-published-components-in-fenix.md | 0 docs/dev/howtos/index.md | 0 5 files changed, 3 insertions(+), 4 deletions(-) rename docs/dev/{howtos => android}/development-with-android-components.md (100%) rename docs/dev/{howtos => android}/locally-published-components-in-fenix.md (100%) delete mode 100644 docs/dev/howtos/index.md diff --git a/bin/build-rust-docs.sh b/bin/build-rust-docs.sh index f149815e63..a6029a5b50 100755 --- a/bin/build-rust-docs.sh +++ b/bin/build-rust-docs.sh @@ -7,7 +7,7 @@ # Build all docs with one command # Documentation will be placed in `build/docs`. -set -e +set -xe CRATE_NAME=glean_core diff --git a/docs/SUMMARY.md b/docs/SUMMARY.md index 656516a9c6..f0ef2e3608 100644 --- a/docs/SUMMARY.md +++ b/docs/SUMMARY.md @@ -47,6 +47,8 @@ - [Android bindings](dev/android/index.md) - [Setup Build Environment](dev/android/setup-android-build-environment.md) - [Android SDK/NDK versions](dev/android/sdk-ndk-versions.md) + - [Development with android-components](dev/android/development-with-android-components.md) + - [Locally-published components in Fenix](dev/android/locally-published-components-in-fenix.md) - [iOS bindings](dev/ios/index.md) - [Setup Build Environment](dev/ios/setup-ios-build-environment.md) - [Debugging Different Versions of Glean](dev/ios/debug-glean-on-ios.md) @@ -72,9 +74,6 @@ - [Debug Pings](dev/core/internal/debug-pings.md) - [Upload mechanism](dev/core/internal/upload.md) - [Implementations](dev/core/internal/implementations.md) - - [Howtos](dev/howtos/index.md) - - [Development with android-components](dev/howtos/development-with-android-components.md) - - [Locally-published components in Fenix](dev/howtos/locally-published-components-in-fenix.md) - [API Documentation](api/index.md) - [Appendix](appendix/index.md) - [Glossary](appendix/glossary.md) diff --git a/docs/dev/howtos/development-with-android-components.md b/docs/dev/android/development-with-android-components.md similarity index 100% rename from docs/dev/howtos/development-with-android-components.md rename to docs/dev/android/development-with-android-components.md diff --git a/docs/dev/howtos/locally-published-components-in-fenix.md b/docs/dev/android/locally-published-components-in-fenix.md similarity index 100% rename from docs/dev/howtos/locally-published-components-in-fenix.md rename to docs/dev/android/locally-published-components-in-fenix.md diff --git a/docs/dev/howtos/index.md b/docs/dev/howtos/index.md deleted file mode 100644 index e69de29bb2..0000000000 From 659a6c47e09ecabdd58172b894db579bc396521b Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 18 May 2020 12:04:23 +0200 Subject: [PATCH 29/63] Upgrade mdbook in use This fixes some rendering issues. [doc only] --- .circleci/config.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 50b8df572e..74d5c2e015 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -346,9 +346,9 @@ jobs: - run: name: Install mdbook-dtmo command: | - MDBOOK_VERSION=0.6.0 + MDBOOK_VERSION=0.6.1 MDBOOK="mdbook-dtmo-${MDBOOK_VERSION}-x86_64-unknown-linux-gnu.tar.gz" - MDBOOK_SHA256=a399a4a478290d1d889b4acefc3aecb8cd1ea051728aed276a9b169c03f8d375 + MDBOOK_SHA256=775124f302e633db91696ff955509e8567305949c79a76ef51649b9871fdd590 curl -sfSL --retry 5 -O "https://github.com/badboy/mdbook-dtmo/releases/download/${MDBOOK_VERSION}/${MDBOOK}" echo "${MDBOOK_SHA256} *${MDBOOK}" | shasum -a 256 -c - tar -xvf "${MDBOOK}" From 16481e7ce9894c4f44e6e9bc8be09dcda29c7075 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 18 May 2020 14:35:40 +0200 Subject: [PATCH 30/63] Fix links to moved chapters [doc only] --- docs/dev/android/locally-published-components-in-fenix.md | 2 +- docs/dev/android/sdk-ndk-versions.md | 2 +- docs/dev/core/internal/index.md | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/dev/android/locally-published-components-in-fenix.md b/docs/dev/android/locally-published-components-in-fenix.md index 585583f6f9..743144517c 100644 --- a/docs/dev/android/locally-published-components-in-fenix.md +++ b/docs/dev/android/locally-published-components-in-fenix.md @@ -81,7 +81,7 @@ You should now be able to build and run Fenix (assuming you could before all thi ## Caveats -1. This assumes you have followed the [android/rust build setup](../android/setup-android-build-environment.md) +1. This assumes you have followed the [Android/Rust build setup](setup-android-build-environment.md) 2. Make sure you're fully up to date in all repositories, unless you know you need to not be. 3. This omits the steps if changes needed because, e.g. Glean made a breaking change to an API used in android-components. These should be understandable to fix, you usually should be able to find a PR with the fixes somewhere in the android-component's list of pending PRs diff --git a/docs/dev/android/sdk-ndk-versions.md b/docs/dev/android/sdk-ndk-versions.md index 559d3e7a80..b16e2144a6 100644 --- a/docs/dev/android/sdk-ndk-versions.md +++ b/docs/dev/android/sdk-ndk-versions.md @@ -10,7 +10,7 @@ The Glean SDK implementation is currently build against the following versions: * NDK r21 * Download link: -For the full setup see [Setup the Android Build Environment](../../android/setup-android-build-environment.html). +For the full setup see [Setup the Android Build Environment](setup-android-build-environment.html). The versions are defined in the following files. All locations need to be updated on upgrades: diff --git a/docs/dev/core/internal/index.md b/docs/dev/core/internal/index.md index 391a0a81c2..91a2deb54c 100644 --- a/docs/dev/core/internal/index.md +++ b/docs/dev/core/internal/index.md @@ -10,4 +10,3 @@ This includes: * [Directory structure](directory-structure.md) * [Debug Pings](debug-pings.md) * [Implementations](implementations.md) -* [Android SDK/NDK versions](sdk-ndk-versions.md) From 182ebc21cfce570795f1ba1649fc2f2abf711cb8 Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Mon, 18 May 2020 14:52:46 +0200 Subject: [PATCH 31/63] Remove the double spacing in the Utf8Error message --- glean-core/src/error.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glean-core/src/error.rs b/glean-core/src/error.rs index bf7b20b1e6..e545fdf3c7 100644 --- a/glean-core/src/error.rs +++ b/glean-core/src/error.rs @@ -102,7 +102,7 @@ impl Display for Error { MemoryUnit(m) => write!(f, "MemoryUnit conversion from {} failed", m), HistogramType(h) => write!(f, "HistogramType conversion from {} failed", h), OsString(s) => write!(f, "OsString conversion from {:?} failed", s), - Utf8Error => write!(f, "Invalid UTF-8 byte sequence in string"), + Utf8Error => write!(f, "Invalid UTF-8 byte sequence in string"), NotInitialized => write!(f, "Global Glean object missing"), __NonExhaustive => write!(f, "Unknown error"), } From b77c4501fc2532e62e3d28ebcddedfa4befad3d6 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 18 May 2020 15:27:36 +0200 Subject: [PATCH 32/63] Document the requirement to configure an HTTP client [doc only] --- docs/user/general-api.md | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/docs/user/general-api.md b/docs/user/general-api.md index 5caf309692..6fc3d7fe41 100644 --- a/docs/user/general-api.md +++ b/docs/user/general-api.md @@ -57,7 +57,7 @@ class SampleApplication : Application() { // Initialize the Glean library. Glean.initialize( - applicationContext, + applicationContext, // Here, `settings()` is a method to get user preferences, specific to // your application and not part of the Glean SDK API. uploadEnabled = settings().isTelemetryEnabled @@ -78,6 +78,25 @@ Library code should never call `Glean.initialize`, since it should be called exa Glean.initialize(applicationContext, Configuration(channel = "beta")) ``` +> **Note**: When the Glean SDK is consumed through Android Components, it is required to configure an HTTP client to be used for upload. +> For example: + +```Kotlin +// Requires `org.mozilla.components:concept-fetch` +import mozilla.components.concept.fetch.Client +// Requires `org.mozilla.components:lib-fetch-httpurlconnection`. +// This can be replaced by other implementations, e.g. `lib-fetch-okhttp` +// or an implementation from `browser-engine-gecko`. +import mozilla.components.lib.fetch.httpurlconnection.HttpURLConnectionClient + +import mozilla.components.service.glean.config.Configuration +import mozilla.components.service.glean.net.ConceptFetchHttpUploader + +val httpClient = ConceptFetchHttpUploader(lazy { HttpURLConnectionClient() as Client }) +val config = Configuration(httpClient = httpClient) +Glean.initialize(context, uploadEnabled = true, configuration = config) +``` +
From 11f9885290274aae7e4cc40a1649876d8b14c97a Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Thu, 14 May 2020 12:22:36 +0200 Subject: [PATCH 33/63] iOS: Don't decode gzipped data for logging --- glean-core/ios/Glean/Net/HttpPingUploader.swift | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/glean-core/ios/Glean/Net/HttpPingUploader.swift b/glean-core/ios/Glean/Net/HttpPingUploader.swift index bc680109d4..71f0d1dfd5 100644 --- a/glean-core/ios/Glean/Net/HttpPingUploader.swift +++ b/glean-core/ios/Glean/Net/HttpPingUploader.swift @@ -54,8 +54,15 @@ public class HttpPingUploader { /// us to easily handle pings coming from Glean on the legacy Mozilla pipeline. func upload(path: String, data: Data, headers: [String: String], callback: @escaping (UploadResult) -> Void) { if config.logPings { - // FIXME: ungzip data if it is gzipped. - ///logPing(path: path, data: data) + // FIXME(bug 1637914): Logging should happen inside glean-core (Rust). + // For now we don't ship Gzip decompression in the Glean SDK for iOS + // due to difficulties delivering dependencies, so we skip logging them. + if headers["Content-Encoding"] == "gzip" { + logPing(path: path, data: "") + } else { + let payload = String(data: data, encoding: .utf8) ?? "" + logPing(path: path, data: payload) + } } // Build the request and create an async upload operation and launch it through the From dfd2cbdcfcea524bf8cd52a1929b4ff4ca6726c5 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Thu, 14 May 2020 13:48:02 +0200 Subject: [PATCH 34/63] iOS: Refactor telemetry endpoint stub into a reusable function --- .../ios/Glean.xcodeproj/project.pbxproj | 4 ++ .../Debug/GleanDebugUtilityTests.swift | 50 +++++-------- glean-core/ios/GleanTests/GleanTests.swift | 71 ++----------------- .../GleanTests/Metrics/EventMetricTests.swift | 14 +--- .../ios/GleanTests/Metrics/PingTests.swift | 18 +---- .../Net/DeletionRequestPingTests.swift | 18 +---- .../Net/HttpPingUploaderTests.swift | 43 ++--------- .../Scheduler/MetricsPingSchedulerTests.swift | 31 ++------ glean-core/ios/GleanTests/TestUtils.swift | 38 ++++++++++ 9 files changed, 87 insertions(+), 200 deletions(-) create mode 100644 glean-core/ios/GleanTests/TestUtils.swift diff --git a/glean-core/ios/Glean.xcodeproj/project.pbxproj b/glean-core/ios/Glean.xcodeproj/project.pbxproj index b2a23d142c..17c8f259c5 100644 --- a/glean-core/ios/Glean.xcodeproj/project.pbxproj +++ b/glean-core/ios/Glean.xcodeproj/project.pbxproj @@ -59,6 +59,7 @@ BFB59A9D2342A24000F40CA8 /* GleanBaseline.swift in Sources */ = {isa = PBXBuildFile; fileRef = BFB59A992342A24000F40CA8 /* GleanBaseline.swift */; }; BFB59A9E2342A24000F40CA8 /* Pings.swift in Sources */ = {isa = PBXBuildFile; fileRef = BFB59A9A2342A24000F40CA8 /* Pings.swift */; }; BFB59A9F2342A24000F40CA8 /* GleanInternalMetrics.swift in Sources */ = {isa = PBXBuildFile; fileRef = BFB59A9B2342A24000F40CA8 /* GleanInternalMetrics.swift */; }; + BFCBD6AB246D55CC0032096D /* TestUtils.swift in Sources */ = {isa = PBXBuildFile; fileRef = BFCBD6AA246D55CC0032096D /* TestUtils.swift */; }; BFE1CDC4233B63A70019EE47 /* LabeledMetric.swift in Sources */ = {isa = PBXBuildFile; fileRef = BFE1CDC3233B63A70019EE47 /* LabeledMetric.swift */; }; BFE1CDC6233B6B6D0019EE47 /* LabeledMetricTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = BFE1CDC5233B6B6D0019EE47 /* LabeledMetricTests.swift */; }; BFE1CDC8233B73B30019EE47 /* Unreachable.swift in Sources */ = {isa = PBXBuildFile; fileRef = BFE1CDC7233B73B30019EE47 /* Unreachable.swift */; }; @@ -139,6 +140,7 @@ BFB59A992342A24000F40CA8 /* GleanBaseline.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = GleanBaseline.swift; sourceTree = ""; }; BFB59A9A2342A24000F40CA8 /* Pings.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = Pings.swift; sourceTree = ""; }; BFB59A9B2342A24000F40CA8 /* GleanInternalMetrics.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = GleanInternalMetrics.swift; sourceTree = ""; }; + BFCBD6AA246D55CC0032096D /* TestUtils.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TestUtils.swift; sourceTree = ""; }; BFE1CDC3233B63A70019EE47 /* LabeledMetric.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LabeledMetric.swift; sourceTree = ""; }; BFE1CDC5233B6B6D0019EE47 /* LabeledMetricTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LabeledMetricTests.swift; sourceTree = ""; }; BFE1CDC7233B73B30019EE47 /* Unreachable.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Unreachable.swift; sourceTree = ""; }; @@ -316,6 +318,7 @@ 1F70B787232A81A4007395FB /* DispatchersTest.swift */, BF3DE39F2243A2F20018E23F /* GleanTests.swift */, BF3DE3A12243A2F20018E23F /* Info.plist */, + BFCBD6AA246D55CC0032096D /* TestUtils.swift */, ); path = GleanTests; sourceTree = ""; @@ -616,6 +619,7 @@ 1F39E7B3239F0777009B13B3 /* GleanDebugUtilityTests.swift in Sources */, BFAED50A2369752400DF293D /* StringListMetricTests.swift in Sources */, BF890561232BC227003CA2BA /* StringMetricTests.swift in Sources */, + BFCBD6AB246D55CC0032096D /* TestUtils.swift in Sources */, 1F58921223C923C4007D2D80 /* MetricsPingSchedulerTests.swift in Sources */, 1FD4527723395EEB00F4C7E8 /* UuidMetricTests.swift in Sources */, BF80AA5B2399301300A8B172 /* HttpPingUploaderTests.swift in Sources */, diff --git a/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift b/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift index 8649548ca4..c8cba19972 100644 --- a/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift +++ b/glean-core/ios/GleanTests/Debug/GleanDebugUtilityTests.swift @@ -18,38 +18,6 @@ class GleanDebugUtilityTests: XCTestCase { Glean.shared.setUploadEnabled(true) } - private func setupHttpResponseStub(statusCode: Int32 = 200) { - expectation = expectation(description: "Ping sent") - // This function will be handling one each of baseline, events, and metrics pings - // so we set the expected count to 3 and set it to assert for overfulfill in order - // to test that unknown pings aren't being sent. - expectation!.expectedFulfillmentCount = 3 - expectation!.assertForOverFulfill = true - let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! - stub(condition: isHost(host)) { request in - let request = request as NSURLRequest - let body = request.ohhttpStubs_HTTPBody() - let json = try! JSONSerialization.jsonObject(with: body!, options: []) as? [String: Any] - XCTAssert(json != nil) - let pingType = request.url?.path.split(separator: "/")[2] - XCTAssertTrue( - Glean.shared.testHasPingType(String(pingType!)), - "\(String(pingType!)) should be registered, but is missing" - ) - - DispatchQueue.main.async { - // Let the response get processed before we mark the expectation fulfilled - self.expectation?.fulfill() - } - - return OHHTTPStubsResponse( - jsonObject: [], - statusCode: statusCode, - headers: ["Content-Type": "application/json"] - ) - } - } - func testHandleCustomUrlTagPings() { // Check invalid tags: This should have the configuration keep the // default value of nil because they don't match the accepted @@ -105,7 +73,23 @@ class GleanDebugUtilityTests: XCTestCase { } func testHandleCustomUrlSendPing() { - setupHttpResponseStub() + expectation = expectation(description: "Ping sent") + // This test will be sending one each of baseline, events, and metrics pings + // so we set the expected count to 3 and set it to assert for overfulfill in order + // to test that unknown pings aren't being sent. + expectation!.expectedFulfillmentCount = 3 + expectation!.assertForOverFulfill = true + stubServerReceive { pingType, _ in + XCTAssertTrue( + Glean.shared.testHasPingType(pingType), + "\(pingType) should be registered, but is missing" + ) + + DispatchQueue.main.async { + // Let the response get processed before we mark the expectation fulfilled + self.expectation?.fulfill() + } + } // Create a dummy event and a dummy metric so that the // respective pings will be sent diff --git a/glean-core/ios/GleanTests/GleanTests.swift b/glean-core/ios/GleanTests/GleanTests.swift index 89b7c7147f..a9474a8f7d 100644 --- a/glean-core/ios/GleanTests/GleanTests.swift +++ b/glean-core/ios/GleanTests/GleanTests.swift @@ -102,13 +102,7 @@ class GleanTests: XCTestCase { } func testSendingOfForegroundBaselinePing() { - // Set up the test stub based on the default telemetry endpoint - let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! - stub(condition: isHost(host)) { data in - let body = (data as NSURLRequest).ohhttpStubs_HTTPBody() - let json = try! JSONSerialization.jsonObject(with: body!, options: []) as? [String: Any] - XCTAssert(json != nil) - + stubServerReceive { _, json in // Check for the "dirty_startup" flag let pingInfo = json?["ping_info"] as? [String: Any] XCTAssertEqual("foreground", pingInfo?["reason"] as? String) @@ -127,12 +121,6 @@ class GleanTests: XCTestCase { // let the response get processed before we mark the expectation fulfilled self.expectation?.fulfill() } - - return OHHTTPStubsResponse( - jsonObject: [], - statusCode: 200, - headers: ["Content-Type": "application/json"] - ) } // Set up the expectation that will be fulfilled by the stub above @@ -157,10 +145,7 @@ class GleanTests: XCTestCase { glean_set_dirty_flag(true.toByte()) // Set up the test stub based on the default telemetry endpoint - let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! - stub(condition: isHost(host)) { data in - let body = (data as NSURLRequest).ohhttpStubs_HTTPBody() - let json = try! JSONSerialization.jsonObject(with: body!, options: []) as? [String: Any] + stubServerReceive { _, json in XCTAssert(json != nil) // Check for the "dirty_startup" flag @@ -181,12 +166,6 @@ class GleanTests: XCTestCase { // let the response get processed before we mark the expectation fulfilled self.expectation?.fulfill() } - - return OHHTTPStubsResponse( - jsonObject: [], - statusCode: 200, - headers: ["Content-Type": "application/json"] - ) } // Set up the expectation that will be fulfilled by the stub above @@ -205,25 +184,13 @@ class GleanTests: XCTestCase { } func testSendingDeletionPingIfDisabledOutsideOfRun() { - // Set up the test stub based on the default telemetry endpoint - let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! - stub(condition: isHost(host)) { data in - let path = (data as NSURLRequest).url! - - let parts = path.absoluteString.split(separator: "/") - - XCTAssertEqual("deletion-request", parts[4]) + stubServerReceive { pingType, _ in + XCTAssertEqual("deletion-request", pingType) DispatchQueue.main.async { // let the response get processed before we mark the expectation fulfilled self.expectation?.fulfill() } - - return OHHTTPStubsResponse( - jsonObject: [], - statusCode: 200, - headers: ["Content-Type": "application/json"] - ) } // Set up the expectation that will be fulfilled by the stub above @@ -242,20 +209,8 @@ class GleanTests: XCTestCase { func testNotSendingDeletionRequestIfUnchangedOutsideOfRun() { // Set up the test stub based on the default telemetry endpoint - let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! - stub(condition: isHost(host)) { _ in + stubServerReceive { _, _ in XCTFail("Should not have recieved any ping") - - DispatchQueue.main.async { - // let the response get processed before we mark the expectation fulfilled - self.expectation?.fulfill() - } - - return OHHTTPStubsResponse( - jsonObject: [], - statusCode: 200, - headers: ["Content-Type": "application/json"] - ) } // Set up the expectation that will NOT be fulfilled by the stub above. If it is @@ -293,14 +248,8 @@ class GleanTests: XCTestCase { stringMetric.set("HELLOOOOO!") // Set up the test stub based on the default telemetry endpoint - let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! - stub(condition: isHost(host)) { data in - let path = (data as NSURLRequest).url! - let parts = path.absoluteString.split(separator: "/") - XCTAssertEqual("baseline", parts[4]) - - let body = (data as NSURLRequest).ohhttpStubs_HTTPBody() - let json = try! JSONSerialization.jsonObject(with: body!, options: []) as? [String: Any] + stubServerReceive { pingType, json in + XCTAssertEqual("baseline", pingType) XCTAssert(json != nil) // Check for the "dirty_startup" flag @@ -317,12 +266,6 @@ class GleanTests: XCTestCase { // let the response get processed before we mark the expectation fulfilled self.expectation?.fulfill() } - - return OHHTTPStubsResponse( - jsonObject: [], - statusCode: 200, - headers: ["Content-Type": "application/json"] - ) } expectation = expectation(description: "baseline ping received") diff --git a/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift b/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift index ecc0210fed..700cbb14d3 100644 --- a/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift +++ b/glean-core/ios/GleanTests/Metrics/EventMetricTests.swift @@ -37,11 +37,8 @@ class EventMetricTypeTests: XCTestCase { var expectation: XCTestExpectation? var lastPingJson: [String: Any]? - private func setupHttpResponseStub(statusCode: Int32 = 200) { - let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! - stub(condition: isHost(host)) { data in - let body = (data as NSURLRequest).ohhttpStubs_HTTPBody() - let json = try! JSONSerialization.jsonObject(with: body!, options: []) as? [String: Any] + private func setupHttpResponseStub() { + stubServerReceive { _, json in XCTAssert(json != nil) self.lastPingJson = json @@ -50,13 +47,6 @@ class EventMetricTypeTests: XCTestCase { // Let the response get processed before we mark the expectation fulfilled self.expectation?.fulfill() } - - // Ensure a response so that the uploader does its job. - return OHHTTPStubsResponse( - jsonObject: [], - statusCode: statusCode, - headers: ["Content-Type": "application/json"] - ) } } diff --git a/glean-core/ios/GleanTests/Metrics/PingTests.swift b/glean-core/ios/GleanTests/Metrics/PingTests.swift index 636e5fbbb5..f9cb4fe34b 100644 --- a/glean-core/ios/GleanTests/Metrics/PingTests.swift +++ b/glean-core/ios/GleanTests/Metrics/PingTests.swift @@ -20,15 +20,10 @@ class PingTests: XCTestCase { } private func setupHttpResponseStub(_ expectedPingType: String) { - let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! - stub(condition: isHost(host)) { request in - let request = request as NSURLRequest - let pingType = request.url?.path.split(separator: "/")[2] - XCTAssertEqual(String(pingType!), expectedPingType, "Wrong ping type received") - - let body = request.ohhttpStubs_HTTPBody() - let json = try! JSONSerialization.jsonObject(with: body!, options: []) as? [String: Any] + stubServerReceive { pingType, json in + XCTAssertEqual(pingType, expectedPingType, "Wrong ping type received") XCTAssert(json != nil) + self.lastPingJson = json // Fulfill test's expectation once we parsed the incoming data. @@ -36,13 +31,6 @@ class PingTests: XCTestCase { // Let the response get processed before we mark the expectation fulfilled self.expectation?.fulfill() } - - // Ensure a response so that the uploader does its job. - return OHHTTPStubsResponse( - jsonObject: [], - statusCode: 200, - headers: ["Content-Type": "application/json"] - ) } } diff --git a/glean-core/ios/GleanTests/Net/DeletionRequestPingTests.swift b/glean-core/ios/GleanTests/Net/DeletionRequestPingTests.swift index 4c1d6cad73..7a9df71806 100644 --- a/glean-core/ios/GleanTests/Net/DeletionRequestPingTests.swift +++ b/glean-core/ios/GleanTests/Net/DeletionRequestPingTests.swift @@ -13,14 +13,9 @@ class DeletionRequestPingTests: XCTestCase { var lastPingJson: [String: Any]? private func setupHttpResponseStub(_ expectedPingType: String) { - let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! - stub(condition: isHost(host)) { request in - let request = request as NSURLRequest - let pingType = request.url?.path.split(separator: "/")[2] - XCTAssertEqual(String(pingType!), expectedPingType, "Wrong ping type received") - - let body = request.ohhttpStubs_HTTPBody() - let json = try! JSONSerialization.jsonObject(with: body!, options: []) as? [String: Any] + stubServerReceive { pingType, json in + XCTAssertEqual(pingType, expectedPingType, "Wrong ping type received") + XCTAssert(json != nil) self.lastPingJson = json @@ -29,13 +24,6 @@ class DeletionRequestPingTests: XCTestCase { // Let the response get processed before we mark the expectation fulfilled self.expectation?.fulfill() } - - // Ensure a response so that the uploader does its job. - return OHHTTPStubsResponse( - jsonObject: [], - statusCode: 200, - headers: ["Content-Type": "application/json"] - ) } } diff --git a/glean-core/ios/GleanTests/Net/HttpPingUploaderTests.swift b/glean-core/ios/GleanTests/Net/HttpPingUploaderTests.swift index 3a5884659b..e777a4a0d5 100644 --- a/glean-core/ios/GleanTests/Net/HttpPingUploaderTests.swift +++ b/glean-core/ios/GleanTests/Net/HttpPingUploaderTests.swift @@ -22,22 +22,6 @@ class HttpPingUploaderTests: XCTestCase { expectation = nil } - private func setupHttpResponseStub(statusCode: Int32 = 200) { - let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! - stub(condition: isHost(host)) { data in - let body = (data as NSURLRequest).ohhttpStubs_HTTPBody() - let json = try! JSONSerialization.jsonObject(with: body!, options: []) as? [String: Any] - XCTAssert(json != nil) - XCTAssertEqual(json?["ping"] as? String, "test") - - return OHHTTPStubsResponse( - jsonObject: [], - statusCode: statusCode, - headers: ["Content-Type": "application/json"] - ) - } - } - func getOrCreatePingDirectory(_ pingDirectory: String) -> URL { let dataPath = getGleanDirectory().appendingPathComponent(pingDirectory) @@ -83,7 +67,10 @@ class HttpPingUploaderTests: XCTestCase { func testHTTPStatusCode() { var testValue: UploadResult? - setupHttpResponseStub(statusCode: 200) + stubServerReceive { _, json in + XCTAssert(json != nil) + XCTAssertEqual(json?["ping"] as? String, "test") + } expectation = expectation(description: "Completed upload") @@ -171,10 +158,7 @@ class HttpPingUploaderTests: XCTestCase { // Now set up our test server var countFilesUploaded = 0 - let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! - stub(condition: isHost(host)) { data in - let body = (data as NSURLRequest).ohhttpStubs_HTTPBody() - let json = try! JSONSerialization.jsonObject(with: body!, options: []) as? [String: Any] + stubServerReceive { _, json in XCTAssert(json != nil) XCTAssertEqual(json?["ping"] as? String, "test") @@ -185,12 +169,6 @@ class HttpPingUploaderTests: XCTestCase { self.expectation?.fulfill() } } - - return OHHTTPStubsResponse( - jsonObject: [], - statusCode: 200, - headers: ["Content-Type": "application/json"] - ) } // Set our expectation that will be fulfilled by the stub above @@ -235,10 +213,7 @@ class HttpPingUploaderTests: XCTestCase { ) // Now set up our test server - let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! - stub(condition: isHost(host)) { data in - let body = (data as NSURLRequest).ohhttpStubs_HTTPBody() - let json = try! JSONSerialization.jsonObject(with: body!, options: []) as? [String: Any] + stubServerReceive { _, json in XCTAssert(json != nil) XCTAssertEqual(json?["ping"] as? String, "test") @@ -246,12 +221,6 @@ class HttpPingUploaderTests: XCTestCase { // let the response get processed before we mark the expectation fulfilled self.expectation?.fulfill() } - - return OHHTTPStubsResponse( - jsonObject: [], - statusCode: 200, - headers: ["Content-Type": "application/json"] - ) } // Set our expectation that will be fulfilled by the stub above diff --git a/glean-core/ios/GleanTests/Scheduler/MetricsPingSchedulerTests.swift b/glean-core/ios/GleanTests/Scheduler/MetricsPingSchedulerTests.swift index 8b20ac90e4..1fb7ffd04f 100644 --- a/glean-core/ios/GleanTests/Scheduler/MetricsPingSchedulerTests.swift +++ b/glean-core/ios/GleanTests/Scheduler/MetricsPingSchedulerTests.swift @@ -135,8 +135,6 @@ class MetricsPingSchedulerTests: XCTestCase { ) } - // swiftlint:disable function_body_length - // REASON: Used in a test func testQueuedDataNotInOverdueMetricsPings() { // Reset Glean and do not start it right away Glean.shared.testDestroyGleanHandle() @@ -182,10 +180,7 @@ class MetricsPingSchedulerTests: XCTestCase { let yesterday = Calendar.current.date(byAdding: Calendar.Component.day, value: -1, to: now) Glean.shared.metricsPingScheduler.updateSentDate(yesterday!) - let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! - stub(condition: isHost(host)) { data in - let body = (data as NSURLRequest).ohhttpStubs_HTTPBody() - let json = try! JSONSerialization.jsonObject(with: body!, options: []) as? [String: Any] + stubServerReceive { _, json in XCTAssert(json != nil) let metrics = json?["metrics"] as? [String: Any] let strings = metrics?["string"] as? [String: Any] @@ -201,12 +196,6 @@ class MetricsPingSchedulerTests: XCTestCase { // let the response get processed before we mark the expectation fulfilled self.expectation?.fulfill() } - - return OHHTTPStubsResponse( - jsonObject: [], - statusCode: 200, - headers: ["Content-Type": "application/json"] - ) } // Set our expectation that will be fulfilled by the stub above @@ -257,24 +246,18 @@ class MetricsPingSchedulerTests: XCTestCase { Glean.shared.metricsPingScheduler.updateSentDate(yesterday!) // Set up the interception of the ping for inspection - let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! - stub(condition: isHost(host)) { data in - let docType = (data as NSURLRequest).url!.path.split(separator: "/")[2] - XCTAssertEqual(docType, "metrics", "Must be a metrics ping") + stubServerReceive { pingType, json in + XCTAssertEqual(pingType, "metrics", "Must be a metrics ping") - let body = String(decoding: (data as NSURLRequest).ohhttpStubs_HTTPBody(), as: UTF8.self) - XCTAssertTrue(body.contains(expectedValue), "Must contain expected value") + let metrics = json?["metrics"] as? [String: Any] + let strings = metrics?["string"] as? [String: Any] + let testMetric = strings?["telemetry.test_applifetime_metric"] as? String + XCTAssertEqual(expectedValue, testMetric, "Must contain expected value") DispatchQueue.main.async { // let the response get processed before we mark the expectation fulfilled self.expectation?.fulfill() } - - return OHHTTPStubsResponse( - jsonObject: [], - statusCode: 200, - headers: ["Content-Type": "application/json"] - ) } // Set our expectation that will be fulfilled by the stub above diff --git a/glean-core/ios/GleanTests/TestUtils.swift b/glean-core/ios/GleanTests/TestUtils.swift new file mode 100644 index 0000000000..02a2e802c1 --- /dev/null +++ b/glean-core/ios/GleanTests/TestUtils.swift @@ -0,0 +1,38 @@ +/* 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/. */ + +@testable import Glean +import OHHTTPStubs + +/// Stub out receiving a request on Glean's default Telemetry endpoint. +/// +/// When receiving a request, it extracts the ping type from the URL +/// according to the endpoint URL format. +/// +/// It assumes the request body is a JSON document and tries to decode it. +/// If necessary, it decompresses the request body first. +/// +/// - parameters +/// * callback: A callback to validate the incoming request. +/// It receives a `pingType` and the ping's JSON-decoded `payload`. +func stubServerReceive(callback: @escaping (String, [String: Any]?) -> Void) { + let host = URL(string: Configuration.Constants.defaultTelemetryEndpoint)!.host! + stub(condition: isHost(host)) { data in + let request = data as NSURLRequest + let url = request.url! + let parts = url.absoluteString.split(separator: "/") + let pingType = String(parts[4]) + + let body = request.ohhttpStubs_HTTPBody()! + let payload = try! JSONSerialization.jsonObject(with: body, options: []) as? [String: Any] + + callback(pingType, payload) + + return OHHTTPStubsResponse( + jsonObject: [], + statusCode: 200, + headers: ["Content-Type": "application/json"] + ) + } +} From 5fdd54e1742b4c1dcbd5df596266a298381a503a Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Thu, 14 May 2020 12:22:36 +0200 Subject: [PATCH 35/63] iOS: Add GzipSwift as a dependency for tests From https://github.com/1024jp/GzipSwift It's a wrapper around zlib, used for gzip (de)compression. It is MIT licensed: https://github.com/1024jp/GzipSwift/blob/e8c00b4bdc621b3c714ab72e3958778f5751e146/LICENSE --- .gitignore | 3 --- Cartfile.private | 1 + glean-core/ios/Glean.xcodeproj/project.pbxproj | 3 +++ 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index 88c38abbdc..b2536640e6 100644 --- a/.gitignore +++ b/.gitignore @@ -20,9 +20,6 @@ raw_xcode*log /glean-core/ios/Pipfile.lock /glean-core/ios/.venv -# Due to a bug in Carthage, we ignore this file. -# See `tests-only-Cartfile` for more info. -/Cartfile # Carthage build artifacts Cartfile.resolved Carthage diff --git a/Cartfile.private b/Cartfile.private index 4d37985a71..52f3f4d1c7 100644 --- a/Cartfile.private +++ b/Cartfile.private @@ -1 +1,2 @@ github "AliSoftware/OHHTTPStubs" "8.0.0" +github "1024jp/GzipSwift" "5.1.1" diff --git a/glean-core/ios/Glean.xcodeproj/project.pbxproj b/glean-core/ios/Glean.xcodeproj/project.pbxproj index 17c8f259c5..591e654eb0 100644 --- a/glean-core/ios/Glean.xcodeproj/project.pbxproj +++ b/glean-core/ios/Glean.xcodeproj/project.pbxproj @@ -114,6 +114,7 @@ BF30FDC3233260B500840607 /* TimespanMetric.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TimespanMetric.swift; sourceTree = ""; }; BF30FDC5233260C400840607 /* TimespanMetricTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TimespanMetricTests.swift; sourceTree = ""; }; BF30FDC72332640400840607 /* TimeUnit.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TimeUnit.swift; sourceTree = ""; }; + BF3A3055246E936E00E4A18E /* Gzip.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Gzip.framework; path = ../../Carthage/Build/iOS/Gzip.framework; sourceTree = ""; }; BF3DE3912243A2F20018E23F /* Glean.framework */ = {isa = PBXFileReference; explicitFileType = wrapper.framework; includeInIndex = 0; path = Glean.framework; sourceTree = BUILT_PRODUCTS_DIR; }; BF3DE3942243A2F20018E23F /* Glean.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = Glean.h; sourceTree = ""; }; BF3DE3952243A2F20018E23F /* Info.plist */ = {isa = PBXFileReference; lastKnownFileType = text.plist.xml; path = Info.plist; sourceTree = ""; }; @@ -369,6 +370,7 @@ BF6F2DA6224BF8F000394062 /* Frameworks */ = { isa = PBXGroup; children = ( + BF3A3055246E936E00E4A18E /* Gzip.framework */, BF1BF3F72333787E0036F4CC /* OHHTTPStubs.framework */, BF6F2DA7224BF8F100394062 /* libglean_ffi.a */, ); @@ -518,6 +520,7 @@ ); outputPaths = ( "$(BUILT_PRODUCTS_DIR)/$(FRAMEWORKS_FOLDER_PATH)/OHHTTPStubs.framework", + "$(BUILT_PRODUCTS_DIR)/$(FRAMEWORKS_FOLDER_PATH)/Gzip.framework", ); runOnlyForDeploymentPostprocessing = 0; shellPath = /bin/sh; From e1981ce89036f72b25978bf9bdff02e4440cfc54 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Thu, 14 May 2020 13:48:48 +0200 Subject: [PATCH 36/63] iOS: Decode gzip payload for tests --- glean-core/ios/Glean.xcodeproj/project.pbxproj | 1 + glean-core/ios/GleanTests/TestUtils.swift | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/glean-core/ios/Glean.xcodeproj/project.pbxproj b/glean-core/ios/Glean.xcodeproj/project.pbxproj index 591e654eb0..c4801a9dff 100644 --- a/glean-core/ios/Glean.xcodeproj/project.pbxproj +++ b/glean-core/ios/Glean.xcodeproj/project.pbxproj @@ -514,6 +514,7 @@ ); inputPaths = ( ../../Carthage/Build/iOS/OHHTTPStubs.framework, + ../../Carthage/Build/iOS/Gzip.framework, ); name = "Copy Carthage Dependencies"; outputFileListPaths = ( diff --git a/glean-core/ios/GleanTests/TestUtils.swift b/glean-core/ios/GleanTests/TestUtils.swift index 02a2e802c1..f17348c8c5 100644 --- a/glean-core/ios/GleanTests/TestUtils.swift +++ b/glean-core/ios/GleanTests/TestUtils.swift @@ -3,6 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ @testable import Glean +import Gzip import OHHTTPStubs /// Stub out receiving a request on Glean's default Telemetry endpoint. @@ -24,7 +25,11 @@ func stubServerReceive(callback: @escaping (String, [String: Any]?) -> Void) { let parts = url.absoluteString.split(separator: "/") let pingType = String(parts[4]) - let body = request.ohhttpStubs_HTTPBody()! + var body = request.ohhttpStubs_HTTPBody()! + if request.value(forHTTPHeaderField: "Content-Encoding") == "gzip" { + body = try! body.gunzipped() + } + let payload = try! JSONSerialization.jsonObject(with: body, options: []) as? [String: Any] callback(pingType, payload) From 112dfa37a3bf8a86689007b5bc87483fb5f83374 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Thu, 14 May 2020 15:00:01 +0200 Subject: [PATCH 37/63] iOS: Add Gzip dependency to sample app It is required for the UI tests as a direct dependency. It is also required as an indirect dependency as part of Glean. Due to how Frameworks and linking works applications still need to explicitly link against those indirect dependencies. See https://github.com/Carthage/Carthage#nested-dependencies --- samples/ios/app/Cartfile | 1 + .../ios/app/glean-sample-app.xcodeproj/project.pbxproj | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/samples/ios/app/Cartfile b/samples/ios/app/Cartfile index d7ee22595e..ee3d21f72d 100644 --- a/samples/ios/app/Cartfile +++ b/samples/ios/app/Cartfile @@ -1,2 +1,3 @@ github "httpswift/swifter" ~> 1.4.7 +github "1024jp/GzipSwift" "5.1.1" github "mozilla/glean" "master" diff --git a/samples/ios/app/glean-sample-app.xcodeproj/project.pbxproj b/samples/ios/app/glean-sample-app.xcodeproj/project.pbxproj index 6ed937e5c2..4b055f1fec 100644 --- a/samples/ios/app/glean-sample-app.xcodeproj/project.pbxproj +++ b/samples/ios/app/glean-sample-app.xcodeproj/project.pbxproj @@ -9,6 +9,7 @@ /* Begin PBXBuildFile section */ BF1BF3FF23337E6F0036F4CC /* Glean.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = BF1BF3FE23337E6F0036F4CC /* Glean.framework */; }; BF89665A2371C82E0097B7F2 /* pings.yaml in Resources */ = {isa = PBXBuildFile; fileRef = BF8966592371C82E0097B7F2 /* pings.yaml */; }; + BF8C3866246D749400274ACE /* Gzip.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = BF8C3865246D749400274ACE /* Gzip.framework */; }; BF90C913236B43A10045BD0E /* Swifter.framework in Frameworks */ = {isa = PBXBuildFile; fileRef = BF90C911236B42F00045BD0E /* Swifter.framework */; }; BFD3AB2E224D475E00AD9255 /* AppDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = BFD3AB2D224D475E00AD9255 /* AppDelegate.swift */; }; BFD3AB30224D475E00AD9255 /* ViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = BFD3AB2F224D475E00AD9255 /* ViewController.swift */; }; @@ -48,6 +49,7 @@ BF1BF3FE23337E6F0036F4CC /* Glean.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Glean.framework; path = Carthage/Build/iOS/Glean.framework; sourceTree = ""; }; BF6F41C4234346A000E222A8 /* metrics.yaml */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.yaml; path = metrics.yaml; sourceTree = ""; }; BF8966592371C82E0097B7F2 /* pings.yaml */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.yaml; path = pings.yaml; sourceTree = ""; }; + BF8C3865246D749400274ACE /* Gzip.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Gzip.framework; path = Carthage/Build/iOS/Gzip.framework; sourceTree = ""; }; BF90C911236B42F00045BD0E /* Swifter.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Swifter.framework; path = Carthage/Build/iOS/Swifter.framework; sourceTree = ""; }; BFD3AB2A224D475E00AD9255 /* glean-sample-app.app */ = {isa = PBXFileReference; explicitFileType = wrapper.application; includeInIndex = 0; path = "glean-sample-app.app"; sourceTree = BUILT_PRODUCTS_DIR; }; BFD3AB2D224D475E00AD9255 /* AppDelegate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AppDelegate.swift; sourceTree = ""; }; @@ -96,6 +98,7 @@ isa = PBXFrameworksBuildPhase; buildActionMask = 2147483647; files = ( + BF8C3866246D749400274ACE /* Gzip.framework in Frameworks */, BF90C913236B43A10045BD0E /* Swifter.framework in Frameworks */, ); runOnlyForDeploymentPostprocessing = 0; @@ -180,6 +183,7 @@ BFD3AB68224D4F9500AD9255 /* Frameworks */ = { isa = PBXGroup; children = ( + BF8C3865246D749400274ACE /* Gzip.framework */, BF90C911236B42F00045BD0E /* Swifter.framework */, BF1BF3FE23337E6F0036F4CC /* Glean.framework */, ); @@ -352,12 +356,14 @@ ); inputPaths = ( "$(SRCROOT)/Carthage/Build/iOS/Swifter.framework", + "$(SRCROOT)/Carthage/Build/iOS/Gzip.framework", ); name = "Copy Carthage frameworks"; outputFileListPaths = ( ); outputPaths = ( "$(BUILT_PRODUCTS_DIR)/$(FRAMEWORKS_FOLDER_PATH)/Swifter.framework", + "$(BUILT_PRODUCTS_DIR)/$(FRAMEWORKS_FOLDER_PATH)/Gzip.framework", ); runOnlyForDeploymentPostprocessing = 0; shellPath = /bin/sh; @@ -372,12 +378,14 @@ ); inputPaths = ( "$(SRCROOT)/Carthage/Build/iOS/Glean.framework", + "$(SRCROOT)/Carthage/Build/iOS/Gzip.framework", ); name = "Copy Carthage frameworks"; outputFileListPaths = ( ); outputPaths = ( "$(BUILT_PRODUCTS_DIR)/$(FRAMEWORKS_FOLDER_PATH)/Glean.framework", + "$(BUILT_PRODUCTS_DIR)/$(FRAMEWORKS_FOLDER_PATH)/Gzip.framework", ); runOnlyForDeploymentPostprocessing = 0; shellPath = /bin/sh; From 132467d7312202ad7af1a89e75de702e0dc7e76f Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Thu, 14 May 2020 15:00:01 +0200 Subject: [PATCH 38/63] iOS: Decode gzip in UI tests --- .../ios/app/glean-sample-appUITests/MockServer.swift | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/samples/ios/app/glean-sample-appUITests/MockServer.swift b/samples/ios/app/glean-sample-appUITests/MockServer.swift index 1dae213307..405c9398fc 100644 --- a/samples/ios/app/glean-sample-appUITests/MockServer.swift +++ b/samples/ios/app/glean-sample-appUITests/MockServer.swift @@ -3,6 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ import Foundation +import Gzip import Swifter // Create a new Glean endpoint HTTP server on localhost and react only for the specified ping type @@ -12,9 +13,13 @@ func mockServer(expectPingType: String, port: UInt16 = 0, callback: @escaping ([ server["/submit/:appid/:ping/:schema/:pinguuid"] = { request in let pingName = request.params[":ping"]! if pingName == expectPingType { - let body = String(bytes: request.body, encoding: .utf8)! - let data = body.data(using: .utf8)! - print("Received data: \(body)") + var data = Data(request.body) + + // Swifter lowercases all headers. + if request.headers["content-encoding"] == "gzip" { + data = try! data.gunzipped() + } + let json = try! JSONSerialization.jsonObject(with: data, options: []) as? [String: Any] callback(json) } From e8485d40b184cd6983090af7b0a2baaea702a6ee Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 18 May 2020 10:22:19 +0200 Subject: [PATCH 39/63] iOS (cleanup): Launch latest available iPhone 11 on iOS 13.x --- .circleci/config.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 50b8df572e..c82823c7ef 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -571,7 +571,7 @@ jobs: rustup target add aarch64-apple-ios x86_64-apple-ios bin/bootstrap.sh # See https://circleci.com/docs/2.0/testing-ios/#pre-starting-the-simulator - xcrun instruments -w "iPhone 11 (13.4) [" || true + xcrun instruments -w "iPhone 11 (13" || true # Store build type for use in cache key if [ -z "${CIRCLE_TAG}" ]; then echo "release" > buildtype.txt @@ -663,7 +663,7 @@ jobs: command: | rustup target add aarch64-apple-ios x86_64-apple-ios # See https://circleci.com/docs/2.0/testing-ios/#pre-starting-the-simulator - xcrun instruments -w "iPhone 11 (13.4) [" || true + xcrun instruments -w "iPhone 11 (13" || true - attach_workspace: at: . - run: From 42714ce569686a60f80823652cff8e51ed0c838b Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 18 May 2020 10:51:52 +0200 Subject: [PATCH 40/63] iOS (cleanup): Split tasks for better insight and timing --- .circleci/config.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index c82823c7ef..59aa182e04 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -683,12 +683,15 @@ jobs: sed -i.bak "/mozilla\/glean/s#.*#binary \"$JSON_URL\" ~> 0.0.1-SNAPSHOT#" "$CARTFILE_PATH" cat "${CARTFILE_PATH}" - run: - name: Build sample app + name: Bootstrap dependencies command: | # Build in Debug mode to speed it all up pushd samples/ios/app carthage bootstrap --platform iOS --cache-builds --configuration Debug --verbose popd + - run: + name: Build sample app + command: | bash bin/run-ios-sample-app-build.sh - store_artifacts: path: raw_sample_xcodebuild.log From dc60e4e17a7049737c8b9149df6d8229805d54e9 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Mon, 18 May 2020 10:55:17 +0200 Subject: [PATCH 41/63] iOS: Build dependencies in Release mode Otherwise we will get an error about missing bitcode in Gzip: > Gzip was built without full bitcode. > All object files and libraries for bitcode must be generated from Xcode Archive or Install build file. There's no proper workaround short of modifying the dependency's build system, which we don't want to do. Bitcode is huge and thus not included in Debug builds by default. It's required for App Store submissions and thus enabled in Release mode. --- .circleci/config.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 59aa182e04..0c6f718ff6 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -685,9 +685,9 @@ jobs: - run: name: Bootstrap dependencies command: | - # Build in Debug mode to speed it all up + # We need release mode to correctly ship bitcode for dependencies pushd samples/ios/app - carthage bootstrap --platform iOS --cache-builds --configuration Debug --verbose + carthage bootstrap --platform iOS --cache-builds --configuration Release --verbose popd - run: name: Build sample app From a34f5154f26d03449f7786786689ae3277105b67 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 19 May 2020 12:32:08 +0200 Subject: [PATCH 42/63] Mention iOS in the changelog [doc only] --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0428de939c..0ec676788f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ [Full changelog](https://github.com/mozilla/glean/compare/v30.0.0...master) -* Android +* Android & iOS * Ping payloads are now compressed using gzip. # v30.0.0 (2020-05-13) From 4b4dc1861016f7f7d59cc7921fcadf712155d5aa Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Tue, 19 May 2020 12:38:22 +0200 Subject: [PATCH 43/63] Fix `memory_distribution` usage in Python --- glean-core/python/glean/_loader.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glean-core/python/glean/_loader.py b/glean-core/python/glean/_loader.py index 876bc3d65a..8825663925 100644 --- a/glean-core/python/glean/_loader.py +++ b/glean-core/python/glean/_loader.py @@ -35,7 +35,7 @@ "labeled_boolean": metrics.LabeledBooleanMetricType, "labeled_counter": metrics.LabeledCounterMetricType, "labeled_string": metrics.LabeledStringMetricType, - "memory_unit": metrics.MemoryDistributionMetricType, + "memory_distribution": metrics.MemoryDistributionMetricType, "ping": metrics.PingType, "string": metrics.StringMetricType, "string_list": metrics.StringListMetricType, From e191e90e0844b791a53fddc84d54dfba4a595ccb Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Thu, 14 May 2020 20:00:16 +0200 Subject: [PATCH 44/63] Get upload tasks using an output parameter Due to the way we use CFFI in Python, we cannot use a `union` type as a return value. On the other hand, this works fine with output parameters. This commit changes the API to use output parameters to accomodate all the supported language bindings. --- glean-core/ffi/glean.h | 2 +- glean-core/ffi/src/lib.rs | 55 +++++++++++++++++++++++++++------ glean-core/ios/Glean/GleanFfi.h | 2 +- 3 files changed, 47 insertions(+), 12 deletions(-) diff --git a/glean-core/ffi/glean.h b/glean-core/ffi/glean.h index 142b288c9a..cb4ce77d7f 100644 --- a/glean-core/ffi/glean.h +++ b/glean-core/ffi/glean.h @@ -278,7 +278,7 @@ char *glean_experiment_test_get_data(FfiStr experiment_id); uint8_t glean_experiment_test_is_active(FfiStr experiment_id); -FfiPingUploadTask glean_get_upload_task(void); +void glean_get_upload_task(FfiPingUploadTask *result); /** * # Safety diff --git a/glean-core/ffi/src/lib.rs b/glean-core/ffi/src/lib.rs index f1b81aec31..d0e855d399 100644 --- a/glean-core/ffi/src/lib.rs +++ b/glean-core/ffi/src/lib.rs @@ -342,23 +342,58 @@ pub extern "C" fn glean_is_first_run() -> u8 { with_glean_value(|glean| glean.is_first_run()) } +// Unfortunately, the way we use CFFI in Python ("out-of-line", "ABI mode") does not +// allow return values to be `union`s, so we need to use an output parameter instead of +// a return value to get the task. The output data will be consumed and freed by the +// `glean_process_ping_upload_response` below. +// +// Arguments: +// +// * `result`: the object the output task will be written to. #[no_mangle] -pub extern "C" fn glean_get_upload_task() -> FfiPingUploadTask { - with_glean_value(|glean| FfiPingUploadTask::from(glean.get_upload_task())) +pub extern "C" fn glean_get_upload_task(result: *mut FfiPingUploadTask) { + with_glean_value(|glean| { + let ffi_task = FfiPingUploadTask::from(glean.get_upload_task()); + unsafe { + std::ptr::write(result, ffi_task); + } + }); } -// We need to pass the whole task instead of only the document id, -// so that we can free the strings properly on Drop. +/// Process and free a `FfiPingUploadTask`. +/// +/// We need to pass the whole task instead of only the document id, +/// so that we can free the strings properly on Drop. +/// +/// After return the `task` should not be used further by the caller. +/// +/// # Safety +/// +/// A valid and non-null upload task object is required for this function. #[no_mangle] -pub extern "C" fn glean_process_ping_upload_response(task: FfiPingUploadTask, status: u32) { +pub unsafe extern "C" fn glean_process_ping_upload_response( + task: *mut FfiPingUploadTask, + status: u32, +) { + // Safety: + // * We null-check the passed task before dereferencing. + // * We replace data behind the pointer with another valid variant. + // * We gracefully handle invalid data in strings. + if task.is_null() { + return; + } + + // Take out task and replace with valid value. + // This value should never be read again on the FFI side, + // but as it controls the memory, we put something valid in place, just in case. + let task = std::ptr::replace(task, FfiPingUploadTask::Done); + with_glean(|glean| { if let FfiPingUploadTask::Upload { document_id, .. } = task { assert!(!document_id.is_null()); - let document_id_str = unsafe { - CStr::from_ptr(document_id) - .to_str() - .map_err(|_| glean_core::Error::utf8_error()) - }?; + let document_id_str = CStr::from_ptr(document_id) + .to_str() + .map_err(|_| glean_core::Error::utf8_error())?; glean.process_ping_upload_response(document_id_str, status.into()); }; Ok(()) diff --git a/glean-core/ios/Glean/GleanFfi.h b/glean-core/ios/Glean/GleanFfi.h index 142b288c9a..cb4ce77d7f 100644 --- a/glean-core/ios/Glean/GleanFfi.h +++ b/glean-core/ios/Glean/GleanFfi.h @@ -278,7 +278,7 @@ char *glean_experiment_test_get_data(FfiStr experiment_id); uint8_t glean_experiment_test_is_active(FfiStr experiment_id); -FfiPingUploadTask glean_get_upload_task(void); +void glean_get_upload_task(FfiPingUploadTask *result); /** * # Safety From ed4a2d7f612eb1cec6b5771c69cd24a814e03f98 Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Mon, 18 May 2020 15:21:00 +0200 Subject: [PATCH 45/63] Use the new uploader API signature in Kotlin --- .../main/java/mozilla/telemetry/glean/net/Upload.kt | 2 +- .../java/mozilla/telemetry/glean/rust/LibGleanFFI.kt | 4 ++-- .../telemetry/glean/scheduler/PingUploadWorker.kt | 11 +++++------ 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/net/Upload.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/net/Upload.kt index 0f20eec750..62e3917c89 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/net/Upload.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/net/Upload.kt @@ -48,7 +48,7 @@ internal open class FfiPingUploadTask( @JvmField var tag: Byte = UploadTaskTag.Done.ordinal.toByte(), @JvmField var upload: UploadBody = UploadBody() ) : Union() { - class ByValue : FfiPingUploadTask(), Structure.ByValue + class ByReference : FfiPingUploadTask(), Structure.ByReference init { // Initialize to be the `tag`-only variant diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt index b236ec78da..91c7842686 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/rust/LibGleanFFI.kt @@ -556,9 +556,9 @@ internal interface LibGleanFFI : Library { storage_name: String ): Int - fun glean_get_upload_task(): FfiPingUploadTask.ByValue + fun glean_get_upload_task(task: FfiPingUploadTask.ByReference) - fun glean_process_ping_upload_response(task: FfiPingUploadTask.ByValue, status: Int) + fun glean_process_ping_upload_response(task: FfiPingUploadTask.ByReference, status: Int) // Misc diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/PingUploadWorker.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/PingUploadWorker.kt index 8f47716be4..db85552b63 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/PingUploadWorker.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/scheduler/PingUploadWorker.kt @@ -16,6 +16,7 @@ import androidx.work.Worker import androidx.work.WorkerParameters import mozilla.telemetry.glean.rust.LibGleanFFI import mozilla.telemetry.glean.Glean +import mozilla.telemetry.glean.net.FfiPingUploadTask import mozilla.telemetry.glean.utils.testFlushWorkManagerJob import mozilla.telemetry.glean.net.PingUploadTask @@ -55,11 +56,6 @@ class PingUploadWorker(context: Context, params: WorkerParameters) : Worker(cont @VisibleForTesting internal const val PINGS_DIR = "pending_pings" - // For this error, the ping will be retried later - internal const val RECOVERABLE_ERROR_STATUS_CODE = 500 - // For this error, the ping data will be deleted and no retry happens - internal const val UNRECOVERABLE_ERROR_STATUS_CODE = 400 - /** * Function to aid in properly enqueuing the worker in [WorkManager] * @@ -103,7 +99,10 @@ class PingUploadWorker(context: Context, params: WorkerParameters) : Worker(cont */ override fun doWork(): Result { do { - val incomingTask = LibGleanFFI.INSTANCE.glean_get_upload_task() + // Create a slot of memory for the task: glean-core will write data into + // the allocated memory. + val incomingTask = FfiPingUploadTask.ByReference() + LibGleanFFI.INSTANCE.glean_get_upload_task(incomingTask) when (val action = incomingTask.toPingUploadTask()) { is PingUploadTask.Upload -> { // Upload the ping request. From df23d63010ec121c7ab211be7f4621fc70c4d79f Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Mon, 18 May 2020 18:25:56 +0200 Subject: [PATCH 46/63] Update the C headers --- glean-core/ffi/glean.h | 14 +++++++++++++- glean-core/ios/Glean/GleanFfi.h | 14 +++++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/glean-core/ffi/glean.h b/glean-core/ffi/glean.h index cb4ce77d7f..74b3683d3a 100644 --- a/glean-core/ffi/glean.h +++ b/glean-core/ffi/glean.h @@ -475,7 +475,19 @@ uint8_t glean_on_ready_to_submit_pings(void); char *glean_ping_collect(uint64_t ping_type_handle, FfiStr reason); -void glean_process_ping_upload_response(FfiPingUploadTask task, uint32_t status); +/** + * Process and free a `FfiPingUploadTask`. + * + * We need to pass the whole task instead of only the document id, + * so that we can free the strings properly on Drop. + * + * After return the `task` should not be used further by the caller. + * + * # Safety + * + * A valid and non-null upload task object is required for this function. + */ +void glean_process_ping_upload_response(FfiPingUploadTask *task, uint32_t status); void glean_quantity_set(uint64_t metric_id, int64_t value); diff --git a/glean-core/ios/Glean/GleanFfi.h b/glean-core/ios/Glean/GleanFfi.h index cb4ce77d7f..74b3683d3a 100644 --- a/glean-core/ios/Glean/GleanFfi.h +++ b/glean-core/ios/Glean/GleanFfi.h @@ -475,7 +475,19 @@ uint8_t glean_on_ready_to_submit_pings(void); char *glean_ping_collect(uint64_t ping_type_handle, FfiStr reason); -void glean_process_ping_upload_response(FfiPingUploadTask task, uint32_t status); +/** + * Process and free a `FfiPingUploadTask`. + * + * We need to pass the whole task instead of only the document id, + * so that we can free the strings properly on Drop. + * + * After return the `task` should not be used further by the caller. + * + * # Safety + * + * A valid and non-null upload task object is required for this function. + */ +void glean_process_ping_upload_response(FfiPingUploadTask *task, uint32_t status); void glean_quantity_set(uint64_t metric_id, int64_t value); From 48c485460d54faab481a57e17c0ddb867dc85c4c Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Tue, 19 May 2020 11:02:24 +0200 Subject: [PATCH 47/63] Fix the C example --- glean-core/ffi/examples/glean_app.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/glean-core/ffi/examples/glean_app.c b/glean-core/ffi/examples/glean_app.c index 8bffd2f00f..03748e291a 100644 --- a/glean-core/ffi/examples/glean_app.c +++ b/glean-core/ffi/examples/glean_app.c @@ -41,16 +41,17 @@ int main(void) // 1 - "upload", there is a new ping to upload and the task will also include the request data; // 2 - "done", there are no more pings to upload. // - // NOTE: If, there are other ping files inside tmp/glean_data directory + // NOTE: If, there are other ping files inside tmp/glean_data directory // they will also be consumed here by `glean_process_ping_upload_response`. - FfiPingUploadTask task = glean_get_upload_task(); - while (task.tag == 1) { + FfiPingUploadTask task; + glean_get_upload_task(&task); + while (task.tag != 2) { printf("tag: %d\n", task.tag); printf("path: %s\n", task.upload.path); printf("body: %s\n", task.upload.body); - glean_process_ping_upload_response(task, 200); - task = glean_get_upload_task(); + glean_process_ping_upload_response(&task, 200); + glean_get_upload_task(&task); } printf("tag: %d\n", task.tag); From 16e08ee13a730e7e987ab740b942b6dc55a1e20e Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Tue, 19 May 2020 11:06:00 +0200 Subject: [PATCH 48/63] Make iOS use the new FFI signature --- glean-core/ios/Glean/Net/HttpPingUploader.swift | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/glean-core/ios/Glean/Net/HttpPingUploader.swift b/glean-core/ios/Glean/Net/HttpPingUploader.swift index bffaf32db0..2bb977c54c 100644 --- a/glean-core/ios/Glean/Net/HttpPingUploader.swift +++ b/glean-core/ios/Glean/Net/HttpPingUploader.swift @@ -100,13 +100,14 @@ public class HttpPingUploader { /// It will continue upload as long as it can fetch new tasks. func process() { while true { - let incomingTask = glean_get_upload_task() + var incomingTask = FfiPingUploadTask() + glean_get_upload_task(&incomingTask) let task = incomingTask.toPingUploadTask() switch task { case let .upload(request): self.upload(path: request.path, data: request.body, headers: request.headers) { result in - glean_process_ping_upload_response(incomingTask, result.toFfi()) + glean_process_ping_upload_response(&incomingTask, result.toFfi()) } case .wait: continue From 4366b0384d8e923cb6d333496502ee33e1a45fef Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Tue, 19 May 2020 11:28:28 +0200 Subject: [PATCH 49/63] Update the internal docs --- docs/dev/core/internal/upload.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/dev/core/internal/upload.md b/docs/dev/core/internal/upload.md index e1624f607c..cebbd1a575 100644 --- a/docs/dev/core/internal/upload.md +++ b/docs/dev/core/internal/upload.md @@ -62,10 +62,10 @@ For FFI consumers (e.g. Kotlin/Swift/Python implementations) these functions are ```rust /// Gets the next task for an uploader. Which can be either: -extern "C" fn glean_get_upload_task() -> FfiPingUploadTask +extern "C" fn glean_get_upload_task(result: *mut FfiPingUploadTask) /// Processes the response from an attempt to upload a ping. -extern "C" fn glean_process_ping_upload_response(task: FfiPingUploadTask, status: u32) +extern "C" fn glean_process_ping_upload_response(task: *mut FfiPingUploadTask, status: u32) ``` See the documentation for additional information about the types: From ee273e2df25312d9849b9c4b96cf09af2567e860 Mon Sep 17 00:00:00 2001 From: Jan-Erik Rediger Date: Tue, 19 May 2020 19:07:06 +0200 Subject: [PATCH 50/63] Properly check return values in C --- glean-core/ffi/examples/glean_app.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/glean-core/ffi/examples/glean_app.c b/glean-core/ffi/examples/glean_app.c index 03748e291a..e0699ea18b 100644 --- a/glean-core/ffi/examples/glean_app.c +++ b/glean-core/ffi/examples/glean_app.c @@ -45,15 +45,17 @@ int main(void) // they will also be consumed here by `glean_process_ping_upload_response`. FfiPingUploadTask task; glean_get_upload_task(&task); - while (task.tag != 2) { - printf("tag: %d\n", task.tag); - printf("path: %s\n", task.upload.path); - printf("body: %s\n", task.upload.body); - glean_process_ping_upload_response(&task, 200); - glean_get_upload_task(&task); + while (task.tag != FfiPingUploadTask_Done) { + printf("tag: %d\n", task.tag); + + if (task.tag == FfiPingUploadTask_Upload) { + printf("path: %s\n", task.upload.path); + printf("body length: %lld\n", task.upload.body.len); + + glean_process_ping_upload_response(&task, UPLOAD_RESULT_HTTP_STATUS | 200); + } } - printf("tag: %d\n", task.tag); glean_destroy_counter_metric(metric); From b9fb404ff8dad9415978e594e9dcd995555b4e24 Mon Sep 17 00:00:00 2001 From: Michael Droettboom Date: Tue, 19 May 2020 14:13:05 -0400 Subject: [PATCH 51/63] Bug 1639158: Fix Python coverage testing (#893) --- .coveragerc | 2 ++ Makefile | 2 +- .../python/glean/_subprocess/_process_dispatcher_helper.py | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) create mode 100644 .coveragerc diff --git a/.coveragerc b/.coveragerc new file mode 100644 index 0000000000..c6ce05be34 --- /dev/null +++ b/.coveragerc @@ -0,0 +1,2 @@ +[run] +source = glean \ No newline at end of file diff --git a/Makefile b/Makefile index 030904a8df..cee74ebe91 100644 --- a/Makefile +++ b/Makefile @@ -178,7 +178,7 @@ rust-coverage: ## Generate code coverage information for Rust code .PHONY: rust-coverage python-coverage: build-python ## Generate a code coverage report for Python - $(GLEAN_PYENV)/bin/python3 -m coverage run --parallel-mode -m pytest + GLEAN_COVERAGE=1 $(GLEAN_PYENV)/bin/python3 -m coverage run --parallel-mode -m pytest $(GLEAN_PYENV)/bin/python3 -m coverage combine $(GLEAN_PYENV)/bin/python3 -m coverage html .PHONY: python-coverage diff --git a/glean-core/python/glean/_subprocess/_process_dispatcher_helper.py b/glean-core/python/glean/_subprocess/_process_dispatcher_helper.py index e199d9002e..144d537557 100644 --- a/glean-core/python/glean/_subprocess/_process_dispatcher_helper.py +++ b/glean-core/python/glean/_subprocess/_process_dispatcher_helper.py @@ -18,7 +18,7 @@ import sys # Run coverage in the subprocess if necessary - if "COVERAGE_PROCESS_START" in os.environ: + if "GLEAN_COVERAGE" in os.environ and "COVERAGE_PROCESS_START" in os.environ: import coverage # type: ignore config_path = os.environ.get("COVERAGE_PROCESS_START") From 6960ef5b1edc138e6c8f69ee25bd160817da3b46 Mon Sep 17 00:00:00 2001 From: Chris H-C Date: Tue, 19 May 2020 17:07:24 -0400 Subject: [PATCH 52/63] Remove `ping_type` from `ping_info` docs It was removed in bug 1609149 --- docs/user/pings/index.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/user/pings/index.md b/docs/user/pings/index.md index 1d127441a4..e6094778d2 100644 --- a/docs/user/pings/index.md +++ b/docs/user/pings/index.md @@ -28,7 +28,6 @@ Optional fields are marked accordingly. | Field name | Type | Description | |---|---|---| -| `ping_type` | String | The name of the ping type (e.g. "baseline", "metrics") | | `seq` | Counter | A running counter of the number of times pings of this type have been sent | | `experiments` | Object | *Optional*. A dictionary of [active experiments](#the-experiments-object) | | `start_time` | Datetime | The time of the start of collection of the data in the ping, in local time and with minute precision, including timezone information. | From bb7c1a26917e82dc9ca1612b9d1e229826e29780 Mon Sep 17 00:00:00 2001 From: Travis Long Date: Tue, 19 May 2020 11:49:34 -0500 Subject: [PATCH 53/63] Bug 1637990 - Prevent Glean.initialize being called from iOS extensions --- CHANGELOG.md | 2 ++ glean-core/ios/Glean/Glean.swift | 30 ++++++++++++++++++++++ glean-core/ios/GleanTests/GleanTests.swift | 15 +++++++++++ 3 files changed, 47 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ec676788f..64aa187fb4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ * Android & iOS * Ping payloads are now compressed using gzip. +* iOS + * `Glean.initialize` is now a no-op if called from an embedded extension. This means that Glean will only run in the base application process in order to prevent extensions from behaving like separate applications with different client ids from the base application. Applications are responsible for ensuring that extension metrics are only collected within the base application. # v30.0.0 (2020-05-13) diff --git a/glean-core/ios/Glean/Glean.swift b/glean-core/ios/Glean/Glean.swift index f2708639fc..6d98b9f61c 100644 --- a/glean-core/ios/Glean/Glean.swift +++ b/glean-core/ios/Glean/Glean.swift @@ -39,6 +39,10 @@ public class Glean { private let logger = Logger(tag: Constants.logTag) + // Cache variable for checking if running in main process. Also used to override for tests in + // order to simulate not running in the main process. DO NOT SET EXCEPT IN TESTS! + var isMainProcess: Bool? + private init() { // intentionally left private, no external user can instantiate a new global object. @@ -50,6 +54,7 @@ public class Glean { self.initialized = false } + // swiftlint:disable function_body_length /// Initialize the Glean SDK. /// /// This should only be initialized once by the application, and not by @@ -67,6 +72,14 @@ public class Glean { /// * configuration: A Glean `Configuration` object with global settings. public func initialize(uploadEnabled: Bool, configuration: Configuration = Configuration()) { + // In certain situations Glean.initialize may be called from a process other than the main + // process such as an embedded extension. In this case we want to just return. + // See https://bugzilla.mozilla.org/show_bug.cgi?id=1625157 for more information. + if !checkIsMainProcess() { + logger.error("Attempted to initialize Glean on a process other than the main process") + return + } + if self.isInitialized() { logger.error("Glean should not be initialized multiple times") return @@ -156,6 +169,8 @@ public class Glean { } } + // swiftlint:enable function_body_length + /// Initialize the core metrics internally managed by Glean (e.g. client id). private func initializeCoreMetrics() { // Set a few more metrics that will be sent as part of every ping. @@ -452,6 +467,21 @@ public class Glean { GleanDebugUtility.handleCustomUrl(url: url) } + /// Returns true if running in the base application process, otherwise returns false + private func checkIsMainProcess() -> Bool { + if isMainProcess == nil { + if let packageType = Bundle.main.object(forInfoDictionaryKey: "CFBundlePackageType") as? String { + isMainProcess = packageType != "XPC!" + } else { + // The `CFBundlePackageType` doesn't get set in tests so we return true to indicate that we are + // running in the main process. + isMainProcess = true + } + } + + return isMainProcess! + } + /// PUBLIC TEST ONLY FUNCTION. /// /// Returns true if a ping by this name is in the ping registry. diff --git a/glean-core/ios/GleanTests/GleanTests.swift b/glean-core/ios/GleanTests/GleanTests.swift index a9474a8f7d..93ba6e44a9 100644 --- a/glean-core/ios/GleanTests/GleanTests.swift +++ b/glean-core/ios/GleanTests/GleanTests.swift @@ -277,4 +277,19 @@ class GleanTests: XCTestCase { XCTAssertNil(error, "Test timed out waiting for upload: \(error!)") } } + + func testGleanIsNotInitializedFromOtherProcesses() { + // Check to see if Glean is initialized + XCTAssert(Glean.shared.isInitialized()) + + // Set the control variable to false to simulate that we are not running + // in the main process + Glean.shared.isMainProcess = false + + // Restart glean + Glean.shared.resetGlean(clearStores: false) + + // Check to see if Glean is initialized + XCTAssertFalse(Glean.shared.isInitialized()) + } } From 94e4722bbb632e075310a6f5a37c20fe8ae309d7 Mon Sep 17 00:00:00 2001 From: Travis Long Date: Tue, 19 May 2020 15:22:01 -0500 Subject: [PATCH 54/63] Bug 1637987 - Document the limitations of using Glean with iOS application extensions --- docs/user/adding-glean-to-your-project.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/user/adding-glean-to-your-project.md b/docs/user/adding-glean-to-your-project.md index 8f859c9b58..dc5c290fb5 100644 --- a/docs/user/adding-glean-to-your-project.md +++ b/docs/user/adding-glean-to-your-project.md @@ -229,6 +229,8 @@ Follow these steps to automatically run the parser at build time: This will ignore files that are generated at build time by the `sdk_generator.sh` script. They don't need to be kept in version control, as they can be re-generated from your `metrics.yaml` and `pings.yaml` files. +> **Important information about Glean and embedded extensions:** Applications for iOS have the ability to define [application extensions](https://developer.apple.com/library/archive/documentation/General/Conceptual/ExtensibilityPG/ExtensionOverview.html#//apple_ref/doc/uid/TP40014214-CH2-SW2) which, while they are contained within the application, do not have access to the same data or APIs, and even run in different processes. Since the extension runs in a separate sandbox from the application, Glean would run in the extension as if it were a completely separate application with different client ids and storage. This complicates the use of Glean because Glean doesn’t know or care about other processes. Because of this, Glean is purposefully prevented from running in an application extension and if metrics are to be collected from extensions, it's up to the integrating application to pass the information to the base application to record in Glean. +
From e8f6d921844eba629cb2165647574d46095add0d Mon Sep 17 00:00:00 2001 From: Travis Long Date: Wed, 20 May 2020 09:01:47 -0500 Subject: [PATCH 55/63] Add better comments to document use of "XPC!" for package type detection --- glean-core/ios/Glean/Glean.swift | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/glean-core/ios/Glean/Glean.swift b/glean-core/ios/Glean/Glean.swift index 6d98b9f61c..b606cc22ef 100644 --- a/glean-core/ios/Glean/Glean.swift +++ b/glean-core/ios/Glean/Glean.swift @@ -471,6 +471,16 @@ public class Glean { private func checkIsMainProcess() -> Bool { if isMainProcess == nil { if let packageType = Bundle.main.object(forInfoDictionaryKey: "CFBundlePackageType") as? String { + // This is the bundle type reported by embedded application extensions and so we test for it to + // make sure that we are not running in an extension. + // + // For more info on XPC services see: + // https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/CreatingXPCServices.html + // + // For more info on CFBundlePackageType see: + // https://developer.apple.com/documentation/bundleresources/information_property_list/cfbundlepackagetype + // and + // https://developer.apple.com/library/archive/documentation/General/Reference/InfoPlistKeyReference/Articles/CoreFoundationKeys.html#//apple_ref/doc/uid/20001431-111321 isMainProcess = packageType != "XPC!" } else { // The `CFBundlePackageType` doesn't get set in tests so we return true to indicate that we are From c8bb6ebf7a4b7dba43c53ec95326dd0efe062e64 Mon Sep 17 00:00:00 2001 From: Travis Long Date: Thu, 21 May 2020 07:35:11 -0500 Subject: [PATCH 56/63] Reword note about iOS extensions --- docs/user/adding-glean-to-your-project.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/user/adding-glean-to-your-project.md b/docs/user/adding-glean-to-your-project.md index dc5c290fb5..8fe2638ea4 100644 --- a/docs/user/adding-glean-to-your-project.md +++ b/docs/user/adding-glean-to-your-project.md @@ -229,7 +229,7 @@ Follow these steps to automatically run the parser at build time: This will ignore files that are generated at build time by the `sdk_generator.sh` script. They don't need to be kept in version control, as they can be re-generated from your `metrics.yaml` and `pings.yaml` files. -> **Important information about Glean and embedded extensions:** Applications for iOS have the ability to define [application extensions](https://developer.apple.com/library/archive/documentation/General/Conceptual/ExtensibilityPG/ExtensionOverview.html#//apple_ref/doc/uid/TP40014214-CH2-SW2) which, while they are contained within the application, do not have access to the same data or APIs, and even run in different processes. Since the extension runs in a separate sandbox from the application, Glean would run in the extension as if it were a completely separate application with different client ids and storage. This complicates the use of Glean because Glean doesn’t know or care about other processes. Because of this, Glean is purposefully prevented from running in an application extension and if metrics are to be collected from extensions, it's up to the integrating application to pass the information to the base application to record in Glean. +> **Important information about Glean and embedded extensions:** Metric collection is a no-op in [application extensions](https://developer.apple.com/library/archive/documentation/General/Conceptual/ExtensibilityPG/ExtensionOverview.html#//apple_ref/doc/uid/TP40014214-CH2-SW2) and Glean will not run. Since extensions run in a separate sandbox and process from the application, Glean would run in an extension as if it were a completely separate application with different client ids and storage. This complicates things because Glean doesn’t know or care about other processes. Because of this, Glean is purposefully prevented from running in an application extension and if metrics need to be collected from extensions, it's up to the integrating application to pass the information to the base application to record in Glean.
From cfdb3b2fe90e935c062fce43bd92dddc8f03faec Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Thu, 21 May 2020 16:00:26 +0200 Subject: [PATCH 57/63] Document the max number of keys for `extra_keys` --- docs/user/metrics/event.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/user/metrics/event.md b/docs/user/metrics/event.md index 79e31365dc..2fba68098a 100644 --- a/docs/user/metrics/event.md +++ b/docs/user/metrics/event.md @@ -119,6 +119,8 @@ assert 0 == metrics.views.login_opened.test_get_num_recorded_errors( * When 500 events are queued on the client an events ping is immediately sent. +* The `extra_keys` allows for a maximum of 10 keys. + * The keys in the `extra_keys` list must be in dotted snake case, with a maximum length of 40 bytes in UTF-8. * The values in the `extras` object have a maximum length of 50 in UTF-8. From 2cacbe6ab6fa432b66a7bdaa0e71a4bb5b7a25e5 Mon Sep 17 00:00:00 2001 From: Michael Droettboom Date: Thu, 21 May 2020 10:49:17 -0400 Subject: [PATCH 58/63] Bug 1635865: Fix ConcurrentModificationException (#900) * Bug 1635865: Fix ConcurrentModificationException * Add CHANGELOG * Call registerPingType instead of calling FFI directly * LINT --- .dictionary | 2 +- CHANGELOG.md | 2 + .../java/mozilla/telemetry/glean/Glean.kt | 14 ++-- .../java/mozilla/telemetry/glean/GleanTest.kt | 79 +++++++++++++++++-- 4 files changed, 81 insertions(+), 16 deletions(-) diff --git a/.dictionary b/.dictionary index a34bfa61d3..4ce0173000 100644 --- a/.dictionary +++ b/.dictionary @@ -1,4 +1,4 @@ -personal_ws-1.1 en 172 utf-8 +personal_ws-1.1 en 173 utf-8 AAR AARs APIs diff --git a/CHANGELOG.md b/CHANGELOG.md index 64aa187fb4..aadf6b54ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ [Full changelog](https://github.com/mozilla/glean/compare/v30.0.0...master) +* Android + * BUGFIX: Fix a race condition that leads to a `ConcurrentModificationException`. [Bug 1635865](https://bugzilla.mozilla.org/1635865) * Android & iOS * Ping payloads are now compressed using gzip. * iOS diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt index fb4d58de81..fcb81f8b5b 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt @@ -79,7 +79,7 @@ open class GleanInternalAPI internal constructor () { internal lateinit var metricsPingScheduler: MetricsPingScheduler // Keep track of ping types that have been registered before Glean is initialized. - private val pingTypeQueue: MutableSet = mutableSetOf() + internal val pingTypeQueue: MutableSet = mutableSetOf() // This is used to cache the process state and is used by the function `isMainProcess()` @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) @@ -166,7 +166,9 @@ open class GleanInternalAPI internal constructor () { // If any pings were registered before initializing, do so now. // We're not clearing this queue in case Glean is reset by tests. - pingTypeQueue.forEach { registerPingType(it) } + synchronized(this@GleanInternalAPI) { + pingTypeQueue.forEach { registerPingType(it) } + } // If this is the first time ever the Glean SDK runs, make sure to set // some initial core metrics in case we need to generate early pings. @@ -666,13 +668,7 @@ open class GleanInternalAPI internal constructor () { // but not the whole process, meaning globals, such as the ping types, still exist from the old run. // It's a set and keeping them around forever should not have much of an impact. - // This function is called from `Glean.initialize` while iterating over pingTypeQueue. - // Only add if it's not already there to avoid a - // ConcurrentModificationException on Android 5. - // See https://bugzilla.mozilla.org/show_bug.cgi?id=1635865 - if (!pingTypeQueue.contains(pingType)) { - pingTypeQueue.add(pingType) - } + pingTypeQueue.add(pingType) } /** diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt index 93ab25f080..8918b9c309 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/GleanTest.kt @@ -5,15 +5,17 @@ package mozilla.telemetry.glean import android.content.Context -import android.os.Build import androidx.lifecycle.Lifecycle import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.LifecycleRegistry import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import kotlinx.coroutines.Dispatchers as KotlinDispatchers +import kotlinx.coroutines.GlobalScope +import kotlinx.coroutines.launch import kotlinx.coroutines.ObsoleteCoroutinesApi import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout import mozilla.telemetry.glean.GleanMetrics.GleanError import mozilla.telemetry.glean.GleanMetrics.GleanInternalMetrics import mozilla.telemetry.glean.GleanMetrics.Pings @@ -47,7 +49,6 @@ import org.junit.runner.RunWith import org.mockito.Mockito.mock import org.mockito.Mockito.spy import org.robolectric.shadows.ShadowProcess -import org.robolectric.annotation.Config import java.io.BufferedReader import java.io.File import java.io.FileReader @@ -757,13 +758,79 @@ class GleanTest { } @Test - @Config(sdk = [Build.VERSION_CODES.O]) - fun `Initialize registering pings shouldn't crash with Oreo`() { - // Can't use resetGlean directly + fun `Initializing while registering pings isn't a race condition`() { + // See bug 1635865 + Glean.testDestroyGleanHandle() - val config = Configuration() + // We need a signal for when "initialization is done", and one doesn't + // really exist. For that, this sets a StringMetric on the pre-init task queue and + // then waits for the task queue to be empty. + + Dispatchers.API.setTaskQueueing(true) + Dispatchers.API.setTestingMode(false) + + val stringMetric = StringMetricType( + disabled = false, + category = "telemetry", + lifetime = Lifetime.Application, + name = "string_metric", + sendInPings = listOf("store1") + ) + stringMetric.set("foo") + // Add a bunch of ping types to the pingTypeQueue. We need many in here so + // that registering pings during initialization is slow enough that we can + // get other pings to be registered at the same time from another thread. + + // However, we don't want to add them to the queue and leave them there for + // other tests (that makes the whole testing suite slower), so we first take + // a copy of the current state, and restore it at the end of this test. + + val pingTypeQueueInitialState = HashSet(Glean.pingTypeQueue) + + for (i in 1..1000) { + val ping = PingType( + name = "race-condition-ping$i", + includeClientId = true, + sendIfEmpty = false, + reasonCodes = listOf() + ) + Glean.registerPingType(ping) + } + + // Initialize Glean. This will do most of the Glean initialization in the main + // Glean coroutine in Dispatchers.API. + val config = Configuration() Glean.initialize(context, true, config) + + // From another coroutine, just register pings as fast as we can to simulate the + // ping registration race condition. Do this until any queued tasks in Glean are + // complete (which signals the end of initialization). After that, restore the + // pingTypeQueue state. + runBlocking { + GlobalScope.launch { + val ping = PingType( + name = "race-condition-ping", + includeClientId = true, + sendIfEmpty = false, + reasonCodes = listOf() + ) + + // This timeout will fail and thereby fail the unit test if the + // Glean.initialize coroutine crashes. + withTimeout(500) { + while (Dispatchers.API.taskQueue.size > 0) { + Glean.registerPingType(ping) + } + } + }.join() + } + + // Restore the initial state of the pingTypeQueue + Glean.pingTypeQueue.clear() + for (it in pingTypeQueueInitialState) { + Glean.pingTypeQueue.add(it) + } } } From 9aff6511025ffed771dc36d03da0726100921d4c Mon Sep 17 00:00:00 2001 From: Travis Long Date: Thu, 21 May 2020 09:59:52 -0500 Subject: [PATCH 59/63] Fix iOS tests --- glean-core/ios/GleanTests/GleanTests.swift | 3 +++ 1 file changed, 3 insertions(+) diff --git a/glean-core/ios/GleanTests/GleanTests.swift b/glean-core/ios/GleanTests/GleanTests.swift index 93ba6e44a9..1ceed83155 100644 --- a/glean-core/ios/GleanTests/GleanTests.swift +++ b/glean-core/ios/GleanTests/GleanTests.swift @@ -291,5 +291,8 @@ class GleanTests: XCTestCase { // Check to see if Glean is initialized XCTAssertFalse(Glean.shared.isInitialized()) + + // Reset variable so as to not interfere with other tests. + Glean.shared.isMainProcess = true } } From 10899f83da4ab5ee66389daf23c05a1e84191e51 Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Thu, 21 May 2020 17:20:31 +0200 Subject: [PATCH 60/63] Clear `lifetime: application` metrics in the Python bindings This makes the Python bindings have the same semantics of the other bindings, i.e. application lifetime metrics are cleared on the next startup after the Glean-owned pings are generated. --- glean-core/python/glean/glean.py | 18 ++++++++++++-- glean-core/python/tests/test_glean.py | 35 +++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/glean-core/python/glean/glean.py b/glean-core/python/glean/glean.py index cd9c489668..b36c8f87e0 100644 --- a/glean-core/python/glean/glean.py +++ b/glean-core/python/glean/glean.py @@ -150,8 +150,12 @@ def initialize( for ping in cls._ping_type_queue: cls.register_ping_type(ping) - # Initialize the core metrics - cls._initialize_core_metrics() + # If this is the first time ever the Glean SDK runs, make sure to set + # some initial core metrics in case we need to generate early pings. + # The next times we start, we would have them around already. + is_first_run = _ffi.lib.glean_is_first_run() != 0 + if is_first_run: + cls._initialize_core_metrics() # Glean Android sets up the metrics ping scheduler here, but we don't # have one. @@ -162,6 +166,16 @@ def submit_pending_events(): if _ffi.lib.glean_on_ready_to_submit_pings(): PingUploadWorker.process() + # Glean Android checks for the "dirty bit" and sends the `baseline` ping + # with reason `dirty_startup`. + + # From the second time we run, after all startup pings are generated, + # make sure to clear `lifetime: application` metrics and set them again. + # Any new value will be sent in newly generated pings after startup. + if not is_first_run: + _ffi.lib.glean_clear_application_lifetime_metrics() + cls._initialize_core_metrics() + Dispatcher.flush_queued_initial_tasks() # Glean Android sets up the lifecycle observer here. We don't really diff --git a/glean-core/python/tests/test_glean.py b/glean-core/python/tests/test_glean.py index 0ffa28631b..86496cd2b1 100644 --- a/glean-core/python/tests/test_glean.py +++ b/glean-core/python/tests/test_glean.py @@ -594,3 +594,38 @@ def broken_process(*args, **kwargs): assert process.returncode == 0 assert 1 == len(safe_httpserver.requests) + + +def test_clear_application_lifetime_metrics(tmpdir): + Glean._reset() + + Glean.initialize( + application_id=GLEAN_APP_ID, + application_version=glean_version, + upload_enabled=True, + data_dir=Path(str(tmpdir)), + ) + + counter_metric = CounterMetricType( + disabled=False, + category="test.telemetry", + lifetime=Lifetime.APPLICATION, + name="lifetime_reset", + send_in_pings=["store1"], + ) + + counter_metric.add(10) + + assert counter_metric.test_has_value() + assert counter_metric.test_get_value() == 10 + + Glean._reset() + + Glean.initialize( + application_id=GLEAN_APP_ID, + application_version=glean_version, + upload_enabled=True, + data_dir=Path(str(tmpdir)), + ) + + assert not counter_metric.test_has_value() From 1d5875ee93e48bb654c074b4c6511524070b3099 Mon Sep 17 00:00:00 2001 From: Alessio Placitelli Date: Thu, 21 May 2020 17:24:54 +0200 Subject: [PATCH 61/63] Add a changelog entry --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index aadf6b54ce..ff7e7dfeda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ * Ping payloads are now compressed using gzip. * iOS * `Glean.initialize` is now a no-op if called from an embedded extension. This means that Glean will only run in the base application process in order to prevent extensions from behaving like separate applications with different client ids from the base application. Applications are responsible for ensuring that extension metrics are only collected within the base application. +* Python + * `lifetime: application` metrics are now cleared after the Glean-owned pings are sent, + after the product starts. # v30.0.0 (2020-05-13) From d1ac4a9ae0d3d250fd8c4e99a6447824a5822841 Mon Sep 17 00:00:00 2001 From: Michael Droettboom Date: Fri, 22 May 2020 14:29:52 -0400 Subject: [PATCH 62/63] Clean up CHANGELOG --- CHANGELOG.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff7e7dfeda..2a7b63734f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,6 @@ [Full changelog](https://github.com/mozilla/glean/compare/v30.0.0...master) -* Android - * BUGFIX: Fix a race condition that leads to a `ConcurrentModificationException`. [Bug 1635865](https://bugzilla.mozilla.org/1635865) * Android & iOS * Ping payloads are now compressed using gzip. * iOS @@ -11,6 +9,9 @@ * Python * `lifetime: application` metrics are now cleared after the Glean-owned pings are sent, after the product starts. + * Glean Python bindings now build in a native Windows environment. + * BUGFIX: `MemoryDistributionMetric` now parses correctly in `metrics.yaml` files. + * BUGFIX: Glean will no longer crash if run as part of another library's coverage testing. # v30.0.0 (2020-05-13) @@ -28,6 +29,13 @@ * iOS: * Refactor the ping uploader to use the new upload mechanism. +# v29.1.1 (2020-05-22) + +[Full changelog](https://github.com/mozilla/glean/compare/v29.1.0...v29.1.1) + +* Android + * BUGFIX: Fix a race condition that leads to a `ConcurrentModificationException`. [Bug 1635865](https://bugzilla.mozilla.org/1635865) + # v29.1.0 (2020-05-11) [Full changelog](https://github.com/mozilla/glean/compare/v29.0.0...v29.1.0) @@ -119,7 +127,7 @@ were unintentionally public, have been made private. * Most Glean work and I/O is now done on its own worker thread. This brings the parallelism Python in line with the other platforms. * The timing distribution, memory distribution, string list, labeled boolean and labeled string metric types are now supported in Python ([#762](https://github.com/mozilla/glean/pull/762), [#763](https://github.com/mozilla/glean/pull/763), [#765](https://github.com/mozilla/glean/pull/765), [#766](https://github.com/mozilla/glean/pull/766)) - + # v25.1.0 (2020-02-26) [Full changelog](https://github.com/mozilla/glean/compare/v25.0.0...v25.1.0) From 6bce0670cccc00bbadc38affb6b4d8546cb89a75 Mon Sep 17 00:00:00 2001 From: Michael Droettboom Date: Fri, 22 May 2020 14:52:34 -0400 Subject: [PATCH 63/63] Bumped version to 30.1.0 --- .buildconfig.yml | 2 +- CHANGELOG.md | 6 +++++- Cargo.lock | 10 +++++----- glean-core/Cargo.toml | 2 +- glean-core/ffi/Cargo.toml | 4 ++-- glean-core/preview/Cargo.toml | 2 +- .../glean-gradle-plugin/GleanGradlePlugin.groovy | 2 +- 7 files changed, 16 insertions(+), 12 deletions(-) diff --git a/.buildconfig.yml b/.buildconfig.yml index e3799ec33e..0ac516e4a7 100644 --- a/.buildconfig.yml +++ b/.buildconfig.yml @@ -1,4 +1,4 @@ -libraryVersion: 30.0.0 +libraryVersion: 30.1.0 groupId: org.mozilla.telemetry projects: glean: diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a7b63734f..afc5967830 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,10 @@ # Unreleased changes -[Full changelog](https://github.com/mozilla/glean/compare/v30.0.0...master) +[Full changelog](https://github.com/mozilla/glean/compare/v30.1.0...master) + +# v30.1.0 (2020-05-22) + +[Full changelog](https://github.com/mozilla/glean/compare/v30.0.0...v30.1.0) * Android & iOS * Ping payloads are now compressed using gzip. diff --git a/Cargo.lock b/Cargo.lock index ad60b576f0..00b14a8e26 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -46,7 +46,7 @@ name = "benchmark" version = "0.1.0" dependencies = [ "criterion 0.3.1 (registry+https://github.com/rust-lang/crates.io-index)", - "glean-core 30.0.0", + "glean-core 30.1.0", "tempfile 3.1.0 (registry+https://github.com/rust-lang/crates.io-index)", ] @@ -312,7 +312,7 @@ dependencies = [ [[package]] name = "glean-core" -version = "30.0.0" +version = "30.1.0" dependencies = [ "bincode 1.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "chrono 0.4.10 (registry+https://github.com/rust-lang/crates.io-index)", @@ -333,12 +333,12 @@ dependencies = [ [[package]] name = "glean-ffi" -version = "30.0.0" +version = "30.1.0" dependencies = [ "android_logger 0.8.6 (registry+https://github.com/rust-lang/crates.io-index)", "env_logger 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)", "ffi-support 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", - "glean-core 30.0.0", + "glean-core 30.1.0", "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", "once_cell 1.3.1 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.48 (registry+https://github.com/rust-lang/crates.io-index)", @@ -350,7 +350,7 @@ name = "glean-preview" version = "0.0.5" dependencies = [ "env_logger 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)", - "glean-core 30.0.0", + "glean-core 30.1.0", "jsonschema-valid 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)", "once_cell 1.3.1 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/glean-core/Cargo.toml b/glean-core/Cargo.toml index 6aeb73808d..948b370bfd 100644 --- a/glean-core/Cargo.toml +++ b/glean-core/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "glean-core" -version = "30.0.0" +version = "30.1.0" authors = ["Jan-Erik Rediger ", "The Glean Team "] description = "A modern Telemetry library" repository = "https://github.com/mozilla/glean" diff --git a/glean-core/ffi/Cargo.toml b/glean-core/ffi/Cargo.toml index 463cdc2b3e..c40e97b357 100644 --- a/glean-core/ffi/Cargo.toml +++ b/glean-core/ffi/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "glean-ffi" -version = "30.0.0" +version = "30.1.0" authors = ["Jan-Erik Rediger ", "The Glean Team "] description = "FFI layer for Glean, a modern Telemetry library" repository = "https://github.com/mozilla/glean" @@ -35,7 +35,7 @@ once_cell = "1.2.0" [dependencies.glean-core] path = ".." -version = "30.0.0" +version = "30.1.0" [target.'cfg(target_os = "android")'.dependencies] android_logger = { version = "0.8.6", default-features = false } diff --git a/glean-core/preview/Cargo.toml b/glean-core/preview/Cargo.toml index 211ab582a4..06711a8b5c 100644 --- a/glean-core/preview/Cargo.toml +++ b/glean-core/preview/Cargo.toml @@ -23,7 +23,7 @@ maintenance = { status = "actively-developed" } [dependencies.glean-core] path = ".." -version = "30.0.0" +version = "30.1.0" [dependencies] once_cell = "1.2.0" diff --git a/gradle-plugin/src/main/groovy/mozilla/telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy b/gradle-plugin/src/main/groovy/mozilla/telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy index 80ccf25c4a..e06d8b35b8 100644 --- a/gradle-plugin/src/main/groovy/mozilla/telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy +++ b/gradle-plugin/src/main/groovy/mozilla/telemetry/glean-gradle-plugin/GleanGradlePlugin.groovy @@ -369,7 +369,7 @@ subprocess.check_call([ } void apply(Project project) { - project.ext.glean_version = "30.0.0" + project.ext.glean_version = "30.1.0" File condaDir = setupPythonEnvironmentTasks(project) project.ext.set("gleanCondaDir", condaDir)