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

VERIFIED FIXED in Firefox 41

Status

defect
VERIFIED FIXED
5 years ago
9 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
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)
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: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attachment #8584508 - Flags: review?(gfritzsche)
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?
(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: nobody → alessio.placitelli
Attachment #8584508 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8612335 - Flags: review?(gfritzsche)
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+
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)
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)
Attachment #8614111 - Attachment is obsolete: true
Attachment #8614608 - Flags: review?(gfritzsche)
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+
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
Rebased the patch.
Attachment #8614098 - Attachment is obsolete: true
Attachment #8622940 - Flags: review+
Rebased
Attachment #8614608 - Attachment is obsolete: true
Attachment #8622943 - Flags: review+
Reporter

Updated

4 years ago
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

Updated

9 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.