Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2911523002:
Add metrics to printer setup flow. (Closed)
|
Created:
3 years, 6 months ago by skau Modified:
3 years, 6 months ago CC:
chromium-reviews, oshima+watch_chromium.org, michaelpg+watch-md-settings_chromium.org, asvitkine+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd metrics to printer setup flow.
Record the source of PPDs and the success or failure of a printer setup
request from the setup flow in settings.
BUG=725741
Review-Url: https://codereview.chromium.org/2911523002
Cr-Commit-Position: refs/heads/master@{#475765}
Committed: https://chromium.googlesource.com/chromium/src/+/387e28ecbcd4f36ed1c4172e2f9ea3da691bf8ff
Patch Set 1 #
Total comments: 7
Patch Set 2 : pretty print #Patch Set 3 : rebase #Patch Set 4 : remove discovered printers #Patch Set 5 : rebase #
Total comments: 6
Patch Set 6 : fixed #
Total comments: 2
Patch Set 7 : setup #Patch Set 8 : rebase #
Messages
Total messages: 34 (13 generated)
skau@chromium.org changed reviewers: + justincarlson@chromium.org, xdai@chromium.org
Hooray! More metrics! In the parent change, I renamed the PrinterSetupResult enum values.
thestig@chromium.org changed reviewers: + thestig@chromium.org
https://codereview.chromium.org/2911523002/diff/1/tools/metrics/histograms/en... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2911523002/diff/1/tools/metrics/histograms/en... tools/metrics/histograms/enums.xml:28629: + <int value="1" label="Succes"/> Typo
https://codereview.chromium.org/2911523002/diff/1/chrome/browser/chromeos/pri... File chrome/browser/chromeos/printing/printer_configurer.h (right): https://codereview.chromium.org/2911523002/diff/1/chrome/browser/chromeos/pri... chrome/browser/chromeos/printing/printer_configurer.h:31: kMaxValue = kPpdUnretrievable I"m suddenly a little suspicious that kMaxValue here is set to the last value, whereas in the other enums it's set to last+1. Is the difference intentional? https://codereview.chromium.org/2911523002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2911523002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:423: UMA_HISTOGRAM_COUNTS_100("Printing.CUPS.PrintersDiscovered", Since this is potentially incremental, Would it be better to record this in OnDiscoveryInitialScanDone()? It's not clear to me that incremental results here are useful things to record.
Fixed the typo. A question regarding OnDiscoveryInitialScanDone. https://codereview.chromium.org/2911523002/diff/1/chrome/browser/chromeos/pri... File chrome/browser/chromeos/printing/printer_configurer.h (right): https://codereview.chromium.org/2911523002/diff/1/chrome/browser/chromeos/pri... chrome/browser/chromeos/printing/printer_configurer.h:31: kMaxValue = kPpdUnretrievable On 2017/05/26 16:58:38, Carlson wrote: > I"m suddenly a little suspicious that kMaxValue here is set to the last value, > whereas in the other enums it's set to last+1. Is the difference intentional? Well, this is the comment on the histogram method: The value in |sample| must be strictly less than |enum_max|. Annoyingly, if I write a switch for PrinterSetupResult, I need to handle kMaxValue as an unexpected value. However, the histogram macros expect last + 1. https://codereview.chromium.org/2911523002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2911523002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:423: UMA_HISTOGRAM_COUNTS_100("Printing.CUPS.PrintersDiscovered", On 2017/05/26 16:58:38, Carlson wrote: > Since this is potentially incremental, Would it be better to record this in > OnDiscoveryInitialScanDone()? It's not clear to me that incremental results here > are useful things to record. > I forgot this is incremental. You're right that recording the incremental values will throw off our counts. Is there a way to get a count after the scan is done? We don't seem to store the whole list. https://codereview.chromium.org/2911523002/diff/1/tools/metrics/histograms/en... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2911523002/diff/1/tools/metrics/histograms/en... tools/metrics/histograms/enums.xml:28629: + <int value="1" label="Succes"/> On 2017/05/25 23:23:40, Lei Zhang wrote: > Typo Done.
https://codereview.chromium.org/2911523002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2911523002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:423: UMA_HISTOGRAM_COUNTS_100("Printing.CUPS.PrintersDiscovered", On 2017/05/26 17:18:02, skau wrote: > On 2017/05/26 16:58:38, Carlson wrote: > > Since this is potentially incremental, Would it be better to record this in > > OnDiscoveryInitialScanDone()? It's not clear to me that incremental results > here > > are useful things to record. > > > > I forgot this is incremental. You're right that recording the incremental > values will throw off our counts. Is there a way to get a count after the scan > is done? We don't seem to store the whole list. It's incremental, but not in the way you're thinking, I think. :) This can be called multiple times, but each time it is called it gets the full list of discovered printers to-date, obsoleting any previous received list. That's why I was suggesting OnDiscoveryInitialScanDone() as a better place to do this. You'll have to stash the size of the most recent printers_list in the object to make it work, but it's not too hacky, I think.
On 2017/05/26 17:29:19, Carlson wrote: > https://codereview.chromium.org/2911523002/diff/1/chrome/browser/ui/webui/set... > File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): > > https://codereview.chromium.org/2911523002/diff/1/chrome/browser/ui/webui/set... > chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:423: > UMA_HISTOGRAM_COUNTS_100("Printing.CUPS.PrintersDiscovered", > On 2017/05/26 17:18:02, skau wrote: > > On 2017/05/26 16:58:38, Carlson wrote: > > > Since this is potentially incremental, Would it be better to record this in > > > OnDiscoveryInitialScanDone()? It's not clear to me that incremental results > > here > > > are useful things to record. > > > > > > > I forgot this is incremental. You're right that recording the incremental > > values will throw off our counts. Is there a way to get a count after the > scan > > is done? We don't seem to store the whole list. > > It's incremental, but not in the way you're thinking, I think. :) > > This can be called multiple times, but each time it is called it gets the full > list of discovered printers to-date, obsoleting any previous received list. > > That's why I was suggesting OnDiscoveryInitialScanDone() as a better place to do > this. You'll have to stash the size of the most recent printers_list in the > object to make it work, but it's not too hacky, I think. Removed discovered printers and handling it in a separate CL.
lgtm
skau@chromium.org changed reviewers: + mpearson@chromium.org
mpearson@: metrics OWNER
Can you please test these histograms interactively using about:histograms? If I found one bug in the histogram logging, it's possible there are more. --mark https://codereview.chromium.org/2911523002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2911523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:53: UMA_HISTOGRAM_ENUMERATION("Printing.CUPS.PpdSource", kUser, kPpdSourceMax); Shouldn't this be source, not kUser? https://codereview.chromium.org/2911523002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2911523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:29164: + <int value="3" label="Could not contact debugd"/> perhaps mention dbus here? https://codereview.chromium.org/2911523002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2911523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:57076: + setup. Recorded when a printer is successfully configured. Only recorded > Recorded when a printer is successfully configured. This statement looks wrong to me. It looks like the error handling code OnAddPrinterError() happens after this histogram is emitted.
I've verified the metrics are being logged properly now. PTAL. https://codereview.chromium.org/2911523002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2911523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:53: UMA_HISTOGRAM_ENUMERATION("Printing.CUPS.PpdSource", kUser, kPpdSourceMax); On 2017/05/26 22:57:29, Mark P wrote: > Shouldn't this be source, not kUser? Done. https://codereview.chromium.org/2911523002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2911523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:29164: + <int value="3" label="Could not contact debugd"/> On 2017/05/26 22:57:29, Mark P wrote: > perhaps mention dbus here? Done. https://codereview.chromium.org/2911523002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2911523002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:57076: + setup. Recorded when a printer is successfully configured. Only recorded On 2017/05/26 22:57:30, Mark P wrote: > > Recorded when a printer is successfully configured. > This statement looks wrong to me. It looks like the error handling code > OnAddPrinterError() happens after this histogram is emitted. The comment has been updated. We want to track attempted configurations.
skau@chromium.org changed reviewers: - thestig@chromium.org
skau@chromium.org changed reviewers: + michaelpg@chromium.org
michealpg@: settings OWNER
xml files lgtm
PTAL?
lgtm https://codereview.chromium.org/2911523002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2911523002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:57084: + The success or error code for the set up of a CUPS printer. Recorded when nit: setup, x2
Thanks for the reviews. https://codereview.chromium.org/2911523002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2911523002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:57084: + The success or error code for the set up of a CUPS printer. Recorded when On 2017/05/30 21:14:48, michaelpg wrote: > nit: setup, x2 Done.
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org, mpearson@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2911523002/#ps120001 (title: "setup")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, michaelpg@chromium.org, xdai@chromium.org Link to the patchset: https://codereview.chromium.org/2911523002/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by skau@chromium.org
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": 140001, "attempt_start_ts": 1496192967398260,
"parent_rev": "f28d27c829e63a20d7a0551afcfd39b88eacee1c", "commit_rev":
"387e28ecbcd4f36ed1c4172e2f9ea3da691bf8ff"}
Message was sent while issue was closed.
Description was changed from ========== Add metrics to printer setup flow. Record the source of PPDs and the success or failure of a printer setup request from the setup flow in settings. BUG=725741 ========== to ========== Add metrics to printer setup flow. Record the source of PPDs and the success or failure of a printer setup request from the setup flow in settings. BUG=725741 Review-Url: https://codereview.chromium.org/2911523002 Cr-Commit-Position: refs/heads/master@{#475765} Committed: https://chromium.googlesource.com/chromium/src/+/387e28ecbcd4f36ed1c4172e2f9e... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/387e28ecbcd4f36ed1c4172e2f9e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
