1476689 - Use GeckoResult in RuntimeTelemetry

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P3)

Description

[Eugen Sawin [:esawin]]

RuntimeTelemetry.getSnapshots is still based on GeckoResponse, we should transition it to use GeckoResult instead.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1476689-1.0-Use-GeckoResult-in-runtime-telemetry.patch

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1476689-2.0-Add-basic-runtime-telemetry-test.-r-.patch

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1476689-1.0-Use-GeckoResult-in-runtime-telemetry.patch

Missed updating the docs.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8993047 [details] [diff] [review]
0001-Bug-1476689-1.0-Use-GeckoResult-in-runtime-telemetry.patch

Review of attachment 8993047 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/RuntimeTelemetry.java
@@ +50,2 @@
>          mEventDispatcher.dispatch("GeckoView:TelemetrySnapshots", msg,
>              new EventCallback() {

Use `GeckoSession.CallbackResult<GeckoBundle>` here

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8993045 [details] [diff] [review]
0002-Bug-1476689-2.0-Add-basic-runtime-telemetry-test.-r-.patch

Review of attachment 8993045 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt
@@ +124,5 @@
> +                return null
> +            }
> +        })
> +
> +        sessionRule.waitForResult(result)

You should do

> val result = sessionRule.waitForResult(telemetry.getSnapshots(true))

and then assert on `result` directly, i.e.

> assertThat("Histograms should not be null", result?.get("histograms"), notNullValue())

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1476689-1.1-Use-GeckoResult-in-runtime-telemetry.patch

Comment 7

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1476689-2.1-Add-basic-runtime-telemetry-test.-r-.patch

Comment 8

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8993625 [details] [diff] [review]
0001-Bug-1476689-1.1-Use-GeckoResult-in-runtime-telemetry.patch

Review of attachment 8993625 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8993626 [details] [diff] [review]
0002-Bug-1476689-2.1-Add-basic-runtime-telemetry-test.-r-.patch

Review of attachment 8993626 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt
@@ +4,5 @@
>  
>  package org.mozilla.geckoview.test
>  
>  import org.mozilla.geckoview.GeckoResult
> +import org.mozilla.gecko.util.GeckoBundle

Move to above `import org.mozilla.geckoview.GeckoResult`

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0048726ae54
[1.1] Use GeckoResult in runtime telemetry API. r=droeh,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/93a71fc8a13d
[2.2] Add basic runtime telemetry test. r=jchen

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d0048726ae54
https://hg.mozilla.org/mozilla-central/rev/93a71fc8a13d

Comment 12

[Eugen Sawin [:esawin]]

This is blocking the bug 1479026 uplift.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you request uplift here? Thanks.

Comment 14

[Eugen Sawin [:esawin]]

Comment on attachment 8993625 [details] [diff] [review]
0001-Bug-1476689-1.1-Use-GeckoResult-in-runtime-telemetry.patch

Approval Request Comment
[Feature/Bug causing the regression]: N/A.
[User impact if declined]: Blocks bug 1479026.
[Is this code covered by automated tests?]: Yes, patch 2 adds a test.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No. 
[List of other uplifts needed for the feature/fix]: Both patches in this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a simple refactoring and a test patch.
[String changes made/needed]: None.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8993625 [details] [diff] [review]
0001-Bug-1476689-1.1-Use-GeckoResult-in-runtime-telemetry.patch

Geckoview telemetry fix, includes new tests. Should uplift for beta 16.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d2a0c97079f6
https://hg.mozilla.org/releases/mozilla-beta/rev/d7b23492ad24