Don't send the "shutdown" ping using the pingsender from the first Firefox session

VERIFIED FIXED in Firefox 55

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Dexter, Assigned: Dexter)

Tracking

Trunk
mozilla55
Points:
1
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
"bot" machines usually spawn Firefox, to their stuff, and then shutdown resulting in a single browsing subsession.

To prevent being flooded by "shutdown" pings from these machines, we could restrict sending the "shutdown" ping using the pingsender from the second subsession on.

This will prevent us from seeing users that open Firefox only once and never again, but would otherwise reduce the data latency for the rest of the population.
(Assignee)

Updated

2 years ago
Blocks: 1336360
Points: --- → 1
Priority: -- → P1
Whiteboard: [measurement:client]
Summary: Send the "shutdown" ping using the pingsender only from the second subsession → Don't send the "shutdown" ping using the pingsender from the first Firefox session
I think the safe option is to only send this from the second Firefox "session" on.
A second subsession might still be triggered for other reasons.
(Assignee)

Comment 2

2 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> I think the safe option is to only send this from the second Firefox
> "session" on.
> A second subsession might still be triggered for other reasons.

Good point!
(Assignee)

Updated

2 years ago
Assignee: nobody → alessio.placitelli

Comment 4

2 years ago
mozreview-review
Comment on attachment 8856484 [details]
Bug 1354482 - Turn on sending the "shutdown" ping using the pingsender from the second Firefox session.

https://reviewboard.mozilla.org/r/128440/#review131474

::: toolkit/components/telemetry/tests/unit/head.js:311
(Diff revision 1)
>    Services.prefs.setBoolPref("toolkit.telemetry.archive.enabled", true);
>    // Telemetry xpcshell tests cannot show the infobar.
>    Services.prefs.setBoolPref("datareporting.policy.dataSubmissionPolicyBypassNotification", true);
>    // FHR uploads should be enabled.
>    Services.prefs.setBoolPref("datareporting.healthreport.uploadEnabled", true);
> +  // We cannot control the pingsender process, so ease testing by disabling it.

We can control it - what exactly do you mean here?

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js:1418
(Diff revision 1)
> -  Preferences.reset(PREF_SHUTDOWN_PINGSENDER);
>    // We cannot reset PREF_BYPASS_NOTIFICATION, as we need it to be
>    // |true| in tests.
>    Preferences.set(PREF_BYPASS_NOTIFICATION, true);
> +
> +  // With both upload and the policy shown, make sure we don't

Can you be more clear? "upload enabled"?
Attachment #8856484 - Flags: review?(gfritzsche) → review+
Comment hidden (mozreview-request)
You should also update the documentation for:
- pingsender
- main ping for "shutdown" scenario
- Telemetry preferences
Flags: needinfo?(alessio.placitelli)
Finally, we also need to update docs/concepts/submission.rst with pingsender info in a follow-up.
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> You should also update the documentation for:
> - pingsender
> - main ping for "shutdown" scenario
> - Telemetry preferences
> -  docs/concepts/submission.rst

Chris, can you specifically review the doc updates in the new patch?

It touches all the points except the *pingsender* doc which didn't need to be updated, AFAICT.
Flags: needinfo?(alessio.placitelli) → needinfo?(chutten)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8856484 [details]
Bug 1354482 - Turn on sending the "shutdown" ping using the pingsender from the second Firefox session.

https://reviewboard.mozilla.org/r/128440/#review131994

::: toolkit/components/telemetry/TelemetrySend.jsm:803
(Diff revision 3)
>        return Promise.resolve();
>      }
>  
>      // Send the ping using the PingSender, if requested and the user was
> -    // notified of our policy.
> -    if (options.usePingSender && TelemetryReportingPolicy.canUpload()) {
> +    // notified of our policy. We don't support the pingsender on Android,
> +    // so ignore this option on that platform (see bug 1335917).

To me it would be better to ensure we only set options.usePingSender on Android, but it doesn't hurt to check it here.

::: toolkit/components/telemetry/tests/unit/head.js:312
(Diff revision 3)
>    // Telemetry xpcshell tests cannot show the infobar.
>    Services.prefs.setBoolPref("datareporting.policy.dataSubmissionPolicyBypassNotification", true);
>    // FHR uploads should be enabled.
>    Services.prefs.setBoolPref("datareporting.healthreport.uploadEnabled", true);
> +  // Many tests expect the shutdown ping to not be sent on shutdown and will fail
> +  // if receive an unexpected ping. Let's globally disable the shutdown pign sender:

s/pign/ping/
Attachment #8856484 - Flags: review?(chutten) → review+
Comment hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8856484 [details]
Bug 1354482 - Turn on sending the "shutdown" ping using the pingsender from the second Firefox session.

https://reviewboard.mozilla.org/r/128440/#review132012

I like your idea of putting this in TelemetryReportingPolicy.
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1343832

Comment 14

2 years ago
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0c7702e42f33
Turn on sending the "shutdown" ping using the pingsender from the second Firefox session. r=chutten,gfritzsche
Comment hidden (mozreview-request)

Comment 17

2 years ago
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7fda04f46791
Turn on sending the "shutdown" ping using the pingsender from the second Firefox session. r=chutten,gfritzsche
(Assignee)

Comment 18

2 years ago
(In reply to Wes Kocher (:KWierso) from comment #15)
> I had to back this out for windows xpcshell failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=90973098&repo=autoland
> 
> https://hg.mozilla.org/integration/autoland/rev/
> 9358a72fbe0c03ee6c745d15c7c77a00fb61c7e1

Thanks for backing this out. It was failing intermittently :-( I slightly changed the patch (carried over the r+, since this was a test-only change) and I'm re-landing this.

It's green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dd94463d31c9579b0ffeb1b025828108c6fd89f&selectedJob=91384279
(Assignee)

Updated

2 years ago
Flags: needinfo?(alessio.placitelli)
(Assignee)

Updated

2 years ago
Flags: needinfo?(chutten)

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7fda04f46791
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 20

2 years ago
Abe, since you validated bug 1336360, would you mind validating this one too?

This enables sending the shutdown ping on shutdown, by default (so you don't need to flip the pref anymore), from the second browsing session on.

For example:

1. Create a new profile
2. Start Firefox and do some stuff
3. Shutdown Firefox
4. No shutdown ping should be received by the server
5. Start Firefox (you might receive the pings from the previous session once Telemetry inits, 60 seconds from now)
6. Shutdown Firefox
7. The shutdown ping must be received by the server
8. If you repeat steps 5-6 you would still receive the shutdown ping as soon as Firefox shuts down.
Flags: needinfo?(amasresha)
Verified-fixed.
We have completed testing this and all are green.
Here are the test cases and runs: https://public.etherpad-mozilla.org/p/Bug_1354482
Please needinfo or email me if you have questions about the testing. Thanks
Status: RESOLVED → VERIFIED
Flags: needinfo?(amasresha)
Depends on: 1356673
(Assignee)

Updated

2 years ago
Depends on: 1357745

Updated

2 years ago
Depends on: 1366082
You need to log in before you can comment on or make changes to this bug.