Telemetry needs to send ping deletion messages to the server when FHR is deactivated

VERIFIED FIXED in Firefox 41

Status

VERIFIED FIXED
4 years ago
2 months ago

People

(Reporter: gfritzsche, Assigned: Dexter)

Tracking

Trunk
Firefox 41
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected, firefox41 verified)

Details

(Whiteboard: [b5] [unifiedTelemetry] )

Attachments

(3 attachments, 9 obsolete attachments)

8.66 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
4.52 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
2.84 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
When Telemetry is turned off by a user, the client needs to send a deletion message to the server, requesting that all associated data for the user will be deleted.
(Reporter)

Comment 1

4 years ago
Clarification: bug 1075055 makes FHR & Telemetry dependent pref-wise. This only needs to send the deletion ping when FHR is turned off.
(Reporter)

Comment 2

4 years ago
For this TelemetryPing should add a pref observer for PREF_FHR_UPLOAD_ENABLED.
That observer would trigger sending a deletion ping with the client id, no environment and an empty payload:

> this.send(PING_TYPE_DELETION, {}, { addClientId: true });

We need to update the documentation here:
https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/toolkit/components/telemetry/telemetry/pings.html#ping-types
... and should add a simple deletion-ping.rst similar to main-ping.rst describing the motivation and format.
(Reporter)

Comment 3

4 years ago
Note that we need to init the pref observer early, but can't send the ping until the delayed initialization finished.
If this._initialized==false we should set a flag, say _sendDeletionPing. The _delayedInitTask should check this once things are set up and send one.

Edge-case: The pref changed between Firefox sessions (e.g. manual profile modification or 3rd party).
(Reporter)

Comment 4

