Closed Bug 1233986 Opened 6 years ago Closed 5 years ago

Move clientId loading, generation and serialization earlier during startup

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
3

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox46 --- affected
firefox50 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [measurement:client])

Attachments

(2 files, 13 obsolete files)

8.02 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
12.87 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
In bug 1221583 we found that a portion of crash pings is missing the clientId due to startup crashes on fresh profiles.

We could probably mitigate this issue by moving clientId loading, generation and serialization code earlier during startup, definitely before the 60s Telemetry delay.
Whiteboard: [measurement:client]
Priority: -- → P3
More information on the expected behaviour is on bug 1221583 comment 8.
(In reply to Benjamin Smedberg  [:bsmedberg] from bug 1221583 comment 8)
> If I do TelemetryController.submitExternalPing( { addClientId: true, }) it
> should not be possible for that ping to ever not have a clientId. If a
> clientId hasn't been generated yet, we should immediately generate the
> client ID and not submit the ping until it's ready.
Points: --- → 3
Priority: P3 → P1
Priority: P1 → P2
Priority: P2 → P3
Priority: P3 → P4
I want this.  I don't think this a known / expected issue.  At least generating the id early seems useful.

Use cases:

- funnelcake experiments that bundle addons, and phone in on startup
- testing / demonstration of experiments, from things like jpm.  (what is burning me)
Georg, I'm similary concerned: 6% of our crash pings still don't have a clientid, and that's going to skew data. IMO this needs to be at least a P3 and not pushed out indefinitely.
Flags: needinfo?(gfritzsche)
Ok, bumping it, we'll see where it fits.
FWIW, this was not pushed out indefinitely, just to next quarter.

If this is critical for us, i think we could:
(1) load the client id from disk early in startup if it is not cached yet and take the I/O hit
(2) mark those submitted pings as not having a client id yet and put it on the ping when sending (awkward & a bit fragile)
Flags: needinfo?(gfritzsche)
Priority: P4 → P3
Georg... alternative...

Maybe only construct the id 
- after `initDelay` OR
- whenever someone actually tries to submit a ping.

It seems... odd... that I can call `submitExternalPing` asking for a clientId.  It will work, except without an id.

