Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1849673002:
Add metrics for user manually added exceptions (Closed)
|
Created:
4 years, 8 months ago by lshang Modified:
4 years, 8 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, asvitkine+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd metrics for user manually added exceptions
This metric is for collecting numbers of exceptions for all content setting types.
BUG=604612
Committed: https://crrev.com/bcbfffbbf5ed955c59e825ebfe92fbff24316aeb
Cr-Commit-Position: refs/heads/master@{#388711}
Patch Set 1 #Patch Set 2 : add metrics for all domain scoped types exceptions #Patch Set 3 : format #
Total comments: 6
Patch Set 4 : rebase #Patch Set 5 : address review comments #Patch Set 6 : fix patch failures #
Total comments: 8
Patch Set 7 : address review comments #Patch Set 8 : remove header file #
Total comments: 11
Patch Set 9 : address review comments #Patch Set 10 : iterate over websiteSettingsInfo and change metrics buckets #
Total comments: 1
Patch Set 11 : merge two same suffixes #
Total comments: 10
Patch Set 12 : address review comments #Patch Set 13 : rebase #
Messages
Total messages: 25 (10 generated)
Description was changed from ========== Add metrics for user manually added exceptions add contentsettingstype enum format histogram.xml format histogram.xml remove previous change in pref provider add metrics add uma for number of exceptions BUG= ========== to ========== Add metrics for user manually added exceptions This metric is for collecting number of times users add manually input exceptions. BUG=551747 ==========
lshang@chromium.org changed reviewers: + raymes@chromium.org
Hi Raymes, please take a look at this cl. I added some ContentSettingsType in histogram.xml, but I'm not sure if we need to list all of them, or get rid of those which don't allow users to manually add exceptions.
Hi Raymes, I added some more metrics. They collect numbers of exceptions of other domain scoped content types. Can you take a look again? Many thanks!
https://codereview.chromium.org/1849673002/diff/40001/components/content_sett... File components/content_settings/core/browser/content_settings_pref.cc (right): https://codereview.chromium.org/1849673002/diff/40001/components/content_sett... components/content_settings/core/browser/content_settings_pref.cc:388: for (ExceptionUmaStats stats : ContentSettingExceptionUmaMap) { Can we move this code higher up the stack? Perhaps into the HostContentSettingsMap constructor? https://codereview.chromium.org/1849673002/diff/40001/components/content_sett... components/content_settings/core/browser/content_settings_pref.cc:388: for (ExceptionUmaStats stats : ContentSettingExceptionUmaMap) { I think that perhaps we can just measure all types in ContentSettingsRegistry. We can generate the histogram name based on the name of the content setting. https://codereview.chromium.org/1849673002/diff/40001/components/content_sett... components/content_settings/core/browser/content_settings_pref.cc:396: ++stats.block_exception_count; I think we should probably only worry about allow/block (or possibly even just measure the total number as a starting point).
I moved the metrics to HCSM and collect uma data for all content types. Could you PTAL? Thanks! https://codereview.chromium.org/1849673002/diff/40001/components/content_sett... File components/content_settings/core/browser/content_settings_pref.cc (right): https://codereview.chromium.org/1849673002/diff/40001/components/content_sett... components/content_settings/core/browser/content_settings_pref.cc:388: for (ExceptionUmaStats stats : ContentSettingExceptionUmaMap) { On 2016/04/07 04:57:59, raymes wrote: > Can we move this code higher up the stack? Perhaps into the > HostContentSettingsMap constructor? Done. https://codereview.chromium.org/1849673002/diff/40001/components/content_sett... components/content_settings/core/browser/content_settings_pref.cc:388: for (ExceptionUmaStats stats : ContentSettingExceptionUmaMap) { On 2016/04/07 04:57:59, raymes wrote: > I think that perhaps we can just measure all types in ContentSettingsRegistry. > We can generate the histogram name based on the name of the content setting. Done. https://codereview.chromium.org/1849673002/diff/40001/components/content_sett... components/content_settings/core/browser/content_settings_pref.cc:396: ++stats.block_exception_count; On 2016/04/07 04:57:59, raymes wrote: > I think we should probably only worry about allow/block (or possibly even just > measure the total number as a starting point). Done.
Thanks Liu! https://codereview.chromium.org/1849673002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/1849673002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:1540: histogram_value, num_values); Do you think this histogram will be useful? https://codereview.chromium.org/1849673002/diff/100001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1849673002/diff/100001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:497: for (const content_settings::ContentSettingsInfo* info : *registry) { If we're just going to record number of exceptions, we might as well do it for all types in the WebsiteSettingsRegistry :) https://codereview.chromium.org/1849673002/diff/100001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:508: UMA_HISTOGRAM_COUNTS("ContentSettings.NumberOf" + type_name + "Exceptions", I think a more standard naming convention would be something like: ContentSettings.NumberOfExceptions.<type> https://codereview.chromium.org/1849673002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1849673002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5935: +</histogram> We can just have one histogram entry for these and say that it is suffixed with the content settings type name. See Settings.JsonDataWriteCount for an example.
Hi Raymes, I add website settings and use dynamically suffixed histogram. PTAL thanks! https://codereview.chromium.org/1849673002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/content_settings_handler.cc (right): https://codereview.chromium.org/1849673002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/content_settings_handler.cc:1540: histogram_value, num_values); On 2016/04/12 23:22:39, raymes wrote: > Do you think this histogram will be useful? Done. https://codereview.chromium.org/1849673002/diff/100001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1849673002/diff/100001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:497: for (const content_settings::ContentSettingsInfo* info : *registry) { On 2016/04/12 23:22:39, raymes wrote: > If we're just going to record number of exceptions, we might as well do it for > all types in the WebsiteSettingsRegistry :) Done. https://codereview.chromium.org/1849673002/diff/100001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:508: UMA_HISTOGRAM_COUNTS("ContentSettings.NumberOf" + type_name + "Exceptions", On 2016/04/12 23:22:39, raymes wrote: > I think a more standard naming convention would be something like: > ContentSettings.NumberOfExceptions.<type> Done. https://codereview.chromium.org/1849673002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1849673002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5935: +</histogram> On 2016/04/12 23:22:39, raymes wrote: > We can just have one histogram entry for these and say that it is suffixed with > the content settings type name. See Settings.JsonDataWriteCount for an example. Done.
https://codereview.chromium.org/1849673002/diff/140001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1849673002/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:498: info->website_settings_info()->name(); I think we can just iterate through WebsiteSettings as we chatted about. Then you could get rid of the map too. https://codereview.chromium.org/1849673002/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:517: histogram_name, 0, 1000, 1001, nit: I think the min should be 1. I'm not sure how many buckets we should have. 1000 might be too many. Maybe 100? Histogram folks will be able to help us. We need to have granularity at the low levels (e.g. 1, 2, 3, 4, 5) but not at the high levels (100, 200, 300). https://codereview.chromium.org/1849673002/diff/140001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1849673002/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:309: // Collect uma data of number of exceptions. nit: Collect UMA data about the number of exceptions. https://codereview.chromium.org/1849673002/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:310: void CollectExceptionNumberUma(); nit: RecordNumberOfExceptions https://codereview.chromium.org/1849673002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1849673002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:87523: <histogram_suffixes name="ContentSettingsTypes"> We might want to rename this to PermissionTypes. https://codereview.chromium.org/1849673002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1849673002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:87531: +<histogram_suffixes name="ContentSettingsType" separator="."> We may want to consider reconciling this with the below suffixes later on but I don't see a good alternative to adding this for the moment :)
Hi Raymes, can you take a look at the new change? Thanks! https://codereview.chromium.org/1849673002/diff/140001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1849673002/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:498: info->website_settings_info()->name(); On 2016/04/14 06:21:59, raymes wrote: > I think we can just iterate through WebsiteSettings as we chatted about. Then > you could get rid of the map too. Done. https://codereview.chromium.org/1849673002/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:517: histogram_name, 0, 1000, 1001, On 2016/04/14 06:21:59, raymes wrote: > nit: I think the min should be 1. I'm not sure how many buckets we should have. > 1000 might be too many. Maybe 100? Histogram folks will be able to help us. We > need to have granularity at the low levels (e.g. 1, 2, 3, 4, 5) but not at the > high levels (100, 200, 300). Done. I also changed the name to 'ContentSettings.NumberOfSingleTypeExceptions.' because there already is a 'NumberOfExceptions' which includes exceptions of all types. https://codereview.chromium.org/1849673002/diff/140001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.h (right): https://codereview.chromium.org/1849673002/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:309: // Collect uma data of number of exceptions. On 2016/04/14 06:21:59, raymes wrote: > nit: Collect UMA data about the number of exceptions. Done. https://codereview.chromium.org/1849673002/diff/140001/components/content_set... components/content_settings/core/browser/host_content_settings_map.h:310: void CollectExceptionNumberUma(); On 2016/04/14 06:21:59, raymes wrote: > nit: RecordNumberOfExceptions Done. https://codereview.chromium.org/1849673002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1849673002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:87523: <histogram_suffixes name="ContentSettingsTypes"> On 2016/04/14 06:21:59, raymes wrote: > We might want to rename this to PermissionTypes. When I change this to be PermissionTypes, I found that there is another suffix called 'PermissionTypes' which seems to do similar things.
lgtm https://codereview.chromium.org/1849673002/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1849673002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:91483: +</histogram_suffixes> I think we can merge this with the one below
Description was changed from ========== Add metrics for user manually added exceptions This metric is for collecting number of times users add manually input exceptions. BUG=551747 ========== to ========== Add metrics for user manually added exceptions This metric is for collecting numbers of exceptions for all content setting types. BUG=551747 ==========
Description was changed from ========== Add metrics for user manually added exceptions This metric is for collecting numbers of exceptions for all content setting types. BUG=551747 ========== to ========== Add metrics for user manually added exceptions This metric is for collecting numbers of exceptions for all content setting types. BUG=604612 ==========
lshang@chromium.org changed reviewers: + isherman@chromium.org
+isherman for OWNER's approval, thanks!
Thanks. LGTM % nits: https://codereview.chromium.org/1849673002/diff/200001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1849673002/diff/200001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:13: #include "base/metrics/histogram_macros.h" nit: Since you're not using the macros, please include base/metrics/histogram.h instead. https://codereview.chromium.org/1849673002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1849673002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:91458: </histogram_suffixes> Hmm, the changes to this suffix list seem unrelated to the rest of the CL. Am I missing the connection? If I'm not, please split this off to a separate CL, for clarity of code change history. https://codereview.chromium.org/1849673002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1849673002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5968: +<histogram name="ContentSettings.NumberOfSingleTypeExceptions"> nit: I'd name this histogram "ContentSettings.Exceptions", so that you'd end up with histogram names like "ContentSettings.Exceptions.Cookies". https://codereview.chromium.org/1849673002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:88132: + <suffix name="cookies" label="Cookies exceptions"/> Optional nit: It would be nice if the histogram name suffixes were written in CamelCase rather than in lower-dashed-case, but it's not super important. (So, I'd probably recommend making this change if it's easy to implement in the C++ code, or not if it's not.) https://codereview.chromium.org/1849673002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:88158: + <suffix name="usb-chooser-data" label="USB chooser data exceptions"/> nit: Please alphabetize this list of suffixes.
https://codereview.chromium.org/1849673002/diff/200001/components/content_set... File components/content_settings/core/browser/host_content_settings_map.cc (right): https://codereview.chromium.org/1849673002/diff/200001/components/content_set... components/content_settings/core/browser/host_content_settings_map.cc:13: #include "base/metrics/histogram_macros.h" On 2016/04/20 00:15:42, Ilya Sherman wrote: > nit: Since you're not using the macros, please include base/metrics/histogram.h > instead. Done. Thanks for reminding! https://codereview.chromium.org/1849673002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1849673002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:91458: </histogram_suffixes> On 2016/04/20 00:15:42, Ilya Sherman wrote: > Hmm, the changes to this suffix list seem unrelated to the rest of the CL. Am I > missing the connection? If I'm not, please split this off to a separate CL, for > clarity of code change history. Done. The moved suffix list should use PermissionTypes instead of ContentSettingsTypes, and I merged these two suffix entries because they seem to do the same thing. But yeah this change is not strongly related to this CL, so I can move them to a new CL.:-) https://codereview.chromium.org/1849673002/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1849673002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:5968: +<histogram name="ContentSettings.NumberOfSingleTypeExceptions"> On 2016/04/20 00:15:42, Ilya Sherman wrote: > nit: I'd name this histogram "ContentSettings.Exceptions", so that you'd end up > with histogram names like "ContentSettings.Exceptions.Cookies". Done. https://codereview.chromium.org/1849673002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:88132: + <suffix name="cookies" label="Cookies exceptions"/> On 2016/04/20 00:15:42, Ilya Sherman wrote: > Optional nit: It would be nice if the histogram name suffixes were written in > CamelCase rather than in lower-dashed-case, but it's not super important. (So, > I'd probably recommend making this change if it's easy to implement in the C++ > code, or not if it's not.) I didn't find places in C++ code that can directly get ContentSettingsType's CamelCase name. So I think we might just leave them here:-) But I will pay attention of the name format in the future~ Thanks for the tip! https://codereview.chromium.org/1849673002/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:88158: + <suffix name="usb-chooser-data" label="USB chooser data exceptions"/> On 2016/04/20 00:15:42, Ilya Sherman wrote: > nit: Please alphabetize this list of suffixes. Done.
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1849673002/#ps240001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1849673002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1849673002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was unchecked by commit-bot@chromium.org
Message was sent while issue was closed.
Description was changed from ========== Add metrics for user manually added exceptions This metric is for collecting numbers of exceptions for all content setting types. BUG=604612 ========== to ========== Add metrics for user manually added exceptions This metric is for collecting numbers of exceptions for all content setting types. BUG=604612 Committed: https://crrev.com/bcbfffbbf5ed955c59e825ebfe92fbff24316aeb Cr-Commit-Position: refs/heads/master@{#388711} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/bcbfffbbf5ed955c59e825ebfe92fbff24316aeb Cr-Commit-Position: refs/heads/master@{#388711} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