4 years ago
(In reply to Georg Fritzsche [:gfritzsche] [away Feb 27 - March 15] from comment #3)
> Edge-case: The pref changed between Firefox sessions (e.g. manual profile
> modification or 3rd party).

Do we need to care about this?
Flags: needinfo?(benjamin)
(Reporter)

Updated

4 years ago
Whiteboard: [help]
(Reporter)

Updated

4 years ago
Summary: Telemetry needs to send ping deletion messages to the server when deactivated → Telemetry needs to send ping deletion messages to the server when FHR is deactivated

Comment 5

4 years ago
Changing from outside Firefox? No.
Flags: needinfo?(benjamin)
(Assignee)

Comment 6

4 years ago
Should we delete the pings saved locally when this preference is switched to false?
Flags: needinfo?(benjamin)

Comment 7

4 years ago
No. In fact, we should be saving the pings locally always, no matter what the preference is set to. But I forgot to list that as a requirement, so I'll file that as a followup.
Flags: needinfo?(benjamin)
(Assignee)

Comment 8

4 years ago
Created attachment 8584508 [details] [diff] [review]
part 1 - Send DELETE PING when disabling FHR upload
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
(Assignee)

Comment 9

4 years ago
Created attachment 8584509 [details] [diff] [review]
part 2 - Add test coverage for the delete pings.
(Assignee)

Updated

4 years ago
Attachment #8584508 - Flags: review?(gfritzsche)
(Assignee)

Updated

4 years ago
Attachment #8584509 - Flags: review?(gfritzsche)
(Reporter)

Comment 10

4 years ago
Comment on attachment 8584508 [details] [diff] [review]
part 1 - Send DELETE PING when disabling FHR upload

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

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +888,5 @@
> +   * pings are not sent to the server (Telemetry is a sub-feature of FHR). If trying
> +   * to send a deletion ping, don't block it.
> +   *
> +   * @param {Object} [ping=null] A ping to be checked.
> +   * @return {Boolean} True if we are allwed to send pings to the server, false otherwise.

Nit: allwed vs. allowed.

@@ +893,3 @@
>     */
> +  _canSend: function(ping = null) {
> +    // Deletion pings are sent when

when...?

@@ +893,5 @@
>     */
> +  _canSend: function(ping = null) {
> +    // Deletion pings are sent when
> +    let deletionPing = !!ping && isNewPingFormat(ping) &&
> +                       (ping.type == PING_TYPE_DELETION);

const isDeletionPing = ...
Would probably be cleaner though as just a helper isDeletionPing(ping).

@@ +898,2 @@
>      return (Telemetry.isOfficialTelemetry || this._testMode) &&
> +           (Preferences.get(PREF_FHR_UPLOAD_ENABLED, false) || deletionPing);

This would be more readable if its not all in one condition.
E.g. early-out:
if (isDeletionPing(ping)) {
  return true;
}

...

@@ +923,5 @@
> +   * Called whenever the FHR Upload preference changes (e.g. when user disables FHR from
> +   * the preferences panel), this triggers sending the deletion ping.
> +   */
> +  _onUploadPrefChange: function() {
> +    let uploadEnabled = Preferences.get(PREF_FHR_UPLOAD_ENABLED, false);

const

@@ +931,5 @@
> +    }
> +
> +    if (!this._initialized) {
> +      this._sendDeletionPing = true;
> +      this._log.trace("_onUploadPrefChange - Scheduling deletion ping after the initialization.");

In bug 1149754, we need to make at least TelemetryPing.send() work before init anyway (i.e. allowing to add pending pings before full TelemetryPing init).

Maybe we should just base this bug on that and drop juggling with the flag here?
Attachment #8584508 - Flags: review?(gfritzsche) → feedback+
(Reporter)

Comment 11

4 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> @@ +931,5 @@
> > +    }
> > +
> > +    if (!this._initialized) {
> > +      this._sendDeletionPing = true;
> > +      this._log.trace("_onUploadPrefChange - Scheduling deletion ping after the initialization.");
> 
> In bug 1149754, we need to make at least TelemetryPing.send() work before
> init anyway (i.e. allowing to add pending pings before full TelemetryPing
> init).
> 
> Maybe we should just base this bug on that and drop juggling with the flag
> here?

Actually, what does fail right now if we try to send early? Did you check?
(Assignee)

Comment 12

4 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> > In bug 1149754, we need to make at least TelemetryPing.send() work before
> > init anyway (i.e. allowing to add pending pings before full TelemetryPing
> > init).
> > 
> > Maybe we should just base this bug on that and drop juggling with the flag
> > here?
> 
> Actually, what does fail right now if we try to send early? Did you check?

That's a good point. Yes, I tried that, we might not have the client id before we complete the initialisation (unless it was cached).
(Reporter)

Comment 13

4 years ago
(In reply to Alessio Placitelli [:Dexter] from comment #12)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> > > In bug 1149754, we need to make at least TelemetryPing.send() work before
> > > init anyway (i.e. allowing to add pending pings before full TelemetryPing
> > > init).
> > > 
> > > Maybe we should just base this bug on that and drop juggling with the flag
> > > here?
> > 
> > Actually, what does fail right now if we try to send early? Did you check?
> 
> That's a good point. Yes, I tried that, we might not have the client id
> before we complete the initialisation (unless it was cached).

Ok, that's a general issue that we should solve independently of this bug (e.g. a ping could also be submitted early that wants the environment submitted too).
Maybe what we need is to keep track of all ping submissions before full init (with the payload, options, ...) and process them after init?
(Reporter)

Updated

4 years ago
Depends on: 1149754
(Reporter)

Comment 14

4 years ago
Comment on attachment 8584509 [details] [diff] [review]
part 2 - Add test coverage for the delete pings.

Cancelling until next round.
Attachment #8584509 - Flags: review?(gfritzsche)
(Reporter)

Comment 15

4 years ago
Unassigning for now as it is blocked.
Assignee: alessio.placitelli → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 16

4 years ago
Created attachment 8612335 [details] [diff] [review]
part 1 - Send DELETE PING when disabling FHR upload - v2
Assignee: nobody → alessio.placitelli
Attachment #8584508 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8612335 - Flags: review?(gfritzsche)
(Assignee)

Comment 17

4 years ago
Created attachment 8612336 [details] [diff] [review]
part 2 - Add test coverage for the delete pings. - v2

This really just updates the patch.
Attachment #8584509 - Attachment is obsolete: true
Attachment #8612336 - Flags: review?(gfritzsche)
(Reporter)

Comment 18

4 years ago
Comment on attachment 8612335 [details] [diff] [review]
part 1 - Send DELETE PING when disabling FHR upload - v2

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

This is missing a documentation update, adding a deletion-ping.rst that documents what this ping is used for, when it is sent and the data it contains.

If you haven't already, please take this through some manual testing with different pref-value scenarios.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +143,5 @@
> + * @param {Object} aPing The ping to check.
> + * @return {Boolean} True if the ping is a deletion ping, false otherwise.
> + */
> +function isDeletionPing(aPing) {
> +  return !!aPing && isNewPingFormat(aPing) && (aPing.type == PING_TYPE_DELETION);

I don't think this makes sense to be called without an argument.
Lets do the check for the ping object being non-null at the caller.

@@ +998,5 @@
>      }
>  
> +    if (IS_UNIFIED_TELEMETRY) {
> +      // Watch the FHR upload setting to trigger deletion pings.
> +      Preferences.observe(PREF_FHR_UPLOAD_ENABLED, this._onUploadPrefChange, this);

It's odd to have a separate function for unregistering, but not registering.
Make it _attach/_detachObservers or *Listeners?

@@ +1171,5 @@
>      // With unified Telemetry, the FHR upload setting controls whether we can send pings.
>      // The Telemetry pref enables sending extended data sets instead.
>      if (IS_UNIFIED_TELEMETRY) {
> +      // Deletion pings are sent even if the upload is disabled.
> +      if (isDeletionPing(ping)) {

Only call this if we have a valid ping instance.

@@ +1225,5 @@
>  
>    /**
> +   * Remove the preference observer to avoid leaks.
> +   */
> +  _uninstall: function() {

_uninstall is very generic, _attach/_detachObservers?
Attachment #8612335 - Flags: review?(gfritzsche) → feedback+
(Reporter)

Comment 19

4 years ago
Comment on attachment 8612336 [details] [diff] [review]
part 2 - Add test coverage for the delete pings. - v2

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

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js
@@ +177,5 @@
> +add_task(function* test_deletionPing() {
> +  // Disable FHR upload: this should trigger a deletion ping.
> +  Preferences.set(PREF_FHR_UPLOAD_ENABLED, false);
> +
> +  let request = yield gRequestIterator.next();

You check isUnified below but not here?
This will probably blow up on try.
Attachment #8612336 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 20

4 years ago
Created attachment 8614090 [details] [diff] [review]
part 2 - Add test coverage for the delete pings. - v3
Attachment #8612336 - Attachment is obsolete: true
Attachment #8614090 - Flags: review+
(Assignee)

Comment 21

4 years ago
Created attachment 8614098 [details] [diff] [review]
part 1 - Send DELETE PING when disabling FHR upload - v3

Thanks Georg, I've addressed you comments. The documentation will follow up as a separate patch.
Attachment #8612335 - Attachment is obsolete: true
Attachment #8614098 - Flags: review?(gfritzsche)
(Assignee)

Comment 22

4 years ago
Created attachment 8614111 [details] [diff] [review]
part 3 -  Update the documentation
Attachment #8614111 - Flags: review?(gfritzsche)
(Reporter)

Comment 23

4 years ago
Comment on attachment 8614098 [details] [diff] [review]
part 1 - Send DELETE PING when disabling FHR upload - v3

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

r=me with the nit addressed and proper manual testing to ensure this doesn't get sent unexpectedly.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +1152,5 @@
>  
>    /**
>     * Check if pings can be sent to the server. If FHR is not allowed to upload,
> +   * pings are not sent to the server (Telemetry is a sub-feature of FHR). If trying
> +   * to send a deletion ping, don't block it.

Nit: That should probably be written clearer.
"Pings of type 'deletion' are an exception, we should always be able to send them to enable clearing out profile data on the server." ?
Attachment #8614098 - Flags: review?(gfritzsche) → review+
(Reporter)

Comment 24

4 years ago
Comment on attachment 8614111 [details] [diff] [review]
part 3 -  Update the documentation

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

::: toolkit/components/telemetry/docs/deletion-ping.rst
@@ +1,5 @@
> +
> +"deletion" ping
> +===============
> +
> +This ping is generated if a user decides to turn off FHR/Telemetry reporting from the Preferences panel. The server will start the user data removal procedure upon reception.

"... when a user turns off FHR upload ..." & mention the associated pref?
+ "This requests that all associated data from that user be deleted." (-> let's not spec out server details here)

@@ +3,5 @@
> +===============
> +
> +This ping is generated if a user decides to turn off FHR/Telemetry reporting from the Preferences panel. The server will start the user data removal procedure upon reception.
> +
> +This ping contains the client id and no environment data.

A ping example would be nice, see e.g. https://reviewboard.mozilla.org/r/9683/diff/1/#5

::: toolkit/components/telemetry/docs/pings.rst
@@ +31,5 @@
>  * :doc:`main <main-ping>` - contains the information collected by Telemetry (Histograms, hang stacks, ...)
>  * :doc:`saved-session <main-ping>` - has the same format as a main ping, but it contains the *"classic"* Telemetry payload with measurements covering the whole browser session. This is only a separate type to make storage of saved-session easier server-side.
>  * ``activation`` - *planned* - sent right after installation or profile creation
>  * ``upgrade`` - *planned* - sent right after an upgrade
> +* :doc:`deletion <deletion-ping>` - sent when user disables FHR/Telemetry reporting

"sent when FHR upload is disabled, requesting deletion of the data associated with this user"
Attachment #8614111 - Flags: review?(gfritzsche)
(Assignee)

Comment 25

4 years ago
Created attachment 8614608 [details] [diff] [review]
part 3 - Update the documentation - v2
Attachment #8614111 - Attachment is obsolete: true
Attachment #8614608 - Flags: review?(gfritzsche)
(Assignee)

Comment 26

4 years ago
I've covered the following test cases, manually:

- Disabling "Share additional data" (extended telemetry recording) through the Preferences panel doesn't trigger a deletion ping
- Enabling "Share additional data" (extended telemetry recording) through the Preferences panel  doesn't trigger a deletion ping
- Disabling FHR Upload through the Preferences panel triggers a deletion ping
- Enabling FHR Upload through the Preferences panel doesn't triggers a deletion ping
- Disabling FHR Upload by setting "datareporting.healthreport.uploadEnabled" to false triggers a deletion ping
- Enabling FHR Upload by setting "datareporting.healthreport.uploadEnabled" to true doesn't trigger a deletion ping
- Starting Firefox with FHR upload disable doesn't trigger a new deletion ping

Please feel free to let me know if you can think of any other cases that I'm missing.
(Reporter)

Updated

4 years ago
Attachment #8614608 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 27

4 years ago
Created attachment 8615887 [details] [diff] [review]
part 2 - Add test coverage for the delete pings. - v4

I've updated the patch to fix the fallout from this try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a604f72d825b

This changes the healthreport_testRemoteCOmnands.html to filter out pings which are not test pings (so that "deletion" pings don't make the test fail).
Attachment #8614090 - Attachment is obsolete: true
Attachment #8615887 - Flags: review+
Attachment #8615887 - Flags: feedback?(gfritzsche)
(Reporter)

Updated

4 years ago
Attachment #8615887 - Flags: feedback?(gfritzsche) → feedback+
(Reporter)

Updated

4 years ago
Depends on: 1174674

Updated

4 years ago
Blocks: 1174925
(Assignee)

Comment 29

4 years ago
Created attachment 8622940 [details] [diff] [review]
part 1 - Send DELETE PING when disabling FHR upload - v4

Rebased the patch.
Attachment #8614098 - Attachment is obsolete: true
Attachment #8622940 - Flags: review+
(Assignee)

Comment 30

4 years ago
Created attachment 8622942 [details] [diff] [review]
part 2 - Add test coverage for the delete pings. - v5

Rebased.
Attachment #8615887 - Attachment is obsolete: true
Attachment #8622942 - Flags: review+
(Assignee)

Comment 31

4 years ago
Created attachment 8622943 [details] [diff] [review]
part 3 - Update the documentation - v3

Rebased
Attachment #8614608 - Attachment is obsolete: true
Attachment #8622943 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/fbf0fdd5face
https://hg.mozilla.org/mozilla-central/rev/113c09b2fd55
https://hg.mozilla.org/mozilla-central/rev/6142099b3260
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
(Reporter)

Updated

4 years ago
status-firefox40: --- → affected
Whiteboard: [help] → [uplift]
(Reporter)

Updated

4 years ago
Whiteboard: [uplift] → [uplift2]
(Reporter)

Updated

4 years ago
Depends on: 1176006
(Reporter)

Updated

4 years ago
Whiteboard: [uplift2] → [b5] [unifiedTelemetry] [uplift2]
(Reporter)

Updated

4 years ago
Whiteboard: [b5] [unifiedTelemetry] [uplift2] → [b5] [unifiedTelemetry]
This bug is verified fixed on Firefox 41.0b6 (20150831172306), using Windows 10 x64, Mac OS X 10.10.4 and Ubuntu 14.04 x64.

Confirmed that:
• there's no deletion ping triggered for disabling "Share additional data (i.e., Telemetry)" from about:preferences#advanced → Data Choices
• there's no deletion ping triggered for enabling "Share additional data (i.e., Telemetry)" from about:preferences#advanced → Data Choices
• a deletion ping is triggered for disabling "Enable Firefox Health Report" from about:preferences#advanced → Data Choices
• there's no deletion ping triggered for enabling "Enable Firefox Health Report" from about:preferences#advanced → Data Choices
• a deletion ping is triggered for setting "datareporting.healthreport.uploadEnabled" to false from about:config
• there's no deletion ping triggered for setting "datareporting.healthreport.uploadEnabled" to true from about:config
• there's no deletion ping triggered for starting the browser with "datareporting.healthreport.uploadEnabled" already set to false
Status: RESOLVED → VERIFIED
status-firefox41: fixed → verified

Updated

2 months ago
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.