The performance team (http://wiki.mozilla.org/Performance) studies how Firefox performs on real user systems using measurements collected by the Telemetry reporting system (https://wiki.mozilla.org/Telemetry). In order to properly interpret Telemetry performance data, we need to know how much different system features (CPU speed, SSD vs HDD, etc) affect different measurements of Firefox performance. Telemetry currently reports information about the computer's CPU, RAM, GPU, OS, storage device and Firefox configuration. See the "System Information" section in about:telemetry for details. We'd also like to know the age of the machine. We can use the OS install date as a proxy for machine age. How to get started: 1. Take a look at about:telemetry to get a sense of the data reported by Telemetry 2. Familiarize yourself with the "getMetadata" function in TelemetryPing.jsm. This JS function calls into C++ code to get the system characteristics: http://hg.mozilla.org/mozilla-central/annotate/81f979b17fbd/toolkit/components/telemetry/TelemetrySession.jsm#l536 3. Familiarize yourself with nsSystemInfo::Init(). This is where you should add code to get the OS install date. http://hg.mozilla.org/mozilla-central/annotate/81f979b17fbd/xpcom/base/nsSystemInfo.cpp#l169 4. You can get the install date on Windows from the registry (in epoch format): HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\InstallDate You'll need to confirm which Windows versions have this key. 5. Telemetry should only report the year of the install date -- too much precision in Telemetry can be a privacy risk. This patch can be Windows-only to start.

Hi! InstallDate value exists in XP, Vista, 7, 8.

Hi! review ping?

Comment on attachment 8582644 [details] [diff] [review] patch Review of attachment 8582644 [details] [diff] [review]: ----------------------------------------------------------------- Hi Anton, sorry for the delay, I didn't spot your message in my bugzilla mail. Next time please mark me with the "review?" flag on the patch :) I'll turn the review of the Window Some comments: - Use two spaces to indent in this file and break up lines that are longer than 80 chars You can find the full coding style guide here https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Naming_and_Formatting_code - You'll also need to modify TelemetryEnvironment.jsm to add this info to the Telemetry packet http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryEnvironment.jsm#590 ::: xpcom/base/nsSystemInfo.cpp @@ +135,5 @@ > + > +nsresult GetInstallYear(uint32_t year) > +{ > + HKEY hkey; > + LSTATUS status = RegOpenKeyEx(HKEY_LOCAL_MACHINE, __TEXT("SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion"), 0, KEY_READ | KEY_WOW64_64KEY, &hkey); The Mozilla codebase is cross-platform so it uses some non-standard conventions. Use NS_LITERAL_STRING instead of __TEXT @@ +298,5 @@ > + > + uint32_t installYear = 0; > + if (NS_SUCCEEDED(GetInstallYear(installYear))) { > + rv = SetPropertyAsUint32( NS_LITERAL_STRING("winInstallYear"), installYear); > + NS_ENSURE_SUCCESS(rv, rv); We don't use NS_ENSURE_SUCCESS anymore, you can use NS_WARN_IF instead https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_the_nice_macros

* I'll turn over the review of the Windows-specific parts to Aaron

Comment on attachment 8582644 [details] [diff] [review] patch Review of attachment 8582644 [details] [diff] [review]: ----------------------------------------------------------------- You're on the right track, most of my comments are requesting style improvements. ::: xpcom/base/nsSystemInfo.cpp @@ +135,5 @@ > + > +nsresult GetInstallYear(uint32_t year) > +{ > + HKEY hkey; > + LSTATUS status = RegOpenKeyEx(HKEY_LOCAL_MACHINE, __TEXT("SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion"), 0, KEY_READ | KEY_WOW64_64KEY, &hkey); Please declare status as a LONG. Typically when we call Windows APIs that accept string parameters, we explicitly call the required ANSI/Unicode variant. In addition to Vladan's remark, please call RegOpenKeyExW directly. @@ +137,5 @@ > +{ > + HKEY hkey; > + LSTATUS status = RegOpenKeyEx(HKEY_LOCAL_MACHINE, __TEXT("SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion"), 0, KEY_READ | KEY_WOW64_64KEY, &hkey); > + > + if (status == ERROR_SUCCESS) { Can you please reformat this code to return immediately if status != ERROR_SUCCESS? See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Return_from_errors_immediately @@ +142,5 @@ > + DWORD type = 0; > + time_t raw_time = 0; > + DWORD time_size = sizeof(time_t); > + > + status = RegQueryValueEx(hkey, __TEXT("InstallDate"), NULL, &type, (LPBYTE)&raw_time, &time_size); Same here, use RegQueryValueExW and replace the __TEXT macro. Also please pass nullptr instead of NULL. @@ +145,5 @@ > + > + status = RegQueryValueEx(hkey, __TEXT("InstallDate"), NULL, &type, (LPBYTE)&raw_time, &time_size); > + RegCloseKey(hkey); > + > + if (status == ERROR_SUCCESS && type == REG_DWORD) { Again, we should be failing immediately here instead of putting the success case inside the if block. @@ +149,5 @@ > + if (status == ERROR_SUCCESS && type == REG_DWORD) { > + > + tm time; > + if (localtime_s(&time, &raw_time) == 0) { > + year = 1970UL + time.tm_year; We should be adding 1900 to tm_year instead of 1970.

Hi guys! Thank you for review. I refactored my code.

Comment on attachment 8588040 [details] [diff] [review] Bug 1132558 - get Windows install date Review of attachment 8588040 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! The control flow is much better, however I still have a few style recommendations. ::: xpcom/base/nsSystemInfo.cpp @@ +134,5 @@ > + > + > +nsresult GetInstallYear(uint32_t* year) > +{ > + if (!year) We always add curly braces around if and else statements. Please fix all of these accordingly. @@ +137,5 @@ > +{ > + if (!year) > + return NS_ERROR_UNEXPECTED; > + > + HKEY hkey; This code still contains tab characters. Please replace the tabs with spaces. @@ +163,5 @@ > + > + RegCloseKey(hkey); > + > + tm time; > + if (localtime_s(&time, &raw_time) != 0) There is a trailing space at the end of this line, please remove that when you add the curly braces. @@ +165,5 @@ > + > + tm time; > + if (localtime_s(&time, &raw_time) != 0) > + return NS_ERROR_UNEXPECTED; > + There are some extra tabs or spaces on this blank line, please remove those. @@ +309,5 @@ > } > + > + uint32_t installYear = 0; > + if (NS_SUCCEEDED(GetInstallYear(&installYear))) { > + rv = SetPropertyAsUint32( NS_LITERAL_STRING("installYear"), installYear); Nit: Please remove the space between the '(' and the NS_LITERAL_STRING

Hi Aaron! I fixed style errors.

Comment on attachment 8590464 [details] [diff] [review] Bug 1132558 - get Windows install date Review of attachment 8590464 [details] [diff] [review]: ----------------------------------------------------------------- We need a couple more changes here. Thanks for your patience, Anton! ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +984,5 @@ > kernelVersion: getSysinfoProperty("kernel_version", null), > #elif defined(XP_WIN) > servicePackMajor: servicePack.major, > servicePackMinor: servicePack.minor, > + installYear: getSysinfoProperty("installDate", null), getSysinfoProperty should be requesting "installYear" instead of "installDate" to match nsSystemInfo.cpp ::: xpcom/base/nsSystemInfo.cpp @@ +132,5 @@ > return NS_OK; > } > + > + > +nsresult GetInstallYear(uint32_t* year) Can you please use a reference instead of a pointer here? Also, please rename |year| to |aYear| to follow with our naming conventions. @@ +148,5 @@ > + > + if (status != ERROR_SUCCESS) { > + return NS_ERROR_UNEXPECTED; > + } > + You will leak hkey if an error occurs past this point. Can you please remove the call to RegCloseKey, add a #include for nsWindowsHelpers.h and use nsAutoRegKey instead? You just instantiate it like: nsAutoRegKey key(hkey); and then nsAutoRegKey will handle the key closing.

Hi Aaron! Sorry for delay. I fixed my code.

Comment on attachment 8596189 [details] [diff] [review] Bug 1132558 - get Windows install date Review of attachment 8596189 [details] [diff] [review]: ----------------------------------------------------------------- A few nits, but overall I'm happy with this patch. Nit: commit message should begin with the bug number. ::: xpcom/base/nsSystemInfo.cpp @@ +20,2 @@ > #include "base/scoped_handle_win.h" > +#include "nsWindowsHelpers.h" Nit: these includes should be in alphabetical order.

I'll push this to try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d3f1666e6a7

Thanks for the patch! https://hg.mozilla.org/integration/mozilla-inbound/rev/b8af7eb3f513

Hi guys! How about find the install date in Unix(Linux)?. We can estimate the install date by searching the oldest file. Most of unix file systems do not store a file's creation time. However several file systems store the file creation time(ext4, ufs2, zfs, btrfs, jfs). We may look oldest file in /boot(Also in Debian and Ubuntu we may check /var/log/installer/syslog creation time). Discussion about this topic: http://unix.stackexchange.com/questions/9971/how-do-i-find-how-long-ago-a-linux-system-was-installed

We're interested in getting install dates for other platforms, but stat()'ing a lot of files doesn't sound like the ideal approach, especially if we're fetching the install date on the event thread as in the Windows patch. Is there a "lighter" way to get the install date from Ubuntu? And do you know how to get the install date on OS X?

Hi Vladan! On Ubuntu and OS X we may check install log creation time. Ubuntu(Debian): /var/log/installer/syslog OS X: /var/log/install.log

That sounds very promising. The OS X/Linux patch will be different from the Windows patch. Can you write a patch that calls the OS.File library from TelemetryEnvironment.jsm to fetch the log creation times? This is going to require using JS Promises for the asynchronous file I/O. OS.File FAQ: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm#Using_OS.File OS.File API documentation, lots of examples in Firefox source code: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_the_main_thread You will need to make _initTask wait for the install log creation time to be returned by OS.File

The /var/log/install.log on my colleague's Mac has a creation date that matches the last OS upgrade date, not the machine age. He suggested using the creation date of /usr/local instead, I'm not sure how reliable that is though

Hi Vladan! Ubuntu doesn't change syslog creation time after upgrade. I check /usr/local or another alternatives on Os X soon. Also I don't know how define different unix systems in js code( I find only Windows and Android defines in TelemetryEnviroment.js).

It is not so easy on Ubuntu. Os.File.info doesn't return BirthDate(https://bugzilla.mozilla.org/show_bug.cgi?id=1156060).In fact stat function returns empty birth in Ubuntu. It's possible to get birth date(https://gist.github.com/moiseevigor/8c496f632137605b322e), however it demands root privilege( as far I understand, it isn't suitable for us). I will search additional information.

Thank you, Anton. I pinged the author of OS.File on bug 1156060