Add histogram to track number of .nexe launches, normalized against opening n...

Add histogram to track number of .nexe launches, normalized against opening new tabs.
BUG=914
(http://code.google.com/p/nativeclient/issues/detail?id=914)
TEST=none
  Relied on trybots and manually checking "about:histograms"


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=64261

[mmortensen, 2010-10-25 16:38:42]

I'm relatively new to NativeClient and haven't modified the chrome source
before...so let me know if there is a better way to accomplish this. 

This is for this issue:
  http://code.google.com/p/nativeclient/issues/detail?id=914

I tried uploading a change to tools\histograms\histograms.xml but the gclient
upload failed and said it was sending a backtrace to the developers.

Here's what that change looks like -- I added it after
MPArch.RWHH_WhiteoutDuration at line 2569.

<histogram name="NaCl.Startups">
  <summary>The number of times that Native Client has been started by loading 
           a .nexe compared to the number of times that a tab has been opened.
  </summary>
</histogram>

I tested this by launching chrome, starting a new tab, and then verifying that
the '0' and '1' entries in "about:histograms" were correct:

Histogram: NaCl.Startups recorded 3 samples, average = 0.3, standard deviation =
0.5 (flags = 0x1)
0  ------------------------------------------------------------------------O (2
= 66.7%)
1  ------------------------------------O                                     (1
= 33.3%) {66.7%}
2  O                                                                         (0
= 0.0%) {100.0%}

[jar (doing other things), 2010-10-25 22:03:14]

http://codereview.chromium.org/4065005/diff/7001/8001
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/4065005/diff/7001/8001#newcode15
chrome/browser/nacl_host/nacl_process_host.cc:15: #include
"base/metrics/histogram.h"
nit: Please alphabetize names.

http://codereview.chromium.org/4065005/diff/7001/8001#newcode51
chrome/browser/nacl_host/nacl_process_host.cc:51: running_on_wow64_(false) { 
set_name(url);
style nit: Code never goes on the same line following a curly.

http://codereview.chromium.org/4065005/diff/7001/8001#newcode117
chrome/browser/nacl_host/nacl_process_host.cc:117:
UMA_HISTOGRAM_ENUMERATION("NaCl.Startups",
This will work.... but all you get is a counter, and I suspect you need a second
counter to normalize this relative to.  I'd suggest using two (or more) buckets
in this enumeration to count both this event (in one bucket), as well as other
event(s) in the other bucket(s).  For example, perchance you want to count the
number of tab startup?

http://codereview.chromium.org/4065005/diff/7001/8002
File chrome/browser/tabs/default_tab_handler.cc (right):

http://codereview.chromium.org/4065005/diff/7001/8002#newcode6
chrome/browser/tabs/default_tab_handler.cc:6: #include
"base/metrics/histogram.h"
nit: alphabetize.

http://codereview.chromium.org/4065005/diff/7001/8002#newcode17
chrome/browser/tabs/default_tab_handler.cc:17:
UMA_HISTOGRAM_ENUMERATION("NaCl.Startups",
This will work (having the same name is several places), but you have to be VERY
careful to use identical size parameters (in this case, the "2".).  I'd suggest
factoring this out, and calling the function (or method) with an enum to
indicate which startup (Nacl, vs tab, vs etc.)

http://codereview.chromium.org/4065005/diff/7001/8002#newcode18
chrome/browser/tabs/default_tab_handler.cc:18: 0,   //  Puts sample into no(0)
bucket when Chrome first starts;
nit: indent 4 characters when doing a line continuation.

http://codereview.chromium.org/4065005/diff/7001/8002#newcode41
chrome/browser/tabs/default_tab_handler.cc:41:
UMA_HISTOGRAM_ENUMERATION("NaCl.Startups",
I'd suggest using a 3rd bucket, rather than re-using "0" as the sample.  This
way you'll have:
0- Nacl true startup
1- Chrome startup
2- Tab startup.

...and of course, you should then make the bounding argument 3, as no samples
will be 3 or higher.

http://codereview.chromium.org/4065005/diff/7001/8002#newcode42
chrome/browser/tabs/default_tab_handler.cc:42: 0    // Puts sample into no(0)
bucket when additional tabs are added;
nit: indent 4 characters on line continuation.

[mmortensen, 2010-10-26 15:11:03]

Made recommeded changes.  Will re-upload for review.

http://codereview.chromium.org/4065005/diff/7001/8001
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/4065005/diff/7001/8001#newcode15
chrome/browser/nacl_host/nacl_process_host.cc:15: #include
"base/metrics/histogram.h"
On 2010/10/25 22:03:14, jar wrote:
> nit: Please alphabetize names.

Done.

http://codereview.chromium.org/4065005/diff/7001/8001#newcode51
chrome/browser/nacl_host/nacl_process_host.cc:51: running_on_wow64_(false) { 
set_name(url);
On 2010/10/25 22:03:14, jar wrote:
> style nit: Code never goes on the same line following a curly.

Done.  Actually, this change was accidental - thanks for catching!

http://codereview.chromium.org/4065005/diff/7001/8001#newcode117
chrome/browser/nacl_host/nacl_process_host.cc:117:
UMA_HISTOGRAM_ENUMERATION("NaCl.Startups",
On 2010/10/25 22:03:14, jar wrote:
> This will work.... but all you get is a counter, and I suspect you need a
second
> counter to normalize this relative to.  I'd suggest using two (or more)
buckets
> in this enumeration to count both this event (in one bucket), as well as other
> event(s) in the other bucket(s).  For example, perchance you want to count the
> number of tab startup?

Done.

http://codereview.chromium.org/4065005/diff/7001/8002
File chrome/browser/tabs/default_tab_handler.cc (right):

http://codereview.chromium.org/4065005/diff/7001/8002#newcode6
chrome/browser/tabs/default_tab_handler.cc:6: #include
"base/metrics/histogram.h"
On 2010/10/25 22:03:14, jar wrote:
> nit: alphabetize.

Done.

http://codereview.chromium.org/4065005/diff/7001/8002#newcode17
chrome/browser/tabs/default_tab_handler.cc:17:
UMA_HISTOGRAM_ENUMERATION("NaCl.Startups",
On 2010/10/25 22:03:14, jar wrote:
> This will work (having the same name is several places), but you have to be
VERY
> careful to use identical size parameters (in this case, the "2".).  I'd
suggest
> factoring this out, and calling the function (or method) with an enum to
> indicate which startup (Nacl, vs tab, vs etc.)

I added a new .h file with a NaCl specific macro.  Of course, I could have
modified an existing file, such as histogram.h or metrics_service.h, but this
seems like the smallest and least obtrusive to the existing code ... esp. since
the NACL histogram only is called in 2 places and not really a fundamental
change to histogram.h or metrics_serice.h that others should be aware of.

http://codereview.chromium.org/4065005/diff/7001/8002#newcode18
chrome/browser/tabs/default_tab_handler.cc:18: 0,   //  Puts sample into no(0)
bucket when Chrome first starts;
On 2010/10/25 22:03:14, jar wrote:
> nit: indent 4 characters when doing a line continuation.
> 
> 

Done.

http://codereview.chromium.org/4065005/diff/7001/8002#newcode41
chrome/browser/tabs/default_tab_handler.cc:41:
UMA_HISTOGRAM_ENUMERATION("NaCl.Startups",
On 2010/10/25 22:03:14, jar wrote:
> I'd suggest using a 3rd bucket, rather than re-using "0" as the sample.  This
> way you'll have:
> 0- Nacl true startup
> 1- Chrome startup
> 2- Tab startup.
> 
> ...and of course, you should then make the bounding argument 3, as no samples
> will be 3 or higher.

Done.

[mmortensen, 2010-10-26 16:22:55]

Added a macro so that we don't have to worry about name-matching in the 3 places
this is called, or on setting the 'max' number of buckets.

[jar (doing other things), 2010-10-26 16:29:33]

http://codereview.chromium.org/4065005/diff/16001/17001
File base/metrics/nacl_histogram.h (right):

http://codereview.chromium.org/4065005/diff/16001/17001#newcode28
base/metrics/nacl_histogram.h:28: #define UMA_NACL_HISTOGRAM_ENUMERATION(hvalue)
UMA_HISTOGRAM_ENUMERATION( \
This would work... but... macros are generally discouraged, and only used by
histogram.h to dramatically change the performance characteristics.

In this case, you may as well have a nacl_histogram.cc which contains a single
central function.  There really is no benefit from using a macro (and the style
guide will be happier with us ;-) ).

As a side benefit, this header won't need to include metrics/histogram.h, and it
will only need to be pulled in by the cc file.

[mmortensen, 2010-10-26 17:19:49]

Here's a function version instead of macro-based.

By the way, I tried to upload my changes to histograms.xml
from the src\tools\histograms directory, and it won't upload.

I noticed though that the code review server is different:
  https://chromereview.prom.corp.google.com

Perhaps I need to get added to that review/repository...

[mmortensen, 2010-10-26 19:03:29]

Fixed .gypi file and trybots are now happy

[jar (doing other things), 2010-10-26 19:15:48]

With one style nit below, LGTM.

It is plausible that the function could actually sit in the nacl implementation
file, rather than its own source file, since it is nacl specific (or at least is
named thus).  I'll live it to your call.  If you are pursuaded to change it (and
avoid adding the new cc file etc., please send it for a review before landing.

http://codereview.chromium.org/4065005/diff/33001/34005
File chrome/browser/tabs/default_tab_handler.cc (right):

http://codereview.chromium.org/4065005/diff/33001/34005#newcode16
chrome/browser/tabs/default_tab_handler.cc:16: model_(new TabStripModel(this,
delegate->GetProfile()))) {
nit: add two more characters of indentation on a "line continuation".

[mmortensen, 2010-10-26 21:31:19]

http://codereview.chromium.org/4065005/diff/33001/34005
File chrome/browser/tabs/default_tab_handler.cc (right):

http://codereview.chromium.org/4065005/diff/33001/34005#newcode16
chrome/browser/tabs/default_tab_handler.cc:16: model_(new TabStripModel(this,
delegate->GetProfile()))) {
On 2010/10/26 19:15:51, jar wrote:
> nit: add two more characters of indentation on a "line continuation".

Sorry....one more question.   The line I added (line 17) is lined up with the
line below it 
    model_->AddObserver(this);
Are you suggested the call to
    UmaNaclHistogramEnumeration
be indented more, or that the line above what I changed (line 16) should have
been indented more?

[mmortensen, 2010-10-26 21:33:03]

Thanks for all your suggestions.  I'm planning on fixing the one nit  and
landing as is.

Do you have any idea who I'd contact about uploading my change to
     src\tools\histograms\histograms.xml
for review?

Thanks,
-Mike

On Tue, Oct 26, 2010 at 1:15 PM, <jar@chromium.org> wrote:

> With one style nit below, LGTM.
>
> It is plausible that the function could actually sit in the nacl
> implementation
> file, rather than its own source file, since it is nacl specific (or at
> least is
> named thus).  I'll live it to your call.  If you are pursuaded to change it
> (and
> avoid adding the new cc file etc., please send it for a review before
> landing.
>
>
> http://codereview.chromium.org/4065005/diff/33001/34005
>
> File chrome/browser/tabs/default_tab_handler.cc (right):
>
> http://codereview.chromium.org/4065005/diff/33001/34005#newcode16
> chrome/browser/tabs/default_tab_handler.cc:16: model_(new
> TabStripModel(this, delegate->GetProfile()))) {
> nit: add two more characters of indentation on a "line continuation".
>
>
> http://codereview.chromium.org/4065005/show
>

[mmortensen, 2010-10-28 16:59:51]

Committed as revision 64261

[jar (doing other things), 2010-10-28 19:41:41]

You should be able to use gcl upload for the xml file.  Note that it
actually uses a different server... so you'll be asked to login again
etc.... but it should all just work (like any other CL).

Jim

On Tue, Oct 26, 2010 at 2:26 PM, Michael Mortensen <mmortensen@google.com>wrote:

> Thanks for all your suggestions.  I'm planning on fixing the one nit  and
> landing as is.
>
> Do you have any idea who I'd contact about uploading my change to
>      src\tools\histograms\histograms.xml
> for review?
>
> Thanks,
> -Mike
>
>
> On Tue, Oct 26, 2010 at 1:15 PM, <jar@chromium.org> wrote:
>
>> With one style nit below, LGTM.
>>
>> It is plausible that the function could actually sit in the nacl
>> implementation
>> file, rather than its own source file, since it is nacl specific (or at
>> least is
>> named thus).  I'll live it to your call.  If you are pursuaded to change
>> it (and
>> avoid adding the new cc file etc., please send it for a review before
>> landing.
>>
>>
>> http://codereview.chromium.org/4065005/diff/33001/34005
>>
>> File chrome/browser/tabs/default_tab_handler.cc (right):
>>
>> http://codereview.chromium.org/4065005/diff/33001/34005#newcode16
>> chrome/browser/tabs/default_tab_handler.cc:16: model_(new
>> TabStripModel(this, delegate->GetProfile()))) {
>> nit: add two more characters of indentation on a "line continuation".
>>
>>
>> http://codereview.chromium.org/4065005/show
>>
>
>

[jar (doing other things), 2010-10-28 19:45:33]

http://codereview.chromium.org/4065005/diff/33001/34005
File chrome/browser/tabs/default_tab_handler.cc (right):

http://codereview.chromium.org/4065005/diff/33001/34005#newcode16
chrome/browser/tabs/default_tab_handler.cc:16: model_(new TabStripModel(this,
delegate->GetProfile()))) {
The word "model_" needs an addiitonal two characters of indentation.  It is a
continuation line, and not aligned wiht a paren.  As a result, it needs a total
of 4 "additional" spaces of indent, and it only has two.

I was surprised you landed without fixing this, per my comment.  Please correct
it. thx.

On 2010/10/26 21:31:19, mmortensen wrote:
> On 2010/10/26 19:15:51, jar wrote:
> > nit: add two more characters of indentation on a "line continuation".
> 
> Sorry....one more question.   The line I added (line 17) is lined up with the
> line below it 
>     model_->AddObserver(this);
> Are you suggested the call to
>     UmaNaclHistogramEnumeration
> be indented more, or that the line above what I changed (line 16) should have
> been indented more?
> 
>

[mmortensen, 2010-10-28 21:37:32]

Sorry about that -- I meant to fix it and then got sidetracked and
overlooked it.
I've sent a new CL with just that.
Thank you!
-Mike

On Thu, Oct 28, 2010 at 1:45 PM, <jar@chromium.org> wrote:

>
> http://codereview.chromium.org/4065005/diff/33001/34005
> File chrome/browser/tabs/default_tab_handler.cc (right):
>
> http://codereview.chromium.org/4065005/diff/33001/34005#newcode16
> chrome/browser/tabs/default_tab_handler.cc:16: model_(new
> TabStripModel(this, delegate->GetProfile()))) {
> The word "model_" needs an addiitonal two characters of indentation.  It
> is a
> continuation line, and not aligned wiht a paren.  As a result, it needs
> a total
> of 4 "additional" spaces of indent, and it only has two.
>
> I was surprised you landed without fixing this, per my comment.  Please
> correct it. thx.
>
>
> On 2010/10/26 21:31:19, mmortensen wrote:
>
>> On 2010/10/26 19:15:51, jar wrote:
>> > nit: add two more characters of indentation on a "line
>>
> continuation".
>
>  Sorry....one more question.   The line I added (line 17) is lined up
>>
> with the
>
>> line below it
>>     model_->AddObserver(this);
>> Are you suggested the call to
>>     UmaNaclHistogramEnumeration
>> be indented more, or that the line above what I changed (line 16)
>>
> should have
>
>> been indented more?
>>
>
>
>
> http://codereview.chromium.org/4065005/show
>