Bluetooth: gather usage metrics

Bluetooth: gather usage metrics

Resubmit; this was reverted in 196993 due to a missing fake initialization in a unit test which has now been added.

BUG=233820
TEST=chrome:///histograms
R=isherman@chromium.org, satorux@chromium.org, youngki@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196990

[keybuk, 2013-04-20 00:07:44]

youngki: for device/bluetooth/ OWNERS
isherman: for /metrics/ OWNERS

[Ilya Sherman, 2013-04-20 04:54:02]

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:316:
"usb:p[0-9a-fA-F][0-9a-fA-F][0-9a-fA-F][0-9a-fA-F]"
nit: Might help with readability to add a line break after "usb:".

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:351:
SystemProfileProto::Bluetooth::PairedDevice::DEVICE_KEYBOARD_MOUSE_COMBO;
nit: This indentation is off.  Perhaps add a "using
SystemProfileProto::Bluetooth" declaration to the top of this file to make all
of these lines shorter and easier to scan?

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:352: default:
nit: Please omit the default case, so that the compiler can catch missing case
stmts.

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:936: base::Unretained(this)));
Why is base::Unretained() safe?  That is, what guarantees that this class will
not have been destroyed yet by the time the callback is fired?

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:1159: devices.begin(); iter !=
devices.end(); ++iter) {
nit: Indent this line four more spaces, since it's wrapped from the previous
one.

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:1164:
static_cast<chromeos::BluetoothDeviceExperimentalChromeOS*>(*iter);
How do you know that this cast is safe?  If it's always safe, why not just
structure the code so that the cast is not necessary?

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:1170: if (MatchPattern(modalias,
kUsbModaliasPattern)) {
Hmm, is this meant to be a regex pattern match?  It looks like you're using
string_util.h::MatchPattern(), which AFAIK does not actually support full regex
syntax like what you're using.

https://codereview.chromium.org/13872017/diff/4001/chrome/common/metrics/prot...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/13872017/diff/4001/chrome/common/metrics/prot...
chrome/common/metrics/proto/system_profile.proto:378: // Assigned class of the
device.
Please document what the class is, i.e. how to interpret this field.

https://codereview.chromium.org/13872017/diff/4001/chrome/common/metrics/prot...
chrome/common/metrics/proto/system_profile.proto:400: // Vendor prefix of the
Bluetooth address.
Please document what this string looks like.  Also, is there any chance we could
upload this as a number, or small tuple of numbers, rather than as a string? 
That would help assuage privacy concerns.

Regardless, please make sure to get approval from the privacy and legal teams to
track this data.

https://codereview.chromium.org/13872017/diff/4001/chrome/common/metrics/prot...
chrome/common/metrics/proto/system_profile.proto:414: optional Bluetooth
bluetooth = 14;
This tag is actually already being used by the internal copy of the protocol
buffer, and tag 15 is about to be.  You'll want to skip all the way to 16.

Better yet, though, would be to put this under the hardware section of the
protocol buffer (IMO).

[youngki, 2013-04-22 15:20:32]

lgtm for device/bluetooth

[keybuk, 2013-04-22 19:30:50]

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:351:
SystemProfileProto::Bluetooth::PairedDevice::DEVICE_KEYBOARD_MOUSE_COMBO;
On 2013/04/20 04:54:02, Ilya Sherman wrote:
> nit: This indentation is off.  Perhaps add a "using
> SystemProfileProto::Bluetooth" declaration to the top of this file to make all
> of these lines shorter and easier to scan?

You can't "using" a class member like that in C++

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:352: default:
On 2013/04/20 04:54:02, Ilya Sherman wrote:
> nit: Please omit the default case, so that the compiler can catch missing case
> stmts.

Done.

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:936: base::Unretained(this)));
On 2013/04/20 04:54:02, Ilya Sherman wrote:
> Why is base::Unretained() safe?  That is, what guarantees that this class will
> not have been destroyed yet by the time the callback is fired?

