Add RemoteGATTDescriptor to WebBluetoothFunction histogram enum.

Add RemoteGATTDescriptor to WebBluetoothFunction histogram enum.

We have telementry which counts the number of times each call to a WebBluetooth
function is made. We recently added reporting for descriptors and need to ensure
that WebBluetoothFunction has the correct methods listed. Currently descriptor
only supports readValue and writeValue.

BUG=None

R=ortuno

Review-Url: https://codereview.chromium.org/2667053002
Cr-Commit-Position: refs/heads/master@{#450352}
Committed: https://chromium.googlesource.com/chromium/src/+/819a862c01939317d2bce4ca08233ef7b9e27dc6

[dougt, 2017-02-01 03:27:28]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-01 03:27:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougt, 2017-02-01 03:27:52]

dougt@chromium.org changed reviewers:
+ isherman@chromium.org

[commit-bot: I haz the power, 2017-02-01 03:40:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-01 03:40:42]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[ortuno, 2017-02-01 18:02:19]

https://codereview.chromium.org/2667053002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2667053002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:109416: +  <int value="11"
label="RemoteGATTCharacteristic.getDescriptor()"/>
These values should match the ones in the UMAWebBluetoothFunction enum[1]. That
is:

11 => DESCRIPTOR_READ_VALUE
12 => DESCRIPTOR_WRITE_VALUE

getDescriptor(s) is not on the enum. We should add them.

[1]
https://cs.chromium.org/chromium/src/content/browser/bluetooth/bluetooth_metr...

[Ilya Sherman, 2017-02-01 22:21:47]

Metrics LGTM with enum labels corrected to match the C++ code.

[dougt, 2017-02-02 02:52:28]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 02:52:57]

Dry run: 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-02-02 04:54:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 04:54:07]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[dougt, 2017-02-03 22:58:27]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[dougt, 2017-02-03 22:58:52]

ortuno, ptal.

[commit-bot: I haz the power, 2017-02-03 22:58:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ortuno, 2017-02-03 23:16:33]

lgtm bar some nits

https://codereview.chromium.org/2667053002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/2667053002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.h:229: // Records the UUID of the
descriptor used when calling getDescriptor.
nit: can you match the order of getCharacteristics functions?

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

https://codereview.chromium.org/2667053002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:91820: +<!-- Hash values can be produced
using tool: bluetooth_metrics_hash (Only built via GN, not GYP) -->
nit: no need for the GYP mention. Also move the comment to the top. scheib
mentioned you need an extra line separating the comment from the values for the
formatting script to leave it alone.

https://codereview.chromium.org/2667053002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:91823: +      label="Characteristic
Aggregate Format;
nit: Let's keep it consistent with the other hash i.e.
characteristic_aggregate_format. No need for the 128bit uuid. That is already
known.

[dougt, 2017-02-03 23:33:49]

ilya, this cl changed a bunch since your lgtm. ptal.

https://codereview.chromium.org/2667053002/diff/60001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_metrics.h (right):

https://codereview.chromium.org/2667053002/diff/60001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.h:229: // Records the UUID of the
descriptor used when calling getDescriptor.
On 2017/02/03 23:16:33, ortuno wrote:
> nit: can you match the order of getCharacteristics functions?

Done.

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

https://codereview.chromium.org/2667053002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:91820: +<!-- Hash values can be produced
using tool: bluetooth_metrics_hash (Only built via GN, not GYP) -->
On 2017/02/03 23:16:33, ortuno wrote:
> nit: no need for the GYP mention. Also move the comment to the top. scheib
> mentioned you need an extra line separating the comment from the values for
the
> formatting script to leave it alone.

Done.

https://codereview.chromium.org/2667053002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:91823: +      label="Characteristic
Aggregate Format;
On 2017/02/03 23:16:33, ortuno wrote:
> nit: Let's keep it consistent with the other hash i.e.
> characteristic_aggregate_format. No need for the 128bit uuid. That is already
> known.

This is the output of the recommended tool.  For example:

./out/Default/bluetooth_metrics_hash 0x2905 "Characteristic Aggregate Format"

Generates:

<int value="475200414" label="Characteristic Aggregate Format;
00002905-0000-1000-8000-00805f9b34fb"/>

[ortuno, 2017-02-09 00:38:53]

isherman: ping?

[Ilya Sherman, 2017-02-11 00:18:28]

Sorry, I missed your previous re-review request.  Thanks for the ping! 
Generally still looks good, just have a couple of questions:

https://codereview.chromium.org/2667053002/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/2667053002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.cc:279: HashUUID(descriptor));
Are the UUIDs whitelisted in any way?  If not, is there anything that
guarantees, or at least makes likely, that this histogram will have a reasonable
number of buckets in use?

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

https://codereview.chromium.org/2667053002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5166: +<histogram
name="Bluetooth.Web.GetDescriptors.Descriptor"
What's the motivation for separating out getDescriptor() from getDescriptors()
calls?  Are they likely to be used in different contexts / have different
behavior?

[dougt, 2017-02-11 00:56:09]

