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.

Added active_ticks as a Scalar.

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 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: http://wiki.mozilla.org/Firefox/Data_Collection ::: toolkit/components/telemetry/Scalars.yaml @@ +140,5 @@ > - rweiss@mozilla.com > 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: > + - bcolloran@mozilla.com 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)."

(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: http://wiki.mozilla.org/Firefox/Data_Collection > > ::: toolkit/components/telemetry/Scalars.yaml > @@ +140,5 @@ > > - rweiss@mozilla.com > > 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: > > + - bcolloran@mozilla.com > > 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!

(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 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 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 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 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: > - rweiss@mozilla.com > 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: >+ - bcolloran@mozilla.com >+ 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");

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

(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 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 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: > - rweiss@mozilla.com > 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: >+ - bcolloran@mozilla.com >+ 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 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: > - rweiss@mozilla.com > 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: >+ - bcolloran@mozilla.com >+ 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");

Ignore - Testing a short reply.

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

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: http://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

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

I see test failures on Android here: http://treeherder.mozilla.org/#/jobs?repo=try&revision=1043fe87e444be7c03d5043e1a501d7bb88b2c6d&selectedJob=113738964 http://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: http://hg.mozilla.org/try/annotate/ea1e7b1d57ba/toolkit/components/telemetry/tests/unit/test_TelemetrySession_activeTicks.js#l28

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.

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

(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 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: http://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.

(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 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 on attachment 8886639 [details] [diff] [review] Added activeTicks as a scalar Review of attachment 8886639 [details] [diff] [review]: ----------------------------------------------------------------- datareview+

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

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?

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

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

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

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.

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/6e1d1a0c2cca3f30e76d7bc30f9bbc4b1f38c5a3 Bug 1376942 - Added activeTicks as a scalar. r=dexter,gfritzsche;data-r=francois http://hg.mozilla.org/integration/mozilla-inbound/rev/479136e0a11e5b3f790ae3972960ef476e8d68bb Bug 1376942 - Corrected incorrect spelling in comment: servers changed to serves. r=gfritzsche

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

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

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

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