add gfxinfo data to telemetry data

RESOLVED FIXED in mozilla12

Status

()

Core
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jtd, Assigned: jtd)

Tracking

(Blocks: 1 bug)

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy][inbound])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Updated

6 years ago
Blocks: 675962
(Assignee)

Comment 1

6 years ago
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.
Attachment #578187 - Flags: feedback?(jmuizelaar)
Comment on attachment 578187 [details] [diff] [review]
patch, add gfxinfo data to telemetry metadata

This seems pretty reasonable to me.
Attachment #578187 - Flags: feedback?(jmuizelaar) → feedback+
(Assignee)

Comment 3

6 years ago
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?
Attachment #578187 - Flags: review?(mozilla)
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.
Keywords: privacy-review-needed

Comment 5

6 years ago
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

6 years ago
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.
Attachment #578187 - Flags: review?(mozilla) → review-
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?
(Assignee)

Comment 8

6 years ago
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.

Updated

6 years ago
Whiteboard: [Snappy]

Comment 9

6 years ago
We need this to track d2d responsiveness problems(if any)
(Assignee)

Comment 10

6 years ago
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":""
Attachment #578187 - Attachment is obsolete: true
Attachment #589835 - Flags: review?(taras.mozilla)
(Assignee)

Comment 11

6 years ago
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

6 years ago
(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
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?
(Assignee)

Comment 14

6 years ago
(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?
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

6 years ago
(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

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

Comment 18

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

r- for sending empty fields.
Attachment #589835 - Flags: review?(taras.mozilla) → review-
(Assignee)

Comment 19

6 years ago
(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.
(Assignee)

Comment 20

6 years ago
Created attachment 590054 [details] [diff] [review]
patch, add gfxinfo data to telemetry metadata w/ tests

Omit empty strings and cleartypeParameters.
Attachment #590054 - Flags: review?(taras.mozilla)
(Assignee)

Comment 21

6 years ago
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.
Attachment #589835 - Attachment is obsolete: true
Attachment #590054 - Attachment is obsolete: true
Attachment #590054 - Flags: review?(taras.mozilla)
Attachment #590087 - Flags: review?(taras.mozilla)

Comment 22

6 years ago
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 }
Attachment #590087 - Flags: review?(taras.mozilla) → review+
(Assignee)

Comment 23

6 years ago
Pushed to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/e29b78989aeb
(Assignee)

Updated

6 years ago
Whiteboard: [Snappy] → [Snappy][inbound]
removing privacy-review-needed keyword.  Resolved in comment 16: needed for gfx regressions.
Keywords: privacy-review-needed
https://hg.mozilla.org/mozilla-central/rev/e29b78989aeb
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Depends on: 720636
You need to log in before you can comment on or make changes to this bug.