841028 - Telemetry field for "build ID changed with this restart"

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[:Irving Reid (No longer working on Firefox)]

A number of things we collect in Telemetry (for example, the addon update information in bug 810146) is affected by whether or not the browser was updated. It would be helpful to have a field in telemetry indicating whether a particular session is the first run after the software was updated so we can filter.

Bug 786788 has one possible approach, just sending the previous build ID as part of the ping - this gives us extra info because we know what build they updated from.

Comment 1

[(dormant account)]

can we just add lastBuildID? it's useful for other things too.

Comment 2

[:Irving Reid (No longer working on Firefox)]

Attachment: Send previousBuildID in telemetry if build ID changed

This uses a pref to store previous build ID, and only adds a previousBuildID field to the Telemetry "System Information" section if the current appBuildID is different from the one found on last run. It sends "unknown" as the previousBuildID the first time.

Comment 3

[(dormant account)]

Comment on attachment 722384 [details] [diff] [review]
Send previousBuildID in telemetry if build ID changed

+    let previousBuildID = "unknown";
     
previousBuildID = undefined. I don't think it's valuable to send "unknown" once for every profile.

Please update the test to check for this field being present & prefs being written.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 722384 [details] [diff] [review]
Send previousBuildID in telemetry if build ID changed

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

Looks good, but needs tests before this can land.

::: toolkit/components/telemetry/TelemetryPing.js
@@ +749,5 @@
>        return;
>      }
>  #endif
>      let enabled = false; 
> +    let previousBuildID = "unknown";

Like Taras said, this needs to be something else--anything false-y so that on first run, when we don't have PREF_PREVIOUS_BUILDID, this._previousBuildID will get set to something false-y a few lines below.  And then the |if (this._previousBuildID)| check several hundred lines above will DTRT.

Comment 5

[:Irving Reid (No longer working on Firefox)]

