1233986 - Move clientId loading, generation and serialization earlier during startup

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

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.

Comment 1

[Alessio Placitelli [:Dexter]]

More information on the expected behaviour is on bug 1221583 comment 8.

Comment 2

[Alessio Placitelli [:Dexter]]

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

Comment 3

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

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)

Comment 4

[Benjamin Smedberg]

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.

Comment 5

[Georg Fritzsche [: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)

Comment 6

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

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

Comment 7

[Alessio Placitelli [:Dexter]]

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?

Comment 8

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

(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)
  }
}
```

Comment 9

[Benjamin Smedberg]

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.

Comment 10

[Georg Fritzsche [:gfritzsche]]

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

Comment 11

[Benjamin Smedberg]

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.

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Load the client id on submitExternalPing

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.

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Add test coverage

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.

Comment 14

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

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

Comment 15

[Alessio Placitelli [:Dexter]]

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

Comment 16

[Georg Fritzsche [:gfritzsche]]

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

Comment 17

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

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

Comment 18

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Load the client id on submitExternalPing

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.

Comment 19

[Alessio Placitelli [:Dexter]]

Attachment: bug1233986_tests.patch

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.

Comment 20

[Georg Fritzsche [: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": ["[e-mail]"],
> +    "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.

Comment 21

[Alessio Placitelli [:Dexter]]

(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?

Comment 22

[Alessio Placitelli [:Dexter]]

(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

Comment 23

[Till Schneidereit [:till]]

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

Comment 24

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Load the client id on submitExternalPing

This patch makes the probe opt out, fixes the description and blocks on client Id loading on shutdown.

Comment 25

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Add test coverage

This patch rebases the test coverage.

Comment 26

[Georg Fritzsche [: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.

Comment 27

[Georg Fritzsche [: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?

Comment 28

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Load the client id on submitExternalPing

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.

Comment 29

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Add test coverage

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.

Comment 30

[Georg Fritzsche [: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.

Comment 31

[Georg Fritzsche [:gfritzsche]]

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

Comment 32

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Load the client id on submitExternalPing

Comment 33

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Add test coverage

Comment 34

[Alessio Placitelli [:Dexter]]

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.

Comment 35

[Alessio Placitelli [:Dexter]]

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

Comment 36

[Alessio Placitelli [:Dexter]]

(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 37

[Benjamin Smedberg]

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() ?

Comment 38

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Add test coverage

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

Comment 39

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Load the client id on submitExternalPing

Fixed the issue Benjamin found.

Comment 40

[Alessio Placitelli [:Dexter]]

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

Comment 41

[Pulsebot]

Backout by [e-mail]:
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

Comment 42

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10041064&repo=fx-team

Comment 43

[Alessio Placitelli [:Dexter]]

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

Comment 44

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Add test coverage

Comment 45

[Alessio Placitelli [:Dexter]]

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

Comment 46

[Georg Fritzsche [: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.

Comment 47

[Matthew N. [:MattN]]

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

Comment 48

[Alessio Placitelli [:Dexter]]

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

Comment 49

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Add test coverage

Comment 50

[Alessio Placitelli [:Dexter]]

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

Comment 51

[Pulsebot]

Backout by [e-mail]:
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

Comment 52

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This caused Windows xpcshell tests to frequently fail like https://treeherder.mozilla.org/logviewer.html#?job_id=10118375&repo=fx-team

Comment 53

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Add test coverage

Fixing the Windows-only test bustages in test_TelemetryController by waiting on the ping activity to settle after sending a deletion ping.

Comment 54

[Alessio Placitelli [:Dexter]]

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

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66e7bb21e857
https://hg.mozilla.org/mozilla-central/rev/1d4ceb3c5d5a

Comment 56

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

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!