Last Comment Bug 706340 - add gfxinfo data to telemetry data
: add gfxinfo data to telemetry data
Status: RESOLVED FIXED
[Snappy][inbound]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: John Daggett (:jtd)
:
:
Mentors:
Depends on: 720636
Blocks: 675962
  Show dependency treegraph
 
Reported: 2011-11-29 18:27 PST by John Daggett (:jtd)
Modified: 2012-01-24 01:50 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, add gfxinfo data to telemetry metadata (1.74 KB, patch)
2011-11-30 23:44 PST, John Daggett (:jtd)
taras.mozilla: review-
jmuizelaar: feedback+
Details | Diff | Splinter Review
patch, add gfxinfo data to telemetry metadata w/ tests (2.73 KB, patch)
2012-01-19 05:11 PST, John Daggett (:jtd)
taras.mozilla: review-
Details | Diff | Splinter Review
patch, add gfxinfo data to telemetry metadata w/ tests (2.02 KB, patch)
2012-01-19 17:09 PST, John Daggett (:jtd)
no flags Details | Diff | Splinter Review
patch, add gfxinfo data to telemetry metadata w/ tests (2.20 KB, patch)
2012-01-19 19:38 PST, John Daggett (:jtd)
taras.mozilla: review+
Details | Diff | Splinter Review

Description John Daggett (:jtd) 2011-11-29 18:27:10 PST
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) 2011-11-30 23:44:16 PST
Created attachment 578187 [details] [diff] [review]
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] 2011-12-01 06:04:53 PST
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) 2011-12-11 21:42:31 PST
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] 2011-12-12 09:26:52 PST
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) 2011-12-12 12:26:18 PST
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) 2011-12-13 11:16:32 PST
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] 2012-01-05 09:45:36 PST
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) 2012-01-05 22:29:45 PST
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) 2012-01-17 11:39:34 PST
We need this to track d2d responsiveness problems(if any)
Comment 10 John Daggett (:jtd) 2012-01-19 05:11:22 PST
Created attachment 589835 [details] [diff] [review]
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) 2012-01-19 05:48:57 PST
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) 2012-01-19 10:26:45 PST
(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] 2012-01-19 13:13:47 PST
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) 2012-01-19 13:20:54 PST
(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] 2012-01-19 13:33:21 PST
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) 2012-01-19 14:28:04 PST
(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) 2012-01-19 14:28:48 PST
John, how much do cleartype values vary, can we condense those?
Comment 18 (dormant account) 2012-01-19 16:32:24 PST
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) 2012-01-19 16:44:57 PST
(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) 2012-01-19 17:09:03 PST
Created attachment 590054 [details] [diff] [review]
patch, add gfxinfo data to telemetry metadata w/ tests

Omit empty strings and cleartypeParameters.
Comment 21 John Daggett (:jtd) 2012-01-19 19:38:49 PST
Created attachment 590087 [details] [diff] [review]
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) 2012-01-20 14:46:41 PST
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) 2012-01-22 23:02:22 PST
Pushed to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/e29b78989aeb
Comment 24 Sid Stamm [:geekboy or :sstamm] 2012-01-23 09:48:59 PST
removing privacy-review-needed keyword.  Resolved in comment 16: needed for gfx regressions.
Comment 25 Ed Morley [:emorley] 2012-01-23 11:51:08 PST
https://hg.mozilla.org/mozilla-central/rev/e29b78989aeb

Note You need to log in before you can comment on or make changes to this bug.