1157359 - profileSubsessionCounter failing to increment, and sometimes skipping.

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[brendan c]

From the way we defined profileSubsessionCounter (in the main ping at
['payload']['info']['profileSubsessionCounter'] ), it should always increment for each subsession, and there should never be two subsessions submitted by a single client that have the same profileSubsessionCounter value in multiple subsessions (at least once you drop idle daily).

This is described in the docs--
https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/toolkit/components/telemetry/telemetry/main-ping.html
-- as:
>    profileSubsessionCounter: <number>, // the running no. of all subsessions for the whole profile life time

Unfortunately, almost all of the clients I've looked at have multiple subsessions with the same profileSubsessionCounter value.

Pasted below are lists of profileSubsessionCounter values for 50 randomly selected clients. As you can see, values are repeated a ton, and occasionally a value is skipped (this latter problem could be because of delayed upload?).

(see http://nbviewer.ipython.org/gist/bcolloran/332aab7da68c1c84d09b for code; this is based on the sample mreid made last week)

- [2, 2, 2, 2, 2, 2, 3, 4, 4, 4, 4, 4, 5, 6, 6, 7, 8]
- [1, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2]
- [1, 1, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 5, 5, 5, 5, 5, 5, 5, 5, 5]
- [1, 2, 3, 4, 5, 5, 5, 5, 5, 5, 5, 5, 5, 6, 7, 8, 8, 9, 10, 10, 11]
- [2, 2, 2, 2, 2]
- [1, 2, 3, 4]
- [1, 2, 4, 4, 6, 8, 8, 8, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10, 10,
  10, 10, 10, 10, 10, 10, 11, 12, 12, 12, 12, 13, 14, 14, 14, 15, 16, 16, 16, 16,
  17, 17, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18,
  18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18,
  18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 19, 20, 20, 20, 20,
  20, 20, 20, 20, 21, 22, 22, 22, 23, 24, 24, 24, 24, 26, 26, 26, 26, 26, 26, 26,
  26, 26, 27, 28, 28, 28, 28, 28, 29]
- [2, 2, 2, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,
  4, 4, 4, 4, 4, 4, 4]
- [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 10, 10, 10, 10, 10, 11, 12]
- [1, 2, 2, 2, 2, 2, 3, 3, 4, 5, 5, 6, 7, 7, 8]
- [2, 2, 3, 3, 4, 5, 6, 8, 9, 11, 13, 21, 21, 21, 21, 22, 24, 26, 28, 30, 30, 32,
  50, 52, 54, 56, 58, 60, 62, 64, 66, 68, 69]
- [2, 2, 2, 2, 2, 2, 2, 2, 2, 3, 4]
- [2, 2, 2, 4, 4, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 7, 7,
  8, 8, 8, 8, 8, 8, 9, 10, 10, 10, 10, 10, 11, 12, 12, 12, 12, 12, 12, 12, 12, 12,
  12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 13, 14, 14, 15, 16, 17,
  18, 19, 19, 19, 19, 19, 19, 19, 19, 19, 19, 19, 19, 19, 19, 19, 20, 21, 22, 22,
  22, 22, 23, 25, 26, 26, 26, 26, 26, 26, 27, 28, 28, 28, 28, 28, 28, 28, 29, 29,
  30, 30, 30, 30, 30, 30, 30, 30, 30, 30]
- [1, 2, 2, 2]
- [1, 2, 3, 5, 6, 7, 8, 9, 10, 10, 10, 11]
- [1, 2, 4, 5, 5, 7, 7, 7, 8, 9, 11, 12, 12, 13, 13, 14, 15, 15, 15, 15, 15, 15, 15,
  16, 17, 17]
- [1, 2]
- [2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 3, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,
  4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 6,
  6, 6, 6, 6, 6, 6, 6, 6, 6]
- [2]
- [1, 1, 2, 2, 2]
- [1, 1, 2, 3, 4, 5, 5, 5, 5, 5, 5, 6, 7, 8, 9, 9, 9, 9, 10, 11, 11, 11, 11, 11, 11,
  12, 13, 13, 13, 14, 14]
- [1, 2, 2, 3]
- [2, 3, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 5, 6, 6, 7, 8, 9, 9, 10, 11, 11,
  12, 12, 12, 14, 15, 16, 17, 17, 18]
- [1, 2, 3]
- [1, 2, 4, 4, 4, 4, 4, 4, 4, 4, 4, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6,
  6, 6, 6, 7, 7, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8]
- [1, 2, 4, 5, 6, 7]
- [2, 4, 5]
- [2, 4, 4, 5, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7]
- [2, 3, 4]
- [2, 2, 2, 2, 2, 2, 2, 3]
- [2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 3, 4, 5, 5, 5, 5, 5, 6, 8, 8, 8,
  8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8]
- [2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 3, 4, 4, 4, 4, 4, 4]
- [1, 1]
- [1, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2]
- [1, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 3, 4, 4, 4, 4, 4, 4, 4,
  4, 4, 5, 6, 6, 6, 6, 6, 6, 6, 7, 8, 8, 8, 8, 8, 8, 8, 8]
- [2, 2, 2]
- [1, 3, 3, 3, 3, 3, 4, 5, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 6, 7]
- [1, 2]
- [1, 2, 2, 2]
- [1, 2, 2, 2, 3, 4, 5, 6, 7]
- [2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
  2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 3, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,
  4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 5, 6, 7, 7, 7, 7, 7, 7,
  7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7]
- [1, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2]
- [1, 1, 1, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 3, 3, 3, 3, 3, 3, 4, 4, 4, 4, 4, 5,
  5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 6, 6, 7, 7, 7,
  7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 7, 8, 8, 9, 9, 9, 9, 9, 9, 9,
  9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 9, 10, 10, 11, 11, 11, 11, 11, 11, 11, 11, 11,
  11, 13, 13, 13, 13, 13, 13, 13, 13, 13, 13, 15, 15, 16, 17, 18, 18, 18, 18, 18,
  18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 18, 19, 20, 20, 20,
  20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 21, 22, 23, 23, 23, 23, 23, 23,
  23, 23, 23, 23, 23, 23, 23, 23, 23, 25, 25, 25, 25, 25, 25, 25, 25, 25, 26, 27,
  27, 27, 27, 27, 27, 27, 29, 29, 29, 29, 29, 29, 29, 29, 29]
- [1, 2, 3, 4, 4, 4, 4, 4, 4, 4, 4, 5, 6, 6, 7, 8, 8, 8]
- [1, 2, 2, 2, 2, 2, 2, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 5, 6, 6, 6, 6,
  6, 6, 6, 6, 6, 6, 6, 6, 7, 9, 9, 9, 9, 9, 9, 9]
- [1, 2]
- [1, 2, 3, 4, 4, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5]
- [2, 3, 4, 4, 6, 6, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8, 9, 10,
  10, 10, 10, 10, 10, 10, 10, 10, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12, 12,
  12, 14, 14, 14, 14, 14, 14, 14, 14, 14, 15, 16, 16, 16, 16, 16, 16, 16, 16, 16,
  16, 16, 16, 18, 18, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 20, 21, 22, 22, 22,
  22, 22, 22, 22, 22, 22, 22, 22, 22, 22, 22, 22, 22, 22, 22, 22, 22, 22, 22, 22,
  22, 22, 22, 24, 24, 24, 24, 24, 24, 24, 24, 24, 24, 24, 24, 24, 24, 24, 24, 24,
  24, 24, 24, 24, 24, 26, 26, 26, 26, 26, 26, 26, 26, 26, 27, 28, 28, 28, 28, 28,
  28, 28, 28, 28, 28, 29, 30, 30, 30, 30, 30, 30, 30, 30, 30, 30, 30, 30, 30, 30,
  30, 30, 30, 30, 30, 30, 30, 32, 32, 32, 32, 32, 32, 32, 34, 34, 34, 35, 36, 36,
  36, 36, 36, 36, 36, 36, 36, 36, 37, 38, 38, 38, 38, 38, 38, 38, 38, 38, 38, 38,
  38, 38, 39, 40, 40, 40, 42, 42, 42, 42, 42, 42, 42, 42, 42, 44, 44, 44, 44, 44,
  44, 44, 44, 44, 44, 44, 44, 44, 44, 44, 44, 44, 44, 44, 44, 44, 44, 44, 44, 44,
  44, 44, 44, 44, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 46, 47, 48, 48, 48, 48,
  48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 48, 49, 50]
- [2, 3, 4]
- [2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
  2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2,
  2, 2, 2, 2, 2, 2, 3, 3, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 5, 5, 6, 6, 6, 6, 6, 6, 6,
  6, 6, 6, 6, 6, 6, 6, 6, 6]

Comment 1

[Alessio Placitelli [:Dexter]]

After digging into this issue, I've found that we're not correctly handling the profile counter updates for sessions lasting less than one minute (before Telemetry initialisation completes).

In [1] we force the init to complete when shutting down. This allows us to load the session data (profile counter and previous session id) before writing the "saved-session" and "shutdown" pings. Unfortunately, we're only persisting session data when breaking sessions [3], so we and up with an outdated profile subsession counter ("saved-session" and "shutdown" don't start new subsessions).
Restarting Firefox would load the outdated session state file and produce pings with the same profile subsession counter value.

