Add Printing.CUPS.PrintersDiscovered to our metrics.

Add Printing.CUPS.PrintersDiscovered to our metrics.

In order to understand how many printers users are seeing in the
discovered printers dialog, track it using UMA.  The metric is recorded
when the discoverer declares scanning to be complete.

BUG=726819
Review-Url: https://codereview.chromium.org/2904243003
Cr-Commit-Position: refs/heads/master@{#475649}
Committed: https://chromium.googlesource.com/chromium/src/+/fc8c7f3440cecb52667340276ea06100e303d4df

[skau, 2017-05-26 20:04:57]

skau@chromium.org changed reviewers:
+ dpapad@chromium.org, justincarlson@chromium.org, rkaplow@chromium.org

[skau, 2017-05-26 20:04:57]

justincarlson@: everything
rkaplow@: metrics
dpapad@: settings

[Carlson, 2017-05-26 20:10:20]

lgtm

https://codereview.chromium.org/2904243003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/printer_discoverer.h (right):

https://codereview.chromium.org/2904243003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/printer_discoverer.h:30: // the initial scan
has been completed.  Observers will not receive
This comment is a little imprecise, as we may call again if a printer disappears
as well.

[skau, 2017-05-26 20:23:52]

Updated comment.

https://codereview.chromium.org/2904243003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/printer_discoverer.h (right):

https://codereview.chromium.org/2904243003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/printer_discoverer.h:30: // the initial scan
has been completed.  Observers will not receive
On 2017/05/26 20:10:20, Carlson wrote:
> This comment is a little imprecise, as we may call again if a printer
disappears
> as well.

Done.

[dpapad, 2017-05-26 21:02:03]

https://codereview.chromium.org/2904243003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/printer_discoverer.cc (right):

https://codereview.chromium.org/2904243003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/printer_discoverer.cc:63:
observer.OnPrintersFound(true /*scan_done*/, all_printers);
When is OnPrintersFound called with |false|? I don't see it in this CL.

https://codereview.chromium.org/2904243003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (left):

https://codereview.chromium.org/2904243003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:413: void
CupsPrintersHandler::OnDiscoveryInitialScanDone() {
Is it necessary to merge OnDiscoveryInitialScanDone to OnPrintersFound? Can we
avoid merging them (and the need for the  |scan_done| boolean) and adding a
boolean to OnPrintersFound by doing the following instead?

1) Keep the 2 observer methods separate.
2) Pass printers_list->GetSize() as a parameter to OnDiscoveryInitialScanDone.
3) Call UMA_HISTOGRAM_COUNTS_100() from OnDiscoveryInitialScanDone.

[Carlson, 2017-05-26 21:10:14]

On 2017/05/26 21:02:03, dpapad wrote:
>
https://codereview.chromium.org/2904243003/diff/40001/chrome/browser/chromeos...
> File chrome/browser/chromeos/printing/printer_discoverer.cc (right):
> 
>
https://codereview.chromium.org/2904243003/diff/40001/chrome/browser/chromeos...
> chrome/browser/chromeos/printing/printer_discoverer.cc:63:
> observer.OnPrintersFound(true /*scan_done*/, all_printers);
> When is OnPrintersFound called with |false|? I don't see it in this CL. 

In the existing implementation, the only thing we have us USB printers, which
don't have discovery latency, which is why the code this replaces always calls
OnDiscoveryInitialScanDone immediately following the first OnPrintersFound call.
 When network discovery lands, this will change.

> 
>
https://codereview.chromium.org/2904243003/diff/40001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc
(left):
> 
>
https://codereview.chromium.org/2904243003/diff/40001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:413: void
> CupsPrintersHandler::OnDiscoveryInitialScanDone() {
> Is it necessary to merge OnDiscoveryInitialScanDone to OnPrintersFound? Can we
> avoid merging them (and the need for the  |scan_done| boolean) and adding a
> boolean to OnPrintersFound by doing the following instead?
> 
> 1) Keep the 2 observer methods separate.
> 2) Pass printers_list->GetSize() as a parameter to OnDiscoveryInitialScanDone.
> 3) Call UMA_HISTOGRAM_COUNTS_100() from OnDiscoveryInitialScanDone.

This was actually the initial idea Sean discussed with me, but I proposed this
instead as I believe it makes more sense.  

Hacking the number of printers found in as an observer parameter seems like a
weird API wart to me.  Gathering stats like this is the only use case for it I
can think of. 

Additionally, separating out the Found/Done API functions doesn't actually have
any clear benefit that I can see, and it has the downside of virally enforcing
that implementors add an extra trivial function separated from related
functionality to implement the observer API.

In general we expect OnDiscoveryInitialScanDone to be called immediately after
(some invocation of) OnPrintersFound.  IMHO "here are the printers and
information about whether or not to expect more calls like this in the future"
is a pretty reasonable API hook.  Can you articulate what you find distasteful
about it?

[skau, 2017-05-26 21:20:10]

I have an answer to the comments which is very similar to justin's.

