Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2739513002:
Settings reset prompt: add UMA metrics reporting. (Closed)
DescriptionSettings reset prompt: add UMA metrics reporting.
BUG=
Review-Url: https://codereview.chromium.org/2739513002
Cr-Commit-Position: refs/heads/master@{#455500}
Committed: https://chromium.googlesource.com/chromium/src/+/cfeba874bf0f2458295ab24268790bf6a80e2d43
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed comments. #
Total comments: 18
Patch Set 3 : Addressed Jesse's comments #Patch Set 4 : Cleanup #
Total comments: 2
Patch Set 5 : More comments... #
Total comments: 4
Patch Set 6 : Adds histograms to log what is reset when prompt is accepted #
Messages
Total messages: 20 (7 generated)
alito@chromium.org changed reviewers: + csharp@chromium.org, jwd@chromium.org, robertshield@chromium.org
Adding UMA reporting to the settings reset prompt. PTAL! /ali
lgtm https://codereview.chromium.org/2739513002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc (right): https://codereview.chromium.org/2739513002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc:34: enum DialogState { Add a comment mentioning this is used for UMA reporting. https://codereview.chromium.org/2739513002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model.h (right): https://codereview.chromium.org/2739513002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model.h:107: // Reports some UMA metrics. nit: redundant comment
lgtm https://codereview.chromium.org/2739513002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc (right): https://codereview.chromium.org/2739513002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc:66: if (model) nit: I wonder if this part would look a bit clearer as: if (!model) return model->ReportUmaMetrics(); if (!model->ShouldPromptForReset()) return (Same number of ifs, but I think a bit clearer) https://codereview.chromium.org/2739513002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2739513002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:64155: +<histogram name="SettingsResetPrompt.ExtensionsToDisable" units="extensions"> nit: ExtensionsToDisable -> NumberOfExtensionsToDisable?
Thanks. All done. https://codereview.chromium.org/2739513002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc (right): https://codereview.chromium.org/2739513002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc:34: enum DialogState { On 2017/03/07 07:43:19, robertshield wrote: > Add a comment mentioning this is used for UMA reporting. Done. https://codereview.chromium.org/2739513002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc:66: if (model) On 2017/03/07 14:39:40, csharp wrote: > nit: I wonder if this part would look a bit clearer as: > if (!model) > return > > model->ReportUmaMetrics(); > > if (!model->ShouldPromptForReset()) > return > > (Same number of ifs, but I think a bit clearer) Done. https://codereview.chromium.org/2739513002/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model.h (right): https://codereview.chromium.org/2739513002/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model.h:107: // Reports some UMA metrics. On 2017/03/07 07:43:19, robertshield wrote: > nit: redundant comment :) indeed. Done. https://codereview.chromium.org/2739513002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2739513002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:64155: +<histogram name="SettingsResetPrompt.ExtensionsToDisable" units="extensions"> On 2017/03/07 14:39:40, csharp wrote: > nit: ExtensionsToDisable -> NumberOfExtensionsToDisable? Done.
https://codereview.chromium.org/2739513002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model.cc (right): https://codereview.chromium.org/2739513002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model.cc:289: UMA_HISTOGRAM_SPARSE_SLOWLY("SettingsResetPrompt.NumberOfExtensionsToDisable", I'm not super keen on this being sparse, since it can have arbitrary values (determined by the client). Although in this case it's not a problem on the client, and will be more memory efficient than a count histogram, when aggregating during analysis, the histogram becomes arbitrarily large, causing problems. Do you need exact numbers for each emit? Using a count histogram will give you an exact sum when aggregating, and the distribution. https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:14338: <action name="SettingsResetPrompt_Shown" not_user_triggered="true"> I'd say omit the not_user_triggered. Having it here would remove it from sequence analysis. I have some regrets about the naming of that property... https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64142: + before attempting to show the settings reset prompt. This is just coming from the variations config, yeah? Why transmit this back? (I'm not saying you shouldn't but it's not clear you need it) https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64151: + how the user interacted with it. This is fine, but many of our tools assume that enum histograms sum to a meaningful value, which is used to compute bucket proportions. Is there a reason you want the shown count, and results grouped into one histogram. I'm fine with you keeping it as is, mostly an FYI. https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64173: +<histogram name="SettingsResetPrompt.PromptWanted" enum="Boolean"> Can you create a more specific enum for this, maybe something like BooleanWanted, with meaningfully labeled enum values. https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64173: +<histogram name="SettingsResetPrompt.PromptWanted" enum="Boolean"> I'm not sure Wanted is the right word here, it implies user preference to me. https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64181: +<histogram name="SettingsResetPrompt.StartupUrlsResetState" I'd change the *ResetState histograms to be reformulated using suffixes. There are instructions at the top of the file, basically you'd add a histogram SettingsResetPrompt.ResetState, with a description that applies to all the cases. Then you'd add suffixes to it (StartupUrls, Homepage), with specific added description. https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:108611: + <int value="4" label="No reset required due to recently prompted"/> I'd recommend slightly shorter labels. Our tools prefer shorter, longer does work though. You can also add a longer description in the body of the <int> tag. I'd suggest changing the labels to "Recently prompted, no reset required" maybe?, so that the more relevant info is first.
Comments addressed. Jesse, PTAnL. https://codereview.chromium.org/2739513002/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model.cc (right): https://codereview.chromium.org/2739513002/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_model.cc:289: UMA_HISTOGRAM_SPARSE_SLOWLY("SettingsResetPrompt.NumberOfExtensionsToDisable", On 2017/03/07 18:08:37, jwd wrote: > I'm not super keen on this being sparse, since it can have arbitrary values > (determined by the client). Although in this case it's not a problem on the > client, and will be more memory efficient than a count histogram, when > aggregating during analysis, the histogram becomes arbitrarily large, causing > problems. > > Do you need exact numbers for each emit? Using a count histogram will give you > an exact sum when aggregating, and the distribution. Changed this to a count histogram. I think that will work just as well for this case. Thx. https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:14338: <action name="SettingsResetPrompt_Shown" not_user_triggered="true"> On 2017/03/07 18:08:37, jwd wrote: > I'd say omit the not_user_triggered. Having it here would remove it from > sequence analysis. I have some regrets about the naming of that property... Done. https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64142: + before attempting to show the settings reset prompt. On 2017/03/07 18:08:38, jwd wrote: > This is just coming from the variations config, yeah? Why transmit this back? > (I'm not saying you shouldn't but it's not clear you need it) This is in case we decide to experiment with the delay to see if it makes a difference in the number of users who accept the prompt. We could then easily separate users based on the value of this histogram. Let me know if there is a better way of supporting this use case. https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64173: +<histogram name="SettingsResetPrompt.PromptWanted" enum="Boolean"> On 2017/03/07 18:08:38, jwd wrote: > Can you create a more specific enum for this, maybe something like > BooleanWanted, with meaningfully labeled enum values. Done. https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64173: +<histogram name="SettingsResetPrompt.PromptWanted" enum="Boolean"> On 2017/03/07 18:08:38, jwd wrote: > I'm not sure Wanted is the right word here, it implies user preference to me. Changed to PromptRequired. https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64181: +<histogram name="SettingsResetPrompt.StartupUrlsResetState" On 2017/03/07 18:08:38, jwd wrote: > I'd change the *ResetState histograms to be reformulated using suffixes. There > are instructions at the top of the file, basically you'd add a histogram > SettingsResetPrompt.ResetState, with a description that applies to all the > cases. Then you'd add suffixes to it (StartupUrls, Homepage), with specific > added description. Done. https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:108611: + <int value="4" label="No reset required due to recently prompted"/> On 2017/03/07 18:08:38, jwd wrote: > I'd recommend slightly shorter labels. Our tools prefer shorter, longer does > work though. You can also add a longer description in the body of the <int> tag. > I'd suggest changing the labels to "Recently prompted, no reset required" > maybe?, so that the more relevant info is first. Done.
https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64142: + before attempting to show the settings reset prompt. On 2017/03/07 20:42:05, alito wrote: > On 2017/03/07 18:08:38, jwd wrote: > > This is just coming from the variations config, yeah? Why transmit this back? > > (I'm not saying you shouldn't but it's not clear you need it) > > This is in case we decide to experiment with the delay to see if it makes a > difference in the number of users who accept the prompt. We could then easily > separate users based on the value of this histogram. Let me know if there is a > better way of supporting this use case. I would support this use case using experiments and variations. As long as you create a new experiment/group for each param value, you would be able to separate the clients. It would also work with our dashboards. Separating based on this histogram will require custom analysis. If you end up keeping this histogram, can you add to the summary a description of when it's logged. E.g. once on startup, once a session, every record, whenever the dialog is shown. https://codereview.chromium.org/2739513002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2739513002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64149: + settings reset prompt. Mention when this is logged, e.g. when the dialog is shown.
More descriptions added. PTAnL. https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64142: + before attempting to show the settings reset prompt. On 2017/03/07 20:53:00, jwd wrote: > On 2017/03/07 20:42:05, alito wrote: > > On 2017/03/07 18:08:38, jwd wrote: > > > This is just coming from the variations config, yeah? Why transmit this > back? > > > (I'm not saying you shouldn't but it's not clear you need it) > > > > This is in case we decide to experiment with the delay to see if it makes a > > difference in the number of users who accept the prompt. We could then easily > > separate users based on the value of this histogram. Let me know if there is a > > better way of supporting this use case. > > I would support this use case using experiments and variations. As long as you > create a new experiment/group for each param value, you would be able to > separate the clients. It would also work with our dashboards. Separating based > on this histogram will require custom analysis. > > If you end up keeping this histogram, can you add to the summary a description > of when it's logged. E.g. once on startup, once a session, every record, > whenever the dialog is shown. Done. https://codereview.chromium.org/2739513002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64142: + before attempting to show the settings reset prompt. On 2017/03/07 20:53:00, jwd wrote: > On 2017/03/07 20:42:05, alito wrote: > > On 2017/03/07 18:08:38, jwd wrote: > > > This is just coming from the variations config, yeah? Why transmit this > back? > > > (I'm not saying you shouldn't but it's not clear you need it) > > > > This is in case we decide to experiment with the delay to see if it makes a > > difference in the number of users who accept the prompt. We could then easily > > separate users based on the value of this histogram. Let me know if there is a > > better way of supporting this use case. > > I would support this use case using experiments and variations. As long as you > create a new experiment/group for each param value, you would be able to > separate the clients. It would also work with our dashboards. Separating based > on this histogram will require custom analysis. > > If you end up keeping this histogram, can you add to the summary a description > of when it's logged. E.g. once on startup, once a session, every record, > whenever the dialog is shown. Lets keep this for now. I've added description of when this is logged. https://codereview.chromium.org/2739513002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2739513002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64149: + settings reset prompt. On 2017/03/07 20:53:00, jwd wrote: > Mention when this is logged, e.g. when the dialog is shown. Done.
https://codereview.chromium.org/2739513002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2739513002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64134: + startup. Note that, if you plan on doing custom analysis based on this, it's possible that this histogram won't be in the same UMA record as the other histograms. https://codereview.chromium.org/2739513002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64150: + settings reset prompt. Logged once after startup. So this is not dependent on whether the prompt is going to show? It may be useful to also track things like "number of extensions accepted to be removed" and number being offered. Just having the number of extensions to disable on every startup may make it hard to tell the actual impact of the prompt. Especially without custom analysis, and it could add extra asterisks to any conclusions drawn from custom analysis. It's usually better to directly log what you want, rather than rely on inferring it.
Added two more histograms based on Jesse's suggestion. PTAnL. https://codereview.chromium.org/2739513002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2739513002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64134: + startup. On 2017/03/07 21:28:08, jwd wrote: > Note that, if you plan on doing custom analysis based on this, it's possible > that this histogram won't be in the same UMA record as the other histograms. Will keep that in mind. Also, if it turns out that it isn't useful in our analysis later, I can remove this. But lets keep it for now. https://codereview.chromium.org/2739513002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64150: + settings reset prompt. Logged once after startup. On 2017/03/07 21:28:08, jwd wrote: > So this is not dependent on whether the prompt is going to show? > > It may be useful to also track things like "number of extensions accepted to be > removed" and number being offered. Just having the number of extensions to > disable on every startup may make it hard to tell the actual impact of the > prompt. Especially without custom analysis, and it could add extra asterisks to > any conclusions drawn from custom analysis. It's usually better to directly log > what you want, rather than rely on inferring it. Good point. I've added two more histograms to record what was actually reset after the user accepted the prompt. We still need the ones that are logged at startup in order to understand the state of users.
LGTM, thanks for all the changes!
The CQ bit was checked by alito@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertshield@chromium.org, csharp@chromium.org Link to the patchset: https://codereview.chromium.org/2739513002/#ps90007 (title: "Adds histograms to log what is reset when prompt is accepted")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 90007, "attempt_start_ts": 1488989778705920,
"parent_rev": "4235a8b857d6685b89ae1e5cc4ffe924a7a8ecf5", "commit_rev":
"00b798c9c8952e3bf3321bf0ef94c606d2a3e69d"}
CQ is committing da patch.
Bot data: {"patchset_id": 90007, "attempt_start_ts": 1488989778705920,
"parent_rev": "4235a8b857d6685b89ae1e5cc4ffe924a7a8ecf5", "commit_rev":
"00b798c9c8952e3bf3321bf0ef94c606d2a3e69d"}
CQ is committing da patch.
Bot data: {"patchset_id": 90007, "attempt_start_ts": 1488989778705920,
"parent_rev": "aed751159609a4e3caacdabbd124dc543e25e28e", "commit_rev":
"cfeba874bf0f2458295ab24268790bf6a80e2d43"}
Message was sent while issue was closed.
Description was changed from ========== Settings reset prompt: add UMA metrics reporting. BUG= ========== to ========== Settings reset prompt: add UMA metrics reporting. BUG= Review-Url: https://codereview.chromium.org/2739513002 Cr-Commit-Position: refs/heads/master@{#455500} Committed: https://chromium.googlesource.com/chromium/src/+/cfeba874bf0f2458295ab2426879... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:90007) as https://chromium.googlesource.com/chromium/src/+/cfeba874bf0f2458295ab2426879... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
