1137353 - Port the client id loading from DRS to TelemetryPing

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

If we want to turn off FHR & DRS on desktop in 38 or later, we need to port the client id loading over to TelemetryPing.jsm.

See _loadClientId etc. here:
https://hg.mozilla.org/mozilla-central/annotate/df3daecd381f/services/datareporting/DataReportingService.js#l305

We will want to do this if we don't have the datareporting service here or if it is disabled:
https://hg.mozilla.org/mozilla-central/annotate/df3daecd381f/toolkit/components/telemetry/TelemetryPing.jsm#l730

I think we should be good with reusing the same file DRS used in this case as we intent to retire that code (at least for desktop).

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1137353.patch

This patch allows TelemetryPing to load/generate a new client id at startup if FHR/DRS is unavailable or disabled.

It also adds the relative test to test_TelemetryPing.js

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8583121 [details] [diff] [review]
bug1137353.patch

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

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +711,5 @@
>          }
>  
> +        // Only use the data reporting service if it's available and enabled.
> +        if ("@mozilla.org/datareporting/service;1" in Cc &&
> +            Preferences.get(PREF_FHR_SERVICE_ENABLED, false)) {

Let's just move all the client id loading logic into a seperate function, say, _loadClientId().

@@ +721,5 @@
>            Preferences.set(PREF_CACHED_CLIENTID, this._clientID);
>          } else {
> +          // DRS is not available or disabled, try to load the client id
> +          // ourselves or generate a new one.
> +          yield this._fetchClientID().then(

Why .then() when you yield?
We can have the client id loading be infallible like the _loadClientId in DRS, so we won't need the reject handler.

@@ +838,5 @@
> +   *
> +   * @return {Promise} Resolved with the newly generated client ID or the one loaded
> +   *                   from the state file (if available).
> +   */
> +  _fetchClientID: Task.async(function* () {

This should mostly be a modified copy of the _loadClientId in DRS.
I'm missing some of the comments (valuable history) and the fallback loading from healthreport/state.json.

@@ +851,5 @@
> +        this._clientID = state.clientID;
> +        return this._clientID;
> +      }
> +    } catch (e) {
> +      this._log.error("_fetchClientID - Cannot load the clientID from " + clientIDFile, e);

That is not an error, it could just mean we are on a new profile.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1137353.patch - v2

Thanks for your input. I've addressed your comments in this patch.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8584422 [details] [diff] [review]
bug1137353.patch - v2

Cancelled per IRC.

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Create the ClientIDProvider module - v3

This patch introduces a new module, the ClientIDProvider, which handles the client ID loading/saving mechanism.

I've retained all the code and the logic from the DRS, as this was heavily tested already.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Make Telemetry and DRS use the ClientIDProvider.

This second patch makes TelemetryPing and the DataReportingService use the client id provider.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8586014 [details] [diff] [review]
part 1 - Create the ClientIDProvider module - v3

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

Nit: Maybe we can just name this (and the export) ClientID without the Provider?

::: toolkit/modules/ClientIDProvider.jsm
@@ +8,5 @@
> +
> +const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
> +
> +Cu.import("resource://gre/modules/osfile.jsm");
> +Cu.import("resource://gre/modules/Preferences.jsm");

Unused.

@@ +15,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "CommonUtils",
> +                                  "resource://services-common/utils.js");
> +
> +XPCOMUtils.defineLazyGetter(this, "DATAREPORTING_PATH", () => {

All-upper-case is for constants, gDatareportingPath?

@@ +19,5 @@
> +XPCOMUtils.defineLazyGetter(this, "DATAREPORTING_PATH", () => {
> +  return OS.Path.join(OS.Constants.Path.profileDir, "datareporting");
> +});
> +
> +XPCOMUtils.defineLazyGetter(this, "CLIENTID_FILE_PATH", () => {

All-upper-case is for constants, gClientIdFilePath?

@@ +29,5 @@
> +   * Get the stable client ID, loading it from the client ID file or generating
> +   * a new one if no previous client ID can be found.
> +   * @return {Promise<string>} A promise resolved with the client id.
> +   */
> +  get clientID() {

Let's keep this a function - it returns a promise and not directly a value.
I don't think the doc comment needs to include details on how we do it. Maybe just re-use the comment from the DRS getClientID() that has some background?

@@ +34,5 @@
> +    return ClientIDProviderImpl.getClientID();
> +  },
> +
> +  /**
> +   * Generates a new client ID and saves it to the client ID file.

Nit: No need to mention implementation details.

@@ +60,5 @@
> +    if (this._loadClientIdTask) {
> +      return this._loadClientIdTask;
> +    }
> +
> +    this._loadClientIdTask = Task.spawn(function* () {

While we're at it, let's avoid nesting and move this to say |_doLoadClient: Task.async(function*() ...|.

@@ +65,5 @@
> +      // Previously we had the stable client ID managed in FHR.
> +      // As we want to start correlating FHR and telemetry data (and moving towards
> +      // unifying the two), we moved the ID management to the datareporting
> +      // service. Consequently, we try to import the FHR ID first, so we can keep
> +      // using it.

That second sentence is outdated.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8586014 [details] [diff] [review]
part 1 - Create the ClientIDProvider module - v3

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

Nit: Maybe we can just name this (and the export) ClientID without the Provider?

::: toolkit/modules/ClientIDProvider.jsm
@@ +45,5 @@
> +  /**
> +   * Only used for testing. Invalidates the client ID so that it gets read
> +   * again from file.
> +   */
> +  _resetProvider: function() {

Just _reset() is fine.

@@ +156,5 @@
> +
> +  /*
> +   * Resets the provider. This is for testing only.
> +   */
> +  _resetProvider: Task.async(function* () {

Just _reset?

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8586016 [details] [diff] [review]
part 2 - Make Telemetry and DRS use the ClientIDProvider.

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

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +546,3 @@
>  
> +  // Load the client ID from the client ID provider to check for pings sanity.
> +  gClientID = yield ClientIDProvider.clientID;

You removed a part of the test here, you need to keep that - that tests for an important bug we fixed.

::: toolkit/modules/tests/xpcshell/test_client_id.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Again, i really like specific diffs when you move files and do only minimal changes.
Otherwise i have to check the whole file or build one myself.

Comment 10

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> ::: toolkit/modules/tests/xpcshell/test_client_id.js
> @@ +1,3 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> 
> Again, i really like specific diffs when you move files and do only minimal
> changes.
> Otherwise i have to check the whole file or build one myself.

(Note that i already checked it myself for this review)

Comment 11

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> Comment on attachment 8586016 [details] [diff] [review]
> part 2 - Make Telemetry and DRS use the ClientIDProvider.
> 
> Review of attachment 8586016 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
> @@ +546,3 @@
> >  
> > +  // Load the client ID from the client ID provider to check for pings sanity.
> > +  gClientID = yield ClientIDProvider.clientID;
> 
> You removed a part of the test here, you need to keep that - that tests for
> an important bug we fixed.

That part of the test, checking for a cached client ID, is already covered by test_TelemetryPing.js. It was duplicated in test_TelemetrySession.js, so I removed it.

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Create the ClientIDProvider module - v4

This patch addresses the comments for part 1.

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Make Telemetry and DRS use the ClientIDProvider - v2

This patch only changes:

- |ClientIDProvider| to |ClientID|
- |ClientIDProvider.clientID| to |ClientID.getClientID()|

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: test_client_id.diff

Diff generated using diff -U8.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8587438 [details] [diff] [review]
test_client_id.diff

This had different formatting for the old file version (the indentation was lost), so everything showed up as a change.

But, as mentioned above, i built a diff myself for this review here, so all good.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8587429 [details] [diff] [review]
part 1 - Create the ClientIDProvider module - v4

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

::: toolkit/modules/ClientID.jsm
@@ +18,5 @@
> +XPCOMUtils.defineLazyGetter(this, "gDatareportingPath", () => {
> +  return OS.Path.join(OS.Constants.Path.profileDir, "datareporting");
> +});
> +
> +XPCOMUtils.defineLazyGetter(this, "gClientIdFilePath", () => {

I missed this the last round - gStateFilePath would be better, the file is not necessarily limited to the client id.

@@ +67,5 @@
> +    this._loadClientIdTask.then(clear, clear);
> +    return this._loadClientIdTask;
> +  },
> +
> +  _doLoadClient: Task.async(function* () {

_doLoadClientId

@@ +70,5 @@
> +
> +  _doLoadClient: Task.async(function* () {
> +    // Previously we had the stable client ID managed in FHR.
> +    // As we want to start correlating FHR and telemetry data (and moving towards
> +    // unifying the two), we try to import the FHR ID first, so we can keep using it.

Ok, that still doesn't seem quite right:
// As we want to correlate FHR and telemetry data (and move towards
// unifying the two), we first moved the ID management from the FHR
// implementation to the datareporting service, then to a common
// shared module.
// Consequently, we try to import an existing FHR ID, so we can keep
// using it.

Comment 17

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Create the ClientIDProvider module - v5

Thanks for the review, I've addressed your comments and rebased the patch.

Comment 18

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Make Telemetry and DRS use the ClientIDProvider - v2

Rebased the patch.

Comment 19

[Alessio Placitelli [:Dexter]]

Try push is hulk-green, except for some unrelated oranges.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b27e3db7c08c

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/5a0d14476706
https://hg.mozilla.org/integration/fx-team/rev/0c4f5e5205b3

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5a0d14476706
https://hg.mozilla.org/mozilla-central/rev/0c4f5e5205b3