Closed Bug 1459143 Opened 3 years ago Closed 3 years ago

Add in-tree docs for GeckoView persistence

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

Attachments

(1 file)

We need to change/add in-tree docs for documenting the GeckoView persistence logic.
Blocks: 1452550
Priority: -- → P2
Assignee: nobody → alessio.placitelli
Priority: P2 → P1
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)
Attachment #8976818 - Flags: review?(chutten) → review+
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?
Attachment #8976818 - Flags: review?(gfritzsche)
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 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.
Attachment #8976818 - Flags: review?(jrediger) → review+
Comment on attachment 8976818 [details]
Bug 1459143 - Add basic documentation for GeckoView.

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

Thanks!
Attachment #8976818 - Flags: review?(gfritzsche) → review+
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/aadef1ad98ef
Add basic documentation for GeckoView. r=chutten,gfritzsche,janerik
https://hg.mozilla.org/mozilla-central/rev/aadef1ad98ef
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1465451
You need to log in before you can comment on or make changes to this bug.