Increased upper limit on renderer event latency UMA histogram.

Increased upper limit on renderer event latency UMA histogram.

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

[bokan, 2014-01-14 19:04:51]

I upped the limit on the event latency UMA to 100s and added 20 buckets. That
should give us a pretty good indication of how that >1s bucket is distributed.

Is there anyway to test histograms without committing? about:stats doesn't show
anything (I assume it doesn't work with histograms)

[jar (doing other things), 2014-01-14 19:09:43]

To test your histogram:

Build chrome
visit about:histograms

IMO, 100 buckets is a lot.  You probably don't need 120.  It is good that you
renamed the histogram when you changed it limits (even when only changing the
max value).

[bokan, 2014-01-14 19:34:38]

On 2014/01/14 19:09:43, jar wrote:
> To test your histogram:
> 
> Build chrome
> visit about:histograms
> 
> IMO, 100 buckets is a lot.  You probably don't need 120.  It is good that you
> renamed the histogram when you changed it limits (even when only changing the
> max value).

Thanks jar@. I've changed back to 100 buckets.

[Rick Byers, 2014-01-14 21:09:09]

lgtm

In particular, use chrome://histograms/Event.Latency.Renderer to see just the
histograms you're changing.

When we rename a histogram like this, is there a process for cleaning up the old
entries in histograms.xml?  Is it OK to just leave them, or do we need to make
sure we come back in a couple releases and remove the old ones (if the latter
then we should probably add a comment to them saying they're obsolete as of
M34).

[jar (doing other things), 2014-01-15 01:01:03]

On 2014/01/14 21:09:09, Rick Byers wrote:
> lgtm
> 
> In particular, use chrome://histograms/Event.Latency.Renderer to see just the
> histograms you're changing.
> 
> When we rename a histogram like this, is there a process for cleaning up the
old
> entries in histograms.xml?  Is it OK to just leave them, or do we need to make
> sure we come back in a couple releases and remove the old ones (if the latter
> then we should probably add a comment to them saying they're obsolete as of
> M34).

re: old name.

Best practice is to add the <obsolete> tag to the old name, with a reference to
what is replacing it.  You can still see histograms that are obsolete... but you
need to click a check-box on the histogram page.

If you think you'll be looking at the old name for a while (in stable builds??),
you could hold-off on OBSOLETEing the old name.  YMMV.

[bokan, 2014-01-17 18:45:38]

+avi@ for content/ OWNER

I think we'll hold off on OBSOLETEing the old histogram until the new one is in
stable. jar@, are you ok with this CL?

[jar (doing other things), 2014-01-18 02:44:47]

https://codereview.chromium.org/138593003/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/138593003/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:4109: +    supersedes the earlier
Event.Latency.Renderer metric.
Rather than putting "supersedes" in here, and referencing outdated histograms,
it is probably better to put the relationship in the old histogram, encouraging
folks to look to the new one.

As you noted, you didn't (yet) want to use the obsolete tag in the old... but
you can at least add a reference.

Removing these repetitious prose-lines will also make this a much more compact
change (of new stuff).

It is also tempting to pull out the common prose, and a common prefix, and use a
<field_trial> tag to list the details.

As you have written this CL, you used the same description in almost all your
histograms, which makes the prose rather useless.  For example, I have *no* idea
what your Reneder2.Char is talking about, as I don't know what a "Char" event
is.  The goal is to make the prose be more self explanatory, and hence generally
specific.

IMO, it would be better to have this (overall) histogram named differently, or
have a different postfix, like:
Event.Latency.Renderer2.AllInputEventsCombined
or possible better (so you can use the field_trial tricks):
Event.LatencyCombined.Renderer2

You can then have just the prefix histogram defined:
Event.Latency.Render2
with *clearer* common prose, and then use field trial to define the extensions:
Char
ContextMenue
GestureDoubleTap
GestureFlingCancel
...

along with clear and distinctive prose elaborating the meaning in each case.

[Avi (use Gerrit), 2014-01-20 22:48:35]

lgtm

content stamp

[bokan, 2014-01-20 22:52:03]

On 2014/01/18 02:44:47, jar wrote:
>
https://codereview.chromium.org/138593003/diff/40001/tools/metrics/histograms...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/138593003/diff/40001/tools/metrics/histograms...
> tools/metrics/histograms/histograms.xml:4109: +    supersedes the earlier
> Event.Latency.Renderer metric.
> Rather than putting "supersedes" in here, and referencing outdated histograms,
> it is probably better to put the relationship in the old histogram,
encouraging
> folks to look to the new one.
> 
> As you noted, you didn't (yet) want to use the obsolete tag in the old... but
> you can at least add a reference.
> 
> Removing these repetitious prose-lines will also make this a much more compact
> change (of new stuff).
> 
> It is also tempting to pull out the common prose, and a common prefix, and use
a
> <field_trial> tag to list the details.
> 
> As you have written this CL, you used the same description in almost all your
> histograms, which makes the prose rather useless.  For example, I have *no*
idea
> what your Reneder2.Char is talking about, as I don't know what a "Char" event
> is.  The goal is to make the prose be more self explanatory, and hence
generally
> specific.
> 
> IMO, it would be better to have this (overall) histogram named differently, or
> have a different postfix, like:
> Event.Latency.Renderer2.AllInputEventsCombined
> or possible better (so you can use the field_trial tricks):
> Event.LatencyCombined.Renderer2
> 
> You can then have just the prefix histogram defined:
> Event.Latency.Render2
> with *clearer* common prose, and then use field trial to define the
extensions:
> Char
> ContextMenue
> GestureDoubleTap
> GestureFlingCancel
> ...
> 
> along with clear and distinctive prose elaborating the meaning in each case.

Thanks for the feedback, I've put the individual events in a fieldtrial and
replaced the descriptions with something more useful. Hopefully I did it
correctly, PTAL. Does the UMA call in C++ stay the same if the histogram is
actually a field trial group?

[jar (doing other things), 2014-01-22 02:37:28]

histograms.xml change LGTM modulo one nit.

Nit: It would have been nicer if someone had given the feedback about using a
field trial for the old histograms. Note that the soon-to-be-obsolete set can
use the same field trial that you provided.  Please factor that into the old
histogram, compacting it in the same way as you did this new histogram.

These files always get larger... and larger... and it is really helpful to do
these re-factors, especially when you get to reuse more compact data.

Note that the comment you added to each old histogram can probably be summarized
in the factored-out-prefix hitogram as:
"This is soon to be replaced by Event.Latency.Renderer2.*"

[jar (doing other things), 2014-01-22 02:39:41]

On 2014/01/20 22:52:03, bokan wrote:
> On 2014/01/18 02:44:47, jar wrote:
> >
>
https://codereview.chromium.org/138593003/diff/40001/tools/metrics/histograms...
> > File tools/metrics/histograms/histograms.xml (right):
> > 
> >
>
https://codereview.chromium.org/138593003/diff/40001/tools/metrics/histograms...
> > tools/metrics/histograms/histograms.xml:4109: +    supersedes the earlier
> > Event.Latency.Renderer metric.
> > Rather than putting "supersedes" in here, and referencing outdated
histograms,
> > it is probably better to put the relationship in the old histogram,
> encouraging
> > folks to look to the new one.
> > 
> > As you noted, you didn't (yet) want to use the obsolete tag in the old...
but
> > you can at least add a reference.
> > 
> > Removing these repetitious prose-lines will also make this a much more
compact
> > change (of new stuff).
> > 
> > It is also tempting to pull out the common prose, and a common prefix, and
use
> a
> > <field_trial> tag to list the details.
> > 
> > As you have written this CL, you used the same description in almost all
your
> > histograms, which makes the prose rather useless.  For example, I have *no*
> idea
> > what your Reneder2.Char is talking about, as I don't know what a "Char"
event
> > is.  The goal is to make the prose be more self explanatory, and hence
> generally
> > specific.
> > 
> > IMO, it would be better to have this (overall) histogram named differently,
or
> > have a different postfix, like:
> > Event.Latency.Renderer2.AllInputEventsCombined
> > or possible better (so you can use the field_trial tricks):
> > Event.LatencyCombined.Renderer2
> > 
> > You can then have just the prefix histogram defined:
> > Event.Latency.Render2
> > with *clearer* common prose, and then use field trial to define the
> extensions:
> > Char
> > ContextMenue
> > GestureDoubleTap
> > GestureFlingCancel
> > ...
> > 
> > along with clear and distinctive prose elaborating the meaning in each case.
> 
> Thanks for the feedback, I've put the individual events in a fieldtrial and
> replaced the descriptions with something more useful. Hopefully I did it
> correctly, PTAL. Does the UMA call in C++ stay the same if the histogram is
> actually a field trial group?

re: does C++ code need to change?

No C/C++ changes are needed.  The <fieldtrial> tag was most useful for
FieldTrials, but it doesn't require a "real" FieldTrial.  It is just nice
sytactic sugar for building up complex names and prose, without being so
repetitive.

[commit-bot: I haz the power, 2014-01-22 03:10:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/138593003/260001

[commit-bot: I haz the power, 2014-01-22 10:22:37]

Change committed as 246269