Include CPU information in UMA system profile.

Include CPU information in UMA system profile.

This is particularly useful to help determine which CPUs fail to tick
monotonically in HighResNow() on Windows.

BUG=254211
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212323

[James Simonsen, 2013-06-26 00:21:50]

Ilya, you're the owner for both of these directories. Please review.

Mark, you also own one of these directories and we'd discussed this a couple of
months ago when you and Tyler met with the speed team. Just want to double check
this is still okay with you.

I ran it by privacy and they're okay with it, because there's no PII. I can
forward the thread if you want.

[Mark P, 2013-06-26 00:29:09]

lgtm
This all looks reasonable to me, but please wait for Ilya's more comprehensive
review.

thanks,
mark

[Ilya Sherman, 2013-06-26 00:43:01]

This looks reasonable to me as well, but you'll need to land the update to the
server-side protocol buffer definition prior to landing this change.  If you're
not sure how to do that, please ping me off this review thread and I can point
you at the right place.

https://codereview.chromium.org/17770005/diff/1/chrome/common/metrics/proto/s...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/17770005/diff/1/chrome/common/metrics/proto/s...
chrome/common/metrics/proto/system_profile.proto:127: optional uint32
extended_family = 7;
Please document each of these fields.

[Mark P, 2013-06-26 01:33:21]

>
> This looks reasonable to me as well, but you'll need to land the update to
> the
> server-side protocol buffer definition prior to landing this change.


Why is that?

I thought unexpected fields were ignored.  (We've done things
unintentionally in the other order before.)

--mark


On Tue, Jun 25, 2013 at 5:43 PM, <isherman@chromium.org> wrote:

