Expand event timestamp validation check to more platforms

Expand event timestamp validation check to more platforms

Collect UMA metric on validity of event timestamp clock on platforms
where we currently check the timebase (i.e., X11, Mac)*.
We still only correct invalid timestamps for X11 but will monitor this histogram
to see if there is a need for similar corrections on other platforms.

Also the histogram is now suffixed to differentiate between BROWSER and
RENDERER processes in anticipation of supporting a similar check
on renderer process.

* The check will cover more platforms in https://codereview.chromium.org/2408063005/

BUG=611950
Committed: https://crrev.com/758cb6139c130fca8d6153d621203c18980005df
Cr-Commit-Position: refs/heads/master@{#425032}

[majidvp, 2016-08-30 17:53:30]

Description was changed from

==========
Expand event timestamp validation check to all platforms

Collect UMA metric on validity of event timestamp on all platforms.
We still only correct the timestamp for X11 but will monitor this histogram
to see if there is a need for similar corrections on other platforms.

BUG=
==========

to

==========
Expand event timestamp validation check to all platforms

Collect UMA metric on validity of event timestamp on all platforms.
We still only correct the timestamp for X11 but will monitor this histogram
to see if there is a need for similar corrections on other platforms.

BUG=611950
==========

[majidvp, 2016-08-30 17:54:04]

The CQ bit was checked by majidvp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 17:54:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 19:10:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-30 19:10:12]

Dry run: This issue passed the CQ dry run.

[majidvp, 2016-08-30 19:12:18]

majidvp@chromium.org changed reviewers:
+ sadrul@chromium.org

[dtapuska, 2016-08-31 13:21:53]

On 2016/08/30 at 19:12:18, majidvp wrote:
> 

So this is a check for validity inside the browser process. What about the
validity across processes? Is that another change and another histogram?

[majidvp, 2016-08-31 15:12:16]

On 2016/08/31 13:21:53, dtapuska wrote:
> On 2016/08/30 at 19:12:18, majidvp wrote:
> > 
> 
> So this is a check for validity inside the browser process. What about the
> validity across processes? Is that another change and another histogram?

Yes. I think we should have something to check across process. In particular
a check in renderer process. Though it makes sense for it to be a separate
histogram. How about naming this:
Event.TimestampHasValidTimebase.Browser

and adding a new one:
Event.TimestampHasValidTimebase.Renderer

[sadrul, 2016-08-31 20:25:39]

On 2016/08/31 15:12:16, majidvp wrote:
> On 2016/08/31 13:21:53, dtapuska wrote:
> > On 2016/08/30 at 19:12:18, majidvp wrote:
> > > 
> > 
> > So this is a check for validity inside the browser process. What about the
> > validity across processes? Is that another change and another histogram?
> 
> Yes. I think we should have something to check across process. In particular
> a check in renderer process. Though it makes sense for it to be a separate
> histogram. How about naming this:
> Event.TimestampHasValidTimebase.Browser
> 
> and adding a new one:
> Event.TimestampHasValidTimebase.Renderer

We have 14 invalids out of 11 million records so far. Do we expect this number
to be actually useful for anything?

[majidvp, 2016-08-31 20:42:35]

On 2016/08/31 20:25:39, sadrul wrote:
> On 2016/08/31 15:12:16, majidvp wrote:
> > On 2016/08/31 13:21:53, dtapuska wrote:
> > > On 2016/08/30 at 19:12:18, majidvp wrote:
> > > > 
> > > 
> > > So this is a check for validity inside the browser process. What about the
> > > validity across processes? Is that another change and another histogram?
> > 
> > Yes. I think we should have something to check across process. In particular
> > a check in renderer process. Though it makes sense for it to be a separate
> > histogram. How about naming this:
> > Event.TimestampHasValidTimebase.Browser
> > 
> > and adding a new one:
> > Event.TimestampHasValidTimebase.Renderer
> 
> We have 14 invalids out of 11 million records so far. Do we expect this number
> to be actually useful for anything?

That is a pretty good number. :)

This number is mostly a canary in a coal mine to alert us in case for
some reason our event timestamp ends up not coming from the same clock as
the base::TimeTicks.

Ideally we  should just have this as a CHECK*. I would be more comfortable 
to add a CHECK after we collect some UMA and know that this number will be
small.
At that point we can remove this histogram.

* I think a DCHECK will not be that useful as our debug build does not cover
lots of
interesting cases which we have in the wild.

