1166705 - Don't send a saved-session ping when extended Telemetry is off

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

This also cleans up the pending pings persistance, putting the control of saving them out of TelemetrySession.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Don't send a saved-session ping when extended Telemetry is off

I noticed that we always sent out both saved-session and shutdown pings. We only need to send saved-session though if the telemetry pref on to keep previous behavior.

Comment 2

[Vladan Djeric (:vladan)]

Are we sure we want to make this change? I agree full-session pings are mostly useful for performance analyses, but will we need the full-session pings for some kind of equivalence comparison on Release-channel?

Comment 3

[Georg Fritzsche [:gfritzsche]]

Any data equivalence checks will happen on pre-release channels.
Once we have the sign-off there, this will ride to release, where i don't think we should have the additional storage and network costs (unless users opted into Telemetry).

Comment 4

[Vladan Djeric (:vladan)]

Ok, go for it

Comment 5

[Vladan Djeric (:vladan)]

Comment on attachment 8608091 [details] [diff] [review]
Don't send a saved-session ping when extended Telemetry is off

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ -1628,3 @@
>  
> -    if (!IS_UNIFIED_TELEMETRY) {
> -      return this.savePendingPingsClassic();

so did we create saved-session payloads even when extended Telemetry was opted out but unified pref=off?

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +571,5 @@
>     *
>     * @param {object} sessionPing The additional session ping.
>     * @returns {promise}
>     */
>    savePendingPings: function(sessionPing) {

do we need the sessionPing parameter anymore?

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #5)
> Comment on attachment 8608091 [details] [diff] [review]
> Don't send a saved-session ping when extended Telemetry is off
> 
> Review of attachment 8608091 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetrySession.jsm
> @@ -1628,3 @@
> >  
> > -    if (!IS_UNIFIED_TELEMETRY) {
> > -      return this.savePendingPingsClassic();
> 
> so did we create saved-session payloads even when extended Telemetry was
> opted out but unified pref=off?

No, because we don't initialize Telemetry then (the unified pref requires a restart).

> ::: toolkit/components/telemetry/TelemetryStorage.jsm
> @@ +571,5 @@
> >     *
> >     * @param {object} sessionPing The additional session ping.
> >     * @returns {promise}
> >     */
> >    savePendingPings: function(sessionPing) {
> 
> do we need the sessionPing parameter anymore?

Ah, no.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Attachment: Don't send a saved-session ping when extended Telemetry is off

This also cleans up the pending pings persistance, putting the control of saving them out of TelemetrySession.

Comment 8

[Vladan Djeric (:vladan)]

Comment on attachment 8609423 [details] [diff] [review]
Don't send a saved-session ping when extended Telemetry is off

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

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +752,4 @@
>     * @returns {promise}
>     */
> +  savePendingPings: function() {
> +    let p = [for (ping of pendingPings) this.savePing(ping, false).catch(ex => {

should pendingPings (and other state variables) really be declared as globals in TelemetryStorage.jsm? I know it's from before unification, but it seems kind of odd. is it to make writing tests easier?

Comment 9

[Vladan Djeric (:vladan)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> This also cleans up the pending pings persistance, putting the control of
> saving them out of TelemetrySession.

Is this a new change added to the v2 patch?

Comment 10

[Georg Fritzsche [:gfritzsche]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #9)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> > This also cleans up the pending pings persistance, putting the control of
> > saving them out of TelemetrySession.
> 
> Is this a new change added to the v2 patch?

No, that was in v1 already. This is bzexport adding the additional line from the patches message as a comment on the patch.

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #8)
> Comment on attachment 8609423 [details] [diff] [review]
> Don't send a saved-session ping when extended Telemetry is off
> 
> Review of attachment 8609423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetryStorage.jsm
> @@ +752,4 @@
> >     * @returns {promise}
> >     */
> > +  savePendingPings: function() {
> > +    let p = [for (ping of pendingPings) this.savePing(ping, false).catch(ex => {
> 
> should pendingPings (and other state variables) really be declared as
> globals in TelemetryStorage.jsm? I know it's from before unification, but it
> seems kind of odd. is it to make writing tests easier?

TelemetryStorage/TelemetryFile used to not have a separate Impl object before, so i think that was the way it did "private" variables.

Cleaning that up is part of bug 1156355.

Comment 12

[Georg Fritzsche [:gfritzsche]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07e0996e2ea1

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/22d3f60fdb07

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/22d3f60fdb07

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8609423 [details] [diff] [review]
Don't send a saved-session ping when extended Telemetry is off

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry
This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2.
[User impact if declined]:
Data & measurement insight projects delayed or blocked with direct impact on projects depending on this.
[Describe test coverage new/current, TreeHerder]:
We have good automation coverage of the feature.
We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here.
[Risks and why]:
Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports.
We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements.
[String/UUID change made/needed]:
The only string changes were to the about:telemetry page.
We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.

Comment 16

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1424c5c5e1d4