Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
424943002:
Add UMA for app info dialog (Closed)
DescriptionAdd UMA for app info dialog
Add UMA to track how many users open the app info dialog.
BUG=395501
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288948
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added histograms for the 3 things to track #Patch Set 3 : Changed launch counter to be separated by launch type, and renamed histograms to remove "AppList" #
Total comments: 2
Patch Set 4 : Added enum for dialog launches #
Total comments: 1
Patch Set 5 : Removed INVALID key #
Total comments: 8
Patch Set 6 : Renamed UMA histogram data and removed LaunchType one #
Total comments: 2
Patch Set 7 : Nit #
Messages
Total messages: 25 (0 generated)
jar@chromium.org: Please review changes in tools/metrics/histograms
https://codereview.chromium.org/424943002/diff/1/chrome/browser/ui/app_list/a... File chrome/browser/ui/app_list/app_context_menu.cc (right): https://codereview.chromium.org/424943002/diff/1/chrome/browser/ui/app_list/a... chrome/browser/ui/app_list/app_context_menu.cc:259: UMA_HISTOGRAM_ENUMERATION("Apps.AppListAppInfoDialog", 1, 2); I think you should use UMA_HISTOGRAM_COUNTS for this, as it isn't an enumeration. But would it be useful to break down how often the dialog is shown for different types of things as well as how often it is shown? Maybe we could add this as a count, and also enumerations for the type of the app and for the location.
Added histograms for the 3 metrics to record :) https://codereview.chromium.org/424943002/diff/1/chrome/browser/ui/app_list/a... File chrome/browser/ui/app_list/app_context_menu.cc (right): https://codereview.chromium.org/424943002/diff/1/chrome/browser/ui/app_list/a... chrome/browser/ui/app_list/app_context_menu.cc:259: UMA_HISTOGRAM_ENUMERATION("Apps.AppListAppInfoDialog", 1, 2); On 2014/07/31 06:13:18, benwells wrote: > I think you should use UMA_HISTOGRAM_COUNTS for this, as it isn't an > enumeration. > > But would it be useful to break down how often the dialog is shown for different > types of things as well as how often it is shown? Maybe we could add this as a > count, and also enumerations for the type of the app and for the location. I was using this because it's a common pattern to measure how many users open the dialog at least once, as opposed to how many times every user has opened the dialog. But either way is fine. Changed this to track: - How many times each user has opened the the dialog - What the location of the app is - What the load type of the app is
https://codereview.chromium.org/424943002/diff/40001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_context_menu.cc (right): https://codereview.chromium.org/424943002/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_context_menu.cc:258: UMA_HISTOGRAM_ENUMERATION("Apps.AppInfoDialogLaunched", 1, 2); Could you define the enum now too?
https://codereview.chromium.org/424943002/diff/40001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_context_menu.cc (right): https://codereview.chromium.org/424943002/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_context_menu.cc:258: UMA_HISTOGRAM_ENUMERATION("Apps.AppInfoDialogLaunched", 1, 2); On 2014/08/04 01:41:40, benwells wrote: > Could you define the enum now too? Done :)
https://codereview.chromium.org/424943002/diff/60001/chrome/browser/ui/apps/a... File chrome/browser/ui/apps/app_info_dialog.h (right): https://codereview.chromium.org/424943002/diff/60001/chrome/browser/ui/apps/a... chrome/browser/ui/apps/app_info_dialog.h:16: enum AppInfoDialogLaunchSource { Why is INVALID needed?
The place I was copying this from (AppListEnableSource) has an INVALID key, but I can see that this is not used elsewhere, so it should be safe without it. Done.
lgtm
The CQ bit was checked by sashab@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sashab@chromium.org/424943002/80001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Oops, sorry - jar@, please review :)
asvitkine: PTAL at histograms
https://codereview.chromium.org/424943002/diff/80001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_context_menu.cc (right): https://codereview.chromium.org/424943002/diff/80001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_context_menu.cc:261: AppInfoDialogLaunchSource::NUM_LAUNCH_SOURCES); Since you only have one launch source, is this even needed? Since you're already adding histograms that get recorded for each time the function below is called. You can just use the Total Count displayed in the dashboard for those histograms to get the same info. I am okay with adding such a histogram once there's more than one launch sources, but with the current code, I don't think it's necessary since the other histograms should provide enough info. https://codereview.chromium.org/424943002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/424943002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:421: +<histogram name="Apps.AppInfoDialogLaunched" enum="AppInfoDialogLaunchSource"> For these histograms, I suggest naming them Apps.AppInfoDialog.Launched, Apps.AppInfoDialog.OpenedForLocation, etc. This way, you get better grouping on the dashboards. https://codereview.chromium.org/424943002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:432: + <summary>The location of the app that the dialog was opened for.</summary> Mention that it's logged every time the app info dialog is opened. https://codereview.chromium.org/424943002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:437: + <summary>The type of the app that the dialog was opened for.</summary> Mention that it's logged every time the app info dialog is opened.
Thanks! :) https://codereview.chromium.org/424943002/diff/80001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_context_menu.cc (right): https://codereview.chromium.org/424943002/diff/80001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_context_menu.cc:261: AppInfoDialogLaunchSource::NUM_LAUNCH_SOURCES); On 2014/08/06 16:46:44, Alexei Svitkine wrote: > Since you only have one launch source, is this even needed? Since you're already > adding histograms that get recorded for each time the function below is called. > You can just use the Total Count displayed in the dashboard for those histograms > to get the same info. I am okay with adding such a histogram once there's more > than one launch sources, but with the current code, I don't think it's necessary > since the other histograms should provide enough info. Ahh yup; agreed :) Done. https://codereview.chromium.org/424943002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/424943002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:421: +<histogram name="Apps.AppInfoDialogLaunched" enum="AppInfoDialogLaunchSource"> On 2014/08/06 16:46:44, Alexei Svitkine wrote: > For these histograms, I suggest naming them Apps.AppInfoDialog.Launched, > Apps.AppInfoDialog.OpenedForLocation, etc. This way, you get better grouping on > the dashboards. Done. https://codereview.chromium.org/424943002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:432: + <summary>The location of the app that the dialog was opened for.</summary> On 2014/08/06 16:46:44, Alexei Svitkine wrote: > Mention that it's logged every time the app info dialog is opened. Done. https://codereview.chromium.org/424943002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:437: + <summary>The type of the app that the dialog was opened for.</summary> On 2014/08/06 16:46:44, Alexei Svitkine wrote: > Mention that it's logged every time the app info dialog is opened. Done.
LGTM https://codereview.chromium.org/424943002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/424943002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:432: + The type of the app that the dialog was opened for. This is gathered each Nit: Remove extra space after "." (or add one in the other description, to be consistent)
https://codereview.chromium.org/424943002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/424943002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:432: + The type of the app that the dialog was opened for. This is gathered each On 2014/08/11 14:52:20, Alexei Svitkine wrote: > Nit: Remove extra space after "." (or add one in the other description, to be > consistent) Done (removed space).
The CQ bit was checked by sashab@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sashab@chromium.org/424943002/120001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Message was sent while issue was closed.
Change committed as 288948 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
