Closed Bug 799228 Opened 7 years ago Closed 7 years ago

Send Android version as OS version for Telemetry

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 19

People

(Reporter: xstevens, Assigned: gcp)

References

Details

Attachments

(2 files)

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.
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)"
The (verbose) kernel version is helpful because it can be used to determine when a device is running alternate OS like CyanogenMod.
(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
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.
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?
Assignee: nobody → gpascutto
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.).
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"?
I think it would make more sense to do sdk_version -> "version" for android and then add "kernel_version" as a new field.
Blocks: 792339
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"
}
Attachment #669909 - Flags: review?(blassey.bugs)
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.
Attachment #669909 - Flags: review?(blassey.bugs) → review-
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 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");
Attachment #669927 - Flags: review?(blassey.bugs) → review+
Attachment #669909 - Flags: review- → review+
>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.
https://hg.mozilla.org/mozilla-central/rev/8b849f99923d
https://hg.mozilla.org/mozilla-central/rev/644a8fff5615
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
You need to log in before you can comment on or make changes to this bug.