Linux: Add a UMA for the glibc version.

Linux: Add a UMA for the glibc version.

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

[jar (doing other things), 2013-04-14 16:14:15]

https://codereview.chromium.org/13901007/diff/2001/chrome/browser/metrics/chr...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/13901007/diff/2001/chrome/browser/metrics/chr...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:36:
UMA_LINUX_GLIBC_2_11 = 11,
See note below about how to avoid having to list the enumerated constants values
(i.e., "= 11" etc.) here.

https://codereview.chromium.org/13901007/diff/2001/chrome/browser/metrics/chr...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:93:
static_cast<UMALinuxGlibcVersion>(glibc_minor_version);
I think this might read better if you have the adjustment factor:

- 11 + UMA_LINUX_GLIBC_2_11;

Then you don't need mess around and specially define UMA_LINUX_GLIBC_2_11 to
match.... and you'll be set up to deal with another other ranges.  It is
generally nicer not to leave unused spots in the enumeration when going into an
enumerated histogram.

Following in this pattern.... after you verify that you have a major version of
2, you could go ahead and translate it, and then check to see that the
translation is between 
UMA_LINUX_GLIBC_2_11
UMA_LINUX_GLIBC_2_19

I think that will also make the connection with the enums a bit clearer (and
avoid having the less obvious magic constants 11 and 19 show up in code.

WDYT?

[Lei Zhang, 2013-04-16 00:54:55]

On 2013/04/14 16:14:15, jar wrote:
>
https://codereview.chromium.org/13901007/diff/2001/chrome/browser/metrics/chr...
> File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
(right):
> 
>
https://codereview.chromium.org/13901007/diff/2001/chrome/browser/metrics/chr...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:36:
> UMA_LINUX_GLIBC_2_11 = 11,
> See note below about how to avoid having to list the enumerated constants
values
> (i.e., "= 11" etc.) here.
> 
>
https://codereview.chromium.org/13901007/diff/2001/chrome/browser/metrics/chr...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:93:
> static_cast<UMALinuxGlibcVersion>(glibc_minor_version);
> I think this might read better if you have the adjustment factor:
> 
> - 11 + UMA_LINUX_GLIBC_2_11;
> 
> Then you don't need mess around and specially define UMA_LINUX_GLIBC_2_11 to
> match.... and you'll be set up to deal with another other ranges.  It is
> generally nicer not to leave unused spots in the enumeration when going into
an
> enumerated histogram.
> 
> Following in this pattern.... after you verify that you have a major version
of
> 2, you could go ahead and translate it, and then check to see that the
> translation is between 
> UMA_LINUX_GLIBC_2_11
> UMA_LINUX_GLIBC_2_19
> 
> I think that will also make the connection with the enums a bit clearer (and
> avoid having the less obvious magic constants 11 and 19 show up in code.
> 
> WDYT?

Sure, how does patch set 2 look?

[jar (doing other things), 2013-04-16 03:31:14]

thanks... I think it is better... two more nits to try to make it easier for
folks to read it.

https://codereview.chromium.org/13901007/diff/8002/chrome/browser/metrics/chr...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/13901007/diff/8002/chrome/browser/metrics/chr...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:88: if
(glibc_major_version == 2 &&
nit: Don't do the translation in lines 86-87 until you verify that you have
version 2.

...and then, since that constant is only used (or applicable) in version 2, just
provide the translation here.  If you want to put it is a translation constant,
move the text down from line 43-45 down here (inside the "if" statement that
verified that we have a major version of 2.

https://codereview.chromium.org/13901007/diff/8002/chrome/browser/metrics/chr...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:94:
glibc_version_result = UMA_LINUX_GLIBC_UNKNOWN;
nit: I'm betting you made the if statement more complex to get this else clause
in one place... but if you just set it before the if statement (about major
version 2) you'd have it as a fallback.

[Lei Zhang, 2013-04-16 03:40:00]

Ok, onwards to patch set 3.

[jar (doing other things), 2013-04-16 18:49:11]

lgtm

[Lei Zhang, 2013-04-16 19:51:53]

Committed patchset #3 manually as r194423 (presubmit successful).

[xhwang, 2013-04-16 20:44:05]

https://codereview.chromium.org/13901007/diff/13001/chrome/browser/metrics/ch...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/13901007/diff/13001/chrome/browser/metrics/ch...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:74:
base::SplitString(glibc_version_string, '.', &split_glibc_version);
FYI: You can use base::Version to get the components.

[Lei Zhang, 2013-04-16 22:36:24]

On 2013/04/16 20:44:05, xhwang wrote:
>
https://codereview.chromium.org/13901007/diff/13001/chrome/browser/metrics/ch...
> File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc
(right):
> 
>
https://codereview.chromium.org/13901007/diff/13001/chrome/browser/metrics/ch...
> chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:74:
> base::SplitString(glibc_version_string, '.', &split_glibc_version);
> FYI: You can use base::Version to get the components.

Thanks. I have the next CL here: https://codereview.chromium.org/13888010