Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
296873002:
Add histograms to be used by easy unlock app (Closed)
|
Created:
6 years, 7 months ago by tbarzic Modified:
6 years, 7 months ago CC:
chromium-reviews, jar (doing other things), asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd histograms to be used by easy unlock app
BUG=355735
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273107
Patch Set 1 #Patch Set 2 : . #
Total comments: 6
Patch Set 3 : . #Patch Set 4 : . #
Total comments: 4
Patch Set 5 : . #Patch Set 6 : . #
Total comments: 4
Patch Set 7 : . #
Messages
Total messages: 14 (0 generated)
can you see if these look ok to you. Also, let me know if you'd prefer to have some of these recorded as user actions rather than histogram.
LGTM, thanks!
https://codereview.chromium.org/296873002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/296873002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:4749: +</histogram> It would be good to contextualize this data by also logging the number of times that the EasyUnlock app was shown. https://codereview.chromium.org/296873002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32953: +</enum> Is it not possible to have been in a success state? https://codereview.chromium.org/296873002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32957: + <int value="1" label="SCREEN_UNLOCKED_USING_EASY_UNLOCK"/> nit: You can use human-friendly strings, like "Screen unlocked (total)" and "Screen unlocked (via EasyUnlock)"
https://codereview.chromium.org/296873002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/296873002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:4749: +</histogram> On 2014/05/22 15:34:26, Ilya Sherman wrote: > It would be good to contextualize this data by also logging the number of times > that the EasyUnlock app was shown. added EasyUnlock.AppLaunched action https://codereview.chromium.org/296873002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32953: +</enum> On 2014/05/22 15:34:26, Ilya Sherman wrote: > Is it not possible to have been in a success state? Yeah, it is. It's actually the value 0, but I see how the label is confusing.. https://codereview.chromium.org/296873002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32957: + <int value="1" label="SCREEN_UNLOCKED_USING_EASY_UNLOCK"/> On 2014/05/22 15:34:26, Ilya Sherman wrote: > nit: You can use human-friendly strings, like "Screen unlocked (total)" and > "Screen unlocked (via EasyUnlock)" Done.
https://codereview.chromium.org/296873002/diff/60001/tools/metrics/actions/ac... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/296873002/diff/60001/tools/metrics/actions/ac... tools/metrics/actions/actions.xml:2452: </action> I'd strongly recommend including this in the relevant histogram, rather than logging it as a user action. User actions are currently not nearly as well supported in the UMA dashboards as histograms are. https://codereview.chromium.org/296873002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/296873002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32952: + <int value="6" label="PAIRING_ERROR"/> Note that you can use human-friendly names for all of the labels in this file, not just the specific two that I explicitly commented on.
https://codereview.chromium.org/296873002/diff/60001/tools/metrics/actions/ac... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/296873002/diff/60001/tools/metrics/actions/ac... tools/metrics/actions/actions.xml:2452: </action> On 2014/05/23 15:26:54, Ilya Sherman wrote: > I'd strongly recommend including this in the relevant histogram, rather than > logging it as a user action. User actions are currently not nearly as well > supported in the UMA dashboards as histograms are. ok, added it to the histogram. https://codereview.chromium.org/296873002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/296873002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32952: + <int value="6" label="PAIRING_ERROR"/> On 2014/05/23 15:26:54, Ilya Sherman wrote: > Note that you can use human-friendly names for all of the labels in this file, > not just the specific two that I explicitly commented on. Done.
LGTM % nits. Thanks. https://codereview.chromium.org/296873002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/296873002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4747: + <owner>joshwoodward@google.com</owner> Note: The list of owners should typically include an eng owner as well. Is there any particular reason you're not listing yourself as an owner? https://codereview.chromium.org/296873002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4748: + <summary>Button clicked in easy unlock app during setup process.</summary> nit: I think we usually write the app name as "EasyUnlock", or perhaps "Easy Unlock", right? It's a little hard to parse the feature name when it's written in lowercase.
https://codereview.chromium.org/296873002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/296873002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4747: + <owner>joshwoodward@google.com</owner> On 2014/05/27 19:00:50, Ilya Sherman wrote: > Note: The list of owners should typically include an eng owner as well. Is > there any particular reason you're not listing yourself as an owner? None, I c/p the template from FileBrowser ones; adding myself.. https://codereview.chromium.org/296873002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4748: + <summary>Button clicked in easy unlock app during setup process.</summary> On 2014/05/27 19:00:50, Ilya Sherman wrote: > nit: I think we usually write the app name as "EasyUnlock", or perhaps "Easy > Unlock", right? It's a little hard to parse the feature name when it's written > in lowercase. Done.
The CQ bit was checked by tbarzic@chromium.org
The CQ bit was unchecked by tbarzic@chromium.org
The CQ bit was checked by tbarzic@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tbarzic@chromium.org/296873002/120001
Message was sent while issue was closed.
Change committed as 273107 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
