Closed
Bug 1137353
Opened 10 years ago
Closed 10 years ago
Port the client id loading from DRS to TelemetryPing
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: gfritzsche, Assigned: Dexter)
References
Details
Attachments
(2 files, 7 obsolete files)
6.01 KB,
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
26.04 KB,
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
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).
Assignee | ||
Comment 1•10 years ago
|
||
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
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attachment #8583121 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 2•10 years ago
|
||
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.
Attachment #8583121 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 3•10 years ago
|
||
Thanks for your input. I've addressed your comments in this patch.
Attachment #8583121 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8584422 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 4•10 years ago
|
||
Attachment #8584422 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 5•10 years ago
|
||
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.
Attachment #8584422 -
Attachment is obsolete: true
Attachment #8586014 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 6•10 years ago
|
||
This second patch makes TelemetryPing and the DataReportingService use the client id provider.
Attachment #8586016 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 7•10 years ago
|
||
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.
Attachment #8586014 -
Flags: review?(gfritzsche) → feedback+
Reporter | ||
Comment 8•10 years ago
|
||
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?
Reporter | ||
Comment 9•10 years ago
|
||
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.
Attachment #8586016 -
Flags: review?(gfritzsche) → feedback+
Reporter | ||
Comment 10•10 years ago
|
||
(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)
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Assignee | ||
Comment 12•10 years ago
|
||
This patch addresses the comments for part 1.
Attachment #8586014 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8587429 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 13•10 years ago
|
||
This patch only changes:
- |ClientIDProvider| to |ClientID|
- |ClientIDProvider.clientID| to |ClientID.getClientID()|
Attachment #8586016 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8587431 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 14•10 years ago
|
||
Diff generated using diff -U8.
Reporter | ||
Comment 15•10 years ago
|
||
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.
Reporter | ||
Updated•10 years ago
|
Attachment #8587431 -
Flags: review?(gfritzsche) → review+
Reporter | ||
Comment 16•10 years ago
|
||
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.
Attachment #8587429 -
Flags: review?(gfritzsche) → review+
Assignee | ||
Comment 17•10 years ago
|
||
Thanks for the review, I've addressed your comments and rebased the patch.
Attachment #8587429 -
Attachment is obsolete: true
Attachment #8587852 -
Flags: review+
Assignee | ||
Comment 18•10 years ago
|
||
Rebased the patch.
Attachment #8587431 -
Attachment is obsolete: true
Attachment #8587438 -
Attachment is obsolete: true
Attachment #8587853 -
Flags: review+
Assignee | ||
Comment 19•10 years ago
|
||
Try push is hulk-green, except for some unrelated oranges.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b27e3db7c08c
Keywords: checkin-needed
Comment 20•10 years ago
|
||
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5a0d14476706
https://hg.mozilla.org/mozilla-central/rev/0c4f5e5205b3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•