Done.

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:1159: devices.begin(); iter !=
devices.end(); ++iter) {
On 2013/04/20 04:54:02, Ilya Sherman wrote:
> nit: Indent this line four more spaces, since it's wrapped from the previous
> one.

Done.

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:1164:
static_cast<chromeos::BluetoothDeviceExperimentalChromeOS*>(*iter);
On 2013/04/20 04:54:02, Ilya Sherman wrote:
> How do you know that this cast is safe?  If it's always safe, why not just
> structure the code so that the cast is not necessary?

Done.

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:1170: if (MatchPattern(modalias,
kUsbModaliasPattern)) {
On 2013/04/20 04:54:02, Ilya Sherman wrote:
> Hmm, is this meant to be a regex pattern match?  It looks like you're using
> string_util.h::MatchPattern(), which AFAIK does not actually support full
regex
> syntax like what you're using.

Done.

https://codereview.chromium.org/13872017/diff/4001/chrome/common/metrics/prot...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/13872017/diff/4001/chrome/common/metrics/prot...
chrome/common/metrics/proto/system_profile.proto:378: // Assigned class of the
device.
On 2013/04/20 04:54:02, Ilya Sherman wrote:
> Please document what the class is, i.e. how to interpret this field.

Done.

https://codereview.chromium.org/13872017/diff/4001/chrome/common/metrics/prot...
chrome/common/metrics/proto/system_profile.proto:400: // Vendor prefix of the
Bluetooth address.
On 2013/04/20 04:54:02, Ilya Sherman wrote:
> Please document what this string looks like.  Also, is there any chance we
could
> upload this as a number, or small tuple of numbers, rather than as a string? 
> That would help assuage privacy concerns.
> 
Done; encoded as a uint32_t
(I couldn't see a useful way to declare it as just three bytes)

> Regardless, please make sure to get approval from the privacy and legal teams
to
> track this data.

In progress currently (since Jan!)

https://codereview.chromium.org/13872017/diff/4001/chrome/common/metrics/prot...
chrome/common/metrics/proto/system_profile.proto:414: optional Bluetooth
bluetooth = 14;
On 2013/04/20 04:54:02, Ilya Sherman wrote:
> This tag is actually already being used by the internal copy of the protocol
> buffer, and tag 15 is about to be.  You'll want to skip all the way to 16.
> 
> Better yet, though, would be to put this under the hardware section of the
> protocol buffer (IMO).

Done.

[keybuk, 2013-04-22 19:31:58]

ptal

youngki: this adds new API to BluetoothDevice to avoid the cast to
ExperimentalChromeOS in the MetricsLog code; these functions can remain as
"return 0" on OS X / Windows

[youngki, 2013-04-22 20:10:34]

lgtm

I think in the future we should just use BluetoothAdapterChromeOS directly
instead of BluetoothAdapter interface in the ChromeOS code, and leave the only
methods that are general across the platforms.

[Ilya Sherman, 2013-04-23 00:09:42]

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/13872017/diff/4001/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log.cc:351:
SystemProfileProto::Bluetooth::PairedDevice::DEVICE_KEYBOARD_MOUSE_COMBO;
On 2013/04/22 19:30:51, keybuk wrote:
> On 2013/04/20 04:54:02, Ilya Sherman wrote:
> > nit: This indentation is off.  Perhaps add a "using
> > SystemProfileProto::Bluetooth" declaration to the top of this file to make
all
> > of these lines shorter and easier to scan?
> 
> You can't "using" a class member like that in C++

How about a typedef, then?

https://codereview.chromium.org/13872017/diff/4001/chrome/common/metrics/prot...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/13872017/diff/4001/chrome/common/metrics/prot...
chrome/common/metrics/proto/system_profile.proto:400: // Vendor prefix of the
Bluetooth address.
On 2013/04/22 19:30:51, keybuk wrote:
> On 2013/04/20 04:54:02, Ilya Sherman wrote:
> > Please document what this string looks like.  Also, is there any chance we
> could
> > upload this as a number, or small tuple of numbers, rather than as a string?

> > That would help assuage privacy concerns.
> > 
> Done; encoded as a uint32_t
> (I couldn't see a useful way to declare it as just three bytes)

Yeah, I'm likewise not aware of any cleaner way to write just three bytes. 
Thanks.

> > Regardless, please make sure to get approval from the privacy and legal
teams
> to
> > track this data.
> 
> In progress currently (since Jan!)

Since we'll actually start uploading the data once this CL lands, it's very
important to get approval from these teams *prior* to landing the code.

https://codereview.chromium.org/13872017/diff/25001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/13872017/diff/25001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:944: hardware));
Sorry, I should have caught this in the previous round of review: If this call
is asynchronous, then there's a really good chance -- possibly a 100% chance --
that it will come in too late to make it into the protocol buffer that's
uploaded to the server.  Generally, any asynchronous fetching of data is handled
by the MetricsService class.

https://codereview.chromium.org/13872017/diff/25001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:1193: vendor_prefix |= component;
Rather than calling HexStringToUInt64 three times, could you just trim out the
colons and call HexStringToUInt64 a single time on the trimmed string?

https://codereview.chromium.org/13872017/diff/25001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.h (right):

https://codereview.chromium.org/13872017/diff/25001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.h:211: // invalidate its weak pointers before
any other members are destroyed.
Hmm, why is the destruction order important?  Is there some particular other
member that relies on this destruction order?

https://codereview.chromium.org/13872017/diff/25001/chrome/common/metrics/pro...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/13872017/diff/25001/chrome/common/metrics/pro...
chrome/common/metrics/proto/system_profile.proto:166: //    
assigned-numbers-overview/baseband
nit: No need to wrap URLs.  They are an exception to the 80-column rule.

[keybuk, 2013-04-23 00:41:41]

https://codereview.chromium.org/13872017/diff/4001/chrome/common/metrics/prot...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/13872017/diff/4001/chrome/common/metrics/prot...
chrome/common/metrics/proto/system_profile.proto:400: // Vendor prefix of the
Bluetooth address.
On 2013/04/23 00:09:43, Ilya Sherman wrote:
> > > Regardless, please make sure to get approval from the privacy and legal
> teams
> > to
> > > track this data.
> > 
> > In progress currently (since Jan!)
> 
> Since we'll actually start uploading the data once this CL lands, it's very
> important to get approval from these teams *prior* to landing the code.

Sure - I had a preliminary "no problem" from the privacy review before I even
wrote the code - I'm just parallelizing the final review with the code review
for efficiency's sake - it won't be committed until there's approval from both

https://codereview.chromium.org/13872017/diff/25001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/13872017/diff/25001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:944: hardware));
On 2013/04/23 00:09:43, Ilya Sherman wrote:
> Sorry, I should have caught this in the previous round of review: If this call
> is asynchronous, then there's a really good chance -- possibly a 100% chance
--
> that it will come in too late to make it into the protocol buffer that's
> uploaded to the server.  Generally, any asynchronous fetching of data is
handled
> by the MetricsService class.

On Chrome OS it's guaranteed to be 100% synchronous, the common method only
appears to be async for the benfit of Windows and OS X where we're not
collecting these metrics.

The MetricsService doesn't use SystemProfileProto, nor any protobuf that I could
see, which is why I opted for the MetricsLog approach with the Chrome OS-only
guard

https://codereview.chromium.org/13872017/diff/25001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:1193: vendor_prefix |= component;
On 2013/04/23 00:09:43, Ilya Sherman wrote:
> Rather than calling HexStringToUInt64 three times, could you just trim out the
> colons and call HexStringToUInt64 a single time on the trimmed string?

Done.

https://codereview.chromium.org/13872017/diff/25001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.h (right):

https://codereview.chromium.org/13872017/diff/25001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.h:211: // invalidate its weak pointers before
any other members are destroyed.
On 2013/04/23 00:09:43, Ilya Sherman wrote:
> Hmm, why is the destruction order important?  Is there some particular other
> member that relies on this destruction order?

comment copied from other code ;) removed

https://codereview.chromium.org/13872017/diff/25001/chrome/common/metrics/pro...
File chrome/common/metrics/proto/system_profile.proto (right):

https://codereview.chromium.org/13872017/diff/25001/chrome/common/metrics/pro...
chrome/common/metrics/proto/system_profile.proto:166: //    
assigned-numbers-overview/baseband
On 2013/04/23 00:09:43, Ilya Sherman wrote:
> nit: No need to wrap URLs.  They are an exception to the 80-column rule.

Done.

[Ilya Sherman, 2013-04-23 01:01:55]

https://codereview.chromium.org/13872017/diff/31001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/13872017/diff/31001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:927:
base::Bind(&MetricsLog::SetBluetoothAdapter,
Looks like the body for this method is missing.

https://codereview.chromium.org/13872017/diff/31001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:929: if (adapter_.get())
Per IM conversation, it sounds like this is expected to always be set on
ChromeOS.  If that's the case, let's drop this if-stmt and go back to
base::Unretained(). with a comment that the call is expected to actually be
synchronous on ChromeOS.  Also, I guess having a DCHECK here couldn't hurt,
since even though the adapter_ is used immediately after the GetAdapter() call,
that code is in a different method, so it's not totally obvious at a glance.

https://codereview.chromium.org/13872017/diff/31001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:1168: RemoveChars(address.substr(0, 9),
":", &vendor_prefix_str);
nit: Just to be extra sure, could you add a DCHECK_EQ(6U,
vendor_prefix_str.size())?

[keybuk, 2013-04-23 01:17:54]

https://codereview.chromium.org/13872017/diff/31001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/13872017/diff/31001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:927:
base::Bind(&MetricsLog::SetBluetoothAdapter,
On 2013/04/23 01:01:55, Ilya Sherman wrote:
> Looks like the body for this method is missing.
oops, editor rebellion

Done.

https://codereview.chromium.org/13872017/diff/31001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:929: if (adapter_.get())
On 2013/04/23 01:01:55, Ilya Sherman wrote:
> Per IM conversation, it sounds like this is expected to always be set on
> ChromeOS.  If that's the case, let's drop this if-stmt and go back to
> base::Unretained(). with a comment that the call is expected to actually be
> synchronous on ChromeOS.  Also, I guess having a DCHECK here couldn't hurt,
> since even though the adapter_ is used immediately after the GetAdapter()
call,
> that code is in a different method, so it's not totally obvious at a glance.

Done.

https://codereview.chromium.org/13872017/diff/31001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:1168: RemoveChars(address.substr(0, 9),
":", &vendor_prefix_str);
On 2013/04/23 01:01:55, Ilya Sherman wrote:
> nit: Just to be extra sure, could you add a DCHECK_EQ(6U,
> vendor_prefix_str.size())?

Done.

[Ilya Sherman, 2013-04-23 04:40:36]

Thanks.  metrics/ changes LG, but I'd like to hold off on final approval until
you get a definitive answer back from the privacy and legal teams.

https://codereview.chromium.org/13872017/diff/35002/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/13872017/diff/35002/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:1169: if (address.length() > 9 &&
Optional nit: I think I've seen .size() rather than .length() more commonly in
Chrome code, but this is fine either way.

https://codereview.chromium.org/13872017/diff/35002/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:1175: DCHECK(vendor_prefix_str.length() ==
6);
nit: Please use DCHECK_EQ

[keybuk, 2013-04-23 16:50:42]

https://codereview.chromium.org/13872017/diff/35002/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log.cc (right):

https://codereview.chromium.org/13872017/diff/35002/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:1169: if (address.length() > 9 &&
On 2013/04/23 04:40:36, Ilya Sherman wrote:
> Optional nit: I think I've seen .size() rather than .length() more commonly in
> Chrome code, but this is fine either way.

Done.

https://codereview.chromium.org/13872017/diff/35002/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log.cc:1175: DCHECK(vendor_prefix_str.length() ==
6);
On 2013/04/23 04:40:36, Ilya Sherman wrote:
> nit: Please use DCHECK_EQ

Done.

[satorux1, 2013-04-23 21:23:15]

chromeos/dbus/ lgtm. optional comments:

https://codereview.chromium.org/13872017/diff/42027/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_device_experimental_chromeos.cc (right):

https://codereview.chromium.org/13872017/diff/42027/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:60: void
ParseModalias(const dbus::ObjectPath& object_path,
function comment is missing. uint16 parameters seem to be optional (NULL is
acceptable)

https://codereview.chromium.org/13872017/diff/42027/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:757: switch
(error_code) {
Converting ConnectErrorCode to UMAPairingResult looks tedious. Would it be bad
to directly use ConnectErrorCode for UMA?

COMPILE_ASSERT may be useful to verify if ConnectErrorCode and UMAPairingResult
are consistent (i.e. comparing the counts)

[Ilya Sherman, 2013-04-23 21:32:26]

https://codereview.chromium.org/13872017/diff/42027/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_device_experimental_chromeos.cc (right):

https://codereview.chromium.org/13872017/diff/42027/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:757: switch
(error_code) {
On 2013/04/23 21:23:15, satorux1 wrote:
> Converting ConnectErrorCode to UMAPairingResult looks tedious. Would it be bad
> to directly use ConnectErrorCode for UMA?
> 
> COMPILE_ASSERT may be useful to verify if ConnectErrorCode and
UMAPairingResult
> are consistent (i.e. comparing the counts)

The really important thing is that enums used with UMA are not allowed to ever
be re-arranged or modified in any way other than appending new values at the
tail.  As long as you can guarantee this behavior, it's fine to structure the
code however you like.  But often the easiest way to guarantee this behavior is
to define a separate enum that's used only with UMA reporting, and have some
translation code like what you see here.

[keybuk, 2013-04-23 21:34:40]

https://codereview.chromium.org/13872017/diff/42027/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_device_experimental_chromeos.cc (right):

https://codereview.chromium.org/13872017/diff/42027/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:60: void
ParseModalias(const dbus::ObjectPath& object_path,
On 2013/04/23 21:23:15, satorux1 wrote:
> function comment is missing. uint16 parameters seem to be optional (NULL is
> acceptable)

they are optional

https://codereview.chromium.org/13872017/diff/42027/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:757: switch
(error_code) {
On 2013/04/23 21:23:15, satorux1 wrote:
> Converting ConnectErrorCode to UMAPairingResult looks tedious. Would it be bad
> to directly use ConnectErrorCode for UMA?
> 
> COMPILE_ASSERT may be useful to verify if ConnectErrorCode and
UMAPairingResult
> are consistent (i.e. comparing the counts)

They're not consistent ...

UMAPairingResult has a "success" case that ConnectErrorCode does not have

[keybuk, 2013-04-26 21:43:45]

Noah Richmond has approved the design doc re: privacy

[Ilya Sherman, 2013-04-26 21:54:29]

On 2013/04/26 21:43:45, keybuk wrote:
> Noah Richmond has approved the design doc re: privacy

Could you please cc me on the email thread?  I think Noah is on the legal team,
not the privacy team.  I'd like to make sure that both teams are happy with this
additional logging.

[Ilya Sherman, 2013-04-27 22:52:31]

Alright, looks like there aren't any serious concerns from the privacy or legal
teams, so LGTM.  Thanks for your patience :)

[keybuk, 2013-04-28 17:03:15]

Committed patchset #12 manually as r196990 (presubmit successful).