Closed Bug 1255472 Opened 4 years ago Closed 4 years ago

Collect actual major and minor version data on Windows 10

Categories

(Toolkit :: Telemetry, defect, P1)

Unspecified
Windows 10
defect
Points:
2

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: RyanVM, Assigned: Dexter)

References

Details

(Whiteboard: [measurement:client])

Attachments

(3 files, 9 obsolete files)

3.15 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
6.42 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
5.33 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
Windows 10 has moved away from traditional service packs in its release model. Microsoft now uses major updates once or twice a year (the Build 1511 November Update, for example) and monthly cumulative updates that deliver bug fixes and security updates at the same time.

These updates follow a predictable version numbering system, where the OS Build (as shown by the winver command) uses an X.Y system that denotes which major and minor updates have been installed. For example, 10240.0 represents an unpatched RTM build, and 10586.164 represents a fully patched build as of today. The X value is incremented for the major releases and the Y value is incremented for each monthly cumulative update.

Right now, we have the os.version, servicePackMajor, and servicePackMinor fields for collecting information about which version of Windows a user is on. For a fully-patched Windows 10 build, we get os.version = 10.0, servicePackMajor = 0, and servicePackMinor = 0, giving us no visibility into the effect any Windows updates might have caused (or usage statistics across the different releases).

I propose that we update servicePackMajor and servicePackMinor on Windows 10 to reflect the various patch levels, i.e. servicePackMajor = 10586 & servicePackMinor = 164 as that will give us more useful data that just treating Windows 10 as a single population like we're currently doing. For example, we'll be able to see if a recent cumulative update is causing a crash spike with better accuracy than relying on the date alone. It was also inform our decision making process about which version of Windows 10 to test in automation.
Can we safely expect servicePackMajor and servicePackMinor to stay 0 in the future?
Priority: -- → P3
Whiteboard: [measurement:client]
No, we should not assume that SP won't be used, even though that's the current plan. Let's record the X,Y values for win10 in new fields.
Relevant-looking Windows registry values:
> [HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion]
> "BuildLab"="10586.th2_release_sec.160223-1728"
> "BuildLabEx"="10586.162.amd64fre.th2_release_sec.160223-1728"
> "CurrentBuild"="10586"
> "CurrentBuildNumber"="10586"
> "ReleaseId"="1511"
> "UBR"=dword:000000a4

The UBR value is 164, which correlates to the current monthly patch level. I'm going to ni? myself to verify next week that the value increments as expected after the next monthly cumulative update.
Flags: needinfo?(ryanvm)
Diving into the client API side quickly:

The relevant data is collected here:
https://dxr.mozilla.org/mozilla-central/rev/b6683e141c47c022598c0caac3ea8ba8c6236d42/toolkit/components/telemetry/TelemetryEnvironment.jsm#1234

We currently submit "$dwMajorVersion.$dwMinorVersion" which comes from GetVersionEx():
https://dxr.mozilla.org/mozilla-central/rev/b6683e141c47c022598c0caac3ea8ba8c6236d42/nsprpub/pr/src/md/windows/ntmisc.c#830

I see that GetVersionEx() is actually suspect to the manifest version lie shim:
https://blogs.msdn.microsoft.com/chuckw/2013/09/10/manifest-madness/

GetVersionEx() would presumably get us the build number, but i'm not sure yet where we can get the X.Y numbers from.
Summary: Make servicePackMajor and servicePackMinor collect useful information on Windows 10 → Collect actual major and minor version data on Windows 10
Aaron, do you happen to know if there is an API that would give us the Windows 10 version details (described in comment 0)?
Flags: needinfo?(aklotz)
Priority: P3 → P2
Looking at windows 10 version history here:
https://en.wikipedia.org/wiki/Windows_10_version_history

