Closed Bug 1445921 Opened 2 years ago Closed 2 years ago

Fix "optout" ping

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: mreid, Assigned: janerik)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

The "deletion" ping is currently mis-named, and was a holdover from the FHR days.

Rename "deletion" ping to "optout" ping, and remove the clientId from the ping.

Make a best effort to send it right away, then turn off data reporting (do not block or keep retrying).

Upon opting out, wipe out the locally-stored clientId to dissociate the profile from any previously collected data.

We may also want to add a boolean scalar or event upon opt-in if a client re-enables Telemetry.
Matt, are there implications here for Shield? Should opting out of Telemetry disable any in-flight experiments?
Flags: needinfo?(mgrimes)
One more consequence here - we should generate a new clientId if someone opts back in to Telemetry reporting.
Notes that come to mind:

1) We aren't the only thing what uses clientId. How do we want this to affect crash reports (for instance)?
2) We also should ensure we reset the session and subsession counters.
3) Should we leave the profile creation date?
4) We should ensure we clear and disable the Telemetry Archive and erase pending pings.
5) crash pings built for pingsender are stored in a non-profile-specific space (the directory named "Pending Pings/"). There shouldn't be anything in there when we're not currently crashing, but we might want to double-check?
IIUC option out of Telemetry should disable any in-flight experiments and prevent you from being enrolled in any new ones. Mythmon can confirm that behavior.
Flags: needinfo?(mgrimes) → needinfo?(mcooper)
Currently disabling telemetry will prevent enrollment in new experiments, but won't unenroll you from existing experiments. This probably isn't good, as it will basically make any experiments a user has active permanent once they turn off telemetry, unless they have built-in expiration. Most add-on experiments have these expirations, but preference experiments do not.

We'll need to rethink Normandy and Shield's relationship to this preference in the coming months.
Flags: needinfo?(mcooper)
Priority: -- → P2
:mreid, when were you hoping to see the client work done for this? We're doing some quarter planing and are trying to figure out relative priorities.
Flags: needinfo?(mreid)
I would really like to see this work land this quarter, if possible.
Flags: needinfo?(mreid)
Assignee: nobody → chutten
Status: NEW → ASSIGNED
Duplicate of this bug: 1174925
Event and TMO work is taking up my time for the next while. Deprioritizing and unassigning for now.
Assignee: chutten → nobody
Status: ASSIGNED → NEW
Priority: P2 → P3
Lining this up for the next iteration to fit it into the quarter.
We should see who can actually take this.
Priority: P3 → P2
Alessio and Jan-Erik will figure out how this fits in.
Priority: P2 → P1
Depends on: 1469840
ni?:mreid

How will the optout ping be used in the pipeline?
Just for statistics how many people turn off telemetry explicitely?
If we don't have the clientId in the ping we can't associate it with previous data.

ni?mythmon