https://codereview.chromium.org/2667053002/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/2667053002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.cc:279: HashUUID(descriptor));
On 2017/02/11 00:18:27, Ilya Sherman wrote:
> Are the UUIDs whitelisted in any way?  If not, is there anything that
> guarantees, or at least makes likely, that this histogram will have a
reasonable
> number of buckets in use?

I do not think so.  The UUIDs can be any value but there are standard values
people tend to use as defined by
https://www.bluetooth.com/specifications/gatt/descriptors

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

https://codereview.chromium.org/2667053002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5166: +<histogram
name="Bluetooth.Web.GetDescriptors.Descriptor"
On 2017/02/11 00:18:27, Ilya Sherman wrote:
> What's the motivation for separating out getDescriptor() from getDescriptors()
> calls?  Are they likely to be used in different contexts / have different
> behavior?

Following what characteristics did. I suspect it's to determine which call path
is more frequently being used.  Gio, do you have any thoughts?

[Ilya Sherman, 2017-02-11 01:02:36]

https://codereview.chromium.org/2667053002/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/2667053002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.cc:279: HashUUID(descriptor));
On 2017/02/11 00:56:09, dougt wrote:
> On 2017/02/11 00:18:27, Ilya Sherman wrote:
> > Are the UUIDs whitelisted in any way?  If not, is there anything that
> > guarantees, or at least makes likely, that this histogram will have a
> reasonable
> > number of buckets in use?
> 
> I do not think so.  The UUIDs can be any value but there are standard values
> people tend to use as defined by
> https://www.bluetooth.com/specifications/gatt/descriptors

I'm not comfortable with recording a histogram that might have thousands of
buckets on any given day -- that gets very expensive in terms of computation. 
Is there a way to gather the data that you need, while providing some (expected)
limits on the number of buckets that are spanned?

[dougt, 2017-02-11 08:08:11]

On 2017/02/11 01:02:36, Ilya Sherman wrote:
>
https://codereview.chromium.org/2667053002/diff/80001/content/browser/bluetoo...
> File content/browser/bluetooth/bluetooth_metrics.cc (right):
> 
>
https://codereview.chromium.org/2667053002/diff/80001/content/browser/bluetoo...
> content/browser/bluetooth/bluetooth_metrics.cc:279: HashUUID(descriptor));
> On 2017/02/11 00:56:09, dougt wrote:
> > On 2017/02/11 00:18:27, Ilya Sherman wrote:
> > > Are the UUIDs whitelisted in any way?  If not, is there anything that
> > > guarantees, or at least makes likely, that this histogram will have a
> > reasonable
> > > number of buckets in use?
> > 
> > I do not think so.  The UUIDs can be any value but there are standard values
> > people tend to use as defined by
> > https://www.bluetooth.com/specifications/gatt/descriptors
> 
> I'm not comfortable with recording a histogram that might have thousands of
> buckets on any given day -- that gets very expensive in terms of computation. 
> Is there a way to gather the data that you need, while providing some
(expected)
> limits on the number of buckets that are spanned?

Isn't the histogram only going to collect values that are in
the GATTDescriptorHash enum?

+<histogram name="Bluetooth.Web.GetDescriptor.Descriptor"
+    enum="GATTDescriptorHash">

[ortuno, 2017-02-13 00:34:16]

https://codereview.chromium.org/2667053002/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/2667053002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.cc:279: HashUUID(descriptor));
On 2017/02/11 at 01:02:36, Ilya Sherman wrote:
> On 2017/02/11 00:56:09, dougt wrote:
> > On 2017/02/11 00:18:27, Ilya Sherman wrote:
> > > Are the UUIDs whitelisted in any way?  If not, is there anything that
> > > guarantees, or at least makes likely, that this histogram will have a
> > reasonable
> > > number of buckets in use?
> > 
> > I do not think so.  The UUIDs can be any value but there are standard values
> > people tend to use as defined by
> > https://www.bluetooth.com/specifications/gatt/descriptors
> 
> I'm not comfortable with recording a histogram that might have thousands of
buckets on any given day -- that gets very expensive in terms of computation. 
Is there a way to gather the data that you need, while providing some (expected)
limits on the number of buckets that are spanned?

Making custom descriptors is quite rare and there are only 15 standard ones.
Furthermore interacting with descriptors is very uncommon; I would be surprised
if the number of buckets ever gets above the tens.

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

https://codereview.chromium.org/2667053002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5166: +<histogram
name="Bluetooth.Web.GetDescriptors.Descriptor"
On 2017/02/11 at 00:56:09, dougt wrote:
> On 2017/02/11 00:18:27, Ilya Sherman wrote:
> > What's the motivation for separating out getDescriptor() from
getDescriptors()
> > calls?  Are they likely to be used in different contexts / have different
> > behavior?
> 
> Following what characteristics did. I suspect it's to determine which call
path is more frequently being used.  Gio, do you have any thoughts?

Also getDescriptor() has a required parameter whereas getDescriptors() has a
optional parameter only. It's useful to know how often getDescriptors() is
called without that parameter.

[Ilya Sherman, 2017-02-13 22:06:57]