... i see that "Treshold 2" versions follow the 10.0.X.Y pattern, but Redstone 1 uses 10.0.X.
That will presumably change when Redstone (Anniversary Update) ships and they freeze on X.
Flags: needinfo?(ryanvm)
The "Pre-release versions of Windows 10 Threshold 2" section shows the same.
GetVersionEx() gives you the X as dwBuildNumber provided that we include Windows 10 compat in our manifest (which I believe we do).

I haven't found any better way to access Y other than to read the "UBR" value from the registry. FWIW it appears as though the UBR is incremented by "cumulative update" patches.

The "ReleaseId" value in the registry is also used in version strings.

For example, run Notepad and click Help->About. The version string is shown as
Version <ReleaseId> (OS Build <CurrentBuild>.<UBR>)

It wouldn't surprise me if API access to this additional information was intentionally omitted. Microsoft has been deprecating version APIs due to application developers consistently writing incorrect code to check version numbers for feature detection purposes.
Flags: needinfo?(aklotz)
Thanks Aaron.

We can add two optional properties:
(1) windowsBuildNumber
(2) windowsUBR

For (1), we can change [0], also returning the build number and renaming the function.
For (2), we can read the registry key from JS [1].
We can add both data points at [2]. Test coverage can be added at [3] and tests run with |mach test toolkit/components/telemetry/tests/unit|.

0: https://dxr.mozilla.org/mozilla-central/rev/29d5a4175c8b74f45482276a53985cf2568b4be2/toolkit/components/telemetry/TelemetryEnvironment.jsm#324
1: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Accessing_the_Windows_Registry_Using_XPCOm
2: https://dxr.mozilla.org/mozilla-central/rev/29d5a4175c8b74f45482276a53985cf2568b4be2/toolkit/components/telemetry/TelemetryEnvironment.jsm#1244
3: https://dxr.mozilla.org/mozilla-central/rev/29d5a4175c8b74f45482276a53985cf2568b4be2/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#509
Whiteboard: [measurement:client] → [measurement:client] [lang=js]
Mentor: gfritzsche
Assignee: nobody → alessio.placitelli
Just wanted to confirm that the UBR value was changed to 218 as expected with today's released cumulative update.
Attached patch bug1255472.patch (obsolete) — Splinter Review
This WIP patch adds the fields and a minimal test coverage.
Attached patch bug1255472.patch (obsolete) — Splinter Review
This patch adds the windows build number and UBR value. We force the 64-bit registry view when reading the UBR value.
Attachment #8740802 - Attachment is obsolete: true
Attachment #8740901 - Flags: review?(gfritzsche)
There's an interesting problem when querying the UBR registry key from a 32 bit process: reads to "HKLM\SOFTWARE\Microsoft\Windows NT\CurrentVersion\UBR" gets automagically transalted to "HKLM\SOFTWARE\WOW6432Node\Microsoft\Windows NT\CurrentVersion\UBR".