[majidvp, 2016-09-06 20:11:16]

sadrul@: friendly ping!

[sadrul, 2016-09-07 12:36:46]

On 2016/09/06 20:11:16, majidvp wrote:
> sadrul@: friendly ping!

I thought you were going to rename this to
Event.TimestampHasValidTimebase.Browser?

Also, can you open a bug to track whether these can be turned into CHECK()s, and
remove the UMA?

[majidvp, 2016-09-26 18:20:36]

The CQ bit was checked by majidvp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 18:21:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[majidvp, 2016-09-26 18:27:25]

On 2016/09/07 12:36:46, sadrul wrote:
> On 2016/09/06 20:11:16, majidvp wrote:
> > sadrul@: friendly ping!
> 
> I thought you were going to rename this to
> Event.TimestampHasValidTimebase.Browser?
Done. I used a histogram suffix. If the metrics OWNERs are not happy
I will change it to two separate histograms.

> Also, can you open a bug to track whether these can be turned into CHECK()s,
and
> remove the UMA?
Done: https://bugs.chromium.org/p/chromium/issues/detail?id=650338

[majidvp, 2016-09-26 18:29:22]

Description was changed from

==========
Expand event timestamp validation check to all platforms

Collect UMA metric on validity of event timestamp on all platforms.
We still only correct the timestamp for X11 but will monitor this histogram
to see if there is a need for similar corrections on other platforms.

BUG=611950
==========

to

==========
Expand event timestamp validation check to all platforms

Collect UMA metric on validity of event timestamp on all platforms.
We still only correct the timestamp for X11 but will monitor this histogram
to see if there is a need for similar corrections on other platforms.

Also the histogram is now suffixed to differentiate between BROWSER and
RENDERER processes. 

BUG=611950
==========

[majidvp, 2016-09-26 18:29:22]

majidvp@chromium.org changed reviewers:
+ isherman@chromium.org

[majidvp, 2016-09-26 18:30:18]

+isherman@: For histogram.xml OWNERS review.

[Ilya Sherman, 2016-09-26 19:04:44]

https://codereview.chromium.org/2295923003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2295923003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:13543: +<histogram
name="Event.TimestampHasValidTimebase" units="boolean">
Why did you remove the enum?

https://codereview.chromium.org/2295923003/diff/20001/ui/events/event_utils.cc
File ui/events/event_utils.cc (right):

https://codereview.chromium.org/2295923003/diff/20001/ui/events/event_utils.c...
ui/events/event_utils.cc:69: int64_t delta = (now -
*timestamp).InMilliseconds();
nit: Please write this as

bool has_valid_timebase = delta >= 0 && delta <= 60 * 1000;
UMA_HISTOGRAM_BOOLEAN("Event.TimestampHasValidTimebase.Browser",
has_valid_timebase);
if (!has_valid_timebase) {
  ...
}

https://codereview.chromium.org/2295923003/diff/20001/ui/events/event_utils.c...
ui/events/event_utils.cc:71:
UMA_HISTOGRAM_BOOLEAN("Event.TimestampHasValidTimebase.Browser", false);
Where is the .Renderer version of this histogram?

