Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2731513002:
Media: Add improved duration histogram (Closed)
DescriptionMedia: Add improved duration histogram
The existing Media.Duration histogram was capped at 1 hour, which is
exceeded by over 10% of videos.
BUG=none
Review-Url: https://codereview.chromium.org/2731513002
Cr-Commit-Position: refs/heads/master@{#454783}
Committed: https://chromium.googlesource.com/chromium/src/+/445361c39af42d3d14152f10a2ae9c786d5d0c68
Patch Set 1 #Patch Set 2 : More active owner for histogram #Patch Set 3 : Deprecate Media.Duration #
Total comments: 4
Patch Set 4 : Down to 50 buckets, make Dale owner #Messages
Total messages: 20 (4 generated)
johnme@chromium.org changed reviewers: + asvitkine@chromium.org, dalecurtis@chromium.org
asvitkine: please review overall patch. Is there a neater way of doing this, e.g. to extend the existing histogram with more buckets without resizing the existing ones? dalecurtis: please approve media/base/pipeline_impl.cc Thanks!
I think .Detailed is a bit of a misnomer since it has less detail :) Something like .Long feels more legible.
(I.e. you say finer, but the buckets are actually courser)
On 2017/03/02 17:46:51, DaleCurtis wrote: > (I.e. you say finer, but the buckets are actually courser) I do mean finer: since the new histogram has twice as many buckets, the new histogram bucket sizes are powers of (3600000*24*7)^(1/99) = 1.226601, whereas the old histogram bucket sizes were powers of (3600000)^(1/49) = 1.360824.
On 2017/03/02 at 18:02:13, johnme wrote: > On 2017/03/02 17:46:51, DaleCurtis wrote: > > (I.e. you say finer, but the buckets are actually courser) > > I do mean finer: since the new histogram has twice as many buckets, the new histogram bucket sizes are powers of (3600000*24*7)^(1/99) = 1.226601, whereas the old histogram bucket sizes were powers of (3600000)^(1/49) = 1.360824. The old buckets are (1..3600000) / 50, your new one is (1..604800000) / 100; or 168x increase in range and only a 2x increase in bucket count. I don't follow how that could end up with smaller finer grained buckets.
On 2017/03/02 18:09:14, DaleCurtis wrote: > On 2017/03/02 at 18:02:13, johnme wrote: > > On 2017/03/02 17:46:51, DaleCurtis wrote: > > > (I.e. you say finer, but the buckets are actually courser) > > > > I do mean finer: since the new histogram has twice as many buckets, the new > histogram bucket sizes are powers of (3600000*24*7)^(1/99) = 1.226601, whereas > the old histogram bucket sizes were powers of (3600000)^(1/49) = 1.360824. > > The old buckets are (1..3600000) / 50, your new one is (1..604800000) / 100; or > 168x increase in range and only a 2x increase in bucket count. I don't follow > how that could end up with smaller finer grained buckets. Look at https://uma.googleplex.com/p/chrome/histograms/?histograms=Media.Duration The bucket sizes increase exponentially, not linearly. (I'm happy to call it .Long anyway though if you think that'd be clearer despite being slightly finer).
I see, I forgot they were exponential buckets and even then I was surprised by just how top-heavy the new curve is relative to the old one. https://www.google.com/webhp?sourceid=chrome-instant&rlz=1CAZZAD_enUS660US660... In that case I'd just say we should obsolete the old one and call this Media.Durations2 or whatever scheme is common these days for more accurate versions.
Also I suspect those 10% are almost all in the range of 120-240 minutes; so 7 days might be a bit overkill.
On 2017/03/02 18:26:17, DaleCurtis wrote: > In that case I'd just say we should obsolete the old one and call this > Media.Durations2 or whatever scheme is common these days for more accurate > versions. Done. On 2017/03/02 18:30:50, DaleCurtis wrote: > Also I suspect those 10% are almost all in the range of 120-240 minutes; so 7 > days might be a bit overkill. Fair - I reduced the max to 1 day (so the buckets are now powers of 1.203 instead of 1.227). That has the nice benefit of making the max bucket a recognizable number: 86,400,000. I'd rather not reduce it further, as I'm aiming for the top buckets to be almost empty so we can assume that anything in the overflow bucket likely had infinite duration. Also, further reductions wouldn't significantly change the bucket size multiplier, for example reducing it all the way to 4 hours would only change the buckets to powers of 1.181.
Update description with some more details on the change and how we came up with the values, but otherwise lgtm https://codereview.chromium.org/2731513002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2731513002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25753: + <owner>mlamouri@chromium.org</owner> You can mark me as owner of this one since it's in media/base area.
https://codereview.chromium.org/2731513002/diff/40001/media/base/pipeline_imp... File media/base/pipeline_impl.cc (right): https://codereview.chromium.org/2731513002/diff/40001/media/base/pipeline_imp... media/base/pipeline_impl.cc:510: base::TimeDelta::FromDays(1), 100 /* bucket_count */); Are you sure you really need more buckets and not just a higher maximum range? We generally advise against more than 50 buckets because it's often not needed and results in increased data use for UMA histograms.
Addressed comments - thanks! https://codereview.chromium.org/2731513002/diff/40001/media/base/pipeline_imp... File media/base/pipeline_impl.cc (right): https://codereview.chromium.org/2731513002/diff/40001/media/base/pipeline_imp... media/base/pipeline_impl.cc:510: base::TimeDelta::FromDays(1), 100 /* bucket_count */); On 2017/03/03 16:37:37, Alexei Svitkine (very slow) wrote: > Are you sure you really need more buckets and not just a higher maximum range? > > We generally advise against more than 50 buckets because it's often not needed > and results in increased data use for UMA histograms. We can probably get by with fewer. I've updated the patch to only use 50 buckets - that means the buckets are now powers of 1.452, whereas before this patch they were powers of 1.361. Dale, are you ok with that? Some alternatives include: - 24h max 60 buckets -> 1.363 - 12h max 50 buckets -> 1.432 - 6h max 50 buckets -> 1.412 https://codereview.chromium.org/2731513002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2731513002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25753: + <owner>mlamouri@chromium.org</owner> On 2017/03/02 19:52:59, DaleCurtis wrote: > You can mark me as owner of this one since it's in media/base area. Done.
lgtm
Still lgtm, though if you are concerned about bucket size we can drop the max value as previously discussed to something like 8 hours.
The CQ bit was checked by johnme@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488664721578070,
"parent_rev": "2ea90f5b612d91e6b9e2f5eb44a26ce43e32bab9", "commit_rev":
"445361c39af42d3d14152f10a2ae9c786d5d0c68"}
Message was sent while issue was closed.
Description was changed from ========== Media: Add improved duration histogram The existing Media.Duration histogram was capped at 1 hour, which is exceeded by over 10% of videos. BUG=none ========== to ========== Media: Add improved duration histogram The existing Media.Duration histogram was capped at 1 hour, which is exceeded by over 10% of videos. BUG=none Review-Url: https://codereview.chromium.org/2731513002 Cr-Commit-Position: refs/heads/master@{#454783} Committed: https://chromium.googlesource.com/chromium/src/+/445361c39af42d3d14152f10a2ae... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/445361c39af42d3d14152f10a2ae... |
