1376942 - Add activeTicks as a scalar

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[bugzilla]

We currently have payload.simpleMeasurements.activeTicks -- we should also put that value as a scalar (probably under the browser.engagement namespace), with the hope that we'll be able to remove simpleMeasurements.activeTicks one day.

Comment 1

[Vanessa Gutierrez]

Attachment: Added active_ticks scalar

Added active_ticks as a Scalar.

Comment 2

[Vanessa Gutierrez]

Comment on attachment 8883133 [details]
Added active_ticks scalar

I am having trouble uploading all the files for this patch, please ignore this attachment as I will try to upload a different way.

Comment 3

[Vanessa Gutierrez]

Attachment: Added active_ticks scalar. Incremented active_ticks where Simple Measurements active ticks is incremented. Implemented equivalent tests where Simple Measurements active ticks is tested

Comment 4

[Vanessa Gutierrez]

Attachment: Ran eslint test, passed. All tests now passing

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8883753 [details] [diff] [review]
Ran eslint test, passed. All tests now passing

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

This looks close.
Running the test though i get test failures: ./mach xpcshell-test toolkit/components/telemetry/tests/unit

There are also some other smaller issues. The comments below may look like a lot, but most of them are just small comments about strings and style.

The next steps are:
- Make the tests work.
- Fix or clear up the small comments below.
- Update the commit message to be more meaningful ("Bug xxx - something about the activeTicks change.").
- ... get review again.

Then after that passed:
- Get data review. [1]
- Then lets get this landed.
- (in future work, a few days later: validate that the incoming data is correct)

1: https://wiki.mozilla.org/Firefox/Data_Collection

::: toolkit/components/telemetry/Scalars.yaml
@@ +140,5 @@
>        - [e-mail]
>      release_channel_collection: opt-out
>      record_in_processes:
>        - 'main'
> +  

There is some trailing whitespace on this line and others below (that should be removed).
Depending on your editor, you can usually configure it to highlight this.
Alternatively you can see it highlighted here when looking into the "splinter review".

@@ +152,5 @@
> +      has focus or is in the foreground, only if it is receiving these interaction events.
> +    expires: never
> +    kind: uint
> +    notification_emails:
> +      - [e-mail]

Did we check with Brendan if he will own this?

