968419 - Persistent document IDs

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

The grapevine says we may be doing persistent document IDs for FHR to help with the orphaned record issue.

I'm ready to implement this change today. If we do it in the next week or two, we might even be able to uplift to Beta 28 and get this to the release channel by mid March.

needinfo on bsmedberg to say the magic words.

Comment 1

[Benjamin Smedberg]

Watch the fhr-dev list. We clearly want to move to a stable ID because the rotating ID has failed and didn't provide any real privacy benefits. We need to work out some details about the transition plan.

Comment 2

[Gregory Szorc [:gps]]

Attachment: Store and submit a persistent health report identifier

Wrote this on a plane Thursday. Wifi at the cabin didn't work. Hopefully
there's enough time to get this uplifted...

rnewman is primary reviewer. bsmedberg's r? is mostly for sign-off.

"clientIDVersion" is the one piece I anticipate feedback on. I didn't
want to incur a global state version bump for this addition because it's
backwards compatible. I wanted to leave the door open for changing the
semantics of how the client ID is generated.

---

Up to this point, Firefox Health Report has generated and submitted a
random UUID with each upload. Generated UUIDs were stored on the client.
During upload, the client asked the server to delete all old UUIDs.
Well-behaving clients thus left at most one record/ID on the server.

Unfortunately, clients in the wild have not been behaving properly. We
are seeing multiple documents on the server that appear to come from the
same client. Clients are uploading new records but failing to delete the
old ones. These old, undeleted "orphan" records are severely impacting
the ability to derive useful knowledge from FHR data because it is
difficult, resource intensive, and error prone to filter the records on
the server. This is undermining the ability for FHR data to be put to
good use.

This patch introduces a persistent client identifier. When the client is
initialized, it generates a random UUID. That UUID is persisted to the
profile and sent as part of every upload.

For privacy reasons, if a client opts out of data submission, the client
ID will be reset as soon as all remote data has been deleted.

We still issue and send upload IDs. They exist mostly for forensics
purposes so we may log client behavior and more accurately determine
what exactly misbehaving, orphan-producing clients are doing.

It is worth noting that this persistent client identifier will not solve
all problems of branching and orphaned records. For example, profile
copying will result in multiple clients sharing a client identifier. A
"client ID version" field has been added to facilitate an upgrade path
towards client IDs with different generation semantics.

Comment 3

[Benjamin Smedberg]

Comment on attachment 8380639 [details] [diff] [review]
Store and submit a persistent health report identifier

I'm not convinced that we need clientIDVersion. If we do add a non-random ID in the future, we'd want to give it a different name like "machineIDHash" or whatever it actually is.

r=me but this has to wait for legal signoff before actually landing.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8380639 [details] [diff] [review]
Store and submit a persistent health report identifier

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

::: services/healthreport/healthreporter.jsm
@@ +59,5 @@
>   * Helper type to assist with management of Health Reporter state.
>   *
>   * Instances are not meant to be created outside of a HealthReporter instance.
>   *
> + * There are types of IDs associated with clients.

s/types/two types

@@ +100,5 @@
> +  /**
> +   * Persistent string identifier associated with this client.
> +   */
> +  get clientID() {
> +    return this._s.clientID;

Naming. Device ID, uploader ID, longitudinalID?

@@ +107,5 @@
> +  /**
> +   * The version associated with the client ID.
> +   */
> +  get clientIDVersion() {
> +    return this._s.clientIDVersion;

I'm personally fine with versioning this, so long as we name the thing being versioned appropriately!

@@ +187,5 @@
>        }
>  
> +      // Generate a client ID if one isn't present.
> +      if (!this._s.clientID) {
> +        this._log.warn("No client ID stored. Generating random ID.");

This *should* never happen if we've ever uploaded data. Record that?

::: services/healthreport/tests/xpcshell/test_healthreporter.js
@@ +1120,5 @@
>      reporter._shutdown();
>    }
>  });
> +
> +// Missing client ID in state should be created on state load.

Can we get a test for a malformed ID, too? (null, empty string, number)

Comment 5

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #4)
> @@ +100,5 @@
> > +  /**
> > +   * Persistent string identifier associated with this client.
> > +   */
> > +  get clientID() {
> > +    return this._s.clientID;
> 
> Naming. Device ID, uploader ID, longitudinalID?

Device ID won't hold since you could have multiple profiles per device.

uploader ID and longitudinal ID both make as much sense as client ID to me.
 
> @@ +187,5 @@
> >        }
> >  
> > +      // Generate a client ID if one isn't present.
> > +      if (!this._s.clientID) {
> > +        this._log.warn("No client ID stored. Generating random ID.");
> 
> This *should* never happen if we've ever uploaded data. Record that?

Most likely won't happen. It's valid if:

1) Client installs Firefox 20
2) <24h later (before initial upload), client upgrades to Firefox 30
3) This code runs