(In:  https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryController.jsm#424-426)

My read is that getting / setting clientId is worth doing ASAP after startup, mostly b/c BDS note about startup crash stuff.  Maybe bootstrapping the CLIENT ID doesn't mean booting the whole telemetry system?


GL
Priority: P3 → P2
Priority: P2 → P1
After thinking about the possible scenarios, we came up with a series of approaches to fix the problem of the missing client ID for pings generated early during startup.

The simplest solution would be to generate/load/write the client id first thing during the Telemetry init, but that would imply hitting the disk at startup.

A second solution would be to stitch the client id on the ping later in TelemetrySend, before it gets sent (sending happens after Telemetry is initialized, so we have a client id by then).

Making the ping assembly logic async and wait for the client id to load is an option as well, but I seem to remember that we didn't want that.

Benjamin, do you think we can afford to take the I/O cost? Can you think of any other option?
Assignee: nobody → alessio.placitelli
Flags: needinfo?(benjamin)
(During testing of this code, i noticed delays of << .1 sec.)  


https://github.com/gregglind/shield-variations-gestalt/blob/master/x-addon/src/experiment-utils.js#L43-L50


```
function generateTelemetryIdIfNeeded() {
  let id = TelemetryController.clientID;
  if (id == undefined) {
    return CID.ClientIDImpl._doLoadClientID()
  } else {
    return Promise.resolve(id)
  }
}
```
submitExternalPing is already async, but assemblePing is not. You could assert that assemblePing already has a clientID available, and make sure that submitExternalPing forces the correct initialization.

I don't think we should block on synchronous I/O.
Flags: needinfo?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> submitExternalPing is already async, but assemblePing is not. You could
> assert that assemblePing already has a clientID available, and make sure
> that submitExternalPing forces the correct initialization.
> 
> I don't think we should block on synchronous I/O.

Current behavior, for context:
* if the id is cached in the pref it is available immediately (i.e. if the profile had at least one non-aborted session)
* otherwise, it is loaded after 60sec after "profile-after-change" is observed (and cached in the prefs)

We definitely shouldn't block on I/O, but we can either:
(1) wait for the client id to be loaded after the 60sec initialization delay or
(2) trigger the client id load much earlier during startup (if it is not cached yet)

If we did (1), we have two options:
(1a) Waiting until the delayed initialization and id loading finished (up to 60sec)
  * this means that we risk losing ping data in case of a crash etc.
  * could we live with that increased risk?
(1b) On the first ping submission that needs a client id, start loading the client id
  * this means hitting the disk as early as those crash pings are submitted
  * we would need to know whether that early hitting the disk is ok
Also, both of these mean adding another async operation to block shutdown on.
I prefer #2, *if* we need the data (e.g. if somebody called submitExternalPing). The async I/O is not a big deal, for the relatively rare cases that we are sending an activation or crash ping. But we do want the activation ping to happen quickly and reliably.
This patch allows |submitExternalPing| to load/generate/write the clientId if it wasn't cached, early during startup.

The ClientID module already takes care of handling multiple calls when spawning a loading task, so we should be safe there.

We also already block on shutdown, since |yield ClientID.getClientId()| is called in the deferred setup task, on which we block during shutdown.
Attachment #8746014 - Flags: review?(gfritzsche)
Attached patch Part 2 - Add test coverage (obsolete) — Splinter Review
This patch slightly refactors the test coverage for the client id and adds some new test cases:

- that we get a client when submitting a ping before telemetry inits;
- that we get a cleint id when submitting after telemetry inits;
- that we get a client id when we have a cached client id.
Attachment #8746015 - Flags: review?(gfritzsche)
Status: NEW → ASSIGNED
Comment on attachment 8746014 [details] [diff] [review]
part 1 - Load the client id on submitExternalPing

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

This looks fine to me.  Creating on first attempt (only if CID is request) OR after 60 seconds seems like a good solution and would solve all my use cases.  

Are there any other ways were the CID is used *other than submit external ping* that should also be handled?   

(I don't see any calls to that module OR pref in DXR.)
(In reply to Gregg Lind (User Advocacy - Heartbeat - Test Pilot) from comment #14)
> Are there any other ways were the CID is used *other than submit external
> ping* that should also be handled?   
> 
> (I don't see any calls to that module OR pref in DXR.)

All the ping requests *must* be dispatched through the Telemetry API. If anything else ends up using the ClientID, we should still be safe.

The only potentially unsafe use that I see is getting the client id using |TelemetryController.clientID;| before Telemetry inits, in case there's no cached client id. That's not really used anywhere.
(In reply to Gregg Lind (User Advocacy - Heartbeat - Test Pilot) from comment #14)
> This looks fine to me.  Creating on first attempt (only if CID is request)
> OR after 60 seconds seems like a good solution and would solve all my use
> cases.  

Can we talk more specifically about your use cases, potentially off-bug?
Loading the client id before the 60sec delay should be a rare occurence, the "activation" and "crash" ping being the only exceptions i can think of.
I'd be curious to know other use-cases where this is relevant to make sure that we are not hitting the disk during startup needlessly.
Flags: needinfo?(glind)
(Happy to take it off bug, but I will also answer here, b/c the answer is short :) )

During testing of addons that pack special telemetry (such as SHIELD-Study addons), I am constantly making new profiles during development.  Since they are 'new' profiles, they don't meet the 60 second rule.  That use case has 0 performance implications or concerns.
Flags: needinfo?(glind)
This adds a probe to count the pings waiting for a client id at startup and removes the bogus TelemetryController.clientID, since we already have a module which provides a way to access it. There's no point, I think, in having multiple APIs.
Attachment #8746014 - Attachment is obsolete: true
Attachment #8746014 - Flags: review?(gfritzsche)
Attachment #8746428 - Flags: review?(gfritzsche)
Attached patch bug1233986_tests.patch (obsolete) — Splinter Review
I've updated the tests accordingly. I found that test_PingAPI.js contains a test that covers the cached client id as well. I think these can be unified, but maybe we can take that to a separate bug.
Attachment #8746015 - Attachment is obsolete: true
Attachment #8746015 - Flags: review?(gfritzsche)
Attachment #8746429 - Flags: review?(gfritzsche)
Comment on attachment 8746428 [details] [diff] [review]
part 1 - Load the client id on submitExternalPing

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

::: toolkit/components/telemetry/Histograms.json
@@ +5029,5 @@
>      "expires_in_version": "never",
>      "kind": "count",
>      "description": "Number of archived Telemetry pings discarded because they exceeded the maximum size"
>    },
> +  "TELEMETRY_PING_SUBMISSION_WAITING_CLIENTID": {

We should make this opt-out, i'm most worried about the impact on release (from experiments, funnel-cakes, ...).
This will only be submitted when a client hits this problem, so there is no cost for the expected case.

Don't forget to needinfo? or feedback? bsmedberg for data collection review.

@@ +5034,5 @@
> +    "alert_emails": ["telemetry-client-dev@mozilla.com"],
> +    "expires_in_version": "never",
> +    "kind": "count",
> +    "bug_numbers": [1233986],
> +    "description": "The number of pings that were submitted and had to wait for a client id (e.g. before Telemetry inits)"

"... (i.e. before it was cached or loaded from disk)"

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +474,5 @@
> +    if (!this._clientID && aOptions.addClientId) {
> +      Telemetry.getHistogramById("TELEMETRY_PING_SUBMISSION_WAITING_CLIENTID").add();
> +      // We can safely call |getClientID| here and during initialization: we would still
> +      // spawn one single loading task and block on it for shutdown.
> +      this._clientID = yield ClientID.getClientID();

This is fragile - it depends on the ClientID implementation as well as startup/shutdown ordering in TelemetryController.

We should:
* check whether TelemetryController is already shut down
* make sure we are waiting on the waiting ping submissions at a well defined point in TelemetryController shutdown

Also, are Tasks guaranteed to run synchronously until the first yield?
Otherwise we just made all submissions async.

@@ -798,5 @@
>      return undefined;
>    },
>  
> -  get clientID() {
> -    return this._clientID;

You need to remove the public getter too.
Attachment #8746428 - Flags: review?(gfritzsche)
Attachment #8746429 - Flags: review?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #20)
> Comment on attachment 8746428 [details] [diff] [review]
> part 1 - Load the client id on submitExternalPing
> 
> Also, are Tasks guaranteed to run synchronously until the first yield?
> Otherwise we just made all submissions async.

@Till, do you know if Tasks are guaranteed to run synchronously until the first yield?
Flags: needinfo?(till)
(In reply to Alessio Placitelli [:Dexter] from comment #21)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #20)
> > Comment on attachment 8746428 [details] [diff] [review]
> > part 1 - Load the client id on submitExternalPing
> > 
> > Also, are Tasks guaranteed to run synchronously until the first yield?
> > Otherwise we just made all submissions async.

My understanding is that yes, Tasks are run synchronously until the first yield.

|Task.async| generates an async function from [1]. Since we're using generator functions, we make use of the TaskImpl code [2]. We eventually get into the |_run| method [3]: the function is executed up to the first |yield|. A new Task is spawned from [4] only if we yielded on a promise/generator.

[1] - https://dxr.mozilla.org/mozilla-central/rev/0a25833062a880f369e6f9f622413a94cc671bf4/toolkit/modules/Task.jsm#243
[2] - https://dxr.mozilla.org/mozilla-central/rev/0a25833062a880f369e6f9f622413a94cc671bf4/toolkit/modules/Task.jsm#254
[3] - https://dxr.mozilla.org/mozilla-central/rev/0a25833062a880f369e6f9f622413a94cc671bf4/toolkit/modules/Task.jsm#280,319
[4] - https://dxr.mozilla.org/mozilla-central/rev/0a25833062a880f369e6f9f622413a94cc671bf4/toolkit/modules/Task.jsm#388
(In reply to Alessio Placitelli [:Dexter] from comment #22)
> (In reply to Alessio Placitelli [:Dexter] from comment #21)
> > (In reply to Georg Fritzsche [:gfritzsche] from comment #20)
> > > Comment on attachment 8746428 [details] [diff] [review]
> > > part 1 - Load the client id on submitExternalPing
> > > 
> > > Also, are Tasks guaranteed to run synchronously until the first yield?
> > > Otherwise we just made all submissions async.
> 
> My understanding is that yes, Tasks are run synchronously until the first
> yield.

Yes, that looks correct to me.
Flags: needinfo?(till)
This patch makes the probe opt out, fixes the description and blocks on client Id loading on shutdown.
Attachment #8746428 - Attachment is obsolete: true
Attachment #8755424 - Flags: review?(gfritzsche)
Attached patch Part 2 - Add test coverage (obsolete) — Splinter Review
This patch rebases the test coverage.
Attachment #8746429 - Attachment is obsolete: true
Attachment #8755425 - Flags: review?(gfritzsche)
Comment on attachment 8755424 [details] [diff] [review]
part 1 - Load the client id on submitExternalPing

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

::: toolkit/components/telemetry/Histograms.json
@@ +5088,5 @@
>      "expires_in_version": "never",
>      "kind": "count",
>      "description": "Number of archived Telemetry pings discarded because they exceeded the maximum size"
>    },
> +  "TELEMETRY_PING_SUBMISSION_WAITING_CLIENTID": {

Remember to flag this for data collection review.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +473,5 @@
> +      // Make sure to block on clientId loading when shutting down, without relying on
> +      // setup/shutdown ordering and on the ClientID implementation.
> +      this._shutdownBarrier.client.addBlocker(
> +        "TelemetryController: loading client ID from the disk", clientIDPromise);
> +      this._clientID = yield clientIDPromise;

This means we are blocking shutdown until here... and the next async operations below might add their shutdown blockers too late.

I think there should be one single promise blocking shutdown that includes loading the client id as well as the async operations below.
We might have to move the code here into a separate function to achieve that.
Attachment #8755424 - Flags: review?(gfritzsche)
Comment on attachment 8755425 [details] [diff] [review]
Part 2 - Add test coverage

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

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js
@@ +196,5 @@
> +
> +  // Make sure we have no cached client ID for this test: we'll try to send
> +  // a ping with it while Telemetry is being initialized.
> +  Preferences.reset(PREF_CACHED_CLIENTID);
> +  yield TelemetryStorage.shutdown();

Why is this explicitely shutting down just TelemetryStorage?
Attachment #8755425 - Flags: review?(gfritzsche) → feedback+
Thanks Georg. I've moved the submission logic to a separate function.

I'll flag Benjamin for the data-review once we're good to go with the patch.
Attachment #8755424 - Attachment is obsolete: true
Attachment #8759691 - Flags: review?(gfritzsche)
Attached patch Part 2 - Add test coverage (obsolete) — Splinter Review
I've changed TelemetryStorage.shutdown to TelemetryController.testShutdown + TelemetryStorage.testClearPendingPings.

There's no real reason here to use TelemetryStorage.shutdown, this was from before the redesign patch.
Attachment #8755425 - Attachment is obsolete: true
Attachment #8759694 - Flags: review?(gfritzsche)
Comment on attachment 8759691 [details] [diff] [review]
part 1 - Load the client id on submitExternalPing

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

If we haven't filed it yet, please file a follow-up on validating the status of "crash" as well as "main" pings once we have some days of data.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +436,5 @@
>  
>    /**
> +   * Internal function to assemble a complete ping, adding environment data, client id
> +   * and some general info. This waits on the client id to be loaded/generated if it's
> +   * not yet available.

Please call out that this runs synchronous unless we need to load a client id.

@@ +465,5 @@
> +
> +    const pingData = this.assemblePing(aType, aPayload, aOptions);
> +    this._log.trace("submitExternalPing - ping assembled, id: " + pingData.id);
> +
> +    // Always persist the pings if we are allowed to.

Call out that we should NOT yield on any of those operations to keep this function synchronous for the majority of the calls.
Attachment #8759691 - Flags: review?(gfritzsche) → review+
Comment on attachment 8759694 [details] [diff] [review]
Part 2 - Add test coverage

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

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js
@@ +210,5 @@
> +  // Wait until we are fully initialized. Pings will be assembled but won't get
> +  // sent before then.
> +  yield initialSetup;
> +  Assert.equal(h.snapshot().sum, 1,
> +               "We must have a ping waiting for the clientId early during startup.");

Move this up after sendPing(). This should be counted as soon as we submitted the ping.

@@ +215,5 @@
>  
> +  // Fetch the client ID after the initialization, so we don't unintentionally
> +  // trigger its loading. We'll still need the client ID to see if the ping
> +  // looks sane.
> +  gClientID = yield ClientID.getClientID();

We can retrieve it after promiseNextPing() - that way we can be sure you didn't trigger anything.

@@ +227,5 @@
> +
> +  // We should have cached the client ID now. Lets confirm that by checking it before
> +  // the async ping setup is finished.
> +  h.clear();
> +  let promisePingSetup = TelemetryController.testReset();

initialSetup, promisePingSetup - lets use only one variable for those.

@@ +238,5 @@
> +  checkPingFormat(ping, TEST_PING_TYPE, true, false);
> +  Assert.equal(ping.clientId, gClientID,
> +               "Telemetry should report the correct cached clientId.");
> +
> +  // Check that sending a ping after without relying on the cache, after the

Remove duplicated "after".
Attachment #8759694 - Flags: review?(gfritzsche) → review+
Attached patch Part 2 - Add test coverage (obsolete) — Splinter Review
Attachment #8759694 - Attachment is obsolete: true
Attachment #8760156 - Flags: review+
Comment on attachment 8760155 [details] [diff] [review]
part 1 - Load the client id on submitExternalPing

Benjamin, we're adding the TELEMETRY_PING_SUBMISSION_WAITING_CLIENTID opt-out probe to the ping.

It measures the number of pings that were submitted and had to wait for a client id (i.e. before it was cached or loaded from disk).

Flagging you for a data-review on this.
Attachment #8760155 - Flags: review?(benjamin)
(In reply to Georg Fritzsche [:gfritzsche] from comment #30)
> Comment on attachment 8759691 [details] [diff] [review]
> part 1 - Load the client id on submitExternalPing
> 
> Review of attachment 8759691 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If we haven't filed it yet, please file a follow-up on validating the status
> of "crash" as well as "main" pings once we have some days of data.

We did, see bug 1274975.
Comment on attachment 8760155 [details] [diff] [review]
part 1 - Load the client id on submitExternalPing

+      let clientIDPromise = ClientID.getClientID();
+      this._clientID = yield clientIDPromise;

Is this better than
this._clientID = yield ClientID.getClientID() ?
Attachment #8760155 - Flags: review?(benjamin) → review+
Attached patch Part 2 - Add test coverage (obsolete) — Splinter Review
This fixes the bustage on Android 4.3. It was happening due to the telemetry server pref not being updated to the address of the local ping server (because a desktop-only test was being skipped).
Attachment #8760156 - Attachment is obsolete: true
Attachment #8763925 - Flags: review+
Fixed the issue Benjamin found.
Attachment #8760155 - Attachment is obsolete: true
Attachment #8763929 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/47cccae80d808da7c0c5ee1f07cc918d5683801b
Bug 1233986 - Move clientId loading, generation and serialization earlier during startup. r=gfritzsche, data-review=bsmedberg

https://hg.mozilla.org/integration/fx-team/rev/0d361d105c86ff3443c06a7387b994d75035178e
Bug 1233986 - Add test coverage. r=gfritzsche
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/8427a465852b
Backed out changeset 0d361d105c86 
https://hg.mozilla.org/integration/fx-team/rev/ee2eba8cdedd
Backed out changeset 47cccae80d80 for test failures in test_TelemetryController.js
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10041064&repo=fx-team
Flags: needinfo?(alessio.placitelli)
Attached patch Part 2 - Add test coverage (obsolete) — Splinter Review
Attachment #8763925 - Attachment is obsolete: true
Flags: needinfo?(alessio.placitelli)
Attachment #8764687 - Attachment description: bug1233986_tests.patch → Part 2 - Add test coverage
Comment on attachment 8764687 [details] [diff] [review]
Part 2 - Add test coverage

That should fix all the bustages. The changes:

- The PingServer is now returning the ping on the next tick, allowing TelemetrySend (and other components) to clear up.
- Due to the fact that submitExternalPing can now be async if no clientID was loaded (and it's being requested), we have to slightly change the test to correctly wait on the ping being generated before trying to look for it in the archive. This required a new test notification from UITour (and probably requires r? from MattN).
Attachment #8764687 - Flags: review?(gfritzsche)
Comment on attachment 8764687 [details] [diff] [review]
Part 2 - Add test coverage

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

This looks ok for me for now, with fixing the ping server syncness in `promiseNextRequest()` instead of `promiseNextPing()`.

MattN, are the UITour changes ok with you?
The changes outside of UITour.jsm and browser_UITour.js shouldn't matter to you.

::: browser/components/uitour/test/browser_UITour.js
@@ +381,5 @@
>      yield gContentAPI.setTreatmentTag("foobar", "baz");
> +    // Wait until the treatment telemetry is sent before looking in the Archive.
> +    // "TreatmentTag:TelemetrySent".
> +    yield BrowserTestUtils.waitForContentEvent(gTestTab.linkedBrowser, "mozUITourNotification", false,
> +                                               event => event.detail.event === "TreatmentTag:TelemetrySent");

This way to fix it seems ok now, although longer-term we should think about alternatives.
Maybe we need test-only functionality to wait for specific ping types to arrive in the archive?

::: toolkit/components/telemetry/tests/unit/head.js
@@ +86,5 @@
> +    // Send the ping to the consumer on the next tick, so that the completion gets
> +    // signaled to Telemetry.
> +    return new Promise(r => {
> +      nextPingPromise.then(ping => Services.tm.currentThread.dispatch(() => r(ping),
> +                                                                      Ci.nsIThread.DISPATCH_NORMAL));

Now you only fixed this for `promiseNextPing()`.
You should fix it in `promiseNextRequest()`, then `promiseNextPing()` should be fixed as well.
Attachment #8764687 - Flags: review?(gfritzsche)
Attachment #8764687 - Flags: review+
Attachment #8764687 - Flags: feedback?(MattN+bmo)
Comment on attachment 8764687 [details] [diff] [review]
Part 2 - Add test coverage

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

::: browser/components/uitour/test/browser_UITour.js
@@ +378,5 @@
>    taskify(function* test_treatment_tag() {
>      let ac = new TelemetryArchiveTesting.Checker();
>      yield ac.promiseInit();
>      yield gContentAPI.setTreatmentTag("foobar", "baz");
> +    // Wait until the treatment telemetry is sent before looking in the Archive.

Nit: lowercase "Archive".
Attachment #8764687 - Flags: feedback?(MattN+bmo) → review+
Attached patch Part 2 - Add test coverage (obsolete) — Splinter Review
Attachment #8764687 - Attachment is obsolete: true
Attachment #8765349 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/bca07aa9ac51fbc5bd3f51c890e7beba3d619999
Bug 1233986 - Move clientId loading, generation and serialization earlier during startup. r=gfritzsche, data-review=bsmedberg

https://hg.mozilla.org/integration/fx-team/rev/8f48d2f1fa58dbb978abc28051f3550724800b2f
Bug 1233986 - Add test coverage. r=gfritzsche
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/0ded67e4c761
Backed out changeset 8f48d2f1fa58 
https://hg.mozilla.org/integration/fx-team/rev/507d73e670e6
Backed out changeset bca07aa9ac51 on developers request
Fixing the Windows-only test bustages in test_TelemetryController by waiting on the ping activity to settle after sending a deletion ping.
Attachment #8765349 - Attachment is obsolete: true
Attachment #8765791 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/66e7bb21e857544b68e0c0ace894bb45e7ee49ab
Bug 1233986 - Move clientId loading, generation and serialization earlier during startup. r=gfritzsche, data-review=bsmedberg

https://hg.mozilla.org/integration/fx-team/rev/1d4ceb3c5d5aed6afc6ec71239b2d741df893379
Bug 1233986 - Add test coverage. r=gfritzsche
https://hg.mozilla.org/mozilla-central/rev/66e7bb21e857
https://hg.mozilla.org/mozilla-central/rev/1d4ceb3c5d5a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Awesome work fixing this!  This is a huge win for more consistent data collection in Firefox, and should remove a lot of edge cases from analysis code!
You need to log in before you can comment on or make changes to this bug.