https://codereview.chromium.org/2904243003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/printing/printer_discoverer.cc (right):

https://codereview.chromium.org/2904243003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/printing/printer_discoverer.cc:63:
observer.OnPrintersFound(true /*scan_done*/, all_printers);
On 2017/05/26 21:02:03, dpapad wrote:
> When is OnPrintersFound called with |false|? I don't see it in this CL.

It's not called with false right now.  The interface anticipates adding network
discovery which will be incremental.

https://codereview.chromium.org/2904243003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (left):

https://codereview.chromium.org/2904243003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:413: void
CupsPrintersHandler::OnDiscoveryInitialScanDone() {
On 2017/05/26 21:02:03, dpapad wrote:
> Is it necessary to merge OnDiscoveryInitialScanDone to OnPrintersFound? Can we
> avoid merging them (and the need for the  |scan_done| boolean) and adding a
> boolean to OnPrintersFound by doing the following instead?
> 
> 1) Keep the 2 observer methods separate.
> 2) Pass printers_list->GetSize() as a parameter to OnDiscoveryInitialScanDone.
> 3) Call UMA_HISTOGRAM_COUNTS_100() from OnDiscoveryInitialScanDone.

Justin and I have been considering whether the current change or your proposed
alternative would be simpler.  We think that the change as written is a more
straightforward API independent of adding metrics.  All
OnDiscoveryInitialScanDone calls will be immediately preceded by an
OnPrintersFound call and we haven't found it useful to have a separate method.

[dpapad, 2017-05-26 21:34:47]

> In the existing implementation, the only thing we have us USB printers, which
don't have discovery latency, which is why the code this replaces always calls
OnDiscoveryInitialScanDone immediately following the first OnPrintersFound call.
 When network discovery lands, this will change.

Is network discovery landing soon-ish? Asking because having a boolean in the
code that always has a |true| value can be confusing.

Optional: Personally I would prefer adding a TODO with a link to a crbug,
indicating that this function should be parameterized with a boolean once it
starts actually returning partial progress, than preemptively making the change.

> 
> In general we expect OnDiscoveryInitialScanDone to be called immediately after
(some invocation of) OnPrintersFound.  IMHO "here are the printers and
information about whether or not to expect more calls like this in the future"
is a pretty reasonable API hook.  Can you articulate what you find distasteful
about it?

None of the approaches seem distasteful, merging the two observer methods to one
is fine. My concern is that the semantics before and after are different now.

Before: An 'on-printer-discovery-done' event was sent to the JS side only from
within WrappedOnPrintersFound, which triggers only when an observer is
registered.
After: An 'on-printer-discovery-done' event is sent to the JS both when an
observer is registered, but also when a new printer is found.

Is this an intentional change? And does the JS side perform additional (and
unnecessary) work because of it?

[skau, 2017-05-26 22:21:42]

It looks like DiscoveryDone was run twice.  I'll punt the interface change for
now.  Network discovery should land sometime next month but it'll be easy enough
to revive.

[rkaplow, 2017-05-29 19:15:39]

lgtm

histogram lg

https://codereview.chromium.org/2904243003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2904243003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:57072: +<histogram
name="Printing.CUPS.PrintersDiscovered" units="count">
I'd make unit 'printers'

[skau, 2017-05-30 16:02:35]

Thanks

https://codereview.chromium.org/2904243003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2904243003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:57072: +<histogram
name="Printing.CUPS.PrintersDiscovered" units="count">
On 2017/05/29 19:15:39, rkaplow wrote:
> I'd make unit 'printers'

Done.

[dpapad, 2017-05-30 17:56:49]

LGTM

[skau, 2017-05-30 18:16:40]

The CQ bit was checked by skau@chromium.org

[skau, 2017-05-30 18:16:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
justincarlson@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2904243003/#ps80001
(title: "change units")

[commit-bot: I haz the power, 2017-05-30 18:17:30]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 20:16:23]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496168200702760,
"parent_rev": "23f70f754086eedbacca3e078faaaf6cb13284a2", "commit_rev":
"fc8c7f3440cecb52667340276ea06100e303d4df"}

[commit-bot: I haz the power, 2017-05-30 20:16:33]

Description was changed from

==========
Add Printing.CUPS.PrintersDiscovered to our metrics.

In order to understand how many printers users are seeing in the
discovered printers dialog, track it using UMA.  The metric is recorded
when the discoverer declares scanning to be complete.

BUG=726819
==========

to

==========
Add Printing.CUPS.PrintersDiscovered to our metrics.

In order to understand how many printers users are seeing in the
discovered printers dialog, track it using UMA.  The metric is recorded
when the discoverer declares scanning to be complete.

BUG=726819

Review-Url: https://codereview.chromium.org/2904243003
Cr-Commit-Position: refs/heads/master@{#475649}
Committed:
https://chromium.googlesource.com/chromium/src/+/fc8c7f3440cecb52667340276ea0...
==========

[commit-bot: I haz the power, 2017-05-30 20:16:34]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/fc8c7f3440cecb52667340276ea0...