Closed Bug 1354482 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Telemetry, enhancement, P1)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement
Points:
1

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file)

Bug 1354482 - Turn on sending the "shutdown" ping using the pingsender from the second Firefox session.
59 bytes, text/x-review-board-request
gfritzsche
: review+
chutten
: review+
Details 
"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.
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.
(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: nobody → alessio.placitelli
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+
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.
(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 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 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.
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
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
(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
Flags: needinfo?(alessio.placitelli)
Flags: needinfo?(chutten)
https://hg.mozilla.org/mozilla-central/rev/7fda04f46791
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
Depends on: 1357745
Depends on: 1366082
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: