1459143 - Add in-tree docs for GeckoView persistence

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Alessio Placitelli [:Dexter]]

We need to change/add in-tree docs for documenting the GeckoView persistence logic.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1459143 - Add basic documentation for GeckoView.

Review commit: https://reviewboard.mozilla.org/r/244908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244908/

Comment 2

[Chris H-C :chutten]

Comment on attachment 8976818 [details]
Bug 1459143 - Add basic documentation for GeckoView.

https://reviewboard.mozilla.org/r/244908/#review251144

Only a bunch of nitpicks.

::: toolkit/components/telemetry/docs/internals/geckoview.rst:6
(Diff revision 1)
> +GeckoView
> +=========
> +
> +Supporting GeckoView in the Telemetry core means enabling GeckoView to easily add and
> +record Telemetry. Since GeckoView can be embedded and have a short life cycle, we need
> +to support measurements across different sessions. The current GeckoView support is limited

Can you expand upon why we need to support it across different sessions? It will be strange for anyone coming from desktop Firefox's idea of how sessions work to a mobile "you can be killed at the OS' whim"  world

::: toolkit/components/telemetry/docs/internals/geckoview.rst:11
(Diff revision 1)
> +to support measurements across different sessions. The current GeckoView support is limited
> +to :doc:`scalars <../collection/scalars>` and :doc:`histograms <../collection/histograms>`.
> +
> +Overview
> +--------
> +GeckoView does not make use of the same `JavaScript modules <https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry+ext%3Ajsm+-path%3Ageckoview&redirect=false>`_ used on Firefox Desktop. Instead, Telemetry gets setup on this product by `GeckoViewTelemetryController.jsm <https://dxr.mozilla.org/mozilla-central/rev/1800b8895c08bc0c60302775dc0a4b5ea4deb310/toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm>`_ .

Are we hard-wrapping lines or soft-wrapping them in this file? Either way we should be consistent.

(I'm for hard-wrapping them with newlines anywhere between 80-100 characters, but that's me)

::: toolkit/components/telemetry/docs/internals/geckoview.rst:11
(Diff revision 1)
> +to support measurements across different sessions. The current GeckoView support is limited
> +to :doc:`scalars <../collection/scalars>` and :doc:`histograms <../collection/histograms>`.
> +
> +Overview
> +--------
> +GeckoView does not make use of the same `JavaScript modules <https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry+ext%3Ajsm+-path%3Ageckoview&redirect=false>`_ used on Firefox Desktop. Instead, Telemetry gets setup on this product by `GeckoViewTelemetryController.jsm <https://dxr.mozilla.org/mozilla-central/rev/1800b8895c08bc0c60302775dc0a4b5ea4deb310/toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm>`_ .

*used in

::: toolkit/components/telemetry/docs/internals/geckoview.rst:13
(Diff revision 1)
> +
> +Overview
> +--------
> +GeckoView does not make use of the same `JavaScript modules <https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry+ext%3Ajsm+-path%3Ageckoview&redirect=false>`_ used on Firefox Desktop. Instead, Telemetry gets setup on this product by `GeckoViewTelemetryController.jsm <https://dxr.mozilla.org/mozilla-central/rev/1800b8895c08bc0c60302775dc0a4b5ea4deb310/toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm>`_ .
> +
> +More importantly, users do not need to worry about handling measurements persistence across sessions:

either "measurement persistence" or "measurements' persistence"

::: toolkit/components/telemetry/docs/internals/geckoview.rst:20
(Diff revision 1)
> +
> +Persistence
> +-----------
> +Measurement persistence across sessions is guaranteed by an automatic serialization and deserialization
> +process performed, behind the scenes, by the Telemetry core. As soon as Telemetry starts up, it
> +checks for the existence of the persisted measurements file (``gv_measurements.json``) in the Android's application `data dir <https://developer.android.com/reference/android/content/pm/ApplicationInfo.html#dataDir>`_. If that file is found, its parsed and the status of the contained

s/its parsed/it is parsed/

"the status of the contained measurements is" is an odd choice of phrasing. I think "the values of the contained measurements are" might be more clear.

::: toolkit/components/telemetry/docs/internals/geckoview.rst:33
(Diff revision 1)
> +  ``internal-telemetry-geckoview-load-complete`` topic is broadcasted to signal that loading
> +  is complete.
> +
> +Once the measurements are restored, their values will be dumped again to the persisted
> +measurements file after the update interval expires. This interval is defined by the
> +``toolkit.telemetry.geckoPersistenceTimeout`` preference, see the :doc:`preferences docs <preferences>`.

Please mention the default value for reference.

