Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1967793002:
Add some UMA for recent tabs (Closed)
|
Created:
4 years, 7 months ago by bburns Modified:
4 years, 7 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, petewil+watch_chromium.org, dewittj+watch_chromium.org, asvitkine+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd some UMA for recent tabs
BUG=608112
Committed: https://crrev.com/34837a8fb19d4c384db7644cd8d3cdc356978d00
Cr-Commit-Position: refs/heads/master@{#393148}
Patch Set 1 #
Total comments: 2
Patch Set 2 : address changes #
Total comments: 5
Patch Set 3 : address comments #Patch Set 4 : address comments #
Total comments: 2
Patch Set 5 : address comments #
Total comments: 6
Patch Set 6 : address comments. #Patch Set 7 : address comments. #
Total comments: 1
Patch Set 8 : address comments #Patch Set 9 : address comments. #
Messages
Total messages: 35 (11 generated)
bburns@chromium.org changed reviewers: + dimich@chromium.org, fgorski@chromium.org, jianli@chromium.org
Please take a look. thanks! --brendan
drive-by! https://codereview.chromium.org/1967793002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/1967793002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.cc:186: static_cast<int>(OfflinePageModel::SavePageResult::RESULT_COUNT)); I don't think we want to reuse this histogram here, since it's already being saved in offline_page_model.cc We'll probably want to get separate histograms for caching bookmarks and caching recent tabs, so let's make a new histogram here.
comment addressed ptal. thanks! --brendan https://codereview.chromium.org/1967793002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/1967793002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/recent_tab_helper.cc:186: static_cast<int>(OfflinePageModel::SavePageResult::RESULT_COUNT)); On 2016/05/10 21:20:36, dewittj wrote: > I don't think we want to reuse this histogram here, since it's already being > saved in offline_page_model.cc > > We'll probably want to get separate histograms for caching bookmarks and caching > recent tabs, so let's make a new histogram here. Done.
dewittj@chromium.org changed reviewers: + dewittj@chromium.org
https://codereview.chromium.org/1967793002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/1967793002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:96: UMA_HISTOGRAM_COUNTS("OfflinePages.CanNotSaveRecentPage", 1); I wonder if we should have an enumerated histogram that shows all the various ways that recent tab helper fails: - !CanSavePage - DeletePageResult is not SUCCESS or NOT_FOUND - !IsSamePage
comment addressed. https://codereview.chromium.org/1967793002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/1967793002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:96: UMA_HISTOGRAM_COUNTS("OfflinePages.CanNotSaveRecentPage", 1); On 2016/05/10 23:07:56, dewittj wrote: > I wonder if we should have an enumerated histogram that shows all the various > ways that recent tab helper fails: > - !CanSavePage > - DeletePageResult is not SUCCESS or NOT_FOUND > - !IsSamePage I don't feel strongly. Is this detailed in a PRD somewhere? I guess if I were chosing, I would have different histograms based on the function call rather than one with multiple failure modes, but I don't super care.
https://codereview.chromium.org/1967793002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/1967793002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:96: UMA_HISTOGRAM_COUNTS("OfflinePages.CanNotSaveRecentPage", 1); On 2016/05/11 16:21:52, bburns wrote: > On 2016/05/10 23:07:56, dewittj wrote: > > I wonder if we should have an enumerated histogram that shows all the various > > ways that recent tab helper fails: > > - !CanSavePage > > - DeletePageResult is not SUCCESS or NOT_FOUND > > - !IsSamePage > > I don't feel strongly. Is this detailed in a PRD somewhere? I guess if I were > chosing, I would have different histograms based on the function call rather > than one with multiple failure modes, but I don't super care. nope, not in the PRD. I don't super care about the shape either, but it seems easier to compare the frequency of all types if we record them all together. Either way, this metric is not correctly implemented. UMA_HISTOGRAM_COUNTS defines a histogram with 50 buckets ranging from 1 to 1e6, and this always records the value '1'. So either you want UMA_HISTOGRAM_BOOLEAN("OfflinePages.CanSaveRecentPage", page_model_->CanSavePage(url)) or UMA_HISTOGRAM_ENUMERATION with all the different problems as I commented before.
https://codereview.chromium.org/1967793002/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/1967793002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:96: UMA_HISTOGRAM_COUNTS("OfflinePages.CanNotSaveRecentPage", 1); On 2016/05/11 16:31:58, dewittj wrote: > On 2016/05/11 16:21:52, bburns wrote: > > On 2016/05/10 23:07:56, dewittj wrote: > > > I wonder if we should have an enumerated histogram that shows all the > various > > > ways that recent tab helper fails: > > > - !CanSavePage > > > - DeletePageResult is not SUCCESS or NOT_FOUND > > > - !IsSamePage > > > > I don't feel strongly. Is this detailed in a PRD somewhere? I guess if I were > > chosing, I would have different histograms based on the function call rather > > than one with multiple failure modes, but I don't super care. > > nope, not in the PRD. I don't super care about the shape either, but it seems > easier to compare the frequency of all types if we record them all together. > > Either way, this metric is not correctly implemented. UMA_HISTOGRAM_COUNTS > defines a histogram with 50 buckets ranging from 1 to 1e6, and this always > records the value '1'. So either you want > UMA_HISTOGRAM_BOOLEAN("OfflinePages.CanSaveRecentPage", > page_model_->CanSavePage(url)) > > or UMA_HISTOGRAM_ENUMERATION with all the different problems as I commented > before. Boolean sounds good for this one, as this is a proactive check and isn't a result of a failure to save. https://codereview.chromium.org/1967793002/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:185: "OfflinePages.SavePageResult.RecentPage", static_cast<int>(result), I don't think this is needed in this shape and form. Please look for email from Jian about adding suffixes. This UMA will be reported in the model, with the right suffix, based on the ClientID.namespace.
On 2016/05/11 16:54:23, fgorski wrote: > https://codereview.chromium.org/1967793002/diff/20001/chrome/browser/android/... > File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): > > https://codereview.chromium.org/1967793002/diff/20001/chrome/browser/android/... > chrome/browser/android/offline_pages/recent_tab_helper.cc:96: > UMA_HISTOGRAM_COUNTS("OfflinePages.CanNotSaveRecentPage", 1); > On 2016/05/11 16:31:58, dewittj wrote: > > On 2016/05/11 16:21:52, bburns wrote: > > > On 2016/05/10 23:07:56, dewittj wrote: > > > > I wonder if we should have an enumerated histogram that shows all the > > various > > > > ways that recent tab helper fails: > > > > - !CanSavePage > > > > - DeletePageResult is not SUCCESS or NOT_FOUND > > > > - !IsSamePage > > > > > > I don't feel strongly. Is this detailed in a PRD somewhere? I guess if I > were > > > chosing, I would have different histograms based on the function call rather > > > than one with multiple failure modes, but I don't super care. > > > > nope, not in the PRD. I don't super care about the shape either, but it seems > > easier to compare the frequency of all types if we record them all together. > > > > Either way, this metric is not correctly implemented. UMA_HISTOGRAM_COUNTS > > defines a histogram with 50 buckets ranging from 1 to 1e6, and this always > > records the value '1'. So either you want > > UMA_HISTOGRAM_BOOLEAN("OfflinePages.CanSaveRecentPage", > > page_model_->CanSavePage(url)) > > > > or UMA_HISTOGRAM_ENUMERATION with all the different problems as I commented > > before. > > Boolean sounds good for this one, as this is a proactive check and isn't a > result of a failure to save. > > https://codereview.chromium.org/1967793002/diff/20001/chrome/browser/android/... > chrome/browser/android/offline_pages/recent_tab_helper.cc:185: > "OfflinePages.SavePageResult.RecentPage", static_cast<int>(result), > I don't think this is needed in this shape and form. > Please look for email from Jian about adding suffixes. > > This UMA will be reported in the model, with the right suffix, based on the > ClientID.namespace. Ok, I removed the SavePageResult metric, staying with Boolean for the other. Please take another look.
https://codereview.chromium.org/1967793002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/1967793002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:96: UMA_HISTOGRAM_COUNTS("OfflinePages.CanNotSaveRecentPage", 1); Did you upload your latest code? This is still not a boolean.
https://codereview.chromium.org/1967793002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/1967793002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:96: UMA_HISTOGRAM_COUNTS("OfflinePages.CanNotSaveRecentPage", 1); On 2016/05/11 18:15:46, dewittj wrote: > Did you upload your latest code? This is still not a boolean. oops, it's there now.
https://codereview.chromium.org/1967793002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/1967793002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:96: UMA_HISTOGRAM_BOOLEAN("OfflinePages.CanNotSaveRecentPage", true); ok, I guess this is turning into a SQL patch after all. I think boolean histogram is the right choice, but I also think you should report it outside (before) you hit the if. That way you will compare can save vs. cannot save. The way the code is written now, we only ever report true. (and yes, the counts are better for that) https://codereview.chromium.org/1967793002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1967793002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34087: +<histogram name="OfflinePages.CanNotSaveRecentPage"> add: enum="Boolean" https://codereview.chromium.org/1967793002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34089: + <summary>Number of recent offline pages that couldn't be saved</summary> Will this description work for a boolean histogram?
comments addressed. thanks --brendan https://codereview.chromium.org/1967793002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/recent_tab_helper.cc (right): https://codereview.chromium.org/1967793002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/recent_tab_helper.cc:96: UMA_HISTOGRAM_BOOLEAN("OfflinePages.CanNotSaveRecentPage", true); On 2016/05/11 19:34:39, fgorski wrote: > ok, I guess this is turning into a SQL patch after all. > > I think boolean histogram is the right choice, but I also think you should > report it outside (before) you hit the if. That way you will compare can save > vs. cannot save. > > The way the code is written now, we only ever report true. (and yes, the counts > are better for that) Done. https://codereview.chromium.org/1967793002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1967793002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34087: +<histogram name="OfflinePages.CanNotSaveRecentPage"> On 2016/05/11 19:34:39, fgorski wrote: > add: enum="Boolean" Done. https://codereview.chromium.org/1967793002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:34089: + <summary>Number of recent offline pages that couldn't be saved</summary> On 2016/05/11 19:34:39, fgorski wrote: > Will this description work for a boolean histogram? Updated.
lgtm
lgtm
The CQ bit was checked by bburns@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967793002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967793002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
bburns@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine: for histograms.xml OWNERS thanks! --brendan
lgtm https://codereview.chromium.org/1967793002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1967793002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:34089: + <summary>Can a recent page be saved or not?</summary> Nit: Mention when it's logged.
On 2016/05/11 22:34:36, Alexei Svitkine wrote: > lgtm > > https://codereview.chromium.org/1967793002/diff/120001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1967793002/diff/120001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:34089: + <summary>Can a recent page be > saved or not?</summary> > Nit: Mention when it's logged. done. submitting. thanks!
The CQ bit was checked by bburns@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, asvitkine@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/1967793002/#ps140001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967793002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967793002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bburns@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, asvitkine@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/1967793002/#ps160001 (title: "address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967793002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967793002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add some UMA for recent tabs BUG=608112 ========== to ========== Add some UMA for recent tabs BUG=608112 Committed: https://crrev.com/34837a8fb19d4c384db7644cd8d3cdc356978d00 Cr-Commit-Position: refs/heads/master@{#393148} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/34837a8fb19d4c384db7644cd8d3cdc356978d00 Cr-Commit-Position: refs/heads/master@{#393148} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