> This looks reasonable to me as well, but you'll need to land the update to
> the
> server-side protocol buffer definition prior to landing this change.  If
> you're
> not sure how to do that, please ping me off this review thread and I can
> point
> you at the right place.
>
>
> https://codereview.chromium.**org/17770005/diff/1/chrome/**
>
common/metrics/proto/system_**profile.proto<https://codereview.chromium.org/17770005/diff/1/chrome/common/metrics/proto/system_profile.proto>
> File chrome/common/metrics/proto/**system_profile.proto (right):
>
> https://codereview.chromium.**org/17770005/diff/1/chrome/**
>
common/metrics/proto/system_**profile.proto#newcode127<https://codereview.chromium.org/17770005/diff/1/chrome/common/metrics/proto/system_profile.proto#newcode127>
> chrome/common/metrics/proto/**system_profile.proto:127: optional uint32
> extended_family = 7;
> Please document each of these fields.
>
>
https://codereview.chromium.**org/17770005/<https://codereview.chromium.org/1...
>

[James Simonsen, 2013-06-26 02:30:38]

https://codereview.chromium.org/17770005/diff/6001/chrome/common/metrics/prot...
File chrome/common/metrics/proto/system_profile.proto (left):

https://codereview.chromium.org/17770005/diff/6001/chrome/common/metrics/prot...
chrome/common/metrics/proto/system_profile.proto:81: // Next tag for Hardware:
13
FYI, there's a discrepancy between this and the one on the server side. I went
with the larger of the two. It seems a bit scary that they're out of sync
though.

[Mark P, 2013-06-26 13:23:24]

https://codereview.chromium.org/17770005/diff/6001/chrome/common/metrics/prot...
File chrome/common/metrics/proto/system_profile.proto (left):

https://codereview.chromium.org/17770005/diff/6001/chrome/common/metrics/prot...
chrome/common/metrics/proto/system_profile.proto:81: // Next tag for Hardware:
13
On 2013/06/26 02:30:38, James Simonsen wrote:
> FYI, there's a discrepancy between this and the one on the server side. I went
> with the larger of the two. It seems a bit scary that they're out of sync
> though.

Looking at the history of the server-side file, the 15 listed there is an error.
 Please go ahead and use tag 13 (next tag 14).

(isherman@ can correct me if I mis-reading the discussions on the code review
that submitted the 15 line, which was partially to correct an earlier error and
it looks like didn't correct it entirely)

[James Simonsen, 2013-06-26 18:22:08]

On 2013/06/26 13:23:24, Mark P wrote:
> Looking at the history of the server-side file, the 15 listed there is an
error.
>  Please go ahead and use tag 13 (next tag 14).

Done. (in both reviews)

[jar (doing other things), 2013-06-26 19:29:33]

This has me a bit concerned.  I can envision Chrome running on a new processor,
that is under development... and so strings extracting info make me a bit
nervous.  It might be more reasonable to map this via a lookup table to a short
list of known enums.

https://codereview.chromium.org/17770005/diff/10001/chrome/common/metrics/pro...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/17770005/diff/10001/chrome/common/metrics/pro...
chrome/common/metrics/proto/system_profile.proto:121: // A 12 character string
naming the vendor, e.g. "GeniuneIntel".
Have you gotten this approved for upload in UMA?  We usually shy away from
uploading strings... although there are cases where we do so.

[James Simonsen, 2013-06-26 21:06:30]

On 2013/06/26 19:29:33, jar wrote:
> This has me a bit concerned.  I can envision Chrome running on a new
processor,
> that is under development... and so strings extracting info make me a bit
> nervous.  It might be more reasonable to map this via a lookup table to a
short
> list of known enums.
> 
>
https://codereview.chromium.org/17770005/diff/10001/chrome/common/metrics/pro...
> File chrome/common/metrics/proto/system_profile.proto (right):
> 
>
https://codereview.chromium.org/17770005/diff/10001/chrome/common/metrics/pro...
> chrome/common/metrics/proto/system_profile.proto:121: // A 12 character string
> naming the vendor, e.g. "GeniuneIntel".
> Have you gotten this approved for upload in UMA?  We usually shy away from
> uploading strings... although there are cases where we do so.

Yes, it's approved by privacy. It's the same as the GPU info that we upload.
That also uploads strings that identify the vendor.

[Ilya Sherman, 2013-06-26 21:57:31]

On 2013/06/26 01:33:21, Mark P wrote:
> >
> > This looks reasonable to me as well, but you'll need to land the update to
> > the
> > server-side protocol buffer definition prior to landing this change.
> 
> 
> Why is that?
> 
> I thought unexpected fields were ignored.  (We've done things
> unintentionally in the other order before.)

On the server-side, we don't strip out unknown fields; we simply save off the
uploaded bits.  This means that technically, we could access the new fields
without ever updating the server-side protocol buffer definition; which means we
could theoretically save privacy-sensitive data without the proper internal
review.  To avoid that situation, we should always make sure to get the internal
approval first.

I'd previously been more lax about this, as the internal review is usually
quick, and it usually works fine to pursue both in parallel.  However, this
actually came up as an issue for the ChromeOS profiler data, for which the
internal review got stalled for several months past when we started uploading
profiler data.  So, I'm being more careful to follow the proper sequencing now.

[James Simonsen, 2013-07-09 23:53:26]

Instead of uploading the CPUID in separate fields, I've switched this to just
upload the whole register as "signature." (The Wikipedia article uses the same
term.) That should save space and solve the problem of whether to omit fields.
PTAL.

https://codereview.chromium.org/17770005/diff/10001/chrome/common/metrics/pro...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/17770005/diff/10001/chrome/common/metrics/pro...
chrome/common/metrics/proto/system_profile.proto:121: // A 12 character string
naming the vendor, e.g. "GeniuneIntel".
On 2013/06/26 19:29:34, jar wrote:
> Have you gotten this approved for upload in UMA?  We usually shy away from
> uploading strings... although there are cases where we do so.

Yes, it's been approved by the privacy team. It's effectively the same as the
GPU info, which also uploads vendor strings.

[Ilya Sherman, 2013-07-10 00:07:40]

On 2013/07/09 23:53:26, James Simonsen wrote:
> Instead of uploading the CPUID in separate fields, I've switched this to just
> upload the whole register as "signature." (The Wikipedia article uses the same
> term.) That should save space and solve the problem of whether to omit fields.
> PTAL.
> 
>
https://codereview.chromium.org/17770005/diff/10001/chrome/common/metrics/pro...
> File chrome/common/metrics/proto/system_profile.proto (right):
> 
>
https://codereview.chromium.org/17770005/diff/10001/chrome/common/metrics/pro...
> chrome/common/metrics/proto/system_profile.proto:121: // A 12 character string
> naming the vendor, e.g. "GeniuneIntel".
> On 2013/06/26 19:29:34, jar wrote:
> > Have you gotten this approved for upload in UMA?  We usually shy away from
> > uploading strings... although there are cases where we do so.
> 
> Yes, it's been approved by the privacy team. It's effectively the same as the
> GPU info, which also uploads vendor strings.

Once again, please get the protocol buffer change approved and landed internally
first.

[James Simonsen, 2013-07-16 23:54:33]

On 2013/06/26 21:06:30, James Simonsen wrote:
> On 2013/06/26 19:29:33, jar wrote:
> > This has me a bit concerned.  I can envision Chrome running on a new
> processor,
> > that is under development... and so strings extracting info make me a bit
> > nervous.  It might be more reasonable to map this via a lookup table to a
> short
> > list of known enums.
> > 
> >
>
https://codereview.chromium.org/17770005/diff/10001/chrome/common/metrics/pro...
> > File chrome/common/metrics/proto/system_profile.proto (right):
> > 
> >
>
https://codereview.chromium.org/17770005/diff/10001/chrome/common/metrics/pro...
> > chrome/common/metrics/proto/system_profile.proto:121: // A 12 character
string
> > naming the vendor, e.g. "GeniuneIntel".
> > Have you gotten this approved for upload in UMA?  We usually shy away from
> > uploading strings... although there are cases where we do so.
> 
> Yes, it's approved by privacy. It's the same as the GPU info that we upload.
> That also uploads strings that identify the vendor.

Jim, are you okay with this? I'd like to land the server-side of this, but we
need to agree on what we're uploading.

There seems to be a lookup table on the CPUID Wikipedia article. I could
implement that as an enum. My concern would be what to do if there were an
unknown/new CPU vendor ID out there. How would we determine its string?

[jar (doing other things), 2013-07-17 00:43:50]

Base LGTM

https://codereview.chromium.org/17770005/diff/10001/chrome/common/metrics/pro...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/17770005/diff/10001/chrome/common/metrics/pro...
chrome/common/metrics/proto/system_profile.proto:121: // A 12 character string
naming the vendor, e.g. "GeniuneIntel".
On 2013/07/09 23:53:26, James Simonsen wrote:
> On 2013/06/26 19:29:34, jar wrote:
> > Have you gotten this approved for upload in UMA?  We usually shy away from
> > uploading strings... although there are cases where we do so.
> 
> Yes, it's been approved by the privacy team. It's effectively the same as the
> GPU info, which also uploads vendor strings.

OK... if you've gotten approval, SGTM ;-).

[James Simonsen, 2013-07-18 00:56:58]

Ilya, the server side has landed. Are you ok with landing this in Chrome now?

[Ilya Sherman, 2013-07-18 01:02:01]

Yep, LGTM.  Thanks!

[commit-bot: I haz the power, 2013-07-18 01:16:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/17770005/25001

[commit-bot: I haz the power, 2013-07-18 11:13:30]

Change committed as 212323