Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1956773003:
UMA for data reduction proxy video percentages are being reported when users don't watch video. (Closed)
DescriptionUMA for data reduction proxy video percentages are being
reported when users don't watch video.
This change scopes down the reporting to only happen when the
user is served video with data reduction proxy enabled and
via data reduction proxy for the two respective histograms.
BUG=591874
Committed: https://crrev.com/f76fc2bbe58660f61465a6746e8f411d636c7b65
Cr-Commit-Position: refs/heads/master@{#392183}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 6
Patch Set 3 : changing histograms.xml #
Total comments: 4
Patch Set 4 : nit fix #
Total comments: 2
Messages
Total messages: 30 (10 generated)
ryansturm@chromium.org changed reviewers: + tbansal@chromium.org
tbansal1: PTAL, thanks
On 2016/05/06 17:51:53, RyanSturm wrote: > tbansal1: PTAL, thanks Should we make it consistent and change all other UMA_HISTOGRAM_PERCENTAGE cases as well?
Description was changed from ========== UMA for data reduction proxy video percentages are being reported when users don't watch video. This change scopes down the reporting to only happen when the user is served video with data reduction proxy enabled and via data reduction proxy for the two respective histograms. BUG=591874 ========== to ========== UMA for data reduction proxy video percentages are being reported when users don't watch video. This change scopes down the reporting to only happen when the user is served video with data reduction proxy enabled and via data reduction proxy for the two respective histograms. BUG=591874 ==========
Also, in the CL description wrap the lines at 80 chars. https://www.chromium.org/developers/contributing-code#TOC-Change-List-Descrip... https://codereview.chromium.org/1956773003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1956773003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:855: "Net.DailyContentSavingPercent_DataReductionProxyEnabled_Video", You need to update the description in histograms.xml. Right now, it says ". The metric is reported when the first response in the current day is received. " https://codereview.chromium.org/1956773003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:907: "Net.DailyContentSavingPercent_ViaDataReductionProxy_Video", Same here, update histograms.xml.
On 2016/05/06 18:12:52, bolian wrote: > On 2016/05/06 17:51:53, RyanSturm wrote: > > tbansal1: PTAL, thanks > > Should we make it consistent and change all other UMA_HISTOGRAM_PERCENTAGE cases > as well? Others that are mesauring savings are: Net.DailyContentSavingPercent Net.DailyContentSavingPercent_DataReductionProxyEnabled Net.DailyContentSavingPercent_ViaDataReductionProxy I am not sure if we want to condtion them, but if we do we should use another histogram, not an existing one.
On 2016/05/06 18:24:23, tbansal1 wrote: > On 2016/05/06 18:12:52, bolian wrote: > > On 2016/05/06 17:51:53, RyanSturm wrote: > > > tbansal1: PTAL, thanks > > > > Should we make it consistent and change all other UMA_HISTOGRAM_PERCENTAGE > cases > > as well? > > Others that are mesauring savings are: > Net.DailyContentSavingPercent > Net.DailyContentSavingPercent_DataReductionProxyEnabled > Net.DailyContentSavingPercent_ViaDataReductionProxy > > I am not sure if we want to condtion them, but if we do > we should use another histogram, not an existing one. I opened a bug for other histograms: https://bugs.chromium.org/p/chromium/issues/detail?id=609907 Some of the other histograms are conditioned by early returns. It looks like Net.DailyContentSavingPercent_ViaDataReductionProxy is the only other one that is not conditioned.
On 2016/05/06 18:24:23, tbansal1 wrote: > On 2016/05/06 18:12:52, bolian wrote: > > On 2016/05/06 17:51:53, RyanSturm wrote: > > > tbansal1: PTAL, thanks > > > > Should we make it consistent and change all other UMA_HISTOGRAM_PERCENTAGE > cases > > as well? > > Others that are mesauring savings are: > Net.DailyContentSavingPercent > Net.DailyContentSavingPercent_DataReductionProxyEnabled > Net.DailyContentSavingPercent_ViaDataReductionProxy > > I am not sure if we want to condtion them, but if we do > we should use another histogram, not an existing one. I opened a bug for other histograms: https://bugs.chromium.org/p/chromium/issues/detail?id=609907 Some of the other histograms are conditioned by early returns. It looks like Net.DailyContentSavingPercent_ViaDataReductionProxy is the only other one that is not conditioned.
Description was changed from ========== UMA for data reduction proxy video percentages are being reported when users don't watch video. This change scopes down the reporting to only happen when the user is served video with data reduction proxy enabled and via data reduction proxy for the two respective histograms. BUG=591874 ========== to ========== UMA for data reduction proxy video percentages are being reported when users don't watch video. This change scopes down the reporting to only happen when the user is served video with data reduction proxy enabled and via data reduction proxy for the two respective histograms. BUG=591874 ==========
https://codereview.chromium.org/1956773003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1956773003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:855: "Net.DailyContentSavingPercent_DataReductionProxyEnabled_Video", On 2016/05/06 18:18:12, tbansal1 wrote: > You need to update the description in histograms.xml. Right now, it says ". The > metric is reported when the first response in the current day is received. " How do you want that updated? I'm not sure what exactly you are asking to be changed.
https://codereview.chromium.org/1956773003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1956773003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:855: "Net.DailyContentSavingPercent_DataReductionProxyEnabled_Video", On 2016/05/06 18:41:51, RyanSturm wrote: > On 2016/05/06 18:18:12, tbansal1 wrote: > > You need to update the description in histograms.xml. Right now, it says ". > The > > metric is reported when the first response in the current day is received. " > > How do you want that updated? I'm not sure what exactly you are asking to be > changed. Mention the additional condition. Say the metric is recorded only if at least non-zero number of video bytes were received by Chromium during the previous calendar day when the data saver was enabled.
https://codereview.chromium.org/1956773003/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1956773003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:855: "Net.DailyContentSavingPercent_DataReductionProxyEnabled_Video", On 2016/05/06 18:49:26, tbansal1 wrote: > On 2016/05/06 18:41:51, RyanSturm wrote: > > On 2016/05/06 18:18:12, tbansal1 wrote: > > > You need to update the description in histograms.xml. Right now, it says ". > > The > > > metric is reported when the first response in the current day is received. " > > > > How do you want that updated? I'm not sure what exactly you are asking to be > > changed. > > Mention the additional condition. Say the metric is recorded only if at least > non-zero number of video bytes were received by Chromium during the previous > calendar day when the data saver was enabled. Done. https://codereview.chromium.org/1956773003/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:907: "Net.DailyContentSavingPercent_ViaDataReductionProxy_Video", On 2016/05/06 18:18:12, tbansal1 wrote: > Same here, update histograms.xml. Done. https://codereview.chromium.org/1956773003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1956773003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:182: received_length_via_data_reduction_proxy >> 10); add linebreak back
lgtm https://codereview.chromium.org/1956773003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1956773003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:182: received_length_via_data_reduction_proxy >> 10); On 2016/05/06 21:06:33, RyanSturm wrote: > add linebreak back Acknowledged. https://codereview.chromium.org/1956773003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1956773003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:89983: - <affected-histogram Interesting. So, this file mentions all these histograms (e.g. Net.DailyContentLength_ViaDataReductionProxy_Application) but Chromium never reported those.
https://codereview.chromium.org/1956773003/diff/40001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc (right): https://codereview.chromium.org/1956773003/diff/40001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_compression_stats.cc:182: received_length_via_data_reduction_proxy >> 10); On 2016/05/06 21:06:33, RyanSturm wrote: > add linebreak back Done.
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org Link to the patchset: https://codereview.chromium.org/1956773003/#ps60001 (title: "nit fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956773003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956773003/60001
tbansal@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine for histograms.xml (The CQ will fail soon).
The CQ bit was unchecked by ryansturm@chromium.org
On 2016/05/06 21:16:52, tbansal1 wrote: > +asvitkine for histograms.xml > (The CQ will fail soon). PTAL: asvitkine Completely forgot that I just modified histograms.xml
https://codereview.chromium.org/1956773003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1956773003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25809: +<histogram name="Net.DailyContentSavingPercent_ViaDataReductionProxy_Video" Why split these out? Is it because you don't want the other suffixes? If so, I'd just create a new histogram_suffixes group with just Video as the single suffix.
https://codereview.chromium.org/1956773003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1956773003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25809: +<histogram name="Net.DailyContentSavingPercent_ViaDataReductionProxy_Video" On 2016/05/06 21:37:03, Alexei Svitkine wrote: > Why split these out? Is it because you don't want the other suffixes? If so, I'd > just create a new histogram_suffixes group with just Video as the single suffix. I actually want text specific to only these metrics in the summary. "If no video bytes were received via the data reduction proxy" and "If no video bytes were received while the data reduction proxy was enabled" are very specific and necessary to convey the meaning of each metric.
lgtm
The CQ bit was checked by ryansturm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956773003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956773003/60001
Message was sent while issue was closed.
Description was changed from ========== UMA for data reduction proxy video percentages are being reported when users don't watch video. This change scopes down the reporting to only happen when the user is served video with data reduction proxy enabled and via data reduction proxy for the two respective histograms. BUG=591874 ========== to ========== UMA for data reduction proxy video percentages are being reported when users don't watch video. This change scopes down the reporting to only happen when the user is served video with data reduction proxy enabled and via data reduction proxy for the two respective histograms. BUG=591874 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== UMA for data reduction proxy video percentages are being reported when users don't watch video. This change scopes down the reporting to only happen when the user is served video with data reduction proxy enabled and via data reduction proxy for the two respective histograms. BUG=591874 ========== to ========== UMA for data reduction proxy video percentages are being reported when users don't watch video. This change scopes down the reporting to only happen when the user is served video with data reduction proxy enabled and via data reduction proxy for the two respective histograms. BUG=591874 Committed: https://crrev.com/f76fc2bbe58660f61465a6746e8f411d636c7b65 Cr-Commit-Position: refs/heads/master@{#392183} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f76fc2bbe58660f61465a6746e8f411d636c7b65 Cr-Commit-Position: refs/heads/master@{#392183} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