::: toolkit/components/telemetry/docs/internals/geckoview.rst:65
(Diff revision 1)
> +        }
> +      },
> +      "histograms": {
> +        "parent": {
> +          "TELEMETRY_TEST_MULTIPRODUCT": {
> +            "sum": 6,

Perhaps chose an example sum that might be possible with the provided counts?

::: toolkit/components/telemetry/docs/internals/geckoview.rst:132
(Diff revision 1)
> +     * Clears any GeckoView persisted data.
> +     * This physically deletes persisted data files.
> +     */
> +    void ClearPersistenceData();
> +
> +Version History

Are we using "Sentence case" (eg "The internal C++ API") or "Title Case" (eg "Version History") for headings in this file?

::: toolkit/components/telemetry/docs/internals/geckoview.rst:136
(Diff revision 1)
> +
> +Version History
> +---------------
> +
> +- Firefox 62: Initial GeckoView support and scalar persistence (`bug 1453591 <https://bugzilla.mozilla.org/show_bug.cgi?id=1453591>`_).
> +- Firefox 62: Persistence support for histograms (`bug 1457127 <https://bugzilla.mozilla.org/show_bug.cgi?id=1457127>`_).

In other files multiple items for the same version are sublisted (see "main" ping docs)

Comment 3

[Alessio Placitelli [:Dexter]]

Comment on attachment 8976818 [details]
Bug 1459143 - Add basic documentation for GeckoView.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244908/diff/1-2/

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8976818 [details]
Bug 1459143 - Add basic documentation for GeckoView.

https://reviewboard.mozilla.org/r/244908/#review251390

::: toolkit/components/telemetry/docs/internals/geckoview.rst:17
(Diff revision 2)
> +--------
> +GeckoView does not make use of the same `JavaScript modules <https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry+ext%3Ajsm+-path%3Ageckoview&redirect=false>`_
> +used in Firefox Desktop. Instead, Telemetry gets setup on this product by `GeckoViewTelemetryController.jsm <https://dxr.mozilla.org/mozilla-central/rev/1800b8895c08bc0c60302775dc0a4b5ea4deb310/toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm>`_ .
> +
> +More importantly, users do not need to worry about handling measurements' persistence across
> +sessions: this is already taken care of, behind the scenes, by the Telemetry core.

Nit: Can you add an explanatory line on what this means?
E.g. something like "this means measurements accumulate across app sessions until cleared".

::: toolkit/components/telemetry/docs/internals/geckoview.rst:17
(Diff revision 2)
> +sessions: this is already taken care of, behind the scenes, by the Telemetry core.
> +

Add something that highlights the consequences of these?
For GeckoView, the Telemetry module does not define any session model, just accumulation and storage.
Defining a session model is the responsibility of the application.

::: toolkit/components/telemetry/docs/internals/geckoview.rst:72
(Diff revision 2)
> +      "histograms": {
> +        "parent": {
> +          "TELEMETRY_TEST_MULTIPRODUCT": {
> +            "sum": 31303,
> +            "counts": [
> +              3, 5, 7, ...

Does this mean we're serializing the dense representation to disk?
Should this use the sparse representation in the future or does this compress away well?

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8976818 [details]
Bug 1459143 - Add basic documentation for GeckoView.

https://reviewboard.mozilla.org/r/244908/#review251390

> Does this mean we're serializing the dense representation to disk?
> Should this use the sparse representation in the future or does this compress away well?

Yes, we're serializing dense representation to disk. I filed bug 1463099 for adding compression to that.

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8976818 [details]
Bug 1459143 - Add basic documentation for GeckoView.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244908/diff/2-3/

Comment 7

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8976818 [details]
Bug 1459143 - Add basic documentation for GeckoView.

https://reviewboard.mozilla.org/r/244908/#review251744

Some small things, but otherwise good to go!

::: toolkit/components/telemetry/docs/internals/geckoview.rst:18
(Diff revision 3)
> +GeckoView does not make use of the same `JavaScript modules <https://dxr.mozilla.org/mozilla-central/search?q=path%3Atoolkit%2Fcomponents%2Ftelemetry+ext%3Ajsm+-path%3Ageckoview&redirect=false>`_
> +used in Firefox Desktop. Instead, Telemetry gets setup on this product by `GeckoViewTelemetryController.jsm <https://dxr.mozilla.org/mozilla-central/rev/1800b8895c08bc0c60302775dc0a4b5ea4deb310/toolkit/components/telemetry/geckoview/GeckoViewTelemetryController.jsm>`_ .
> +
> +More importantly, users do not need to worry about handling measurements' persistence across
> +sessions: this means measurements accumulate across application sessions until cleared. In
> +contrast with Firefox Desktop, for which Telemetry defines a strict :doc:`session <../concepts/sessions>` model, for GeckoView, the Telemetry module does not define it: it just provides

Should we hard-wrap the lines here?

::: toolkit/components/telemetry/docs/internals/geckoview.rst:32
(Diff revision 3)
> +Android's application `data dir <https://developer.android.com/reference/android/content/pm/ApplicationInfo.html#dataDir>`_. If that file is found, it is parsed and the values of the
> +contained measurements are loaded in memory.
> +
> +.. note::
> +
> +  While it's possible to take a snapshot of the measurements using the GeckoView API before

Maybe say "it's possible to request a snapshot", as you're talking about the requests afterwards as well.

::: toolkit/components/telemetry/docs/internals/geckoview.rst:46
(Diff revision 3)
> +:doc:`preferences docs <preferences>`.
> +
> +The persistence file format
> +---------------------------
> +All the supported measurements are serialized to the persistence file using the JSON format.
> +The top level entries in the file are the measurements types. Each measurement section contains

nit. measurement types.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8976818 [details]
Bug 1459143 - Add basic documentation for GeckoView.

https://reviewboard.mozilla.org/r/244908/#review251826

Thanks!

Comment 9

[Alessio Placitelli [:Dexter]]

Comment on attachment 8976818 [details]
Bug 1459143 - Add basic documentation for GeckoView.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244908/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aadef1ad98ef
Add basic documentation for GeckoView. r=chutten,gfritzsche,janerik

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/aadef1ad98ef