To deal with this case I think we should be persisting the session data also when Firefox shuts down.

[1] - https://hg.mozilla.org/mozilla-central/annotate/1ad65cbeb2f4/toolkit/components/telemetry/TelemetrySession.jsm#l1916
[2] - https://hg.mozilla.org/mozilla-central/annotate/1ad65cbeb2f4/toolkit/components/telemetry/TelemetrySession.jsm#l1951
[3] - https://hg.mozilla.org/mozilla-central/annotate/1ad65cbeb2f4/toolkit/components/telemetry/TelemetrySession.jsm#l1425

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: bug1157359.patch

This patch writes the session data on shutdown to make sure short sessions correctly update the |profileSubsessionCounter|.

It also adds test coverage.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8599834 [details] [diff] [review]
bug1157359.patch

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

From the above, we are missing persisting the state on startup, not on shutdown.
We should fix the problem there, making sure we always persist the changed state in the delayed startup routine.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +1221,5 @@
> +  // Create the directory which will contain the data file, if it doesn't already
> +  // exist.
> +  yield OS.File.makeDir(DATAREPORTING_PATH);
> +
> +  // Write test data to the session data file.

I don't think we need to explicitly write this.
Also, we should cover previousSubsessionId too while we're here (presumably affected by the same issue?).

Let's:
* shutdown TelemetrySession
* delete the state file if needed
* start it, assert profileSubsessionCounter is 1
* restart, recheck & check previousSubsessionId

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Correctly update the profileSubsessionCounter and the previousSubsessionId