[commit-bot: I haz the power, 2016-09-26 19:54:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 19:55:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[majidvp, 2016-10-11 16:02:07]

https://codereview.chromium.org/2295923003/diff/20001/ui/events/event_utils.cc
File ui/events/event_utils.cc (right):

https://codereview.chromium.org/2295923003/diff/20001/ui/events/event_utils.c...
ui/events/event_utils.cc:69: int64_t delta = (now -
*timestamp).InMilliseconds();
On 2016/09/26 19:04:44, Ilya Sherman wrote:
> nit: Please write this as
> 
> bool has_valid_timebase = delta >= 0 && delta <= 60 * 1000;
> UMA_HISTOGRAM_BOOLEAN("Event.TimestampHasValidTimebase.Browser",
> has_valid_timebase);
> if (!has_valid_timebase) {
>   ...
> }

Done.

https://codereview.chromium.org/2295923003/diff/20001/ui/events/event_utils.c...
ui/events/event_utils.cc:71:
UMA_HISTOGRAM_BOOLEAN("Event.TimestampHasValidTimebase.Browser", false);
On 2016/09/26 19:04:44, Ilya Sherman wrote:
> Where is the .Renderer version of this histogram?

It is more involved so I am working on adding it in a separate patch here:
https://codereview.chromium.org/1936853002

[sadrul, 2016-10-11 17:52:36]

lgtm ... but it looks like we call ValidateEventTimeClock() only on x11 and mac?
If so, can you clarify in the CL description that for now, this is going to be
active only for x11 and mac, and I suppose future CLs will add code to use this
on android + windows + ozone-chromeos too?

[Ilya Sherman, 2016-10-11 23:54:09]

https://codereview.chromium.org/2295923003/diff/20001/ui/events/event_utils.cc
File ui/events/event_utils.cc (right):

https://codereview.chromium.org/2295923003/diff/20001/ui/events/event_utils.c...
ui/events/event_utils.cc:71:
UMA_HISTOGRAM_BOOLEAN("Event.TimestampHasValidTimebase.Browser", false);
On 2016/10/11 16:02:07, majidvp wrote:
> On 2016/09/26 19:04:44, Ilya Sherman wrote:
> > Where is the .Renderer version of this histogram?
> 
> It is more involved so I am working on adding it in a separate patch here:
> https://codereview.chromium.org/1936853002

In general, it's best to modify histograms.xml in precisely the same CL as the
one where the new metric is being added.

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

https://codereview.chromium.org/2295923003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:13543: +<histogram
name="Event.TimestampHasValidTimebase" units="boolean">
Why did you remove the enum association for this histogram?

[majidvp, 2016-10-12 13:44:22]

https://codereview.chromium.org/2295923003/diff/20001/ui/events/event_utils.cc
File ui/events/event_utils.cc (right):

https://codereview.chromium.org/2295923003/diff/20001/ui/events/event_utils.c...
ui/events/event_utils.cc:71:
UMA_HISTOGRAM_BOOLEAN("Event.TimestampHasValidTimebase.Browser", false);
On 2016/10/11 23:54:08, Ilya Sherman wrote:
> On 2016/10/11 16:02:07, majidvp wrote:
> > On 2016/09/26 19:04:44, Ilya Sherman wrote:
> > > Where is the .Renderer version of this histogram?
> > 
> > It is more involved so I am working on adding it in a separate patch here:
> > https://codereview.chromium.org/1936853002
> 
> In general, it's best to modify histograms.xml in precisely the same CL as the
> one where the new metric is being added.

I thought it will be odd to have a histogram_suffixes with a 
single suffix. But if that is acceptable I will change.

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

https://codereview.chromium.org/2295923003/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:13543: +<histogram
name="Event.TimestampHasValidTimebase" units="boolean">
On 2016/10/11 23:54:09, Ilya Sherman wrote:
> Why did you remove the enum association for this histogram?

Ouch! Somehow I got it to my head that the enum is actually not defined
and this is was not correct. Fixed.

[majidvp, 2016-10-12 15:37:21]

Description was changed from

==========
Expand event timestamp validation check to all platforms

Collect UMA metric on validity of event timestamp on all platforms.
We still only correct the timestamp for X11 but will monitor this histogram
to see if there is a need for similar corrections on other platforms.

Also the histogram is now suffixed to differentiate between BROWSER and
RENDERER processes. 

BUG=611950
==========

to

==========
Expand event timestamp validation check to more platforms

Collect UMA metric on validity of event timestamp clock on platforms
where we currently check the timebase (i.e., X11, Mac)*. 
We still only correct invalid timestamps for X11 but will monitor this histogram
to see if there is a need for similar corrections on other platforms.

Also the histogram is now suffixed to differentiate between BROWSER and
RENDERER processes in anticipation of supporting a similar check
on renderer process. 

* The check will cover more platforms in
https://codereview.chromium.org/2408063005/

BUG=611950
==========

[majidvp, 2016-10-12 15:39:28]

On 2016/10/11 17:52:36, sadrul wrote:
> lgtm ... but it looks like we call ValidateEventTimeClock() only on x11 and
mac?
> If so, can you clarify in the CL description that for now, this is going to be
> active only for x11 and mac, and I suppose future CLs will add code to use
this
> on android + windows + ozone-chromeos too?

Done. Here is the follow up CL for Android/Ozone:
https://codereview.chromium.org/2408063005/

The check is redundant on Windows, see
https://codesearch.chromium.org/chromium/src/ui/events/win/events_win.cc?type...

[Ilya Sherman, 2016-10-13 00:40:17]

metrics lgtm

https://codereview.chromium.org/2295923003/diff/20001/ui/events/event_utils.cc
File ui/events/event_utils.cc (right):

https://codereview.chromium.org/2295923003/diff/20001/ui/events/event_utils.c...
ui/events/event_utils.cc:71:
UMA_HISTOGRAM_BOOLEAN("Event.TimestampHasValidTimebase.Browser", false);
On 2016/10/12 13:44:21, majidvp wrote:
> On 2016/10/11 23:54:08, Ilya Sherman wrote:
> > On 2016/10/11 16:02:07, majidvp wrote:
> > > On 2016/09/26 19:04:44, Ilya Sherman wrote:
> > > > Where is the .Renderer version of this histogram?
> > > 
> > > It is more involved so I am working on adding it in a separate patch here:
> > > https://codereview.chromium.org/1936853002
> > 
> > In general, it's best to modify histograms.xml in precisely the same CL as
the
> > one where the new metric is being added.
> 
> I thought it will be odd to have a histogram_suffixes with a 
> single suffix. But if that is acceptable I will change.

It's acceptable, yeah.  It's not a big deal either way in this particular case,
though: Mainly the reason that it helps to change histograms.xml in the same CL
is that it loops in a metrics reviewer, and we look at the C++ code as well as
the XML changes.  In this case I've already glanced at your follow-up CL and it
looks fine, so there's not necessarily a need for the additional metrics review
there.

[majidvp, 2016-10-13 14:12:41]

The CQ bit was checked by majidvp@chromium.org

[majidvp, 2016-10-13 14:12:41]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2295923003/#ps60001
(title: "Fix mistake")

[commit-bot: I haz the power, 2016-10-13 14:13:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 15:09:06]

Description was changed from

==========
Expand event timestamp validation check to more platforms

Collect UMA metric on validity of event timestamp clock on platforms
where we currently check the timebase (i.e., X11, Mac)*. 
We still only correct invalid timestamps for X11 but will monitor this histogram
to see if there is a need for similar corrections on other platforms.

Also the histogram is now suffixed to differentiate between BROWSER and
RENDERER processes in anticipation of supporting a similar check
on renderer process. 

* The check will cover more platforms in
https://codereview.chromium.org/2408063005/

BUG=611950
==========

to

==========
Expand event timestamp validation check to more platforms

Collect UMA metric on validity of event timestamp clock on platforms
where we currently check the timebase (i.e., X11, Mac)*. 
We still only correct invalid timestamps for X11 but will monitor this histogram
to see if there is a need for similar corrections on other platforms.

Also the histogram is now suffixed to differentiate between BROWSER and
RENDERER processes in anticipation of supporting a similar check
on renderer process. 

* The check will cover more platforms in
https://codereview.chromium.org/2408063005/

BUG=611950
==========

[commit-bot: I haz the power, 2016-10-13 15:09:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-13 15:11:25]

Description was changed from

==========
Expand event timestamp validation check to more platforms

Collect UMA metric on validity of event timestamp clock on platforms
where we currently check the timebase (i.e., X11, Mac)*. 
We still only correct invalid timestamps for X11 but will monitor this histogram
to see if there is a need for similar corrections on other platforms.

Also the histogram is now suffixed to differentiate between BROWSER and
RENDERER processes in anticipation of supporting a similar check
on renderer process. 

* The check will cover more platforms in
https://codereview.chromium.org/2408063005/

BUG=611950
==========

to

==========
Expand event timestamp validation check to more platforms

Collect UMA metric on validity of event timestamp clock on platforms
where we currently check the timebase (i.e., X11, Mac)*.
We still only correct invalid timestamps for X11 but will monitor this histogram
to see if there is a need for similar corrections on other platforms.

Also the histogram is now suffixed to differentiate between BROWSER and
RENDERER processes in anticipation of supporting a similar check
on renderer process.

* The check will cover more platforms in
https://codereview.chromium.org/2408063005/

BUG=611950

Committed: https://crrev.com/758cb6139c130fca8d6153d621203c18980005df
Cr-Commit-Position: refs/heads/master@{#425032}
==========

[commit-bot: I haz the power, 2016-10-13 15:11:27]

Patchset 4 (id:??) landed as
https://crrev.com/758cb6139c130fca8d6153d621203c18980005df
Cr-Commit-Position: refs/heads/master@{#425032}