706340 - add gfxinfo data to telemetry data

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[John Daggett (:jtd)]

In addition to system data (num cpu's, memory, arch) it would be very useful to have basic gfxinfo included with telemetry submissions.  At least the basic D2D/DirectWrite enabled/disabled along with details of the graphics driver.

Proposal: add this to the getMetadata method within TelemetryPing
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.js#203

Comment 1

[John Daggett (:jtd)]

Attachment: patch, add gfxinfo data to telemetry metadata

Does this look like what we want?  I'm mimicing the gfxinfo data in about:support, we may need more/less than this.  Not quite sure how to test this but there appear to be unit tests for TelemetryPing.js.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 578187 [details] [diff] [review]
patch, add gfxinfo data to telemetry metadata

This seems pretty reasonable to me.

Comment 3

[John Daggett (:jtd)]

Comment on attachment 578187 [details] [diff] [review]
patch, add gfxinfo data to telemetry metadata

Taras, any tips on how to put in a test for this code?

Comment 4

[Nathan Froyd [:froydnj]]

Is there a reason for the #ifdef XP_WIN fields?  All the Windows-specific fields should trigger the catch handler below and be ignored, right?

Also, this is going to need some privacy review love.

Comment 5

[(dormant account)]

Comment on attachment 578187 [details] [diff] [review]
patch, add gfxinfo data to telemetry metadata

Are these fields available in xpcshell? If so we can modify test_TelemetryPing.js to check for these values

Comment 6

[(dormant account)]

Comment on attachment 578187 [details] [diff] [review]
patch, add gfxinfo data to telemetry metadata

Do not need ifdef here. Still waiting on response on whether this is reachable in xpcshell.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

To document what we're collecting in the interest of full transparency, could someone provide a sample of the data collected (I see the fields in the patch, but would like to see an example submission) and what we hope to learn from collecting this data?

Comment 8

[John Daggett (:jtd)]

The fields are those that are listed under "Graphics" on the about:support page.  Including this info allows use to correlate better perf problems related to specific graphics cards, drivers or support library versions (e.g. DirectWrite version).

I'll include a sample when I update the patch.

Comment 9

[(dormant account)]

We need this to track d2d responsiveness problems(if any)

Comment 10

[John Daggett (:jtd)]

Attachment: patch, add gfxinfo data to telemetry metadata w/ tests

Adds gfxinfo metadata to telemetry metadata.

On OSX this looks like:

"adapterDescription":"","adapterVendorID":"0x1002","adapterDeviceID":"0x944a","adapterRAM":"","adapterDriver":"","adapterDriverVersion":"","adapterDriverDate":""

Comment 11

[John Daggett (:jtd)]

Additional metadata on Windows 7:

"adapterDescription":"Intel(R) HD Graphics","adapterVendorID":"0x8086","adapterDeviceID":"0x0046","adapterRAM":"Unknown",
"adapterDriver":"igdumd64 igd10umd64 igdumdx32 igd10umd32","adapterDriverVersion":"8.15.10.2202","adapterDriverDate":"8-25-2010",
"adapterDescription2":"","adapterVendorID2":"","adapterDeviceID2":"","adapterRAM2":"","adapterDriver2":"","adapterDriverVersion2":"","adapterDriverDate2":"",
"isGPU2Active":false,"DWriteEnabled":true,"DWriteVersion":"6.1.7601.17563",
"cleartypeParameters":"DISPLAY1 [ Gamma: 2200 Pixel Structure: RGB ClearType Level: 100 Enhanced Contrast: 50 ] DISPLAY2 [ Gamma: 2200 Pixel Structure: RGB ClearType Level: 100 Enhanced Contrast: 50 ] "

Comment 12

[(dormant account)]

(In reply to John Daggett (:jtd) from comment #11)
> Additional metadata on Windows 7:
> 
> "adapterDescription":"Intel(R) HD
> Graphics","adapterVendorID":"0x8086","adapterDeviceID":"0x0046","adapterRAM":
> "Unknown",
> "adapterDriver":"igdumd64 igd10umd64 igdumdx32
> igd10umd32","adapterDriverVersion":"8.15.10.2202","adapterDriverDate":"8-25-
> 2010",
> "adapterDescription2":"","adapterVendorID2":"","adapterDeviceID2":"",
> "adapterRAM2":"","adapterDriver2":"","adapterDriverVersion2":"",
> "adapterDriverDate2":"",
> "isGPU2Active":false,"DWriteEnabled":true,"DWriteVersion":"6.1.7601.17563",
> "cleartypeParameters":"DISPLAY1 [ Gamma: 2200 Pixel Structure: RGB ClearType
> Level: 100 Enhanced Contrast: 50 ] DISPLAY2 [ Gamma: 2200 Pixel Structure:
> RGB ClearType Level: 100 Enhanced Contrast: 50 ] "

I think we should omit empty fields

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Can we prune the list of items collected to a subset of these (on top of omitting only empty fields), or are all the data points necessary?

Comment 14

[John Daggett (:jtd)]

(In reply to Sid Stamm [:geekboy] from comment #13)
> Can we prune the list of items collected to a subset of these (on top of
> omitting only empty fields), or are all the data points necessary?

This is graphics card info so I think whether something is needed or not would depend on the nature of a specific problem.  Your concern is that a combination of info items would id individual users?

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

I don't have a direct concern, but am driving at minimizing what we collect.  If we need it all, that's fine, but I'm just asking: can we get by without some of this or do we really need it all?  

See principle 4 in this blog post:
http://blog.mozilla.com/privacy/2011/01/12/mozillas-privacy-data-operating-principles/

Comment 16

[(dormant account)]

(In reply to Sid Stamm [:geekboy] from comment #15)
> I don't have a direct concern, but am driving at minimizing what we collect.
> If we need it all, that's fine, but I'm just asking: can we get by without
> some of this or do we really need it all?  

I'm not familiar with cleartype, but everything else is required to do gfx regressions.

Comment 17

[(dormant account)]

John, how much do cleartype values vary, can we condense those?

Comment 18

[(dormant account)]

Comment on attachment 589835 [details] [diff] [review]
patch, add gfxinfo data to telemetry metadata w/ tests

r- for sending empty fields.

Comment 19

[John Daggett (:jtd)]

(In reply to Taras Glek (:taras) from comment #17)
> John, how much do cleartype values vary, can we condense those?

It might be useful to correlate with specific perf problems but it's the least useful of those fields.  I'll trim it.

Comment 20

[John Daggett (:jtd)]

Attachment: patch, add gfxinfo data to telemetry metadata w/ tests

Omit empty strings and cleartypeParameters.

Comment 21

[John Daggett (:jtd)]

Attachment: patch, add gfxinfo data to telemetry metadata w/ tests

Revised to only test on Win/OSX since some Linux platforms don't report adapter info.

Comment 22

[(dormant account)]

Comment on attachment 590087 [details] [diff] [review]
patch, add gfxinfo data to telemetry metadata w/ tests

+    if (gfxInfo) {
+      for each (let field in gfxfields) {
+        let value = "";
+        try {
+          value = gfxInfo[field];
+        } catch (e) {
+          continue
+        }
+        if (value != "")
+          ret[field] = value;
+      }
+    }


value should be defined within try
try { let value = ...; if (value != "") ret[field]=value }

Comment 23

[John Daggett (:jtd)]

Pushed to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/e29b78989aeb

Comment 24

[Sid Stamm [:geekboy or :sstamm]]

removing privacy-review-needed keyword.  Resolved in comment 16: needed for gfx regressions.

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e29b78989aeb