diff --git a/.dictionary b/.dictionary index 9eedaa12c..0ba09685b 100644 --- a/.dictionary +++ b/.dictionary @@ -1,4 +1,4 @@ -personal_ws-1.1 en 215 utf-8 +personal_ws-1.1 en 216 utf-8 AAR AARs ABI @@ -140,6 +140,7 @@ hotfix html illumos init +inlined integrations io ios diff --git a/CHANGELOG.md b/CHANGELOG.md index c5bc6f893..233633aba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ [Full changelog](https://github.com/mozilla/glean/compare/v36.0.0...main) +* Android + * BUGFIX: `TimespanMetricType.measure` and `TimingDistributionMetricType.measure` won't get inlined anymore ([#1560](https://github.com/mozilla/glean/pull/1560)). + This avoids a potential bug where a `return` used inside the closure would end up not measuring the time. + Use `return@measure ` for early returns. + # v36.0.0 (2021-03-16) [Full changelog](https://github.com/mozilla/glean/compare/v35.0.0...v36.0.0) diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/TimespanMetricType.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/TimespanMetricType.kt index 8e9df1230..c7afec5c4 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/TimespanMetricType.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/TimespanMetricType.kt @@ -94,7 +94,7 @@ class TimespanMetricType internal constructor( * If the measured function throws, the measurement is canceled and the exception rethrown. */ @Suppress("TooGenericExceptionCaught") - inline fun measure(funcToMeasure: () -> U): U { + fun measure(funcToMeasure: () -> U): U { start() val returnValue = try { diff --git a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/TimingDistributionMetricType.kt b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/TimingDistributionMetricType.kt index 1d2e20461..6dd2b4364 100644 --- a/glean-core/android/src/main/java/mozilla/telemetry/glean/private/TimingDistributionMetricType.kt +++ b/glean-core/android/src/main/java/mozilla/telemetry/glean/private/TimingDistributionMetricType.kt @@ -113,7 +113,7 @@ class TimingDistributionMetricType internal constructor( * If the measured function throws, the measurement is canceled and the exception rethrown. */ @Suppress("TooGenericExceptionCaught") - inline fun measure(funcToMeasure: () -> U): U { + fun measure(funcToMeasure: () -> U): U { val timerId = start() val returnValue = try { diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/TimespanMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/TimespanMetricTypeTest.kt index dd2ef68b4..0237f35d4 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/TimespanMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/TimespanMetricTypeTest.kt @@ -288,6 +288,36 @@ class TimespanMetricTypeTest { assertTrue("Metric value must be greater than zero", metric.testGetValue() >= 0) } + @Test + fun `measure function does not change behavior with early return`() { + val metric = TimespanMetricType( + disabled = false, + category = "telemetry", + lifetime = Lifetime.Ping, + name = "inlined", + sendInPings = listOf("store1"), + timeUnit = TimeUnit.Nanosecond + ) + + // We define a function that measures the whole function call runtime + fun testFunc(): Long = metric.measure { + // We want to simulate an early return. + if (true) { + // Blank 'return' is not allowed here, because `measure` is not inlined. + // We can return by label though. + return@measure 17 + } + + 42 + } + + val res = testFunc() + assertEquals("Test value must match", 17, res) + + assertTrue("Metric must have a value", metric.testHasValue()) + assertTrue("Metric value must be greater than zero", metric.testGetValue() >= 0) + } + @Test fun `measure function bubbles up exceptions and timing is canceled`() { // Define a timespan metric, which will be stored in "store1" diff --git a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/TimingDistributionMetricTypeTest.kt b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/TimingDistributionMetricTypeTest.kt index 42cd74cfa..2deb3265b 100644 --- a/glean-core/android/src/test/java/mozilla/telemetry/glean/private/TimingDistributionMetricTypeTest.kt +++ b/glean-core/android/src/test/java/mozilla/telemetry/glean/private/TimingDistributionMetricTypeTest.kt @@ -280,6 +280,44 @@ class TimingDistributionMetricTypeTest { assertEquals(1L, snapshot.values[3]) } + @Test + fun `measure function does not change behavior with early return`() { + val metric = spy(TimingDistributionMetricType( + disabled = false, + category = "telemetry", + lifetime = Lifetime.Ping, + name = "inlined", + sendInPings = listOf("store1"), + timeUnit = TimeUnit.Nanosecond + )) + + // We define a function that measures the whole function call runtime + fun testFunc(): Long = metric.measure { + // Stop should call `getElapsedTimeNanos` again, + // so we give it a later timestamp + `when`(metric.getElapsedTimeNanos()).thenReturn(10L) + + // We want to simulate an early return. + if (true) { + // Blank 'return' is not allowed here, because `measure` is not inlined. + // We can return by label though. + return@measure 17 + } + + 42 + } + + // We start time at `0` + `when`(metric.getElapsedTimeNanos()).thenReturn(0L) + + val res = testFunc() + assertEquals("Test value must match", 17, res) + + assertTrue("Metric must have a value", metric.testHasValue()) + val snapshot = metric.testGetValue() + assertEquals("Should have stored 10 nanoseconds", 10L, snapshot.sum) + } + @Test fun `measure function bubbles up exceptions and timing is canceled`() { val metric = TimingDistributionMetricType(