Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1771093002:
Adding UMA for daily video percent savings for data reduction proxy (Closed)
DescriptionAdding UMA for daily video percent savings for data reduction proxy
This will track the savings percent of video that went through flywheel
and the video that was downloaded while flywheel was enabled on a daily
basis.
BUG=591874
Committed: https://crrev.com/cc1cf83356706986647d5a77f39e060a499dce82
Cr-Commit-Position: refs/heads/master@{#386743}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Adding a comment to clarify why only reporting a savings percentage is justified #
Total comments: 4
Patch Set 3 : Rebase, nits #
Messages
Total messages: 16 (5 generated)
ryansturm@chromium.org changed reviewers: + bengr@chromium.org
bengr: PTAL
https://codereview.chromium.org/1771093002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1771093002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:857: if (original_length_with_data_reduction_enabled_video > Where do we record if we've inflated video?
https://codereview.chromium.org/1771093002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1771093002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:857: if (original_length_with_data_reduction_enabled_video > On 2016/03/07 23:19:22, bengr wrote: > Where do we record if we've inflated video? Would you like a Net.DailyContentInflatedPercent_DataReductionProxyEnabled_Video that would represent this (I would want to represent it as 0-100%, so this is probably a misnomer)? I thought we only serve the compressed video from the server if it has reduced size, so this case is not possible, but I am unsure.
https://codereview.chromium.org/1771093002/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1771093002/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:857: if (original_length_with_data_reduction_enabled_video > On 2016/03/07 23:33:04, RyanSturm wrote: > On 2016/03/07 23:19:22, bengr wrote: > > Where do we record if we've inflated video? > > Would you like a Net.DailyContentInflatedPercent_DataReductionProxyEnabled_Video > that would represent this (I would want to represent it as 0-100%, so this is > probably a misnomer)? I thought we only serve the compressed video from the > server if it has reduced size, so this case is not possible, but I am unsure. If it is possible that the received size is greater than the original size, we should track the inflation. If we don't know if this is possible, we should track (with a special value or boolean) if it happens. Please chat with the video compression people.
On 2016/03/15 23:21:33, bengr wrote: > https://codereview.chromium.org/1771093002/diff/1/components/data_reduction_p... > File > components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc > (right): > > https://codereview.chromium.org/1771093002/diff/1/components/data_reduction_p... > components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:857: > if (original_length_with_data_reduction_enabled_video > > On 2016/03/07 23:33:04, RyanSturm wrote: > > On 2016/03/07 23:19:22, bengr wrote: > > > Where do we record if we've inflated video? > > > > Would you like a > Net.DailyContentInflatedPercent_DataReductionProxyEnabled_Video > > that would represent this (I would want to represent it as 0-100%, so this is > > probably a misnomer)? I thought we only serve the compressed video from the > > server if it has reduced size, so this case is not possible, but I am unsure. > > If it is possible that the received size is greater than the original size, we > should track the inflation. If we don't know if this is possible, we should > track (with a special value or boolean) if it happens. Please chat with the > video compression people. Because it is possible with header inflation or by other means to have negative savings, a bug has been opened to resolve all of the percentage base DataReductionProxy histograms: https://bugs.chromium.org/p/chromium/issues/detail?id=595818
lgtm with nits. https://codereview.chromium.org/1771093002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1771093002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:858: // if the the optimized content is smaller than the original content. Please reference the bug to track inflations here. https://codereview.chromium.org/1771093002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:906: // if the the optimized content is smaller than the original content. typo here and on line 858: the the
ryansturm@chromium.org changed reviewers: + holte@chromium.org
ptal: holte for metrics https://codereview.chromium.org/1771093002/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1771093002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:858: // if the the optimized content is smaller than the original content. On 2016/03/23 21:32:32, bengr wrote: > Please reference the bug to track inflations here. Done. https://codereview.chromium.org/1771093002/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:906: // if the the optimized content is smaller than the original content. On 2016/03/23 21:32:32, bengr wrote: > typo here and on line 858: the the Done.
On 2016/03/24 18:10:18, RyanSturm wrote: > ptal: holte for metrics > > https://codereview.chromium.org/1771093002/diff/20001/components/data_reducti... > File > components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc > (right): > > https://codereview.chromium.org/1771093002/diff/20001/components/data_reducti... > components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:858: > // if the the optimized content is smaller than the original content. > On 2016/03/23 21:32:32, bengr wrote: > > Please reference the bug to track inflations here. > > Done. > > https://codereview.chromium.org/1771093002/diff/20001/components/data_reducti... > components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:906: > // if the the optimized content is smaller than the original content. > On 2016/03/23 21:32:32, bengr wrote: > > typo here and on line 858: the the > > Done. lgtm, sorry for the delay, this got lost in inbox.
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org Link to the patchset: https://codereview.chromium.org/1771093002/#ps40001 (title: "Rebase, nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1771093002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1771093002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Adding UMA for daily video percent savings for data reduction proxy This will track the savings percent of video that went through flywheel and the video that was downloaded while flywheel was enabled on a daily basis. BUG=591874 ========== to ========== Adding UMA for daily video percent savings for data reduction proxy This will track the savings percent of video that went through flywheel and the video that was downloaded while flywheel was enabled on a daily basis. BUG=591874 Committed: https://crrev.com/cc1cf83356706986647d5a77f39e060a499dce82 Cr-Commit-Position: refs/heads/master@{#386743} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cc1cf83356706986647d5a77f39e060a499dce82 Cr-Commit-Position: refs/heads/master@{#386743} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