(In reply to Taras Glek (:taras) from comment #3)
> +    let previousBuildID = "unknown";
>      
> previousBuildID = undefined. I don't think it's valuable to send "unknown"
> once for every profile.

I did that on purpose, to handle the first run when somebody got updated code including this patch (which would have to be a build ID change, since they got the new code); that would give us the ability to filter out updates from regular restarts one update earlier. Not sending previousBuildID="unknown" the first time is fine with me, if that's what you want.

(In reply to Nathan Froyd (:froydnj) from comment #4)
> And then the |if (this._previousBuildID)| check several hundred lines above
> will DTRT.

It did the right thing in my (admittedly manual) test already; the missing field tests as false in the if statement. If that's considered bad style, I'll set it to something falsy.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Irving Reid (:irving) from comment #5)
> (In reply to Nathan Froyd (:froydnj) from comment #4)
> > And then the |if (this._previousBuildID)| check several hundred lines above
> > will DTRT.
> 
> It did the right thing in my (admittedly manual) test already; the missing
> field tests as false in the if statement. If that's considered bad style,
> I'll set it to something falsy.

Sorry!  I think I had convinced myself that we wanted to test |if (!this.---)|.  My mistake.  You'll have to hash out the on-first-run bits with Taras, though.

Comment 7

[:Irving Reid (No longer working on Firefox)]

Attachment: Added unit test, don't send "unknown" previous build ID on first run

Taras, are you OK with my strategy of bypassing xpcom to test TelemetryPing.js?

Comment 8

[(dormant account)]

Comment on attachment 724949 [details] [diff] [review]
Added unit test, don't send "unknown" previous build ID on first run

I let let Nathan decide on rest of this bug. I was mainly concerned with what data gets submitted

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 724949 [details] [diff] [review]
Added unit test, don't send "unknown" previous build ID on first run

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

+1 for writing new tests instead of cramming this into test_TelemetryPing.js.

::: toolkit/components/telemetry/TelemetryPing.js
@@ +368,5 @@
>        platformBuildID: ai.platformBuildID,
>        revision: HISTOGRAMS_FILE_VERSION,
>        locale: getLocale()
>      };
> +    if (this._previousBuildID) {

Please add a definition of _previousBuildID up with the rest of TelemetryPing's variables for consistency's sake.

@@ +770,5 @@
> +      this._previousBuildID = previousBuildID;
> +      Services.prefs.setCharPref(PREF_PREVIOUS_BUILDID, thisBuildID);
> +    }
> +
> +

Nit: one less line of whitespace, please.

::: toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ 

Nit: trailing whitespace.

@@ +14,5 @@
> + */
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +// Get the TelemetryPing definitions directly so we can test it without going through xpcom

Nit: can you make comments full sentences with periods at the end?  Here and elsewhere.

@@ +65,5 @@
> +
> +
> +function run_test() {
> +  // Make sure we have a profile directory?
> +  // do_get_profile();

I think it would be good to run do_get_profile even though we technically don't use it in this test.  TelemetryPing wants a profile directory and might eventually start behaving in funny ways if one's not there.

Comment 10

[:Irving Reid (No longer working on Firefox)]

Attachment: Patch as landed, with review nits cleaned up

https://hg.mozilla.org/integration/mozilla-inbound/rev/3614febde426

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/828162ad3ab0

https://tbpl.mozilla.org/php/getParsedLog.php?id=20658920&tree=Mozilla-Inbound

14:09:56     INFO -  TEST-INFO | /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js | running test ...
14:09:56  WARNING -  TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js | test failed (with xpcshell return code: 0), see following log:
14:09:56     INFO -  >>>>>>>
14:09:56     INFO -  ### XPCOM_MEM_LEAK_LOG defined -- logging leaks to /tmp/tmpl3HjTg/runxpcshelltests_leaks.log
14:09:56     INFO -  Profiler: ----------------- MOZ_PROFILER_NEW not set -----------------
14:09:56     INFO -  JS Component Loader: WARNING /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js -> resource://gre/components/TelemetryPing.js:44
14:09:57     INFO -                       octal literals and octal escape sequences are deprecated
14:09:57     INFO -  JS Component Loader: WARNING /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js -> resource://gre/components/TelemetryPing.js:44
14:09:57     INFO -                       octal literals and octal escape sequences are deprecated
14:09:57     INFO -  JS Component Loader: WARNING /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js -> resource://gre/components/TelemetryPing.js:44
14:09:57     INFO -                       octal literals and octal escape sequences are deprecated
14:09:57     INFO -  JS Component Loader: WARNING /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js -> resource://gre/components/TelemetryPing.js:45
14:09:57     INFO -                       octal literals and octal escape sequences are deprecated
14:09:57     INFO -  JS Component Loader: WARNING /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js -> resource://gre/components/TelemetryPing.js:45
14:09:57     INFO -                       octal literals and octal escape sequences are deprecated
14:09:57     INFO -  JS Component Loader: WARNING /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js -> resource://gre/components/TelemetryPing.js:45
14:09:57     INFO -                       octal literals and octal escape sequences are deprecated
14:09:57     INFO -  TEST-INFO | (xpcshell/head.js) | test 1 pending
14:09:57     INFO -  LoadPlugin() /builds/slave/test/build/application/firefox/plugins/libnptest.so returned 9e0f370
14:09:57     INFO -  LoadPlugin() /builds/slave/test/build/application/firefox/plugins/libnpsecondtest.so returned 9e0f6b8
14:09:57     INFO -  TEST-PASS | /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js | [testFirstRun : 34] false == false
14:09:57  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell/head.js | [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: /builds/slave/test/build/tests/xpcshell/tests/toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js :: testFirstRun :: line 36"  data: no]
14:09:57     INFO -  WARNING: nsExceptionService ignoring thread destruction after shutdown: file ../../../xpcom/base/nsExceptionService.cpp, line 167
14:09:57     INFO -  WARNING: NS_ENSURE_TRUE(compMgr) failed: file nsComponentManagerUtils.cpp, line 58
14:09:57     INFO -  WARNING: OOPDeinit() without successful OOPInit(): file ../../../toolkit/crashreporter/nsExceptionHandler.cpp, line 2288
14:09:57     INFO -  nsStringStats
14:09:57     INFO -   => mAllocCount:           2290
14:09:57     INFO -   => mReallocCount:          186
14:09:57     INFO -   => mFreeCount:            2290
14:09:57     INFO -   => mShareCount:           8148
14:09:57     INFO -   => mAdoptCount:            119
14:09:57     INFO -   => mAdoptFreeCount:        119
14:09:57     INFO -  <<<<<<<

Comment 12

[:Irving Reid (No longer working on Firefox)]

Attachment: Setup code moved to pass tests on Ubuntu

For some reason the tests work on all other platforms but fail on Ubuntu. Moving the setting of the previousBuildID preference before the early exits in TelemetryPing.setup() made the problem go away, though to be honest I can't figure out why this could would execute differently on Ubuntu than on Fedora.

That said, I think it makes sense to have the code this way; we keep track of previousBuildID whether or not Telemetry is currently gathering other data, so we could expose it to other modules in future if they're interested, and we'll do the right thing if somebody turns on telemetry later.

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 725790 [details] [diff] [review]
Setup code moved to pass tests on Ubuntu

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

I don't see why it would make any difference either, but I agree it's better.

::: toolkit/components/telemetry/TelemetryPing.js
@@ +767,5 @@
>        Telemetry.canRecord = false;
>        return;
>      }
>  #endif
> +

Nit: no need for the blank line.

@@ +772,5 @@
>      let enabled = false; 
>      try {
>        enabled = Services.prefs.getBoolPref(PREF_ENABLED);
>        this._server = Services.prefs.getCharPref(PREF_SERVER);
> +      previousBuildID = Services.prefs.getCharPref(PREF_PREVIOUS_BUILDID);

This bit isn't necessary anymore, since we've done it above.

Comment 14

[:Irving Reid (No longer working on Firefox)]

Landed with nits and bitrot fixed, r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/13ddefe3068a

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/13ddefe3068a