1443603 - Stop sending `saved-session` pings from Firefox Desktop

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Chris H-C :chutten]

This covers stopping TelemetrySession from sending "saved-session" pings on desktop Firefox, and updating necessary documentation and tests for the change.

Comment 1

[Jan-Erik Rediger [:janerik]]

Attachment: Stop sending `saved-session` pings from Firefox Desktop

Comment 2

[Chris H-C :chutten]

Comment on attachment 8960164 [details] [diff] [review]
Stop sending `saved-session` pings from Firefox Desktop

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

For the docs, could you add a Version History section at the bottom like docs/collection/scalars.rst has? I should've asked this for the childPayloads change as well, come to think of it...

Speaking of child payloads, now that it has landed, you'll want to rebase these changes.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1735,3 @@
>  
>      // As a temporary measure, we want to submit saved-session too if extended Telemetry is enabled
>      // to keep existing performance analysis working.

I think we can remove this comment now.

::: toolkit/components/telemetry/docs/concepts/pings.rst
@@ -22,4 @@
>  Important examples are:
>  
>  * :doc:`main <../data/main-ping>` - contains the information collected by Telemetry (Histograms, Scalars, ...)
> -* :doc:`saved-session <../data/main-ping>` - has the same format as a main ping, but it contains the *"classic"* Telemetry payload with measurements covering the whole browser session. This is only a separate type to make storage of saved-session easier server-side. This is temporary and will be removed soon.

Theoretically our in-tree docs should cover Android as well as Desktop. This doc is particularly bad for having a Desktop-only focus.

Maybe instead of "This is temporary and will be removed soon" we can say "As of Firefox 61 this is sent on Android only."

::: toolkit/components/telemetry/docs/data/main-ping.rst
@@ -14,4 @@
>  * ``environment-change`` - the :doc:`environment` changed, so the session measurements got reset and a new subsession starts
>  * ``shutdown`` - triggered when the browser session ends. For the first browsing session, this ping is saved to disk and sent on the next browser restart. From the second browsing session on, this ping is sent immediately on shutdown using the :doc:`../internals/pingsender`, unless the OS is shutting down
>  * ``daily`` - a session split triggered in 24h hour intervals at local midnight. If an ``environment-change`` ping is generated by the time it should be sent, the daily ping is rescheduled for the next midnight
> -* ``saved-session`` - the *"classic"* Telemetry payload with measurements covering the whole browser session (only submitted for a transition period)

(only submitted on Android)

@@ -21,5 @@
>  After a new subsession split, the ``internal-telemetry-after-subsession-split`` topic is notified to all the observers. *This is an internal topic and is only meant for internal Telemetry usage.*
>  
> -.. note::
> -
> -  ``saved-session`` is sent with a different ping type (``saved-session``, not ``main``), but otherwise has the same format as discussed here.

We can leave this

@@ +146,4 @@
>  -------------
>  The Telemetry payloads sent by child processes, recorded on child process shutdown (event ``content-child-shutdown`` observed). They are reduced session payloads, only available with e10s. Among some other things, they don't contain histograms, keyed histograms, add-on details, or UI Telemetry.
>  
> +Note: Child payloads are not collected and cleared with subsession splits, they are currently only meaningful when analysed from ``main`` pings with ``reason`` set to ``shutdown``.

This will interfere nicely with your other work. Merge conflicts ahoy!

@@ -233,4 @@
>  ~~~~~~~~~~~
>  Integer count of the number of five-second intervals ('ticks') the user was considered 'active' (sending UI events to the window). An extra event is fired immediately when the user becomes active after being inactive. This is for some mouse and gamepad events, and all touch, keyboard, wheel, and pointer events (see `EventStateManager.cpp <https://dxr.mozilla.org/mozilla-central/rev/e6463ae7eda2775bc84593bb4a0742940bb87379/dom/events/EventStateManager.cpp#549>`_).
>  This measure might be useful to give a trend of how much a user actually interacts with the browser when compared to overall session duration. It does not take into account whether or not the window has focus or is in the foreground. Just if it is receiving these interaction events.
> -Note that in ``main`` pings, this measure is reset on subsession splits, while in ``saved-session`` pings it covers the whole browser session.

We'll have to leave this, for Android's sake.

Comment 3

[Jan-Erik Rediger [:janerik]]

Attachment: Stop sending `saved-session` pings from Firefox Desktop

Comment 4

[Chris H-C :chutten]

Comment on attachment 8960231 [details] [diff] [review]
Stop sending `saved-session` pings from Firefox Desktop

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

Looks good.

Do you have a plan for how to verify that Android is unaffected by these changes? This might be the riskiest of the "stop doing legacy stuff" bugs because of the requirement to keep using the machinery on Android.

Comment 5

[Jan-Erik Rediger [:janerik]]

I am not familiar with the Android build, so I don't have a plan on how to verify these changes don't affect the Android build.
The first changed test does test for the right ping to be sent on shutdown and covers Android. If needed I can kick off a try-run to verify it.
The other test is not run on Android in the first place.

If you have another idea on how to verify please let me know.

Comment 6

[Chris H-C :chutten]

I would recommend checking the volume of "saved-session" pings we receive from Android Nightly in the days following this landing. If the volumes suddenly decrease, we know something went wrong and can quickly fix it.

Comment 7

[Jan-Erik Rediger [:janerik]]

I once again investigated this change for its impact.
We're special casing for Android exactly once now and keep the old check for `Telemetry.canRecordExtended` there as well.
We do have exactly one test for this which now explicitely checks for `saved-session` when on Android.

I ran the Android tests here[1].
The TelemetrySession tests involving this check pass[2].


[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec2578876e7df66c002c9e20f783b5614e4f0bc5
[2]: https://tools.taskcluster.net/groups/TDTqL4BORGGSD8jpa1jONg/tasks/WNCNsnnbSZuHIFCwkwaD8g/runs/0/logs/public%2Flogs%2F%2Flog_raw.log#L1542-1543


I also have set up a monitor for the `saved-session` pings from Android Nightly here[3], which I will use as a safety net to check that it stays consistent after this is merged.

[3]: https://pipeline-cep.prod.mozaws.net/dashboard_output/graphs/analysis.jrediger_mozilla_com.jrediger_mozilla_com_saved_session_fennec.volume.html


Should we move forward and merge?

Comment 8

[Chris H-C :chutten]

Thumbs-up from me, thank you for the extra validation here.

Client is go for launch.

Comment 9

[Frank Bertsch [:frank]]

Great, I appreciate the thoroughness!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea1a3efdde8b
Stop sending `saved-session` pings from Firefox Desktop r=chutten

Comment 11

[Eliza Balazs [:ebalazs_]]

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