On 2017/02/11 08:08:11, dougt wrote:
> On 2017/02/11 01:02:36, Ilya Sherman wrote:
> >
>
https://codereview.chromium.org/2667053002/diff/80001/content/browser/bluetoo...
> > File content/browser/bluetooth/bluetooth_metrics.cc (right):
> > 
> >
>
https://codereview.chromium.org/2667053002/diff/80001/content/browser/bluetoo...
> > content/browser/bluetooth/bluetooth_metrics.cc:279: HashUUID(descriptor));
> > On 2017/02/11 00:56:09, dougt wrote:
> > > On 2017/02/11 00:18:27, Ilya Sherman wrote:
> > > > Are the UUIDs whitelisted in any way?  If not, is there anything that
> > > > guarantees, or at least makes likely, that this histogram will have a
> > > reasonable
> > > > number of buckets in use?
> > > 
> > > I do not think so.  The UUIDs can be any value but there are standard
values
> > > people tend to use as defined by
> > > https://www.bluetooth.com/specifications/gatt/descriptors
> > 
> > I'm not comfortable with recording a histogram that might have thousands of
> > buckets on any given day -- that gets very expensive in terms of
computation. 
> > Is there a way to gather the data that you need, while providing some
> (expected)
> > limits on the number of buckets that are spanned?
> 
> Isn't the histogram only going to collect values that are in
> the GATTDescriptorHash enum?
> 
> +<histogram name="Bluetooth.Web.GetDescriptor.Descriptor"
> +    enum="GATTDescriptorHash">

No, the contents of histograms.xml have no effect on what gets collected. 
histograms.xml provides metadata that's used by the dashboards to make the data
human-friendly to consume.

https://codereview.chromium.org/2667053002/diff/80001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_metrics.cc (right):

https://codereview.chromium.org/2667053002/diff/80001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.cc:279: HashUUID(descriptor));
On 2017/02/13 00:34:15, ortuno wrote:
> On 2017/02/11 at 01:02:36, Ilya Sherman wrote:
> > On 2017/02/11 00:56:09, dougt wrote:
> > > On 2017/02/11 00:18:27, Ilya Sherman wrote:
> > > > Are the UUIDs whitelisted in any way?  If not, is there anything that
> > > > guarantees, or at least makes likely, that this histogram will have a
> > > reasonable
> > > > number of buckets in use?
> > > 
> > > I do not think so.  The UUIDs can be any value but there are standard
values
> > > people tend to use as defined by
> > > https://www.bluetooth.com/specifications/gatt/descriptors
> > 
> > I'm not comfortable with recording a histogram that might have thousands of
> buckets on any given day -- that gets very expensive in terms of computation. 
> Is there a way to gather the data that you need, while providing some
(expected)
> limits on the number of buckets that are spanned?
> 
> Making custom descriptors is quite rare and there are only 15 standard ones.
> Furthermore interacting with descriptors is very uncommon; I would be
surprised
> if the number of buckets ever gets above the tens.

Okay, thanks for clarifying that.  LGTM assuming that the number of buckets seen
in practice will be quite small.

[dougt, 2017-02-14 00:58:04]

The CQ bit was checked by dougt@chromium.org

[dougt, 2017-02-14 00:58:05]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org
Link to the patchset: https://codereview.chromium.org/2667053002/#ps80001
(title: "ortuno review.")

[commit-bot: I haz the power, 2017-02-14 00:58:58]

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-02-14 02:34:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 02:34:14]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dougt, 2017-02-14 05:03:37]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-14 05:03:58]

Dry run: 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-02-14 07:46:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 07:46:43]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-02-14 15:00:48]

The CQ bit was checked by dougt@chromium.org

[dougt, 2017-02-14 15:00:48]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2667053002/#ps100001
(title: "Missing return stmt.")

[commit-bot: I haz the power, 2017-02-14 15:01:09]

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-02-14 15:05:47]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487084448461750,
"parent_rev": "a62a2c95a18cbb75dadb1c95744a5082c22a704e", "commit_rev":
"819a862c01939317d2bce4ca08233ef7b9e27dc6"}

[commit-bot: I haz the power, 2017-02-14 15:06:26]

Description was changed from

==========
Add RemoteGATTDescriptor to WebBluetoothFunction histogram enum.

We have telementry which counts the number of times each call to a WebBluetooth
function is made. We recently added reporting for descriptors and need to ensure
that WebBluetoothFunction has the correct methods listed. Currently descriptor
only supports readValue and writeValue.

BUG=None

R=ortuno
==========

to

==========
Add RemoteGATTDescriptor to WebBluetoothFunction histogram enum.

We have telementry which counts the number of times each call to a WebBluetooth
function is made. We recently added reporting for descriptors and need to ensure
that WebBluetoothFunction has the correct methods listed. Currently descriptor
only supports readValue and writeValue.

BUG=None

R=ortuno

Review-Url: https://codereview.chromium.org/2667053002
Cr-Commit-Position: refs/heads/master@{#450352}
Committed:
https://chromium.googlesource.com/chromium/src/+/819a862c01939317d2bce4ca0823...
==========

[commit-bot: I haz the power, 2017-02-14 15:06:27]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/819a862c01939317d2bce4ca0823...