799228 - Send Android version as OS version for Telemetry

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Xavier Stevens [:xstevens]]

Telemetry on Android should be changed so that it sends the Android version rather than the Linux kernel version for the "version" field under "info" in Telemetry data.

Comment 1

[(dormant account)]

Easiest way to do this would be to change "version" field in system-info propertybag, see http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.js#338

If kernel version is important, perhaps it could report "4.0 (2.6.32)"

Comment 2

[Chris Peterson [:cpeterson]]

The (verbose) kernel version is helpful because it can be used to determine when a device is running alternate OS like CyanogenMod.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Chris Peterson (:cpeterson) from comment #2)
> The (verbose) kernel version is helpful because it can be used to determine
> when a device is running alternate OS like CyanogenMod.

This is very true. I am not against stuffing the android version in the kernel version though.

See also: Bug 704718

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Are all official Android releases on the same kernel version? i.e. is ICS always 3.0.8 etc?

If not then putting the two together will make filtering very painful. I'd prefer we add the kernel version as an extra field.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

To explain more, for example my Nexus S on ICS 4.0.4 has kernel: 3.0.8-g6656123
Do other phones on 4.0.4 have the exact same, or do they have 3.0.8-foobar?

If they have -foobar, then this part would need to put cut off to get meaningful filtering. On the other hand, if they don't, then the question is:

Do unofficial builds like CyanogenMod typically have 3.0.8-xxx too, or do they tend to have 3.1.234-yyy style versions?

Comment 6

[Xavier Stevens [:xstevens]]

Just from what I've seen in the data so far. Kernel versions are not exactly tied to one particular version of android. ICS in particular seems to have several different versions of the kernel depending on the manufacturer and chipsets used (2.6.39.4, 3.0.1, 3.0.8, and lots of variants of these etc.).

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Xavier, would just adding an "sdk_version" field be enough for you, or would you prefer if sdk_version -> "version" and the kernel goes into something like "kernel_version"?

Comment 8

[Xavier Stevens [:xstevens]]

I think it would make more sense to do sdk_version -> "version" for android and then add "kernel_version" as a new field.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

This will make the relevant metadata we send for Telemetry pings look like this:

{
  "reason":"idle-daily",
  "OS":"Android",
  "appID":"{aa3c5121-dab2-40e2-81ca-7ea25febc110}",
  "appVersion":"18.0a1",
  "appName":"Fennec",
  "appBuildID":"20121010115835",
  "appUpdateChannel":"default",
  "platformBuildID":"20121010115835",
  "locale":"en-US",
  "cpucount":1,
  "memsize":335,
  "arch":"arm",
  "version":15,
  "kernel_version":"3.0.8-g6656123",
  "device":"Nexus S",
  "manufacturer":"samsung",
  "hardware":"herring",
  "hasMMX":false,
  "hasSSE":false,
  "hasSSE2":false,
  "hasSSE3":false,
  "hasSSSE3":false,
  "hasSSE4A":false,
  "hasSSE4_1":false,
  "hasSSE4_2":false,
  "hasEDSP":true,
  "hasARMv6":true,
  "hasARMv7":true,
  "hasNEON":true,
  "adapterDescription":"Imagination Technologies -- PowerVR SGX 540 -- OpenGL ES 2.0 build 1.8@785978 -- Model: Nexus S, Product: soju, Manufacturer: samsung, Hardware: herring",
  "adapterVendorID":"Imagination Technologies","adapterDeviceID":"PowerVR SGX 540","adapterDriverVersion":"OpenGL ES 2.0 build 1.8@785978"
}

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Rework version and kernel_version

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 669909 [details] [diff] [review]
Patch 1. Rework version and kernel_version

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

::: toolkit/components/telemetry/TelemetryPing.js
@@ +335,5 @@
>  
>      // sysinfo fields are not always available, get what we can.
>      let sysInfo = Cc["@mozilla.org/system-info;1"].getService(Ci.nsIPropertyBag2);
> +    let fields = ["cpucount", "memsize", "arch", "version", "kernel_version",
> +                  "device", "manufacturer", "hardware",

why not include the "shellVersion" that already exists in the ping?

::: xpcom/base/nsSystemInfo.cpp
@@ +195,5 @@
> +        rv = GetPropertyAsAString(NS_LITERAL_STRING("version"), str);
> +        if (NS_SUCCEEDED(rv)) {
> +            SetPropertyAsAString(NS_LITERAL_STRING("kernel_version"), str);
> +        }
> +        SetPropertyAsInt32(NS_LITERAL_STRING("version"), android_sdk_version);

changing the meaning of version is fairly disruptive. Telemetry isn't the only thing using this information.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

After some discussion:

1) It seems that nobody is using the meaning of sysinfo.version on Android. (Which isn't surprising, given what it contains)
2) There are a few users of shellVersion, which have to do some extra parsing because it's in an awkward format.
3) Nobody is using shellName.

Based on this, I'll redo the patch:

1) version will be the Android version. kernel_version will be the kernel version. (This part stays the same)
2) Users of shellVersion will be changed to use version.
3) Remove shellName.

Comment 13

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 2. Rework users of shellVersion & shellName

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 669927 [details] [diff] [review]
Patch 2. Rework users of shellVersion & shellName

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

::: accessible/src/jsat/Utils.jsm
@@ +54,5 @@
>  
>    get AndroidSdkVersion() {
>      if (!this._AndroidSdkVersion) {
> +      if (Services.appinfo.OS == 'Android') {
> +        this._AndroidSdkVersion = parseFloat(Services.sysinfo.get('version'));

sysInfo.getPropertyAsInt32("version");

::: mobile/xul/chrome/content/Util.js
@@ +141,5 @@
>        return this._isTablet = true;
>  
>  #ifdef ANDROID
>      let sysInfo = Cc["@mozilla.org/system-info;1"].getService(Ci.nsIPropertyBag2);
> +    let sdkVersion = parseFloat(sysInfo.get("version"));

let sdkVersion = sysInfo.getPropertyAsInt32("version");

Comment 15

[Gian-Carlo Pascutto [:gcp]]

https://tbpl.mozilla.org/?tree=Try&rev=194b45da27ea

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b849f99923d
https://hg.mozilla.org/integration/mozilla-inbound/rev/644a8fff5615

Comment 16

[Gian-Carlo Pascutto [:gcp]]

>It seems that nobody is using the meaning of sysinfo.version on Android. (Which 
>isn't surprising, given what it contains)

Okay, blocklist pings were, but it obviously didn't contain what they expected it to, see bug 704718.

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8b849f99923d
https://hg.mozilla.org/mozilla-central/rev/644a8fff5615