Closed Bug 1132558 Opened 9 years ago Closed 9 years ago

Report OS install date to Telemetry (as a proxy for the machine's age)

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: vladan, Assigned: myagkov-a, Mentored)

References

Details

(Whiteboard: [lang=c++])

Attachments

(1 file, 3 obsolete files)

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.
Mentor: aklotz, vdjeric
Whiteboard: [lang=c++]
Attached patch patch (obsolete) — Splinter Review
Hi!

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

review ping?
Flags: needinfo?(vdjeric)
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
Attachment #8582644 - Flags: review?(aklotz)
* I'll turn over the review of the Windows-specific parts to Aaron
Flags: needinfo?(vdjeric)
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.
Attachment #8582644 - Flags: review?(aklotz) → review-
Hi guys! 

Thank you for review. I refactored my code.
Flags: needinfo?(aklotz)
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
Attachment #8588040 - Flags: feedback+
Flags: needinfo?(aklotz)
Hi Aaron! I fixed style errors.
Attachment #8582644 - Attachment is obsolete: true
Attachment #8588040 - Attachment is obsolete: true
Flags: needinfo?(aklotz)
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.
Attachment #8590464 - Flags: feedback+
Flags: needinfo?(aklotz)
Hi Aaron! Sorry for delay. I fixed my code.
Attachment #8590464 - Attachment is obsolete: true
Flags: needinfo?(aklotz)
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.
Attachment #8596189 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b8af7eb3f513
Assignee: nobody → myagkov-a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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
Flags: needinfo?(vdjeric)
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?
Flags: needinfo?(vdjeric)
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
Flags: needinfo?(vdjeric)
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
Flags: needinfo?(vdjeric)
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
You need to log in before you can comment on or make changes to this bug.