Georg Fritzsche [:gfritzsche] Reporter
	Description • 9 years ago

From bug 1128491 it seems like (besides histograms) we only need to reset simpleMeasurements.activeTicks for now.

So, we need to pass the subsession and clearSubsession flags to TelemetrySession.getSimpleMeasurements().
We need to remember the last activeTicks value when we the clearSubsession flag is set.
If the subsession flag is set, we just submit the difference between the last activeTicks value and sessionRecorder.activeTicks.

Lets add test-coverage in test_TelemetrySession.js.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 2 • 9 years ago

Comment on attachment 8582546 [details] [diff] [review]
bug1137222.patch

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +490,5 @@
>    _profileSubsessionCounter: 0,
>    // Date of the last session split
>    _subsessionStartDate: null,
> +  // The active ticks counted when the subsession starts
> +  _subsessionStartActiveTicks: 0,

We need to reset this in the TelemetrySession init to properly handle test resets.

@@ +507,4 @@
>     *
>     * @return simple measurements as a dictionary.
>     */
> +  getSimpleMeasurements: function getSimpleMeasurements(forSavedSession, subsession, clearSubsession) {

Lets make the param |isSubsession|.

@@ +600,5 @@
>        let sr = drs.getSessionRecorder();
>        if (sr) {
> +        let activeTicks = sr.activeTicks;
> +        if (clearSubsession) {
> +          this._subsessionStartActiveTicks = activeTicks;

We want to clear this after basing the returned value on it, otherwise we lose the subsession info.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +723,5 @@
>      }
>    };
>  
> +  // Keep track of the active ticks count if the session recorder is available.
> +  let clearSessionActiveTicks = 0;

I can't tell what this is supposed to represent. Can we name it more clearly (like e.g. "activeTicksAtSubsessionStart")?

@@ +732,5 @@
> +    clearSessionActiveTicks = sessionRecorder.activeTicks;
> +    expectedActiveTicks = clearSessionActiveTicks;
> +  }
> +
> +  checkActiveTicks = (classic, subsession) => {

Nit: classicValue, subsessionValue.

@@ +754,5 @@
>    // Both classic and subsession payload histograms should start the same.
>    // The payloads should be identical for now except for the reason.
>    count.clear();
>    keyed.clear();
> +  incrementActiveTicks();

I think this will be more readable & maintainable if we keep this test as test_checkSubsessionHistograms() and add a separate one below for other session & subsession data.

	Alessio Placitelli [:Dexter] Assignee
	Comment 3 • 9 years ago

Thanks for the review, I've addressed your comments in this patch.

	Alessio Placitelli [:Dexter] Assignee
	Comment 4 • 9 years ago

Fixed a typo.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 5 • 9 years ago

Comment on attachment 8583723 [details] [diff] [review]
bug1137222.patch - v2

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

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +853,5 @@
>  });
>  
> +add_task(function* test_checkSubsessionData() {
> +  if (gIsAndroid || !SESSION_RECORDER_EXPECTED) {
> +    // We don't support subsessions yet on Android.

Fixup the comment - the check is not only handling Android.

	Alessio Placitelli [:Dexter] Assignee
	Comment 6 • 9 years ago

Fixed the comment.

	Alessio Placitelli [:Dexter] Assignee
	Comment 7 • 9 years ago

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

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 8 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/676a58f45385

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/676a58f45385