Change the logic for discarding all non-histogram logs because of actions/omnibox events to truncat…

Change the logic for discarding all non-histogram logs because of actions/omnibox events to truncating.

I also expanded the limit to 5k events per type (omnibox, user actions).

BUG=683934
Review-Url: https://codereview.chromium.org/2910023003
Cr-Commit-Position: refs/heads/master@{#476992}
Committed: https://chromium.googlesource.com/chromium/src/+/78985a058ff8bf5d831d8d27fcb40bdf1e54df66

[rkaplow, 2017-05-30 19:28:35]

Description was changed from

==========
Change the logic for discarding all non-histogram logs because of
actions/omnibox events to truncating.

I also expanded the limit to 5k events per type (omnibox, user actions).

BUG=683934
==========

to

==========
Change the logic for discarding all non-histogram logs because of
actions/omnibox events to truncating.

I also expanded the limit to 5k events per type (omnibox, user actions).

BUG=683934
==========

[rkaplow, 2017-05-30 19:28:36]

rkaplow@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2017-05-30 21:57:00]

lgtm % comment

https://codereview.chromium.org/2910023003/diff/20001/components/metrics/metr...
File components/metrics/metrics_log_unittest.cc (right):

https://codereview.chromium.org/2910023003/diff/20001/components/metrics/metr...
components/metrics/metrics_log_unittest.cc:430: }
Nit: Add one for omnibox events too. You can just add them directly to the proto
rather if there's no good API on the log to do it, like there is for user
actions.

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

https://codereview.chromium.org/2910023003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:78215: +<histogram
name="UMA.TruncateOmnibox" units="events">
Nit: Make the metric name more clearly reflect what this is.

e.g. UMA.TruncatedEvents.Omnibox

The above syntax also allows to use histogram_suffixes - although for just 2
it's probably fine without it, but if we expand this to more events in the
future, we can still keep the same naming conventions.

[rkaplow, 2017-05-31 15:17:06]

https://codereview.chromium.org/2910023003/diff/20001/components/metrics/metr...
File components/metrics/metrics_log_unittest.cc (right):

https://codereview.chromium.org/2910023003/diff/20001/components/metrics/metr...
components/metrics/metrics_log_unittest.cc:430: }
On 2017/05/30 21:57:00, Alexei Svitkine (slow) wrote:
> Nit: Add one for omnibox events too. You can just add them directly to the
proto
> rather if there's no good API on the log to do it, like there is for user
> actions.

Done.

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

https://codereview.chromium.org/2910023003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:78215: +<histogram
name="UMA.TruncateOmnibox" units="events">
On 2017/05/30 21:57:00, Alexei Svitkine (slow) wrote:
> Nit: Make the metric name more clearly reflect what this is.
> 
> e.g. UMA.TruncatedEvents.Omnibox
> 
> The above syntax also allows to use histogram_suffixes - although for just 2
> it's probably fine without it, but if we expand this to more events in the
> future, we can still keep the same naming conventions.

Done.

[Alexei Svitkine (slow), 2017-05-31 15:31:35]

lgtm

Please also update our server-side monitoring since you're removing a metric we
have an alert for (it's subscribed to in the dashboard UI).

[rkaplow, 2017-05-31 15:44:12]

The CQ bit was checked by rkaplow@chromium.org

[commit-bot: I haz the power, 2017-05-31 15:44:49]

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, 2017-05-31 15:54:43]

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

[commit-bot: I haz the power, 2017-05-31 15:54:44]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[rkaplow, 2017-05-31 17:37:30]

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

[commit-bot: I haz the power, 2017-05-31 17:37:52]

Dry run: CQ is trying da patch.

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

[rkaplow, 2017-05-31 17:56:46]

rkaplow@chromium.org changed reviewers:
+ olivierrobin@chromium.org

[rkaplow, 2017-05-31 17:58:13]

Olivier PTAL at the change to today. It's actually a functional change which
increases the limit to 5000. I thought it would be ok to change the limit and
keeping it the same as the chrome logging would be fine, but let me know if you
want it to have custom logic.

[commit-bot: I haz the power, 2017-05-31 18:25:55]

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

[commit-bot: I haz the power, 2017-05-31 18:25:56]

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...)

[rkaplow, 2017-06-01 22:00:31]

rkaplow@chromium.org changed reviewers:
+ rohitrao@chromium.org
- olivierrobin@chromium.org

[rkaplow, 2017-06-02 13:56:41]

rkaplow@chromium.org changed reviewers:
+ droger@chromium.org

[rkaplow, 2017-06-02 13:56:58]

adding non olivier reviews. Thanks

[rohitrao (ping after 24h), 2017-06-05 14:35:54]

ios/ lgtm

[rkaplow, 2017-06-05 14:45:06]

The CQ bit was checked by rkaplow@chromium.org

[rkaplow, 2017-06-05 14:45:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2910023003/#ps60001
(title: "today extension")

[commit-bot: I haz the power, 2017-06-05 14:46:01]

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, 2017-06-05 16:13:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496673906871760,
"parent_rev": "9171a2ab9637d50fb574ebc354a859776efa9dd0", "commit_rev":
"78985a058ff8bf5d831d8d27fcb40bdf1e54df66"}

[commit-bot: I haz the power, 2017-06-05 16:13:33]

Description was changed from

==========
Change the logic for discarding all non-histogram logs because of
actions/omnibox events to truncating.

I also expanded the limit to 5k events per type (omnibox, user actions).

BUG=683934
==========

to

==========
Change the logic for discarding all non-histogram logs because of
actions/omnibox events to truncating.

I also expanded the limit to 5k events per type (omnibox, user actions).

BUG=683934

Review-Url: https://codereview.chromium.org/2910023003
Cr-Commit-Position: refs/heads/master@{#476992}
Committed:
https://chromium.googlesource.com/chromium/src/+/78985a058ff8bf5d831d8d27fcb4...
==========

[commit-bot: I haz the power, 2017-06-05 16:13:35]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/78985a058ff8bf5d831d8d27fcb4...