New issue
Advanced search Search tips

Issue 830853 link

Starred by 1 user
Status: Fixed
Owner:
dskiba@chromium.org
Closed: Apr 2018
Cc:
asvitk...@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Warn about high-res TimeUnits used with TimesHistogram

Project Member Reported by dskiba@chromium.org, Apr 9 2018 
CachedMetrics.TimesHistogramSample takes TimeUnit, but internally converts all samples to milliseconds (see RecordHistogram.recordCustomTimesHistogramMilliseconds). I.e. if TimeUnit is MICROSECONDS or NANOSECONDS most samples are likely to be 0.

Same is true for RecordHistogram.recordTimesHistogram and its variants.

On the native side CountHistograms are used with high-res times, but CachedMetrics doesn't support CountHistogram.

In the scope of this issue we need to:

1. Support CountHistograms in CachedMetrics.

2. Assert when high-res TimeUnits are used with TimesHistogram.
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 9 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1bc486162f640781a162186315a1d61d5594e16c

commit 1bc486162f640781a162186315a1d61d5594e16c
Author: Dmitry Skiba <dskiba@chromium.org>
Date: Mon Apr 09 20:56:11 2018

Support CountHistogram in CachedMetrics.

Bug:  830853 

Change-Id: I893b5d205d6f3e033e0d5cc7f95d4acc4391fd23
Reviewed-on: https://chromium-review.googlesource.com/1001475
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Dmitry Skiba <dskiba@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549270}
[modify] https://crrev.com/1bc486162f640781a162186315a1d61d5594e16c/base/android/java/src/org/chromium/base/metrics/CachedMetrics.java
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 10 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1b2c2bed19310df1f581aeff0b0663cdb2fab1da

commit 1b2c2bed19310df1f581aeff0b0663cdb2fab1da
Author: Dmitry Skiba <dskiba@chromium.org>
Date: Tue Apr 10 00:58:58 2018

Validate TimeUnit used in TimesHistogram.

TimesHistogram takes TimeUnit, but internally converts all samples to
milliseconds. I.e. MICROSECONDS and NANOSECONDS time units silently
produce zero samples.

This CL asserts if unsupported TimeUnit used with TimesHistogram.

Bug:  830853 

Change-Id: Ie0cf1adda53978dbf331d20eb2a1ad826795ff8d
Reviewed-on: https://chromium-review.googlesource.com/1001513
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Dmitry Skiba <dskiba@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549354}
[modify] https://crrev.com/1b2c2bed19310df1f581aeff0b0663cdb2fab1da/base/android/java/src/org/chromium/base/metrics/CachedMetrics.java
[modify] https://crrev.com/1b2c2bed19310df1f581aeff0b0663cdb2fab1da/base/android/java/src/org/chromium/base/metrics/RecordHistogram.java
[modify] https://crrev.com/1b2c2bed19310df1f581aeff0b0663cdb2fab1da/chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java

Comment 3 by dskiba@chromium.org, Apr 10 2018

Status: Fixed (was: Assigned)
►

Sign in to add a comment