Thanks Georg.

As discussed over IRC, this patch also fixes a problem we've been having with the |previousSubsessionId|:

- Start Firefox, no state file
- 1 subsession, null previous
- Close Firefox
-> State file will contain 1 subsession, null previous

Repeat that procedure N times and the state file will keep having "null" as the previous session id (for short sessions). But the problem is there also for long sessions: we are always storing the second to last session id as the "previousSubsessionId" in the state file.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8599882 [details] [diff] [review]
part 1 - Correctly update the profileSubsessionCounter and the previousSubsessionId

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

I think i still have concerns about what happens across multiple subsessions.
That expands the scope of this patch a little, but i think we need this (or rather, we should have had it already).

Some questions that i think we should expand our test coverage to:
a) if we re-use a main ping A as an aborted-session B, do we make the B.previousSubsessionId==A.subsessionId?
b) if we find an aborted-session, do we need to increment the profileSubsessionCounter by 1?
c) do normal aborted-sessions have the right previousSubsessionId?
d) if we find an aborted ping, do we correctly use its subsessionId as the previousSubsessionId in the first (new) subsession?
e) do we correctly increment profileSubsessionCounter across multiple subsessions in the same session too?

One possible approach here would be to go trigger a sequence of events (each bullet being a ping-generating event, in parentheses the expected resulting ping data):
* startup & shutdown (shutdown ping with profileSubsessionCounter: 1, subsessionCounter: 1, subsessionId: A, previousSubsessionId: null)
* startup & shutdown quickly (shutdown ping with profileSubsessionCounter: 2, subsessionCounter: 1, subsessionId: B, previousSubsessionId: A)
* startup & abort (aborted-session ping with profileSubsessionCounter: 3, subsessionCounter: 1, subsessionId: C, previousSubsessionId: B)
* startup & addon install (environment-change ping with profileSubsessionCounter: 4, subsessionCounter: 1, subsessionId: D, previousSubsessionId: C)
* shutdown (shutdown ping with profileSubsessionCounter: 5, subsessionCounter: 2, subsessionId: E, previousSubsessionId: D)
* startup & daily (... etc.)
* addon install
* shutdown
* startup & addon install
* abort
* startup & daily
* addon install
* abort
* ... more?

