Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2314273003:
[Offline Pages] Adds duplicate detection metrics. (Closed)
|
Created:
4 years, 3 months ago by dewittj Modified:
4 years, 3 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, asvitkine+watch_chromium.org, dimich+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Adds duplicate detection metrics.
We want to know whether the UI for showing page downloads as starting
is sufficient to let the user know that something is happening. The
hypothesis is that if users get too many duplicate pages then they aren't
noticing that the pages are getting saved on the first button click.
BUG=643748
Committed: https://crrev.com/1e3105ed9096bf45f4ca2d79b4a682adc1d1d9ae
Cr-Commit-Position: refs/heads/master@{#418331}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix the nit and add a clarifying comment. #Patch Set 3 : Fix uninitialized variable #
Total comments: 14
Patch Set 4 : Fix nits and comments. #
Total comments: 5
Patch Set 5 : Move to CUSTOM_COUNTS macro instead of CUSTOM_TIMES. #Patch Set 6 : rebase #
Messages
Total messages: 45 (26 generated)
Description was changed from ========== [Offline Pages] Adds duplicate detection metrics. We want to know whether the UI for showing page downloads as starting is sufficient to let the user know that something is happening. The hypothesis is that if users get too many duplicate pages then they aren't noticing that the pages are getting saved on the first button click. BUG=643748 ========== to ========== [Offline Pages] Adds duplicate detection metrics. We want to know whether the UI for showing page downloads as starting is sufficient to let the user know that something is happening. The hypothesis is that if users get too many duplicate pages then they aren't noticing that the pages are getting saved on the first button click. BUG=643748 ==========
dewittj@chromium.org changed reviewers: + fgorski@chromium.org
ptal, thanks!
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2314273003/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2314273003/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model_impl.cc:170: count++; shouldn't this be incremented when you have a matching page only? if so can you replace matching_page with count > 0? https://codereview.chromium.org/2314273003/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model_impl.cc:235: if (iter == offline_pages.end()) { nit: {} not needed.
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
https://codereview.chromium.org/2314273003/diff/1/components/offline_pages/of... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2314273003/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model_impl.cc:170: count++; On 2016/09/07 22:08:35, fgorski wrote: > shouldn't this be incremented when you have a matching page only? In the case of saving then it's equivalent. But if we are deleting a page, we want the count for all pages saved before and after. https://codereview.chromium.org/2314273003/diff/1/components/offline_pages/of... components/offline_pages/offline_page_model_impl.cc:235: if (iter == offline_pages.end()) { On 2016/09/07 22:08:35, fgorski wrote: > nit: {} not needed. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal, thanks!
lgtm
dewittj@chromium.org changed reviewers: + mpearson@chromium.org
mpearson: PTAL at histograms, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2314273003/diff/40001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2314273003/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:222: UMA_HISTOGRAM_CUSTOM_TIMES( Do you really care about millisecond accuracy? You may want to consider making the minimum bucket one second. Also, please play around with about:histograms with various delays to investigate the bucketing. I'm surprised you need 200 buckets. https://codereview.chromium.org/2314273003/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:228: UMA_HISTOGRAM_COUNTS_100("OfflinePages.DownloadSavedPageDuplicateCount", Do you really think that users will have up to a 100 copies of a page? I wonder if something like a max of 50 with fewer buckets (say, 20) is more appropriate. https://codereview.chromium.org/2314273003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2314273003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37741: + How many downloaded pages with the same URL exist at the time that we delete nit: How many -> The number of (sounds less like a question) ditto below https://codereview.chromium.org/2314273003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37741: + How many downloaded pages with the same URL exist at the time that we delete nit: exist -> that exist ditto below https://codereview.chromium.org/2314273003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37742: + a new downloaded page. The page that is being deleted is counted in this nit: omit "new" -- I don't think it adds anything here. Furthermore, I don't think you're restricting this metric to only be emitted when a user deletes downloaded pages that are downloaded recently (new). ditto below https://codereview.chromium.org/2314273003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37743: + metric because the minimum value of a histogram bucket is "1". The "because" part of this statement is wrong. Every histogram includes an "underflow" bucket to capture emits of 0. If you want to include the page being deleted in this metric, you can, but it's not necessary. For what it's worth, I think this metric would make more sense if you don't. https://codereview.chromium.org/2314273003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37756: +<histogram name="OfflinePages.DownloadSavedPageTimeSinceDuplicateSaved"> units="ms"?
thanks, ptal https://codereview.chromium.org/2314273003/diff/40001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2314273003/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:222: UMA_HISTOGRAM_CUSTOM_TIMES( On 2016/09/09 18:41:05, Mark P wrote: > Do you really care about millisecond accuracy? > You may want to consider making the minimum bucket one second. > > Also, please play around with about:histograms with various delays to > investigate the bucketing. I'm surprised you need 200 buckets. I wanted reasonable resolution within a few minutes despite using millisecond accuracy. I guess our actions would be the same if there were too many within 1 second or too many within 700 ms so second accuracy is fine. Then I can reduce the bucket count as well. https://codereview.chromium.org/2314273003/diff/40001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:228: UMA_HISTOGRAM_COUNTS_100("OfflinePages.DownloadSavedPageDuplicateCount", On 2016/09/09 18:41:04, Mark P wrote: > Do you really think that users will have up to a 100 copies of a page? I wonder > if something like a max of 50 with fewer buckets (say, 20) is more appropriate. Done. https://codereview.chromium.org/2314273003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2314273003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37741: + How many downloaded pages with the same URL exist at the time that we delete On 2016/09/09 18:41:05, Mark P wrote: > nit: How many -> The number of > (sounds less like a question) > > ditto below Done. https://codereview.chromium.org/2314273003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37741: + How many downloaded pages with the same URL exist at the time that we delete On 2016/09/09 18:41:05, Mark P wrote: > nit: exist -> that exist > ditto below Done. https://codereview.chromium.org/2314273003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37742: + a new downloaded page. The page that is being deleted is counted in this On 2016/09/09 18:41:05, Mark P wrote: > nit: omit "new" -- I don't think it adds anything here. Furthermore, I don't > think you're restricting this metric to only be emitted when a user deletes > downloaded pages that are downloaded recently (new). > ditto below Done. https://codereview.chromium.org/2314273003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37743: + metric because the minimum value of a histogram bucket is "1". On 2016/09/09 18:41:05, Mark P wrote: > The "because" part of this statement is wrong. Every histogram includes an > "underflow" bucket to capture emits of 0. If you want to include the page being > deleted in this metric, you can, but it's not necessary. For what it's worth, I > think this metric would make more sense if you don't. Done. https://codereview.chromium.org/2314273003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37756: +<histogram name="OfflinePages.DownloadSavedPageTimeSinceDuplicateSaved"> On 2016/09/09 18:41:05, Mark P wrote: > units="ms"? Done.
https://codereview.chromium.org/2314273003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2314273003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37742: + we delete a downloaded page. On 2016/09/12 18:22:50, dewittj wrote: > On 2016/09/09 18:41:05, Mark P wrote: > > The "because" part of this statement is wrong. Every histogram includes an > > "underflow" bucket to capture emits of 0. If you want to include the page > being > > deleted in this metric, you can, but it's not necessary. For what it's worth, > I > > think this metric would make more sense if you don't. > > Done. You corrected the description here to remove the the statement "The page that is being deleted is counted in this metric." yet I don't see a code change elsewhere in this patchset. https://codereview.chromium.org/2314273003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37756: + units="seconds"> On 2016/09/12 18:22:50, dewittj wrote: > On 2016/09/09 18:41:05, Mark P wrote: > > units="ms"? > > Done. No, I think it should be seconds. You're still using histogram times, which emits in seconds. You only changed your minimum bucket to be 1 second (=1000ms).
https://codereview.chromium.org/2314273003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2314273003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37742: + we delete a downloaded page. On 2016/09/12 18:32:09, Mark P wrote: > On 2016/09/12 18:22:50, dewittj wrote: > > On 2016/09/09 18:41:05, Mark P wrote: > > > The "because" part of this statement is wrong. Every histogram includes an > > > "underflow" bucket to capture emits of 0. If you want to include the page > > being > > > deleted in this metric, you can, but it's not necessary. For what it's > worth, > > I > > > think this metric would make more sense if you don't. > > > > Done. > > You corrected the description here to remove the the statement "The page that is > being deleted is counted in this metric." yet I don't see a code change > elsewhere in this patchset. When I looked back at the code I found that the description was just wrong, must have been a product of iteration on the metric before review. https://codereview.chromium.org/2314273003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37756: + units="seconds"> On 2016/09/12 18:32:09, Mark P wrote: > On 2016/09/12 18:22:50, dewittj wrote: > > On 2016/09/09 18:41:05, Mark P wrote: > > > units="ms"? > > > > Done. > > No, I think it should be seconds. You're still using histogram times, which > emits in seconds. You only changed your minimum bucket to be 1 second > (=1000ms). Isn't it now in seconds?
https://codereview.chromium.org/2314273003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2314273003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:37756: + units="seconds"> On 2016/09/12 18:36:00, dewittj wrote: > On 2016/09/12 18:32:09, Mark P wrote: > > On 2016/09/12 18:22:50, dewittj wrote: > > > On 2016/09/09 18:41:05, Mark P wrote: > > > > units="ms"? > > > > > > Done. > > > > No, I think it should be seconds. You're still using histogram times, which > > emits in seconds. You only changed your minimum bucket to be 1 second > > (=1000ms). > > Isn't it now in seconds? No, you're using the UMA_HISTOGRAM_CUSTOM_TIMES, which, if you dig into it, calls AddTime(sample), which emits an entry with the time in milliseconds. https://cs.chromium.org/chromium/src/base/metrics/histogram_base.cc?dr=C&sq=p... Your previous revisions simply changed the minimum and maximum bucket values and bucket count.
okay, so it seems like all the time-oriented ones assume samples in millis. So should I use a CUSTOM_COUNTS histogram instead?
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
I moved to CUSTOM_COUNTS instead, wdyt about that?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This histogram use lgtm to me. (The previous version of the code was fine too; you just needed the right units for the histograms.xml display.) --mark
The CQ bit was checked by dewittj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2314273003/#ps80001 (title: "Move to CUSTOM_COUNTS macro instead of CUSTOM_TIMES.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by dewittj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2314273003/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Adds duplicate detection metrics. We want to know whether the UI for showing page downloads as starting is sufficient to let the user know that something is happening. The hypothesis is that if users get too many duplicate pages then they aren't noticing that the pages are getting saved on the first button click. BUG=643748 ========== to ========== [Offline Pages] Adds duplicate detection metrics. We want to know whether the UI for showing page downloads as starting is sufficient to let the user know that something is happening. The hypothesis is that if users get too many duplicate pages then they aren't noticing that the pages are getting saved on the first button click. BUG=643748 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Adds duplicate detection metrics. We want to know whether the UI for showing page downloads as starting is sufficient to let the user know that something is happening. The hypothesis is that if users get too many duplicate pages then they aren't noticing that the pages are getting saved on the first button click. BUG=643748 ========== to ========== [Offline Pages] Adds duplicate detection metrics. We want to know whether the UI for showing page downloads as starting is sufficient to let the user know that something is happening. The hypothesis is that if users get too many duplicate pages then they aren't noticing that the pages are getting saved on the first button click. BUG=643748 Committed: https://crrev.com/1e3105ed9096bf45f4ca2d79b4a682adc1d1d9ae Cr-Commit-Position: refs/heads/master@{#418331} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1e3105ed9096bf45f4ca2d79b4a682adc1d1d9ae Cr-Commit-Position: refs/heads/master@{#418331} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