This patch adds the 64-bit registry view support to WindowsRegistry.
Comment on attachment 8740901 [details] [diff] [review]
bug1255472.patch

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +326,4 @@
>   *
>   * @return An object containing the service pack major and minor versions.
>   */
> +function getBuildAndServicePack() {

`GetWindowsVersionInfo()`?

@@ +330,1 @@
>    const UNKNOWN_SERVICE_PACK = {major: null, minor: null};

Add buildNumber here.

@@ +1247,5 @@
>  
>      if (["gonk", "android"].includes(AppConstants.platform)) {
>        data.kernelVersion = getSysinfoProperty("kernel_version", null);
>      } else if (AppConstants.platform === "win") {
> +      let servicePack = getBuildAndServicePack();

`let versionInfo ...`?

@@ +1252,3 @@
>        data.servicePackMajor = servicePack.major;
>        data.servicePackMinor = servicePack.minor;
> +      data.windowsBuildNumber = servicePack.buildNumber;

We don't intend to use build number or UBR on Windows < 10.
Let's only add it for >=10 (and not pay the bandwidth/storage cost elsewhere).

@@ +1255,5 @@
> +      // Query the UBR key and only add it to the environment if it's available.
> +      // |readRegKey| doesn't throw, but rather returns 'undefined' on error.
> +      let ubr = WindowsRegistry.readRegKey(Ci.nsIWindowsRegKey.ROOT_KEY_LOCAL_MACHINE,
> +                                           WINDOWS_UBR_KEY_PATH, "UBR");
> +      if (typeof(ubr) != 'undefined') {

Why not just `ubr != undefined`?
Attachment #8740901 - Flags: review?(gfritzsche)
Comment on attachment 8740902 [details] [diff] [review]
Part 2 - Add the WOW64_64 flag support to WindowsRegistry

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +1255,5 @@
>        // Query the UBR key and only add it to the environment if it's available.
>        // |readRegKey| doesn't throw, but rather returns 'undefined' on error.
>        let ubr = WindowsRegistry.readRegKey(Ci.nsIWindowsRegKey.ROOT_KEY_LOCAL_MACHINE,
> +                                           WINDOWS_UBR_KEY_PATH, "UBR",
> +                                           Ci.nsIWindowsRegKey.WOW64_64);

Does this still work fine in x64 builds?
Nit, it would have to be `ubr !== undefined`.
This also checks for the Windows major version to be >= 10.
Attachment #8740901 - Attachment is obsolete: true
Attachment #8740978 - Flags: review?(gfritzsche)
Attachment #8740902 - Attachment description: bug1255472_winregistry.patch → Part 2 - Add the WOW64_64 flag support to WindowsRegistry
Attachment #8740902 - Flags: review?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> Comment on attachment 8740902 [details] [diff] [review]
> Part 2 - Add the WOW64_64 flag support to WindowsRegistry
> 
> Review of attachment 8740902 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/TelemetryEnvironment.jsm
> @@ +1255,5 @@
> >        // Query the UBR key and only add it to the environment if it's available.
> >        // |readRegKey| doesn't throw, but rather returns 'undefined' on error.
> >        let ubr = WindowsRegistry.readRegKey(Ci.nsIWindowsRegKey.ROOT_KEY_LOCAL_MACHINE,
> > +                                           WINDOWS_UBR_KEY_PATH, "UBR",
> > +                                           Ci.nsIWindowsRegKey.WOW64_64);
> 
> Does this still work fine in x64 builds?

That's a good point. Given [1], this should not be a problem: we're saying that we want to access the 64 bit registry which is the default registry for x64 builds. Again, to be 100% sure, I guess we should manually test drive this.

[1] - https://msdn.microsoft.com/it-it/library/windows/desktop/aa384129%28v=vs.85%29.aspx
(In reply to Alessio Placitelli [:Dexter] from comment #19)
> That's a good point. Given [1], this should not be a problem: we're saying
> that we want to access the 64 bit registry which is the default registry for
> x64 builds. Again, to be 100% sure, I guess we should manually test drive
> this.

Yes, let's verify that before landing.
Comment on attachment 8740978 [details] [diff] [review]
Part 1 - Add the build number and UBR for Windows >= 10

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +43,5 @@
>  // The maximum length of a string (e.g. description) in the addons section.
>  const MAX_ADDON_STRING_LENGTH = 100;
>  
> +// The path to the "UBR" key, queried to get additional version details on Windows.
> +const WINDOWS_UBR_KEY_PATH = "SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion";

It's only used in one place below, let's move it there.

@@ +329,3 @@
>   */
> +function getWindowsVersionInfo() {
> +  const UNKNOWN_SERVICE_PACK = {major: null, minor: null, buildNumber: null};

* UNKNOWN_VERSION_INFO
* with the function renaming, we should rename major/minor to servicePackMajor/Minor

@@ +398,5 @@
> +  const majorVersionNumber = parseInt(aVersionString.split(".")[0]);
> +  if (!Number.isFinite(majorVersionNumber)) {
> +    return false;
> +  }
> +  // Only return true if the major version number is > 10.

>= 10... not sure that needs a comment though.

@@ +1272,5 @@
> +      let versionInfo = getWindowsVersionInfo();
> +      data.servicePackMajor = versionInfo.major;
> +      data.servicePackMinor = versionInfo.minor;
> +      // We only need the build number and UBR if we're at or above Windows 10.
> +      if (isVersionGreaterWindows10(data.version)) {

Let's use the nsIVersionComparator.
Attachment #8740978 - Flags: review?(gfritzsche)
Comment on attachment 8740902 [details] [diff] [review]
Part 2 - Add the WOW64_64 flag support to WindowsRegistry

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

::: toolkit/modules/WindowsRegistry.jsm
@@ +18,5 @@
>     * @param aKey
>     *        The key name.
> +   * @param [aRegistryView=0]
> +   *        Optionally set to nsIWindowsRegKey.WOW64_64 (or nsIWindowsRegKey.WOW64_32)
> +   *        to access a 64-bit (32-bit) key from either a 32-bit or 64-bit application.

The documentation seems to talk about "nodes", not "views" - lets use that too.
Ditto below.
Attachment #8740902 - Flags: review?(gfritzsche) → review+
Ah, I didn't know about the version comparator. Good to know.
Attachment #8740978 - Attachment is obsolete: true
Attachment #8741432 - Flags: review?(gfritzsche)
Attachment #8740902 - Attachment is obsolete: true
Attachment #8741433 - Flags: review+
Comment on attachment 8741432 [details] [diff] [review]
Part 1 - Add the build number and UBR for Windows >= 10

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +1237,5 @@
>     * @return Object containing the OS data.
>     */
>    _getOSData: function () {
> +    // The path to the "UBR" key, queried to get additional version details on Windows.
> +    const WINDOWS_UBR_KEY_PATH = "SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion";

Let's move this into the Windows specific branch.
Attachment #8741432 - Flags: review?(gfritzsche) → review+
Attachment #8741432 - Attachment is obsolete: true
Attachment #8741666 - Flags: review+
Points: --- → 2
Priority: P2 → P1
Mentor: gfritzsche
Whiteboard: [measurement:client] [lang=js] → [measurement:client]
(In reply to Georg Fritzsche [:gfritzsche] from comment #20)
> (In reply to Alessio Placitelli [:Dexter] from comment #19)
> > That's a good point. Given [1], this should not be a problem: we're saying
> > that we want to access the 64 bit registry which is the default registry for
> > x64 builds. Again, to be 100% sure, I guess we should manually test drive
> > this.
> 
> Yes, let's verify that before landing.

I just verified that it still works on FF x64, using the build produced by the try push: http://archive.mozilla.org/pub/firefox/try-builds/alessio.placitelli@gmail.com-a4fe5d5945396c7987f5ff5a84f86b8b7895368b/try-win64/

Both the UBR and the build number are correct.
Status: NEW → ASSIGNED
Attached patch Part 3 - Update the docs. (obsolete) — Splinter Review
Attachment #8741767 - Flags: review?(gfritzsche)
Attached patch Part 3 - Update the docs. (obsolete) — Splinter Review
Attachment #8741767 - Attachment is obsolete: true
Attachment #8741767 - Flags: review?(gfritzsche)
Attachment #8741777 - Flags: review?(gfritzsche)
Comment on attachment 8741777 [details] [diff] [review]
Part 3 - Update the docs.

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

::: toolkit/components/telemetry/docs/environment.rst
@@ +108,5 @@
>              kernelVersion: <string>, // android/b2g only or null on failure
>              servicePackMajor: <number>, // windows only or null on failure
>              servicePackMinor: <number>, // windows only or null on failure
> +            windowsBuildNumber: <number>, // windows 10 only or null on failure
> +            windowsUBR: <number>, // windows 10 only or non-present on failure

This is a bit inconsistent.
Lets change this (and the code) to be "null on failure" too.

@@ +333,5 @@
> +~~
> +
> +This object contains operating system information.
> +
> +- ``name``: the name of the os.

Nit: Upper-case "OS" here and below.

@@ +335,5 @@
> +This object contains operating system information.
> +
> +- ``name``: the name of the os.
> +- ``version``: a string representing the os version.
> +- ``kernelVersion``: an android/b2g only string representing the kernel version.

"Android/B2G"
Attachment #8741777 - Flags: review?(gfritzsche) → review+
Attachment #8741777 - Attachment is obsolete: true
Attachment #8741875 - Flags: review+
|windowsUBR| is now null on failure.
Attachment #8741666 - Attachment is obsolete: true
Attachment #8741877 - Flags: review+
Slightly changed the test to account for |windowsUBR| being null.
Attachment #8741433 - Attachment is obsolete: true
Attachment #8741878 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/508d2f80ae5e
https://hg.mozilla.org/mozilla-central/rev/e49ca5f6fef2
https://hg.mozilla.org/mozilla-central/rev/abe15212891c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Very limited) initial data is coming in and showing sane-looking results \m/. Is this something we could safely uplift to Aurora as well?
(In reply to Ryan VanderMeulen [:RyanVM] from comment #38)
> (Very limited) initial data is coming in and showing sane-looking results
> \m/. Is this something we could safely uplift to Aurora as well?

Awesome! I'll do some validity checks on the incoming data and proceed with the aurora uplifts tomorrow.
I ran a notebook to sanity check the data [1], everything looks good to me. Georg, is there any other check you think we should do? If not, I'd move on with the aurora uplift.

[1] - https://gist.github.com/Dexterp37/2813698cf14ab611d980e1264449e8e3
Flags: needinfo?(gfritzsche)
Lets also check the distribution of actual build number & UBR values (to avoid surprises later) before uplifting - those could be broken for a variety of reasons.
Otherwise this looks good.
Flags: needinfo?(gfritzsche)
I had Anthony put together a crude dashboard for this. So far, so good.
nbviewer.jupyter.org/urls/s3-us-west-2.amazonaws.com/telemetry-public-analysis-2/windows-distribution/data/windows-build-distribution.ipynb
(In reply to Ryan VanderMeulen [:RyanVM] from comment #42)
> I had Anthony put together a crude dashboard for this. So far, so good.
> nbviewer.jupyter.org/urls/s3-us-west-2.amazonaws.com/telemetry-public-
> analysis-2/windows-distribution/data/windows-build-distribution.ipynb

Oh, whoops. I updated the gist in comment 40 before reading your comment. Everything looks good in that gist as well.
Comment on attachment 8741877 [details] [diff] [review]
Part 1 - Add the build number and UBR for Windows >= 10

Approval Request Comment
[Feature/regressing bug #]: None
[User impact if declined]: No impact, but it would be nice to have win10 version data from aurora on as it could be used to see if a cumulative update is causing a crash spike with better accuracy and to inform the decision making process about which version of Windows 10 to test in automation.
[Describe test coverage new/current, TreeHerder]: This stack of patches adds some sanity checks for the new fields to the existing xpcshell tests.
[Risks and why]: Low, the code has been out for two nightlies and was manually tested. We also performed server-side validation on the incoming nightly data to make sure nothing weird was happening.
[String/UUID change made/needed]: None
Attachment #8741877 - Flags: approval-mozilla-aurora?
Comment on attachment 8741878 [details] [diff] [review]
Part 2 - Add the WOW64_64 flag support to WindowsRegistry

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8741878 - Flags: approval-mozilla-aurora?
Attachment #8741875 - Flags: approval-mozilla-aurora?
Comment on attachment 8741877 [details] [diff] [review]
Part 1 - Add the build number and UBR for Windows >= 10

More telemetry data on win10 builds/versions, Awesome job by the team verifying the data on Nightly (thanks!!!), Aurora47+
Attachment #8741877 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8741875 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8741878 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.