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

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gfritzsche, Assigned: gfritzsche)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed, firefox41 fixed)

Details

(Whiteboard: [uplift])

Attachments

(1 attachment, 1 obsolete attachment)

This also cleans up the pending pings persistance, putting the control of saving them out of TelemetrySession.
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.
Attachment #8608091 - Flags: review?(vdjeric)
Blocks: 1120356
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?
Flags: needinfo?(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).
Flags: needinfo?(gfritzsche)
Ok, go for it
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?
Attachment #8608091 - Flags: review?(vdjeric)
(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.
This also cleans up the pending pings persistance, putting the control of saving them out of TelemetrySession.
Attachment #8609423 - Flags: review?(vdjeric)
Attachment #8608091 - Attachment is obsolete: true
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?
Attachment #8609423 - Flags: review?(vdjeric) → review+
(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?
(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.
(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.
Whiteboard: [uplift]
https://hg.mozilla.org/mozilla-central/rev/22d3f60fdb07
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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.
Attachment #8609423 - Flags: approval-mozilla-aurora?
Attachment #8609423 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.