1457472 - Support a single storage clearing point in the GeckoView API

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P2)

Description

[Alessio Placitelli [:Dexter]]

In order to ensure that the meaning of "subsession" is consistent across all the different probe types, we need to make sure that "clear" is called almost at the same time for all of them.

We need to change the GeckoView API so that:

- the snapshotting function returns the data for all the measurement types by taking a snapshot for each of them without calling clear;
- calling nsITelemetry.clearProbes (which is getting exposed in bug 1453591.

Additionally, we could also address:

- removal of the opt-in/opt-out flags for snapshotting: we can always snapshot for opt-in and trust Telemetry to give back the right data based on the flags that are set (which depend on the build and the channel);
- return the snapshot after the initialization topic introduced in bug 1453591 gets notified.

Please note that we didn't have this problem on Desktop as we controlled TelemetrySession and implicitly did that.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1457472-1.0-Simplify-telemetry-snapshot-API-to-r.patch

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1457472-2.0-Adjust-telemetry-controller-to-suppo.patch

The telemetry controller now has to wait for the load complete event before handling snapshot requests, support the new GV API, use the unified clear method and default to opt-in telemetry.

Comment 3

[Eugen Sawin [:esawin]]

Alessio, do you consider this a blocker for bug 1453591? I think with patch 2 we may have a cyclic bug dependency now.

Comment 4

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

Comment on attachment 8972587 [details] [diff] [review]
0001-Bug-1457472-1.0-Simplify-telemetry-snapshot-API-to-r.patch

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

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/RuntimeTelemetry.java
@@ -16,2 @@
>  import android.support.annotation.NonNull;
>  import android.support.annotation.Nullable;

Not used

@@ -16,3 @@
>  import android.support.annotation.NonNull;
>  import android.support.annotation.Nullable;
>  import android.util.Log;

Not used

@@ +34,5 @@
> +     * The response bundle will contain following snapshots:
> +     * - histogram
> +     * - keyedHistogram
> +     * - scalars
> +     * - keyedScalars

This will not be formatted correctly unfortunately. You have to use HTML, e.g.

> <ul>
> <li>histogram</li>
> <li>keyedHistogram</li>
> ...

Comment 5

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

> This will not be formatted correctly unfortunately. You have to use HTML,

Or use something like <pre>

Comment 6

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

Comment on attachment 8972588 [details] [diff] [review]
0002-Bug-1457472-2.0-Adjust-telemetry-controller-to-suppo.patch

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

::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm
@@ +27,5 @@
>     * Setup the Telemetry recording flags. This must be called
>     * in all the processes that need to collect Telemetry.
>     */
>    setup() {
>      debug `setup`;

Don't need this

Comment 7

[Alessio Placitelli [:Dexter]]

Comment on attachment 8972588 [details] [diff] [review]
0002-Bug-1457472-2.0-Adjust-telemetry-controller-to-suppo.patch

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

::: toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm
@@ +89,5 @@
> +   */
> +  retrieveSnapshots(aClear, aCallback) {
> +    debug `retrieveSnapshots`;
> +
> +    const dataset = Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN;

nit: can you drop a comment here explaining why we're always snapshotting opt-in? Basically, we trust the Telemetry core to only record probes we can record to (opt-in probes if canRecordPreRelease is true, opt-out if canRecordRelease is true). So, even if the snapshot always asks for opt-in, it will just return opt-out stuff if we're on Release. Please note that this is only true in recent versions of Firefox :)

Comment 8

[Alessio Placitelli [:Dexter]]

(In reply to Eugen Sawin [:esawin] from comment #3)
> Alessio, do you consider this a blocker for bug 1453591? I think with patch
> 2 we may have a cyclic bug dependency now.

Whoops, sorry, I almost missed this. I think that bug 1453591 is a blocker for this. I've changed the dependency direction in the bug. I'm planning to land the persistence bug as soon as the code freeze is over, so Monday (I hope :) ).

Comment 9

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1457472-1.1-Simplify-telemetry-snapshot-API-to-r.patch

Addressed comments.

Comment 10

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1457472-2.1-Adjust-telemetry-controller-to-suppo.patch

Addressed comments.

Comment 11

[Alessio Placitelli [:Dexter]]

Hey Eugen, bug 1453591 finally landed :) Can we land this as well? Sorry for the delay!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29375577d699
[1.1] Simplify telemetry snapshot API to reflect new implementation constraints. r=snorp,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/23532c17a812
[2.1] Adjust telemetry controller to support the simplified GeckoView API, the unified clear method and the snapshot persistence lifecycle. r=Dexter,jchen

Comment 13

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/29375577d699
https://hg.mozilla.org/mozilla-central/rev/23532c17a812