::: toolkit/components/telemetry/TelemetryScalar.cpp
@@ +179,5 @@
>    return &gScalarsStringTable[this->expiration_offset];
>  }
>  
>  /**
> + * The base scalar object, that serves as a common ancestor for storage

Nice catch.
Can we have this in a separate patch though?
This is unrelated to the required changes for activeTicks though and touches a C++ file.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1949,5 @@
>      // Don't count the first active tick after we get out of
>      // inactivity, because it is just the start of this active tick.
>      if (needsUpdate) {
>        this._sessionActiveTicks++;
> +      Telemetry.scalarAdd("browser.engagement.active_ticks", 1);

Woohoo. This is so much cleaner.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +947,5 @@
>    let subsession = TelemetrySession.getPayload("environment-change");
>    Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
>                 "Classic pings must count active ticks since the beginning of the session.");
>    Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
> +               "Subsessions must count (Simple Measurements) active ticks as classic pings on the first subsession.");

The line is getting pretty long, let's just move it in (e.g. indented two spaces more than the Assert).
Minor: Let's use "active ticks (in simpleMeasurements)".

@@ +948,5 @@
>    Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
>                 "Classic pings must count active ticks since the beginning of the session.");
>    Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
> +               "Subsessions must count (Simple Measurements) active ticks as classic pings on the first subsession.");
> +  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks,

I get test failures here; i don't think this is the right way to access this.
You can e.g. test around by using the Web Console in about:telemetry and trying out these commands.

@@ +949,5 @@
>                 "Classic pings must count active ticks since the beginning of the session.");
>    Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
> +               "Subsessions must count (Simple Measurements) active ticks as classic pings on the first subsession.");
> +  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks,
> +               "(Scalar) active ticks is incorrect for the first subsession.");

"Subsession must count active ticks (in scalars) as classic pings on the first subsession."

@@ +961,5 @@
>                 "Classic pings must count active ticks since the beginning of the session.");
>    Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
>                 "Pings must not loose the tick count when starting a new subsession.");
> +  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks,
> +               "(Scalar) active ticks must not lose count for a previous subsession when starting a new subsession.");

Lets also indent this line less.

@@ +971,5 @@
>    Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
>                 "Classic pings must count active ticks since the beginning of the session.");
>    Assert.equal(subsession.simpleMeasurements.activeTicks,
>                 expectedActiveTicks - activeTicksAtSubsessionStart,
> +               "Subsessions must count (Simple Measurements) active ticks since the last new subsession.");

"Subsessions must count active ticks (in simpleMeasurements) ..."

@@ +972,5 @@
>                 "Classic pings must count active ticks since the beginning of the session.");
>    Assert.equal(subsession.simpleMeasurements.activeTicks,
>                 expectedActiveTicks - activeTicksAtSubsessionStart,
> +               "Subsessions must count (Simple Measurements) active ticks since the last new subsession.");
> +  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks - activeTicksAtSubsessionStart,

This line is pretty long, lets put the second parameter on a new line.

@@ +973,5 @@
>    Assert.equal(subsession.simpleMeasurements.activeTicks,
>                 expectedActiveTicks - activeTicksAtSubsessionStart,
> +               "Subsessions must count (Simple Measurements) active ticks since the last new subsession.");
> +  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks - activeTicksAtSubsessionStart,
> +               "Active ticks (Scalar version) must count active ticks since the last new subsession.");

"Subsessions must count active ticks (in scalars) ..."

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js
@@ +19,5 @@
>  add_task(async function test_record_activeTicks() {
>    await TelemetryController.testSetup();
>  
>    let checkActiveTicks = (expected) => {
> +    // Make sure we get a subsession payload

How about: "// Scalars are only present in subsession payloads."

@@ +25,2 @@
>      Assert.equal(payload.simpleMeasurements.activeTicks, expected,
> +                 "TelemetrySession must record the expected number of (Simple Measurement) active ticks.");

"... active ticks (in simpleMeasurements)."

@@ +25,4 @@
>      Assert.equal(payload.simpleMeasurements.activeTicks, expected,
> +                 "TelemetrySession must record the expected number of (Simple Measurement) active ticks.");
> +    Assert.equal(payload.processes.parent.scalars["browser.engagement.active_ticks"], expected,
> +                 "TelemetrySession must record the expected number of (Scalar) active ticks.");

"... active ticks (in scalars)."

Comment 6

[Vanessa Gutierrez]

(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> Comment on attachment 8883753 [details] [diff] [review]
> Ran eslint test, passed. All tests now passing
> 
> Review of attachment 8883753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks close.
> Running the test though i get test failures: ./mach xpcshell-test
> toolkit/components/telemetry/tests/unit
> 
> There are also some other smaller issues. The comments below may look like a
> lot, but most of them are just small comments about strings and style.
> 
> The next steps are:
> - Make the tests work.
> - Fix or clear up the small comments below.
> - Update the commit message to be more meaningful ("Bug xxx - something
> about the activeTicks change.").
> - ... get review again.
> 
> Then after that passed:
> - Get data review. [1]
> - Then lets get this landed.
> - (in future work, a few days later: validate that the incoming data is
> correct)
> 
> 1: https://wiki.mozilla.org/Firefox/Data_Collection
> 
> ::: toolkit/components/telemetry/Scalars.yaml
> @@ +140,5 @@
> >        - [e-mail]
> >      release_channel_collection: opt-out
> >      record_in_processes:
> >        - 'main'
> > +  
> 
> There is some trailing whitespace on this line and others below (that should
> be removed).
> Depending on your editor, you can usually configure it to highlight this.
> Alternatively you can see it highlighted here when looking into the
> "splinter review".
> 
> @@ +152,5 @@
> > +      has focus or is in the foreground, only if it is receiving these interaction events.
> > +    expires: never
> > +    kind: uint
> > +    notification_emails:
> > +      - [e-mail]
> 
> Did we check with Brendan if he will own this?
> 
> ::: toolkit/components/telemetry/TelemetryScalar.cpp
> @@ +179,5 @@
> >    return &gScalarsStringTable[this->expiration_offset];
> >  }
> >  
> >  /**
> > + * The base scalar object, that serves as a common ancestor for storage
> 
> Nice catch.
> Can we have this in a separate patch though?
> This is unrelated to the required changes for activeTicks though and touches
> a C++ file.
> 
> ::: toolkit/components/telemetry/TelemetrySession.jsm
> @@ +1949,5 @@
> >      // Don't count the first active tick after we get out of
> >      // inactivity, because it is just the start of this active tick.
> >      if (needsUpdate) {
> >        this._sessionActiveTicks++;
> > +      Telemetry.scalarAdd("browser.engagement.active_ticks", 1);
> 
> Woohoo. This is so much cleaner.
> 
> ::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
> @@ +947,5 @@
> >    let subsession = TelemetrySession.getPayload("environment-change");
> >    Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
> >                 "Classic pings must count active ticks since the beginning of the session.");
> >    Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
> > +               "Subsessions must count (Simple Measurements) active ticks as classic pings on the first subsession.");
> 
> The line is getting pretty long, let's just move it in (e.g. indented two
> spaces more than the Assert).
> Minor: Let's use "active ticks (in simpleMeasurements)".
> 
> @@ +948,5 @@
> >    Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
> >                 "Classic pings must count active ticks since the beginning of the session.");
> >    Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
> > +               "Subsessions must count (Simple Measurements) active ticks as classic pings on the first subsession.");
> > +  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks,
> 
> I get test failures here; i don't think this is the right way to access this.
> You can e.g. test around by using the Web Console in about:telemetry and
> trying out these commands.
> 
> @@ +949,5 @@
> >                 "Classic pings must count active ticks since the beginning of the session.");
> >    Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
> > +               "Subsessions must count (Simple Measurements) active ticks as classic pings on the first subsession.");
> > +  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks,
> > +               "(Scalar) active ticks is incorrect for the first subsession.");
> 
> "Subsession must count active ticks (in scalars) as classic pings on the
> first subsession."
> 
> @@ +961,5 @@
> >                 "Classic pings must count active ticks since the beginning of the session.");
> >    Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
> >                 "Pings must not loose the tick count when starting a new subsession.");
> > +  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks,
> > +               "(Scalar) active ticks must not lose count for a previous subsession when starting a new subsession.");
> 
> Lets also indent this line less.
> 
> @@ +971,5 @@
> >    Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
> >                 "Classic pings must count active ticks since the beginning of the session.");
> >    Assert.equal(subsession.simpleMeasurements.activeTicks,
> >                 expectedActiveTicks - activeTicksAtSubsessionStart,
> > +               "Subsessions must count (Simple Measurements) active ticks since the last new subsession.");
> 
> "Subsessions must count active ticks (in simpleMeasurements) ..."
> 
> @@ +972,5 @@
> >                 "Classic pings must count active ticks since the beginning of the session.");
> >    Assert.equal(subsession.simpleMeasurements.activeTicks,
> >                 expectedActiveTicks - activeTicksAtSubsessionStart,
> > +               "Subsessions must count (Simple Measurements) active ticks since the last new subsession.");
> > +  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks - activeTicksAtSubsessionStart,
> 
> This line is pretty long, lets put the second parameter on a new line.
> 
> @@ +973,5 @@
> >    Assert.equal(subsession.simpleMeasurements.activeTicks,
> >                 expectedActiveTicks - activeTicksAtSubsessionStart,
> > +               "Subsessions must count (Simple Measurements) active ticks since the last new subsession.");
> > +  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks - activeTicksAtSubsessionStart,
> > +               "Active ticks (Scalar version) must count active ticks since the last new subsession.");
> 
> "Subsessions must count active ticks (in scalars) ..."
> 
> :::
> toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js
> @@ +19,5 @@
> >  add_task(async function test_record_activeTicks() {
> >    await TelemetryController.testSetup();
> >  
> >    let checkActiveTicks = (expected) => {
> > +    // Make sure we get a subsession payload
> 
> How about: "// Scalars are only present in subsession payloads."
> 
> @@ +25,2 @@
> >      Assert.equal(payload.simpleMeasurements.activeTicks, expected,
> > +                 "TelemetrySession must record the expected number of (Simple Measurement) active ticks.");
> 
> "... active ticks (in simpleMeasurements)."
> 
> @@ +25,4 @@
> >      Assert.equal(payload.simpleMeasurements.activeTicks, expected,
> > +                 "TelemetrySession must record the expected number of (Simple Measurement) active ticks.");
> > +    Assert.equal(payload.processes.parent.scalars["browser.engagement.active_ticks"], expected,
> > +                 "TelemetrySession must record the expected number of (Scalar) active ticks.");
> 
> "... active ticks (in scalars)."

Hi Georg!
Can I ask which tests are failing? I just ran ./mach xpcshell-test toolkit/components/telemetry/tests/unit and all of my tests passed. I'll try some web console commands like you mentioned.

I will fix the trailing white spaces, thanks for the tip to use splinter review. I wonder why the eslint tests didn't catch those.

I checked with Brendan during All Hands, so we are good there!

I'll remove the c++ file edit from the next patch. Would I create a new bug and submit it as a patch for that?

As for the comment and formatting clean up, will do!

Comment 7

[Georg Fritzsche [:gfritzsche]]

(In reply to Vanessa Gutierrez from comment #6)
> > @@ +948,5 @@
> > >    Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
> > >                 "Classic pings must count active ticks since the beginning of the session.");
> > >    Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
> > > +               "Subsessions must count (Simple Measurements) active ticks as classic pings on the first subsession.");
> > > +  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks,
> > 
> > I get test failures here; i don't think this is the right way to access this.
> > You can e.g. test around by using the Web Console in about:telemetry and
> > trying out these commands.
...
> Hi Georg!
> Can I ask which tests are failing? I just ran ./mach xpcshell-test
> toolkit/components/telemetry/tests/unit and all of my tests passed. I'll try
> some web console commands like you mentioned.

I checked again today and it succeeded. So i think this was my fault for rebasing your patch incorrectly yesterday.

> I'll remove the c++ file edit from the next patch. Would I create a new bug
> and submit it as a patch for that?

A new bug would be more clear, but i wouldn't mind in a separate patch here either.

Comment 8

[Vanessa Gutierrez]

Attachment: Corrected incorrect spelling in comment: "servers" changed to "serves".

Comment 9

[Vanessa Gutierrez]

Attachment: Updated indents so long lines don't extend far right, and changed wording in comments to refer to active ticks (in scalars) or (in simpleMeasurements). Active ticks (in scalar) is passing all tests and matches the active ticks (in simpleMeasurements) when

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8884417 [details] [diff] [review]
Corrected incorrect spelling in comment: "servers" changed to "serves".

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

This looks good.
The commit message has a trailing ' though, please fix that.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8884420 [details] [diff] [review]
Updated indents so long lines don't extend far right, and changed wording in comments to refer to active ticks (in scalars) or (in simpleMeasurements). Active ticks (in scalar) is passing all tests and matches the active ticks (in simpleMeasurements) when

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

This looks good, thanks.
Some smaller things need fixing, then we can proceed.

The commit message here needs improving.
The first line should be "Bug xxx - Short summary of what it does.".
Additional details can follow after an empty line (if needed).

::: toolkit/components/telemetry/Scalars.yaml
@@ +147,5 @@
> +      - 1376942
> +    description: >
> +      The count of the number of five-second intervals ('ticks') the user 
> +      was considered 'active' in a subsession. Session activity involves keyboard or mouse 
> +      interaction with the application. It does not take into account whether or not the window 

This file still has trailing whitespace.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js
@@ +19,5 @@
>  add_task(async function test_record_activeTicks() {
>    await TelemetryController.testSetup();
>  
>    let checkActiveTicks = (expected) => {
> +    // Scalars are only present in subsession payloads

Small nit: Comments should also end with ".".

Comment 12

[Vanessa Gutierrez]

Comment on attachment 8884417 [details] [diff] [review]
Corrected incorrect spelling in comment: "servers" changed to "serves".

># HG changeset patch
># Parent  71df9ddca4fe68d05b1b3f7b4c01f64e73c7c2d5
>Bug 1376942 - Correct incorrect spelling in comment.
>
>diff --git a/toolkit/components/telemetry/TelemetryScalar.cpp b/toolkit/components/telemetry/TelemetryScalar.cpp
>--- a/toolkit/components/telemetry/TelemetryScalar.cpp
>+++ b/toolkit/components/telemetry/TelemetryScalar.cpp
>@@ -175,17 +175,17 @@ ScalarInfo::name() const
> 
> const char *
> ScalarInfo::expiration() const
> {
>   return &gScalarsStringTable[this->expiration_offset];
> }
> 
> /**
>- * The base scalar object, that servers as a common ancestor for storage
>+ * The base scalar object, that serves as a common ancestor for storage
>  * purposes.
>  */
> class ScalarBase
> {
> public:
>   virtual ~ScalarBase() = default;
> 
>   // Set, Add and SetMaximum functions as described in the Telemetry IDL.

Comment 13

[Vanessa Gutierrez]

Comment on attachment 8884420 [details] [diff] [review]
Updated indents so long lines don't extend far right, and changed wording in comments to refer to active ticks (in scalars) or (in simpleMeasurements). Active ticks (in scalar) is passing all tests and matches the active ticks (in simpleMeasurements) when

># HG changeset patch
># Parent  8f80d594c08d5c7a112e5d4b9eb44ffca717eb7b
>Bug 1376942 - Updated indents for long lines & fixed wording in comments. Scalar is passing all tests and matches the simpleMeasurements when run locally. Removed whitespace.
>
>diff --git a/toolkit/components/telemetry/Scalars.yaml b/toolkit/components/telemetry/Scalars.yaml
>--- a/toolkit/components/telemetry/Scalars.yaml
>+++ b/toolkit/components/telemetry/Scalars.yaml
>@@ -137,16 +137,32 @@ browser.engagement:
>     expires: never
>     kind: uint
>     notification_emails:
>       - [e-mail]
>     release_channel_collection: opt-out
>     record_in_processes:
>       - 'main'
> 
>+  active_ticks:
>+    bug_numbers:
>+      - 1376942
>+    description: >
>+      The count of the number of five-second intervals ('ticks') the user 
>+      was considered 'active' in a subsession. Session activity involves keyboard or mouse 
>+      interaction with the application. It does not take into account whether or not the window 
>+      has focus or is in the foreground, only if it is receiving these interaction events.
>+    expires: never
>+    kind: uint
>+    notification_emails:
>+      - [e-mail]
>+    release_channel_collection: opt-out
>+    record_in_processes:
>+      - 'main'
>+
> # The following section contains the browser engagement scalars.
> browser.engagement.navigation:
>   urlbar:
>     bug_numbers:
>       - 1271313
>     description: >
>       The count URI loads triggered in a subsession from the urlbar (awesomebar),
>       broken down by the originating action.
>diff --git a/toolkit/components/telemetry/TelemetrySession.jsm b/toolkit/components/telemetry/TelemetrySession.jsm
>--- a/toolkit/components/telemetry/TelemetrySession.jsm
>+++ b/toolkit/components/telemetry/TelemetrySession.jsm
>@@ -1945,16 +1945,17 @@ var Impl = {
>   _onActiveTick(aUserActive) {
>     const needsUpdate = aUserActive && this._isUserActive;
>     this._isUserActive = aUserActive;
> 
>     // Don't count the first active tick after we get out of
>     // inactivity, because it is just the start of this active tick.
>     if (needsUpdate) {
>       this._sessionActiveTicks++;
>+      Telemetry.scalarAdd("browser.engagement.active_ticks", 1);
>     }
>   },
> 
>   /**
>    * This observer drives telemetry.
>    */
>   observe(aSubject, aTopic, aData) {
>     // Prevent the cycle collector begin topic from cluttering the log.
>diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
>--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
>+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
>@@ -941,39 +941,46 @@ add_task(async function test_checkSubses
> 
>   await TelemetryController.testReset();
> 
>   // Both classic and subsession payload data should be the same on the first subsession.
>   incrementActiveTicks();
>   let classic = TelemetrySession.getPayload();
>   let subsession = TelemetrySession.getPayload("environment-change");
>   Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Classic pings must count active ticks since the beginning of the session.");
>+    "Classic pings must count active ticks (in simpleMeasurements) since the beginning of the session.");
>   Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Subsessions must count active ticks as classic pings on the first subsession.");
>+    "Subsessions must count active ticks (in simpleMeasurements) as classic pings on the first subsession.");
>+  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks,
>+    "Subsessions must count active ticks (in scalars) as classic pings on the first subsession.");
> 
>   // Start a new subsession and check that the active ticks are correctly reported.
>   incrementActiveTicks();
>   activeTicksAtSubsessionStart = getActiveTicks();
>   classic = TelemetrySession.getPayload();
>   subsession = TelemetrySession.getPayload("environment-change", true);
>   Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Classic pings must count active ticks since the beginning of the session.");
>+    "Classic pings must count active ticks (in simpleMeasurements) since the beginning of the session.");
>   Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Pings must not loose the tick count when starting a new subsession.");
>+    "Pings must not lose the tick count when starting a new subsession.");
>+  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks,
>+    "Active ticks (in scalars) must not lose count for a previous subsession when starting a new subsession.");
> 
>   // Get a new subsession payload without clearing the subsession.
>   incrementActiveTicks();
>   classic = TelemetrySession.getPayload();
>   subsession = TelemetrySession.getPayload("environment-change");
>   Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Classic pings must count active ticks since the beginning of the session.");
>+    "Classic pings must count active ticks (in simpleMeasurements) since the beginning of the session.");
>   Assert.equal(subsession.simpleMeasurements.activeTicks,
>-               expectedActiveTicks - activeTicksAtSubsessionStart,
>-               "Subsessions must count active ticks since the last new subsession.");
>+    expectedActiveTicks - activeTicksAtSubsessionStart,
>+    "Subsessions must count active ticks (in simpleMeasurements) since the last new subsession.");
>+  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"],
>+    expectedActiveTicks - activeTicksAtSubsessionStart,
>+    "Subsessions must count active ticks (in scalars) since the last new subsession.");
> });
> 
> add_task(async function test_dailyCollection() {
>   if (gIsAndroid) {
>     // We don't do daily collections yet on Android.
>     return;
>   }
> 
>diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js b/toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js
>--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js
>+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js
>@@ -15,20 +15,23 @@ add_task(async function test_setup() {
> 
>   Services.prefs.setBoolPref(PREF_TELEMETRY_ENABLED, true);
> });
> 
> add_task(async function test_record_activeTicks() {
>   await TelemetryController.testSetup();
> 
>   let checkActiveTicks = (expected) => {
>-    let payload = TelemetrySession.getPayload();
>+    // Scalars are only present in subsession payloads
>+    let payload = TelemetrySession.getPayload("main");
>     Assert.equal(payload.simpleMeasurements.activeTicks, expected,
>-                 "TelemetrySession must record the expected number of active ticks.");
>-  };
>+                 "TelemetrySession must record the expected number of active ticks (in simpleMeasurements).");
>+    Assert.equal(payload.processes.parent.scalars["browser.engagement.active_ticks"], expected,
>+                 "TelemetrySession must record the expected number of active ticks (in scalars).");
>+  }
> 
>   for (let i = 0; i < 3; i++) {
>     Services.obs.notifyObservers(null, "user-interaction-active");
>   }
>   checkActiveTicks(3);
> 
>   // Now send inactive. This must not increment the active ticks.
>   Services.obs.notifyObservers(null, "user-interaction-inactive");

Comment 14

[Vanessa Gutierrez]

(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> Comment on attachment 8884417 [details] [diff] [review]
> Corrected incorrect spelling in comment: "servers" changed to "serves".
> 
> Review of attachment 8884417 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good.
> The commit message has a trailing ' though, please fix that.

I thought I had fixed that but I had corrected it in the wrong place. Fixed now!

Comment 15

[Vanessa Gutierrez]

(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> Comment on attachment 8884420 [details] [diff] [review]
> Updated indents so long lines don't extend far right, and changed wording in
> comments to refer to active ticks (in scalars) or (in simpleMeasurements).
> Active ticks (in scalar) is passing all tests and matches the active ticks
> (in simpleMeasurements) when
> 
> Review of attachment 8884420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, thanks.
> Some smaller things need fixing, then we can proceed.
> 
> The commit message here needs improving.
> The first line should be "Bug xxx - Short summary of what it does.".
> Additional details can follow after an empty line (if needed).
> 
> ::: toolkit/components/telemetry/Scalars.yaml
> @@ +147,5 @@
> > +      - 1376942
> > +    description: >
> > +      The count of the number of five-second intervals ('ticks') the user 
> > +      was considered 'active' in a subsession. Session activity involves keyboard or mouse 
> > +      interaction with the application. It does not take into account whether or not the window 
> 
> This file still has trailing whitespace.
> 
> :::
> toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js
> @@ +19,5 @@
> >  add_task(async function test_record_activeTicks() {
> >    await TelemetryController.testSetup();
> >  
> >    let checkActiveTicks = (expected) => {
> > +    // Scalars are only present in subsession payloads
> 
> Small nit: Comments should also end with ".".

Noted! That commit message has been shortened.

Comment 16

[Vanessa Gutierrez]

Attachment: Added activeTicks as a scalar.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8884859 [details] [diff] [review]
Added activeTicks as a scalar.

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

I think the commit message should be about adding activeTicks as a scalar, not "Removed trailing whitespace & fixed punctuation".

Comment 18

[Vanessa Gutierrez]

Comment on attachment 8884859 [details] [diff] [review]
Added activeTicks as a scalar.

># HG changeset patch
># Parent  8f80d594c08d5c7a112e5d4b9eb44ffca717eb7b
>Bug 1376942 - Added active ticks as a scalar.
>
>diff --git a/toolkit/components/telemetry/Scalars.yaml b/toolkit/components/telemetry/Scalars.yaml
>--- a/toolkit/components/telemetry/Scalars.yaml
>+++ b/toolkit/components/telemetry/Scalars.yaml
>@@ -137,16 +137,32 @@ browser.engagement:
>     expires: never
>     kind: uint
>     notification_emails:
>       - [e-mail]
>     release_channel_collection: opt-out
>     record_in_processes:
>       - 'main'
> 
>+  active_ticks:
>+    bug_numbers:
>+      - 1376942
>+    description: >
>+      The count of the number of five-second intervals ('ticks') the user
>+      was considered 'active' in a subsession. Session activity involves keyboard or mouse
>+      interaction with the application. It does not take into account whether or not the window
>+      has focus or is in the foreground, only if it is receiving these interaction events.
>+    expires: never
>+    kind: uint
>+    notification_emails:
>+      - [e-mail]
>+    release_channel_collection: opt-out
>+    record_in_processes:
>+      - 'main'
>+
> # The following section contains the browser engagement scalars.
> browser.engagement.navigation:
>   urlbar:
>     bug_numbers:
>       - 1271313
>     description: >
>       The count URI loads triggered in a subsession from the urlbar (awesomebar),
>       broken down by the originating action.
>diff --git a/toolkit/components/telemetry/TelemetrySession.jsm b/toolkit/components/telemetry/TelemetrySession.jsm
>--- a/toolkit/components/telemetry/TelemetrySession.jsm
>+++ b/toolkit/components/telemetry/TelemetrySession.jsm
>@@ -1945,16 +1945,17 @@ var Impl = {
>   _onActiveTick(aUserActive) {
>     const needsUpdate = aUserActive && this._isUserActive;
>     this._isUserActive = aUserActive;
> 
>     // Don't count the first active tick after we get out of
>     // inactivity, because it is just the start of this active tick.
>     if (needsUpdate) {
>       this._sessionActiveTicks++;
>+      Telemetry.scalarAdd("browser.engagement.active_ticks", 1);
>     }
>   },
> 
>   /**
>    * This observer drives telemetry.
>    */
>   observe(aSubject, aTopic, aData) {
>     // Prevent the cycle collector begin topic from cluttering the log.
>diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
>--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
>+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
>@@ -941,39 +941,46 @@ add_task(async function test_checkSubses
> 
>   await TelemetryController.testReset();
> 
>   // Both classic and subsession payload data should be the same on the first subsession.
>   incrementActiveTicks();
>   let classic = TelemetrySession.getPayload();
>   let subsession = TelemetrySession.getPayload("environment-change");
>   Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Classic pings must count active ticks since the beginning of the session.");
>+    "Classic pings must count active ticks (in simpleMeasurements) since the beginning of the session.");
>   Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Subsessions must count active ticks as classic pings on the first subsession.");
>+    "Subsessions must count active ticks (in simpleMeasurements) as classic pings on the first subsession.");
>+  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks,
>+    "Subsessions must count active ticks (in scalars) as classic pings on the first subsession.");
> 
>   // Start a new subsession and check that the active ticks are correctly reported.
>   incrementActiveTicks();
>   activeTicksAtSubsessionStart = getActiveTicks();
>   classic = TelemetrySession.getPayload();
>   subsession = TelemetrySession.getPayload("environment-change", true);
>   Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Classic pings must count active ticks since the beginning of the session.");
>+    "Classic pings must count active ticks (in simpleMeasurements) since the beginning of the session.");
>   Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Pings must not loose the tick count when starting a new subsession.");
>+    "Pings must not lose the tick count when starting a new subsession.");
>+  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks,
>+    "Active ticks (in scalars) must not lose count for a previous subsession when starting a new subsession.");
> 
>   // Get a new subsession payload without clearing the subsession.
>   incrementActiveTicks();
>   classic = TelemetrySession.getPayload();
>   subsession = TelemetrySession.getPayload("environment-change");
>   Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Classic pings must count active ticks since the beginning of the session.");
>+    "Classic pings must count active ticks (in simpleMeasurements) since the beginning of the session.");
>   Assert.equal(subsession.simpleMeasurements.activeTicks,
>-               expectedActiveTicks - activeTicksAtSubsessionStart,
>-               "Subsessions must count active ticks since the last new subsession.");
>+    expectedActiveTicks - activeTicksAtSubsessionStart,
>+    "Subsessions must count active ticks (in simpleMeasurements) since the last new subsession.");
>+  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"],
>+    expectedActiveTicks - activeTicksAtSubsessionStart,
>+    "Subsessions must count active ticks (in scalars) since the last new subsession.");
> });
> 
> add_task(async function test_dailyCollection() {
>   if (gIsAndroid) {
>     // We don't do daily collections yet on Android.
>     return;
>   }
> 
>diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js b/toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js
>--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js
>+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js
>@@ -15,20 +15,23 @@ add_task(async function test_setup() {
> 
>   Services.prefs.setBoolPref(PREF_TELEMETRY_ENABLED, true);
> });
> 
> add_task(async function test_record_activeTicks() {
>   await TelemetryController.testSetup();
> 
>   let checkActiveTicks = (expected) => {
>-    let payload = TelemetrySession.getPayload();
>+    // Scalars are only present in subsession payloads.
>+    let payload = TelemetrySession.getPayload("main");
>     Assert.equal(payload.simpleMeasurements.activeTicks, expected,
>-                 "TelemetrySession must record the expected number of active ticks.");
>-  };
>+                 "TelemetrySession must record the expected number of active ticks (in simpleMeasurements).");
>+    Assert.equal(payload.processes.parent.scalars["browser.engagement.active_ticks"], expected,
>+                 "TelemetrySession must record the expected number of active ticks (in scalars).");
>+  }
> 
>   for (let i = 0; i < 3; i++) {
>     Services.obs.notifyObservers(null, "user-interaction-active");
>   }
>   checkActiveTicks(3);
> 
>   // Now send inactive. This must not increment the active ticks.
>   Services.obs.notifyObservers(null, "user-interaction-inactive");

Comment 19

[Vanessa Gutierrez]

Comment on attachment 8884859 [details] [diff] [review]
Added activeTicks as a scalar.

># HG changeset patch
># Parent  8f80d594c08d5c7a112e5d4b9eb44ffca717eb7b
>Bug 1376942 - Added activeTicks as a scalar.
>
>diff --git a/toolkit/components/telemetry/Scalars.yaml b/toolkit/components/telemetry/Scalars.yaml
>--- a/toolkit/components/telemetry/Scalars.yaml
>+++ b/toolkit/components/telemetry/Scalars.yaml
>@@ -137,16 +137,32 @@ browser.engagement:
>     expires: never
>     kind: uint
>     notification_emails:
>       - [e-mail]
>     release_channel_collection: opt-out
>     record_in_processes:
>       - 'main'
> 
>+  active_ticks:
>+    bug_numbers:
>+      - 1376942
>+    description: >
>+      The count of the number of five-second intervals ('ticks') the user
>+      was considered 'active' in a subsession. Session activity involves keyboard or mouse
>+      interaction with the application. It does not take into account whether or not the window
>+      has focus or is in the foreground, only if it is receiving these interaction events.
>+    expires: never
>+    kind: uint
>+    notification_emails:
>+      - [e-mail]
>+    release_channel_collection: opt-out
>+    record_in_processes:
>+      - 'main'
>+
> # The following section contains the browser engagement scalars.
> browser.engagement.navigation:
>   urlbar:
>     bug_numbers:
>       - 1271313
>     description: >
>       The count URI loads triggered in a subsession from the urlbar (awesomebar),
>       broken down by the originating action.
>diff --git a/toolkit/components/telemetry/TelemetrySession.jsm b/toolkit/components/telemetry/TelemetrySession.jsm
>--- a/toolkit/components/telemetry/TelemetrySession.jsm
>+++ b/toolkit/components/telemetry/TelemetrySession.jsm
>@@ -1945,16 +1945,17 @@ var Impl = {
>   _onActiveTick(aUserActive) {
>     const needsUpdate = aUserActive && this._isUserActive;
>     this._isUserActive = aUserActive;
> 
>     // Don't count the first active tick after we get out of
>     // inactivity, because it is just the start of this active tick.
>     if (needsUpdate) {
>       this._sessionActiveTicks++;
>+      Telemetry.scalarAdd("browser.engagement.active_ticks", 1);
>     }
>   },
> 
>   /**
>    * This observer drives telemetry.
>    */
>   observe(aSubject, aTopic, aData) {
>     // Prevent the cycle collector begin topic from cluttering the log.
>diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
>--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
>+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
>@@ -941,39 +941,46 @@ add_task(async function test_checkSubses
> 
>   await TelemetryController.testReset();
> 
>   // Both classic and subsession payload data should be the same on the first subsession.
>   incrementActiveTicks();
>   let classic = TelemetrySession.getPayload();
>   let subsession = TelemetrySession.getPayload("environment-change");
>   Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Classic pings must count active ticks since the beginning of the session.");
>+    "Classic pings must count active ticks (in simpleMeasurements) since the beginning of the session.");
>   Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Subsessions must count active ticks as classic pings on the first subsession.");
>+    "Subsessions must count active ticks (in simpleMeasurements) as classic pings on the first subsession.");
>+  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks,
>+    "Subsessions must count active ticks (in scalars) as classic pings on the first subsession.");
> 
>   // Start a new subsession and check that the active ticks are correctly reported.
>   incrementActiveTicks();
>   activeTicksAtSubsessionStart = getActiveTicks();
>   classic = TelemetrySession.getPayload();
>   subsession = TelemetrySession.getPayload("environment-change", true);
>   Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Classic pings must count active ticks since the beginning of the session.");
>+    "Classic pings must count active ticks (in simpleMeasurements) since the beginning of the session.");
>   Assert.equal(subsession.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Pings must not loose the tick count when starting a new subsession.");
>+    "Pings must not lose the tick count when starting a new subsession.");
>+  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"], expectedActiveTicks,
>+    "Active ticks (in scalars) must not lose count for a previous subsession when starting a new subsession.");
> 
>   // Get a new subsession payload without clearing the subsession.
>   incrementActiveTicks();
>   classic = TelemetrySession.getPayload();
>   subsession = TelemetrySession.getPayload("environment-change");
>   Assert.equal(classic.simpleMeasurements.activeTicks, expectedActiveTicks,
>-               "Classic pings must count active ticks since the beginning of the session.");
>+    "Classic pings must count active ticks (in simpleMeasurements) since the beginning of the session.");
>   Assert.equal(subsession.simpleMeasurements.activeTicks,
>-               expectedActiveTicks - activeTicksAtSubsessionStart,
>-               "Subsessions must count active ticks since the last new subsession.");
>+    expectedActiveTicks - activeTicksAtSubsessionStart,
>+    "Subsessions must count active ticks (in simpleMeasurements) since the last new subsession.");
>+  Assert.equal(subsession.processes.parent.scalars["browser.engagement.active_ticks"],
>+    expectedActiveTicks - activeTicksAtSubsessionStart,
>+    "Subsessions must count active ticks (in scalars) since the last new subsession.");
> });
> 
> add_task(async function test_dailyCollection() {
>   if (gIsAndroid) {
>     // We don't do daily collections yet on Android.
>     return;
>   }
> 
>diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js b/toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js
>--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js
>+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js
>@@ -15,20 +15,23 @@ add_task(async function test_setup() {
> 
>   Services.prefs.setBoolPref(PREF_TELEMETRY_ENABLED, true);
> });
> 
> add_task(async function test_record_activeTicks() {
>   await TelemetryController.testSetup();
> 
>   let checkActiveTicks = (expected) => {
>-    let payload = TelemetrySession.getPayload();
>+    // Scalars are only present in subsession payloads.
>+    let payload = TelemetrySession.getPayload("main");
>     Assert.equal(payload.simpleMeasurements.activeTicks, expected,
>-                 "TelemetrySession must record the expected number of active ticks.");
>-  };
>+                 "TelemetrySession must record the expected number of active ticks (in simpleMeasurements).");
>+    Assert.equal(payload.processes.parent.scalars["browser.engagement.active_ticks"], expected,
>+                 "TelemetrySession must record the expected number of active ticks (in scalars).");
>+  }
> 
>   for (let i = 0; i < 3; i++) {
>     Services.obs.notifyObservers(null, "user-interaction-active");
>   }
>   checkActiveTicks(3);
> 
>   // Now send inactive. This must not increment the active ticks.
>   Services.obs.notifyObservers(null, "user-interaction-inactive");

Comment 20

[Vanessa Gutierrez]

Ignore - Testing a short reply.

Comment 21

[Vanessa Gutierrez]

Comment on attachment 8884417 [details] [diff] [review]
Corrected incorrect spelling in comment: "servers" changed to "serves".

Please review.

Comment 22

[Vanessa Gutierrez]

Attachment: Added activeTicks as a scalar

Comment 23

[Vanessa Gutierrez]

Attachment: Corrected incorrect spelling in comment: servers changed to serves

Comment 24

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8885767 [details] [diff] [review]
Added activeTicks as a scalar

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

This looks good, thanks!

Next steps:
- get data review on this patch: https://wiki.mozilla.org/Firefox/Data_Collection
- push to try (test run for CI build, i will push it there)
- ... with those resolved, we can get this bug landed

Comment 25

[Georg Fritzsche [:gfritzsche]]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1043fe87e444be7c03d5043e1a501d7bb88b2c6d

Comment 26

[Georg Fritzsche [:gfritzsche]]

I see test failures on Android here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1043fe87e444be7c03d5043e1a501d7bb88b2c6d&selectedJob=113738964
https://treeherder.mozilla.org/logviewer.html#?job_id=113738964&repo=try&lineNumber=2259

We don't support subsessions & scalars on Firefox for Android, so we need to skip this check there:
https://hg.mozilla.org/try/annotate/ea1e7b1d57ba/toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js#l28

Comment 27

[Vanessa Gutierrez]

Comment on attachment 8885767 [details] [diff] [review]
Added activeTicks as a scalar

Please review for data collection compliance: added a scalar version of pre-existing simpleMeasurement activeTicks.

Comment 28

[Vanessa Gutierrez]

Attachment: Added activeTicks as a scalar

Comment 29

[Vanessa Gutierrez]

(In reply to Vanessa Gutierrez from comment #28)
> Created attachment 8886287 [details] [diff] [review]
> Added activeTicks as a scalar

Excluded Android in subsession tests.

Comment 30

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8886287 [details] [diff] [review]
Added activeTicks as a scalar

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

I don't see any Android checks in this patch.
Are you sure the changes were included correctly?

Comment 31

[Vanessa Gutierrez]

Attachment: Added activeTicks as a scalar

Comment 32

[Vanessa Gutierrez]

(In reply to Vanessa Gutierrez from comment #31)
> Created attachment 8886350 [details] [diff] [review]
> Added activeTicks as a scalar

Ok, now there's an Android check in test_TelemetrySession_activeTicks.js

Comment 33

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8886350 [details] [diff] [review]
Added activeTicks as a scalar

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

This looks good (with the redundant line removed).

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a977013081e32a0288879903b35b0060d367ac36

Don't forget about data review.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js
@@ +3,5 @@
>  */
>  
>  Cu.import("resource://gre/modules/TelemetryController.jsm", this);
>  Cu.import("resource://gre/modules/TelemetrySession.jsm", this);
> +Cu.import("resource://gre/modules/AppConstants.jsm");

You don't need to import AppConstants here, you only use the `gIsAndroid` from head.js.

Comment 34

[Vanessa Gutierrez]

Attachment: Added activeTicks as a scalar

Comment 35

[Vanessa Gutierrez]

(In reply to Vanessa Gutierrez from comment #34)
> Created attachment 8886639 [details] [diff] [review]
> Added activeTicks as a scalar

Removed the unnecessary import line in the activeTicks test.

Comment 36

[Vanessa Gutierrez]

Comment on attachment 8886639 [details] [diff] [review]
Added activeTicks as a scalar

Please review for data collection compliance: added a scalar version of pre-existing simpleMeasurement activeTicks.

Comment 37

[François Marier [:francois]]

Comment on attachment 8886639 [details] [diff] [review]
Added activeTicks as a scalar

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

datareview+

Comment 38

[Ryan VanderMeulen [:RyanVM]]

This patch is based on a parent revision from June 27th and doesn't apply cleanly to inbound. Please rebase the patch.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8885772 [details] [diff] [review]
Corrected incorrect spelling in comment: servers changed to serves

Can we just fold this change into the main patch?

Comment 40

[Georg Fritzsche [:gfritzsche]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #39)
> Can we just fold this change into the main patch?

This is completely unrelated to the main patch, i'd rather not have it folded.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

(In reply to Georg Fritzsche [away Jul 19 - 24] [:gfritzsche] from comment #40)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #39)
> > Can we just fold this change into the main patch?
> 
> This is completely unrelated to the main patch, i'd rather not have it
> folded.

Arguably it shouldn't be landing under this bug number then :).

Comment 42

[Vanessa Gutierrez]

Attachment: Added activeTicks as a scalar

Comment 43

[Vanessa Gutierrez]

(In reply to Vanessa Gutierrez from comment #42)
> Created attachment 8888101 [details] [diff] [review]
> Added activeTicks as a scalar

Rebased the patch.

Comment 44

[Alessio Placitelli [:Dexter]]

Comment on attachment 8888101 [details] [diff] [review]
Added activeTicks as a scalar

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

Stealing the review: the rebased patch seem to contain the ' npm-shrinkwrap.json' file which is unrelated to your changes.

Comment 45

[Vanessa Gutierrez]

Attachment: Added activeTicks as a scalar

Comment 46

[Vanessa Gutierrez]

(In reply to Vanessa Gutierrez from comment #45)
> Created attachment 8888389 [details] [diff] [review]
> Added activeTicks as a scalar

Removed the 'npm-shrinkwrap.json" file from the patch.

Comment 47

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e1d1a0c2cca3f30e76d7bc30f9bbc4b1f38c5a3
Bug 1376942 - Added activeTicks as a scalar. r=dexter,gfritzsche;data-r=francois

https://hg.mozilla.org/integration/mozilla-inbound/rev/479136e0a11e5b3f790ae3972960ef476e8d68bb
Bug 1376942 - Corrected incorrect spelling in comment: servers changed to serves. r=gfritzsche

Comment 48

[Alessio Placitelli [:Dexter]]

No need to add the checkin-needed flag, I've already pushed this to "inbound" for you (see comment 47)! :)

Comment 49

[Vanessa Gutierrez]

(In reply to Alessio Placitelli [:Dexter] from comment #48)
> No need to add the checkin-needed flag, I've already pushed this to
> "inbound" for you (see comment 47)! :)

Oh thank you! I didn't realize that's what that meant. WOO! What happens from here?

Comment 50

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

https://hg.mozilla.org/mozilla-central/rev/6e1d1a0c2cca
https://hg.mozilla.org/mozilla-central/rev/479136e0a11e

Comment 51

[Alessio Placitelli [:Dexter]]

(In reply to Vanessa Gutierrez from comment #49)
> (In reply to Alessio Placitelli [:Dexter] from comment #48)
> > No need to add the checkin-needed flag, I've already pushed this to
> > "inbound" for you (see comment 47)! :)
> 
> Oh thank you! I didn't realize that's what that meant. WOO! What happens
> from here?

Sorry for the delay! Merge happened :) Your patch didn't result in any problems while it was on "inbound", and the sheriffs merged it to mozilla-central. It should have made it's way to Nightly by now (see comment 50)!