So, we collect the generated pings (from the archive or from the test server) for all the above steps, then check for each ping: profileSubsessionCounter, subsessionCounter, subsessionId, previousSubsessionId
... and verify that we can chain properly.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Fix daily pings incorrectly being saved as aborted-session pings

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8600827 [details] [diff] [review]
part 2 - Fix daily pings incorrectly being saved as aborted-session pings

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +2046,5 @@
>      this._log.trace("_saveAbortedSessionPing - ping path: " + FILE_PATH);
>  
>      let payload = null;
>      if (aProvidedPayload) {
> +      payload = Cu.cloneInto(aProvidedPayload, myScope);

We need to decide on what we want here:
* should _saveAbortedSessionPing always clone the payload?
* should callers of _saveAbortedSessionPing always clone the payload?

With this we have a redundant clone of the payload through _onEnvironmentChange.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8599882 [details] [diff] [review]
part 1 - Correctly update the profileSubsessionCounter and the previousSubsessionId

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

The more complete test-coverage requested in comment 5 is coming in a separate patch here, so this looks good.

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Fix daily pings incorrectly being saved as aborted-session pings

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Subsession chaining test

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8600856 [details] [diff] [review]
part 4 - Subsession chaining test

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

::: toolkit/components/telemetry/tests/unit/test_SubsessionChaining.js
@@ +22,5 @@
> +  return OS.Path.join(OS.Constants.Path.profileDir, "datareporting");
> +});
> +
> +let promiseValidateArchivedPings = Task.async(function*() {
> +  let list = yield TelemetryArchive.promiseArchivedPingList();

This function assumes we're only getting the pings we are looking for in the archive and that we get them in order. Should I make this a little bit more complicated to handle other pings being found?

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8600856 [details] [diff] [review]
part 4 - Subsession chaining test

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

::: toolkit/components/telemetry/tests/unit/test_SubsessionChaining.js
@@ +22,5 @@
> +  return OS.Path.join(OS.Constants.Path.profileDir, "datareporting");
> +});
> +
> +let promiseValidateArchivedPings = Task.async(function*() {
> +  let list = yield TelemetryArchive.promiseArchivedPingList();

You will need to filter for "main" pings here.
Any other pings in the archive - like e.g. "saved-session" - would break this this.

Assuming that we only get the expected "main" pings is fine and part of the test.
promiseArchivedPingList() documents that the list is ordered by creation date, so that assumption is fine too.

What i am missing in this test is:
* verifying the ping reasons
* verifying the subsessionCounter monotonically increases up to each "aborted-session" or "shutdown"

In test_subsessionsChaining(), each time we trigger a ping generating event, we could push the expected reason on a list and pass it to this function.

@@ +69,5 @@
> +
> +  // Fake the clock data to manually trigger an aborted-session ping and a daily ping.
> +  // This is also helpful to make sure we get the archived pings in an expected order.
> +  let now = new Date(2009, 09, 18, 00, 00, 0);
> +  fakeNow(now);

let now = fakeNow(2009, 09, 18, ...

@@ +71,5 @@
> +  // This is also helpful to make sure we get the archived pings in an expected order.
> +  let now = new Date(2009, 09, 18, 00, 00, 0);
> +  fakeNow(now);
> +
> +  let tickClock = (minutes) => {

moveClockForward()?

@@ +96,5 @@
> +  fakeSchedulerTimer(callback => schedulerTickCallback = callback, () => {});
> +  yield TelemetrySession.reset();
> +  tickClock(6);
> +  // Trigger the an aborted session ping save (in testing, it gets not saved when |reset|
> +  // is called).

I don't understand this part, can you expand the comment?

@@ +135,5 @@
> +  Preferences.set(PREF_TEST, 0);
> +
> +  // Shut down Telemetry and trigger a shutdown ping.
> +  tickClock(30);
> +  yield TelemetrySession.shutdown();

What happened to the other parts of the suggested coverage?
* startup & addon install
* abort
* startup & daily
* addon install
* abort

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Enable the archiving of aborted-session pings.

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8600908 [details] [diff] [review]
part 3 - Enable the archiving of aborted-session pings.

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

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +739,5 @@
> +  addAbortedSessionPing: function addAbortedSessionPing(aFilePath) {
> +    this._log.trace("addAbortedSessionPing");
> +
> +    return TelemetryStorage.loadPingFile(aFilePath).then(ping => {
> +      let doAppend = () => this.addPendingPingFromFile(aFilePath, true);

I don't see us using TelemetryController.addPendingPingFromFile() for anything else, so we could just inline it here (and especially remove it from the public interface).

Comment 15

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Subsession chaining test

Thanks Georg, I've addressed your concerns in this patch.

Comment 16

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Enable the archiving of aborted-session pings.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8600942 [details] [diff] [review]
part 4 - Subsession chaining test

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

r=me with the TelemetryEnvironment changes removed and the comments fixed.

::: toolkit/components/telemetry/tests/unit/test_SubsessionChaining.js
@@ +29,5 @@
> +
> +let promiseValidateArchivedPings = Task.async(function*(aExpectedReasons) {
> +  // The list of ping reasons which mark the session end (and must reset the subsession
> +  // count).
> +  const SESSION_END_PING_REASONS = [ REASON_ABORTED_SESSION, REASON_SHUTDOWN ];

Nit: Much nicer to read below if you use Set().

@@ +33,5 @@
> +  const SESSION_END_PING_REASONS = [ REASON_ABORTED_SESSION, REASON_SHUTDOWN ];
> +
> +  let list = yield TelemetryArchive.promiseArchivedPingList();
> +
> +  // We're just interested to the "main" pings.

Nit: "in the"

@@ +38,5 @@
> +  list = list.filter(p => p.type == "main");
> +
> +  let previousPing = yield TelemetryArchive.promiseArchivedPingById(list[0].id);
> +  Assert.equal(aExpectedReasons.shift(), previousPing.payload.info.reason,
> +               "Telemetry should only get pings with expected reasons.");

Also check that for this ping:
* previousSessionId
* profileSubsessionCounter
* subsessionCounter

@@ +43,5 @@
> +
> +  let expectedSubsessionCounter = 1;
> +
> +  for (let i = 1; i < list.length; i++) {
> +    let currentPing = yield TelemetryArchive.promiseArchivedPingById(list[i].id);

Let's add least print the index, reason and maybe id here.
Otherwise it will be impossible to deduce what failed from logs.

@@ +54,5 @@
> +                 "Telemetry must correctly chain subsession identifiers.");
> +    Assert.equal(currentInfo.profileSubsessionCounter, previousInfo.profileSubsessionCounter + 1,
> +                 "Telemetry must correctly track the profile subsessions count.");
> +    Assert.equal(currentInfo.subsessionCounter, expectedSubsessionCounter,
> +              "The subsession counter should be monotonically increasing.");

Nit: indentation.

@@ +64,5 @@
> +    expectedSubsessionCounter =
> +      (SESSION_END_PING_REASONS.indexOf(currentInfo.reason) != -1) ? 1 : (expectedSubsessionCounter + 1);
> +  }
> +
> +  Assert.equal(aExpectedReasons.length, 0, "All the expected pings must be received.");

Lets move the check up to the start of this test and check that: equal(aExpectedReasons.length, list.length)
We should know about all the triggered main pings here.

@@ +75,5 @@
> +  do_get_profile();
> +  loadAddonManager("[e-mail]", "XPCShell", "1", "1.9.2");
> +
> +  Preferences.set(PREF_ENABLED, true);
> +  Preferences.set(PREF_ARCHIVE_ENABLED, true);

We already set the archive pref in head.js.

@@ +95,5 @@
> +
> +  // Fake the clock data to manually trigger an aborted-session ping and a daily ping.
> +  // This is also helpful to make sure we get the archived pings in an expected order.
> +  let now = fakeNow(2009, 09, 18, 00, 00, 0);
> +  fakeNow(now);

This last line is redundant now.

@@ +102,5 @@
> +    now = futureDate(now, minutes * MILLISECONDS_PER_MINUTE);
> +    fakeNow(now);
> +  }
> +
> +  // Keep tracl of the ping reasons we're expecting in this test.

Nit: "keep track"

@@ +158,5 @@
> +  yield TelemetrySession.reset();
> +
> +  // Delay the callback around midnight.
> +  now = futureDate(now, MS_IN_ONE_DAY);
> +  fakeNow(now);

Nit: now = fakeNow(futureDate(...

Comment 18

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8600944 [details] [diff] [review]
part 3 - Enable the archiving of aborted-session pings.

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

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +712,5 @@
> +    let onError =
> +      error => this._log.error("addAbortedSessionPing - Unable to add the pending ping", error);
> +
> +    let doAddPending = () => TelemetryStorage.addPendingPingFromFile(aFilePath)
> +                                             .then(() => OS.File.remove(aFilePath), onError);

Can we make this a Task now so it becomes readable again?

Comment 19

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Subsession chaining test - v2

Comment 20

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Enable the archiving of aborted-session pings. - v2

Comment 21

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Enable the archiving of aborted-session pings. - v3

Comment 22

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Subsession chaining test - v3

This fixes the failing Android tests from the first try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42907ffda702

A followup try for Android: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2422a4a3b47

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f8379fc805bd
https://hg.mozilla.org/integration/fx-team/rev/97299f59d4ca
https://hg.mozilla.org/integration/fx-team/rev/b67b22a57dfa
https://hg.mozilla.org/integration/fx-team/rev/34e86cf1cc38

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f8379fc805bd
https://hg.mozilla.org/mozilla-central/rev/97299f59d4ca
https://hg.mozilla.org/mozilla-central/rev/b67b22a57dfa
https://hg.mozilla.org/mozilla-central/rev/34e86cf1cc38