Re comment 5, was this discussed for Normandy/Shield yet?
Upon disabling Telemetry we could notify an event, e.g. "telemetry-disabled", on which Normandy/Shield could listen and take appropriate action. Would this help?
Flags: needinfo?(mreid)
Flags: needinfo?(mcooper)
(In reply to Jan-Erik Rediger [:janerik] from comment #12)
> ni?:mreid
> 
> How will the optout ping be used in the pipeline?
> Just for statistics how many people turn off telemetry explicitely?
> If we don't have the clientId in the ping we can't associate it with
> previous data.

Correct - we just want aggregate counts of how many profiles are opting out. I don't think it matters which ones, specifically, they are.
Flags: needinfo?(mreid)
(In reply to Jan-Erik Rediger [:janerik] from comment #12)
> ni?mythmon
> 
> Re comment 5, was this discussed for Normandy/Shield yet?
> Upon disabling Telemetry we could notify an event, e.g.
> "telemetry-disabled", on which Normandy/Shield could listen and take
> appropriate action. Would this help?

Emitting an event, like "telemetry-disabled" should work well for Normandy.
Flags: needinfo?(mcooper)
Blocks: 1470924
Assignee: nobody → jrediger
MozReview-Commit-ID: K4O9SXlmQkI
This follows the steps from the specification and also ensures sending
the optout ping is only tried once and discarded if that fails.

Depends on D1947

MozReview-Commit-ID: 99peURNq9jx
MozReview-Commit-ID: 8aPTGmYv6Wf

Depends on D1948
MozReview-Commit-ID: FCjCpozJ5yS

Depends on D1949
Comment on attachment 8989773 [details]
Bug 1445921 - Test for canary client id in received pings.

Alessio Placitelli [:Dexter] has approved the revision.

https://phabricator.services.mozilla.com/D1949
Attachment #8989773 - Flags: review+
Comment on attachment 8989771 [details]
Bug 1445921 - Allow client ID to be set and reset.

Alessio Placitelli [:Dexter] has approved the revision.

https://phabricator.services.mozilla.com/D1947
Attachment #8989771 - Flags: review+
One question that came up in conversation here:
Scenario:
- disable telemetry upload
- restart the browser
- re-enable telemetry upload
  - generate a new client id
  - ... now crash before we async saved it to disk

What happens when we restart?
Do we create a new client id again?
Looks like we will create a new one, but we won't have sent the other one so there shouldn't be double-counting: https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/toolkit/modules/ClientID.jsm#115
Comment on attachment 8989774 [details]
Bug 1445921 - Document new optout ping

Alessio Placitelli [:Dexter] has approved the revision.

https://phabricator.services.mozilla.com/D1950
Attachment #8989774 - Flags: review+
Comment on attachment 8989772 [details]
Bug 1445921 - Implement and test new optout ping logic.

Alessio Placitelli [:Dexter] has approved the revision.

https://phabricator.services.mozilla.com/D1948
Attachment #8989772 - Flags: review+
Attachment #8990237 - Flags: review?(chutten)
Blocks: 1473874
(In reply to Georg Fritzsche [:gfritzsche] [away June 19 - July 3] from comment #21)
> One question that came up in conversation here:
> Scenario:
> - disable telemetry upload
> - restart the browser
> - re-enable telemetry upload
>   - generate a new client id
>   - ... now crash before we async saved it to disk
> 
> What happens when we restart?
> Do we create a new client id again?

The very first thing we do when resetting the client ID is to remove the disk-stored ID.
If at boot this file doesn't exist a new id is created (and written to that file, as mentioned by :chutten).
But we also depend on the Telemetry upload preference. So if that isn't synced to disk yet, after a restart of the browser upload will still be disabled through the preference and the user would need to re-enable it again, triggering the whole resetting once more.
Comment on attachment 8990237 [details]
data-review-request.txt

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, Scalars.yaml

    Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, standard Telemetry mechanisms apply.

    If the request is for permanent data collection, is there someone who will monitor the data over time?**

Yes, :janerik will.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? **

Category 2, interaction.

    Is the data collection request for default-on or default-off?

Default on.

    Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No.

    Is the data collection covered by the existing Firefox privacy notice? 

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data? 

No, it is permanent.

---
Result: datareview+
Attachment #8990237 - Flags: review?(chutten) → review+
Comment on attachment 8989772 [details]
Bug 1445921 - Implement and test new optout ping logic.

Chris H-C :chutten has approved the revision.

https://phabricator.services.mozilla.com/D1948
Attachment #8989772 - Flags: review+
Pushed by jrediger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f3ef58bc6b38
Allow client ID to be set and reset. r=Dexter
Pushed by jrediger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c23db3a5f53
Implement and test new optout ping logic. r=Dexter,chutten
Pushed by jrediger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b859e76fcc67
Test for canary client id in received pings. r=Dexter
Backed out 4 changesets (bug 1445921) for Linting failure on components/telemetry/tests/unit/test_TelemetryController.js on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/autoland/rev/c73eb22a077df9a4238426135ab4e4025f954abe

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7c23db3a5f530f001bfb1dc989ab61e3b882804f

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=187107238&repo=autoland&lineNumber=240

Log snippet:

[vcs 2018-07-09T09:43:17.885Z] 245506 files updated, 0 files merged, 0 files removed, 0 files unresolved
[vcs 2018-07-09T09:43:18.052Z] updated to 7c23db3a5f530f001bfb1dc989ab61e3b882804f
[vcs 2018-07-09T09:43:18.057Z] PERFHERDER_DATA: {"framework": {"name": "vcs"}, "suites": [{"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "clone", "shouldAlert": false, "subtests": [], "value": 106.30561518669128}, {"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "pull", "shouldAlert": false, "subtests": [], "value": 7.696439027786255}, {"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "update", "shouldAlert": false, "subtests": [], "value": 77.21055388450623}, {"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "overall", "shouldAlert": false, "subtests": [], "value": 192.3637878894806}]}
[vcs 2018-07-09T09:43:18.486Z] TinderboxPrint:<a href=https://hg.mozilla.org/integration/autoland/rev/7c23db3a5f530f001bfb1dc989ab61e3b882804f title='Built from autoland revision 7c23db3a5f530f001bfb1dc989ab61e3b882804f'>7c23db3a5f530f001bfb1dc989ab61e3b882804f</a>
[task 2018-07-09T09:43:18.486Z] executing ['bash', '-cx', 'cd /builds/worker/checkouts/gecko/ && cp -r /build/node_modules_eslint node_modules && ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules && ln -s ../tools/lint/eslint/eslint-plugin-spidermonkey-js node_modules && ./mach lint -l eslint -f treeherder --quiet\n']
[task 2018-07-09T09:43:18.494Z] + cd /builds/worker/checkouts/gecko/
[task 2018-07-09T09:43:18.495Z] + cp -r /build/node_modules_eslint node_modules
[task 2018-07-09T09:43:19.061Z] + ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules
[task 2018-07-09T09:43:19.063Z] + ln -s ../tools/lint/eslint/eslint-plugin-spidermonkey-js node_modules
[task 2018-07-09T09:43:19.063Z] + ./mach lint -l eslint -f treeherder --quiet
[task 2018-07-09T09:43:19.792Z] New python executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python2.7
[task 2018-07-09T09:43:19.792Z] Also creating executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python
[task 2018-07-09T09:43:21.344Z] Installing setuptools, pip, wheel...done.
[task 2018-07-09T09:43:22.430Z] running build_ext
[task 2018-07-09T09:43:22.430Z] building 'psutil._psutil_linux' extension
[task 2018-07-09T09:43:22.430Z] creating build
[task 2018-07-09T09:43:22.430Z] creating build/temp.linux-x86_64-2.7
[task 2018-07-09T09:43:22.430Z] creating build/temp.linux-x86_64-2.7/psutil
[task 2018-07-09T09:43:22.430Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-07-09T09:43:22.430Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-07-09T09:43:22.430Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o
[task 2018-07-09T09:43:22.430Z] creating build/lib.linux-x86_64-2.7
[task 2018-07-09T09:43:22.430Z] creating build/lib.linux-x86_64-2.7/psutil
[task 2018-07-09T09:43:22.430Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so
[task 2018-07-09T09:43:22.430Z] building 'psutil._psutil_posix' extension
[task 2018-07-09T09:43:22.430Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-07-09T09:43:22.430Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-07-09T09:43:22.430Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2018-07-09T09:43:22.430Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2018-07-09T09:43:22.430Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-07-09T09:43:22.430Z] 
[task 2018-07-09T09:43:22.430Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-07-09T09:48:05.298Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/telemetry/tests/unit/test_TelemetryController.js:177:11 | 'snapshot' is already declared in the upper scope. (no-shadow)
[taskcluster 2018-07-09 09:48:05.714Z] === Task Finished ===
[taskcluster 2018-07-09 09:48:05.715Z] Unsuccessful task run with exit code: 1 completed in 511.35 seconds
Flags: needinfo?(jrediger)
lint failure fixed, xpcshell failures fixed. Try run for Linux: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5263bb29e624d005dcd1b783f1557443f60de3dd

Waiting for it before landing again.
Flags: needinfo?(jrediger)
Pushed by jrediger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f157650a9f5
Allow client ID to be set and reset. r=Dexter
Pushed by jrediger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0218f7afb67f
Implement and test new optout ping logic. r=Dexter,chutten
Pushed by jrediger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd46cd235092
Test for canary client id in received pings. r=Dexter
See Also: → 1489520
You need to log in before you can comment on or make changes to this bug.