bluetooth: Implement Service.getCharacteristics

content/browser/bluetooth/bluetooth_metrics.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/bluetooth/bluetooth_metrics.h"
 
 #include <stdint.h>
 
 #include <map>
 #include <set>
@@ skipping 151 matching lines @@
       NOTREACHED();
       return;
   }
 }
 
 void RecordGetCharacteristicCharacteristic(const std::string& characteristic) {
   UMA_HISTOGRAM_SPARSE_SLOWLY("Bluetooth.Web.GetCharacteristic.Characteristic",
                               HashUUID(characteristic));
 }
 
+// getCharacteristics
+
+void RecordGetCharacteristicsOutcome(UMAGetCharacteristicOutcome outcome) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "Bluetooth.Web.GetCharacteristics.Outcome", static_cast<int>(outcome),
+      static_cast<int>(UMAGetCharacteristicOutcome::COUNT));
+}
+
+void RecordGetCharacteristicsOutcome(CacheQueryOutcome outcome) {
+  switch (outcome) {
+    case CacheQueryOutcome::SUCCESS:
+    case CacheQueryOutcome::BAD_RENDERER:
+      NOTREACHED() << "No need to record a success or renderer crash";

[Ilya Sherman, 2016/02/26 01:16:27]

nit: Could this text be moved into a comment, rather than a string which will be
compiled into the Chrome binary?

[ortuno, 2016/02/26 16:24:58]

Done. Fixed other instances as well.

+      return;
+    case CacheQueryOutcome::NO_DEVICE:
+      RecordGetCharacteristicsOutcome(UMAGetCharacteristicOutcome::NO_DEVICE);
+      return;
+    case CacheQueryOutcome::NO_SERVICE:
+      RecordGetCharacteristicsOutcome(UMAGetCharacteristicOutcome::NO_SERVICE);
+      return;
+    case CacheQueryOutcome::NO_CHARACTERISTIC:
+      NOTREACHED();
+      return;
+  }
+}
+
+void RecordGetCharacteristicsCharacteristic(const std::string& characteristic) {
+  UMA_HISTOGRAM_SPARSE_SLOWLY("Bluetooth.Web.GetCharacteristics.Characteristic",
+                              HashUUID(characteristic));

[Ilya Sherman, 2016/02/26 01:16:27]

Just to confirm: Are you reasonably sure that the set of characteristics is not
tremendously large -- say, under 1,000 distinct values on any given day?  It's
important that even sparse histograms don't have too many distinct buckets, when
aggregated across all Chrome clients.

If you're not quite sure, is there a way to whitelist a subset of
characteristics that are particularly interesting?  Or is it the goal to try to
detect whether there are characteristics that we wouldn't even expect to be
common, which nonetheless are?

[ortuno, 2016/02/26 16:24:58]

What's the reason for this? I checked the histograms for the past couple of
days. The number of unique UUIDs never goes above 50. For now we are under a
flag and only available in whitelisted origins. I opened a bug to revisit this
decision before launching. http://crbug.com/590228
The latter.

[Jeffrey Yasskin, 2016/02/26 16:42:06]

The set of characteristics could be large; a couple times the number of
Services, where each manufacturer might invent a new service or two for each of
their types of devices.

Maybe we don't need to collect Characteristic UUIDs at all: we're looking for
evidence that particular tasks/protocols are popular enough to build a dedicated
API around, but that'll happen at the Service level, not the Characteristic
level.

The number of Service UUIDs could also be large, but probably not over 1000 a
day? And we'll see the number of services (or characteristics) ramping up over
time, as websites adopt the API, so we'll have time to bail out if it looks too
high.

We could probably subset by adding an enum for known service UUIDs. We'd gather
UUIDs by crawling popular websites, and then add them for the next Chrome
release. This isn't as good as gathering the hashes, since it means we don't
know if a bunch of long-tail sites happen to be using a particular Bluetooth
service a lot, although we could use a high 'unknown' count as a sign to keep
adding UUIDs.

+1 for not discussing too long before the branch point, and fixing it in M51 if
we pick a different route. We definitely won't have "too many" characteristics
before then.

[Ilya Sherman, 2016/02/26 18:00:24]

On the server-side processing backend, we aggregated data from all clients, and
build up a single histogram that contains all of the reported buckets.  The data
processing pipeline isn't designed to handle arbitrarily many buckets.

It sounds fine to emit this histogram as-is for now.  If, at some point in the
future, we start to see too many buckets in use, then we might have to find an
alternate approach.

+}
+
 // GATT Operations
 
 void RecordGATTOperationOutcome(UMAGATTOperation operation,
                                 UMAGATTOperationOutcome outcome) {
   switch (operation) {
     case UMAGATTOperation::CHARACTERISTIC_READ:
       RecordCharacteristicReadValueOutcome(outcome);
       return;
     case UMAGATTOperation::CHARACTERISTIC_WRITE:
       RecordCharacteristicWriteValueOutcome(outcome);
@@ skipping 60 matching lines @@
       static_cast<int>(outcome),
       static_cast<int>(UMAGATTOperationOutcome::COUNT));
 }
 
 void RecordStartNotificationsOutcome(CacheQueryOutcome outcome) {
   RecordStartNotificationsOutcome(
       TranslateCacheQueryOutcomeToGATTOperationOutcome(outcome));
 }
 
 }  // namespace content