1137222 - Submit subsession-specific value for simpleMeasurements.activeTicks

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

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.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1137222.patch

Comment 2

[Georg Fritzsche [:gfritzsche]]

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.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1137222.patch

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

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: bug1137222.patch - v2

Fixed a typo.

Comment 5

[Georg Fritzsche [:gfritzsche]]

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.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: bug1137222.patch - v3

Fixed the comment.

Comment 7

[Alessio Placitelli [:Dexter]]

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

Comment 8

[Georg Fritzsche [:gfritzsche]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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