> ::: services/healthreport/tests/xpcshell/test_healthreporter.js
> @@ +1120,5 @@
> >      reporter._shutdown();
> >    }
> >  });
> > +
> > +// Missing client ID in state should be created on state load.
> 
> Can we get a test for a malformed ID, too? (null, empty string, number)

Sure.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Store and submit a persistent health report identifier

Added some more tests for empty/bad clientID value. I still call it
"client ID."

Comment 7

[Gregory Szorc [:gps]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> Comment on attachment 8380639 [details] [diff] [review]
> Store and submit a persistent health report identifier
> 
> I'm not convinced that we need clientIDVersion. If we do add a non-random ID
> in the future, we'd want to give it a different name like "machineIDHash" or
> whatever it actually is.

If we ditch clientIDVersion and allocate a new property to a subsequent generation mechanism, that has bad side-effects for downgraded clients:

1) Client upgrades to future Firefox with "machineIDHash" in the state
2) FHR removes clientID from state and generates machineIDHash in its place
3) Client starts uploading machineIDHash
4) Client experiences a lot of crashes and decides to downgrade to fix the problem
5) Old FHR sees state with machineIDHash but no clientID. It doesn't know anything about machineIDHash, just that clientID is not present
6) Client generates new clientID
7) Client starts uploading clientID
8) clientID and machineIDHash are now on the server*, creating orphans

* As long as we have upload IDs, the client *should* delete old documents and their associated clientID/machineIDHash. However, that relies on that mechanism being bulletproof. And we know from the rationale for this bug that it is not.

Contrast this with a versioned client ID:

1) Client upgrades to future Firefox supporting client ID v2
2) FHR regenerates clientID using new algorithm, let's call it clientID'. Marks client ID version as 2
3) Client uploads clientID'. Presumably it also uploads something that says "I used to be clientID" (we don't need to implement that until the client supports changing client ID)
4) Client experiences a lot of crashes and downgrades
5) Old FHR sees clientID' with version 2. It happily ignores the version difference because it doesn't need to regenerate the client ID.
6) Old FHR continues uploading clientID'.
7) No orphans - yay!

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8381013 [details] [diff] [review]
Store and submit a persistent health report identifier

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

::: services/healthreport/docs/dataformat.rst
@@ +385,5 @@
>  
> +clientID
> +    An identifier that uniquely identifies the client that is submitting data.
> +
> +    This property may not be present in older clients.

Worth noting that this doesn't have a 1:1 mapping with clients (it can change if they turn off FHR then back on again), nor is it actually unique (because profiles can be copied). Both of those are useful things to know when doing analysis.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8381013 [details] [diff] [review]
Store and submit a persistent health report identifier

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

::: services/healthreport/healthreporter.jsm
@@ +185,5 @@
>          // We explicitly don't save here in the hopes an application re-upgrade
>          // comes along and fixes us.
>        }
>  
> +      // Generate a client ID if:

Hm, if what?

Comment 10

[Gregory Szorc [:gps]]

Attachment: Store and submit a persistent health report identifier

I added a new rst page documenting the different identifiers and a
history of the problems. It's far from complete, but better than
nothing.

Comment 11

[Robert Kaiser]

(In reply to Gregory Szorc [:gps] from comment #5)
> (In reply to Richard Newman [:rnewman] from comment #4)
> > Naming. Device ID, uploader ID, longitudinalID?
> 
> Device ID won't hold since you could have multiple profiles per device.
> 
> uploader ID and longitudinal ID both make as much sense as client ID to me.

profile ID? or by function instead, e.g. deduplication ID? stable ID?

Comment 12

[David E. Ross]

Is the persistent ID something that non-Mozilla Web sites can access?  If so, this would defeat the purpose of bug #572650.  Actually, it would make the tracking of users worse than before any part of bug #572650 was implemented.

Comment 13

[Benjamin Smedberg]

No, of course not. This is specifically about the Firefox Health Report.

Comment 14

[Nick Alexander :nalexander [he/him]]

I filed Bug 981698 to track the equivalent work for the Android client.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8381533 [details] [diff] [review]
Store and submit a persistent health report identifier

Please grant r+ when this is ready to land. Please clarify which trees this should land on when you do.

Comment 16

[Benjamin Smedberg]

Comment on attachment 8381533 [details] [diff] [review]
Store and submit a persistent health report identifier

This is ready to land to nightly. After a few days of bake we'll ask for aurora but not beta.

Comment 17

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/fx-team/rev/0202df4146f5

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0202df4146f5

Comment 19

[Benjamin Smedberg]

Comment on attachment 8381533 [details] [diff] [review]
Store and submit a persistent health report identifier

[Approval Request Comment]
Bug caused by (feature/regressing bug #): longstanding FHR issue
User impact if declined: worse FHR metrics
Testing completed (on m-c, etc.): I did some basic data verification from the nightly landing: all appears good
Risk to taking this patch (and alternatives if risky): fairly low risk, and all risk is to FHR not to anything outside
String or IDL/UUID changes made by this patch: none

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/751264a8a3a0