Record histograms for timing information in DomDistiller.

Record histograms for timing information in DomDistiller.

https://codereview.chromium.org/499623002/ adds timing diagnostics to
DomDistiller. Report histograms for these to help measure real-world
perf.

Also includes a boolean histogram for distillability based on code from internal repo.

Committed: https://crrev.com/49e25f54b69dd7196cc2b70f124e228fb0e6b2f1
Cr-Commit-Position: refs/heads/master@{#292282}

[Yaron, 2014-08-22 19:55:36]

Depends on upstream review & roll which I'll land before t his. Also adds
metrics for https://chrome-internal-review.googlesource.com/#/c/173500/

[cjhopman, 2014-08-23 00:26:44]

lgtm

[Yaron, 2014-08-23 01:54:46]

isherman: metrics & histograms

[Ilya Sherman, 2014-08-23 07:06:18]

+jwd for metrics review

[jwd, 2014-08-25 20:30:14]

https://codereview.chromium.org/501553002/diff/20001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/501553002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:2478: Records page loads for which using
DomDistiller is possible.
Can these be caused without user action, such as an auto refreshing page?

Do you actually care about the extra timing information that user actions
provides? Or do you simply want a count? If it's just a count, then you can
achieve it using a UMA_HISTOGRAM_BOOLEAN histogram. This would give you better
dashboard support for filters and splitting by experiments, should you ever want
that.

[Yaron, 2014-08-25 20:41:32]

https://codereview.chromium.org/501553002/diff/20001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/501553002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:2478: Records page loads for which using
DomDistiller is possible.
On 2014/08/25 20:30:14, Jesse Doherty wrote:
> Can these be caused without user action, such as an auto refreshing page?

Currently, yes: https://chrome-internal-review.googlesource.com/#/c/173500/
> 
> Do you actually care about the extra timing information that user actions
> provides? Or do you simply want a count? If it's just a count, then you can
> achieve it using a UMA_HISTOGRAM_BOOLEAN histogram. This would give you better
> dashboard support for filters and splitting by experiments, should you ever
want
> that.

Which extra timing info are you referring to?

[jwd, 2014-08-25 21:34:45]

https://codereview.chromium.org/501553002/diff/20001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/501553002/diff/20001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:2478: Records page loads for which using
DomDistiller is possible.
On 2014/08/25 20:41:32, Yaron wrote:
> On 2014/08/25 20:30:14, Jesse Doherty wrote:
> > Can these be caused without user action, such as an auto refreshing page?
> 
> Currently, yes: https://chrome-internal-review.googlesource.com/#/c/173500/
> > 
> > Do you actually care about the extra timing information that user actions
> > provides? Or do you simply want a count? If it's just a count, then you can
> > achieve it using a UMA_HISTOGRAM_BOOLEAN histogram. This would give you
better
> > dashboard support for filters and splitting by experiments, should you ever
> want
> > that.
> 
> Which extra timing info are you referring to?
Each user action that is recorded also records "when" it was recorded (it's not
an actual timestamp, but it lets you sequence data). This information is
currently only available through raw logs. 

Given that it's currently possible that this can be triggered by non user
actions, and since you weren't aware of the extra timing info, would it be
possible to change the implementation to use UMA_HISTOGRAM_BOOLEAN?

Like I said, we have much better dashboard support for it.

[Yaron, 2014-08-25 23:17:07]

On 2014/08/25 21:34:45, Jesse Doherty wrote:
>
https://codereview.chromium.org/501553002/diff/20001/tools/metrics/actions/ac...
> File tools/metrics/actions/actions.xml (right):
> 
>
https://codereview.chromium.org/501553002/diff/20001/tools/metrics/actions/ac...
> tools/metrics/actions/actions.xml:2478: Records page loads for which using
> DomDistiller is possible.
> On 2014/08/25 20:41:32, Yaron wrote:
> > On 2014/08/25 20:30:14, Jesse Doherty wrote:
> > > Can these be caused without user action, such as an auto refreshing page?
> > 
> > Currently, yes: https://chrome-internal-review.googlesource.com/#/c/173500/
> > > 
> > > Do you actually care about the extra timing information that user actions
> > > provides? Or do you simply want a count? If it's just a count, then you
can
> > > achieve it using a UMA_HISTOGRAM_BOOLEAN histogram. This would give you
> better
> > > dashboard support for filters and splitting by experiments, should you
ever
> > want
> > > that.
> > 
> > Which extra timing info are you referring to?
> Each user action that is recorded also records "when" it was recorded (it's
not
> an actual timestamp, but it lets you sequence data). This information is
> currently only available through raw logs. 
> 
> Given that it's currently possible that this can be triggered by non user
> actions, and since you weren't aware of the extra timing info, would it be
> possible to change the implementation to use UMA_HISTOGRAM_BOOLEAN?
> 
> Like I said, we have much better dashboard support for it.

Thanks for the detailed explanation. Updated.

[Ilya Sherman, 2014-08-26 01:40:11]

To expand a little on Jesse's commentary: When designing metrics, you typically
want to think about what you're going to use as a reference point for
comparison.  User actions are generally useful if you want to see how much some
feature is used, in which case you want to compare its usage to that of other
features.  Histograms are recommended for pretty much everything else.

Moreover, single-bucket histograms are generally discouraged, because they make
data analysis hard.  The one exception is if you're logging metrics for an event
that you expect to *never* happen, except possibly in exceptional circumstances.
 In that case, your reference point is "If this metric is ever non-zero, I'd
better investigate what's going wrong."  Though, in such a case, you're often
better served by uploading some sort of detailed analytics to the crash server.

In pretty much every other case, it's a good idea to build your reference point
into the histogram itself, as a baseline metric.  For this particular histogram,
you're probably interested in comparing the number of DomDistiller possible
cases to the number of not possible cases, so that you know what fraction of the
time distillation is possible.  (Incidentally, where is the code that emits this
boolean histogram?  I don't see it included in this CL, though maybe I missed
it.)

[Yaron, 2014-08-26 16:40:27]

On 2014/08/26 01:40:11, Ilya Sherman wrote:
> To expand a little on Jesse's commentary: When designing metrics, you
typically
> want to think about what you're going to use as a reference point for
> comparison.  User actions are generally useful if you want to see how much
some
> feature is used, in which case you want to compare its usage to that of other
> features.  Histograms are recommended for pretty much everything else.
> 
> Moreover, single-bucket histograms are generally discouraged, because they
make
> data analysis hard.  The one exception is if you're logging metrics for an
event
> that you expect to *never* happen, except possibly in exceptional
circumstances.
>  In that case, your reference point is "If this metric is ever non-zero, I'd
> better investigate what's going wrong."  Though, in such a case, you're often
> better served by uploading some sort of detailed analytics to the crash
server.
> 
> In pretty much every other case, it's a good idea to build your reference
point
> into the histogram itself, as a baseline metric.  For this particular
histogram,
> you're probably interested in comparing the number of DomDistiller possible
> cases to the number of not possible cases, so that you know what fraction of
the
> time distillation is possible.  (Incidentally, where is the code that emits
this
> boolean histogram?  I don't see it included in this CL, though maybe I missed
> it.)

Makes sense. 
This was the original CL:
https://chrome-internal-review.googlesource.com/#/c/173500/
And I'm changing to boolean histogram:
https://chrome-internal-review.googlesource.com/#/c/173816/
Based on your comments, I think this is the right way to go now.

[jwd, 2014-08-27 15:10:13]

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

https://codereview.chromium.org/501553002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5042: +<histogram
name="DomDistiller.PageDistillable" enum="BooleanEnabled">
I'd use a different enum type, maybe BooleanAvailable.

[Yaron, 2014-08-27 18:13:01]

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

https://codereview.chromium.org/501553002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5042: +<histogram
name="DomDistiller.PageDistillable" enum="BooleanEnabled">
On 2014/08/27 15:10:13, Jesse Doherty wrote:
> I'd use a different enum type, maybe BooleanAvailable.

Done.

[jwd, 2014-08-27 20:03:26]

lgtm

[Ilya Sherman, 2014-08-27 22:12:08]

LGTM with a couple of suggestions.

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

https://codereview.chromium.org/501553002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5035: +<histogram
name="DomDistiller.MarkupParsingTime" units="milliseconds">
Optional: You might want to name this histograms as "DomDistiller.Time.Foo", so
that they are alphabetically sorted together.

https://codereview.chromium.org/501553002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5042: +<histogram
name="DomDistiller.PageDistillable" enum="BooleanAvailable">
Even better than using BooleanAvailable would be to define a custom enum, with
buckets labelled something like "Distillable" and "Not distillable".  (Note:
This comment applies just to the histograms.xml file, no changes needed
elsewhere).

[Yaron, 2014-08-27 23:12:26]

Thanks

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

https://codereview.chromium.org/501553002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5035: +<histogram
name="DomDistiller.MarkupParsingTime" units="milliseconds">
On 2014/08/27 22:12:08, Ilya Sherman wrote:
> Optional: You might want to name this histograms as "DomDistiller.Time.Foo",
so
> that they are alphabetically sorted together.

I like it. Done.

https://codereview.chromium.org/501553002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:5042: +<histogram
name="DomDistiller.PageDistillable" enum="BooleanAvailable">
On 2014/08/27 22:12:08, Ilya Sherman wrote:
> Even better than using BooleanAvailable would be to define a custom enum, with
> buckets labelled something like "Distillable" and "Not distillable".  (Note:
> This comment applies just to the histograms.xml file, no changes needed
> elsewhere).

Done.

[Yaron, 2014-08-27 23:12:30]

The CQ bit was checked by yfriedman@chromium.org

[commit-bot: I haz the power, 2014-08-27 23:13:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yfriedman@chromium.org/501553002/80001

[commit-bot: I haz the power, 2014-08-28 00:39:09]

Committed patchset #5 (id:80001) as 32d5790f3e7c6cc84d04d47b3bd47179db3935e3

[commit-bot: I haz the power, 2014-09-10 02:56:08]

Patchset 5 (id:??) landed as
https://crrev.com/49e25f54b69dd7196cc2b70f124e228fb0e6b2f1
Cr-Commit-Position: refs/heads/master@{#292282}