Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2912633002:
bluetooth: macOS: Adding histograms for NSError values (Closed)
Descriptionbluetooth: macOS: Adding histograms for NSError values
BUG=755590
Review-Url: https://codereview.chromium.org/2912633002
Cr-Commit-Position: refs/heads/master@{#495405}
Committed: https://chromium.googlesource.com/chromium/src/+/4f908508fa602b8415510a9f56e6ad9ffe8d9ff7
Patch Set 1 #
Total comments: 28
Patch Set 2 : . #Patch Set 3 : . #
Total comments: 8
Patch Set 4 : Merge #Patch Set 5 : Resolving comments #
Total comments: 8
Patch Set 6 : Vincent's comments #
Total comments: 4
Patch Set 7 : . #Messages
Total messages: 37 (20 generated)
Description was changed from ========== Adding histograms for NSError values BUG= ========== to ========== bluetooth: macOS: Adding histograms for NSError values BUG= ==========
Patchset #1 (id:1) has been deleted
jlebel@chromium.org changed reviewers: + ortuno@chromium.org, scheib@chromium.org
Hello Vincent and Giovanni, If I remember correctly we talked about having histograms to record the macOS errors. I created this patch. Let me know what you think. I'm not sure about the naming I got... Thanks,
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.h (right): https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.h:11: WEB_BLUETOOTH_MACOS_APIS_DID_FAIL_TO_CONNECT_TO_PERIPHERAL = 0, Because this is an enum class we need to spell out the whole name: WebBluetoothMacOSAPIs::WEB_BLUETOOTH_MACOS_APIS_DID_FAIL_TO_CONNECT_PERIPHERAL; So the WEB_BLUETOOTH_MACOS_APIS_ portion becomes redundant. Change it to just: DID_FAIL_TO_CONNECT_TO_PERIPHERAL = 0, DID_DISCONNECT_PERIPHERAL, // etc. https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.h:25: WEB_BLUETOOTH_MACOS_ERRORS_CBATT_ERROR_SUCCESS = 0, Does this mean that we could get error != nil but still have the operation succeeding? If not maybe we should consider error == nil as CBATT_ERROR_SUCCESS. That way we could get a sense of how many operations succeed and how many fail. What do you think? https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.h:44: WEB_BLUETOOTH_MACOS_ERRORS_CBERROR_UNKNOWN = 1000, Do CBErrors values overlap with CBATT errors? If so could we somehow avoid that overlap? We already have most of this values in AndroidGATTStatusResult and it would be nice if we could just reuse them. https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.mm (right): https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.mm:22: if ([error_domain isEqualToString:CBErrorDomain]) { We don't need all these enums. We can just histogram the integer value of the error like we do for android[1] [1] https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device_androi... https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.mm:132: void LogNSErrorToHistogram(NSError* error, Add if (error == nil) { return; } That way we don't have to check before calling this function. optional: I'm not sure which one is better: a big switch statement like we have here or separate functions. What do you think? https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/enums.before.pretty-print.xml (right): https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/enums.before.pretty-print.xml:37047: +<enum name="WebBluetoothMacOSErrors" type="int"> Remove this file. https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6054: +<histogram name="Bluetooth.Web.MacOS.Errors" enum="WebBluetoothMacOSErrors" base="true"> If we use ATT_ERROR_SUCCESS for error == nil then we can name this just Bluetooth.Web.MacOS to match android. https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6056: + <owner>jlebel@chromium.org</owner> Add scheib@chromium.org or maybe just web-bluetooth@chromium.org. scheib: Thoughts? Is there a more general mailing list that will always be monitored? https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:94164: +<histogram_suffises name="WebBluetoothMacOSAPIs" separator="."> I think you mean suffixes instead of suffises.
https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6056: + <owner>jlebel@chromium.org</owner> On 2017/05/31 06:59:52, ortuno wrote: > Add mailto:scheib@chromium.org or maybe just mailto:web-bluetooth@chromium.org. > > scheib: Thoughts? Is there a more general mailing list that will always be > monitored? How about scheib@chromium.org and device-dev@chromium.org
Patchset #2 (id:120001) has been deleted
https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.h (right): https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.h:11: WEB_BLUETOOTH_MACOS_APIS_DID_FAIL_TO_CONNECT_TO_PERIPHERAL = 0, On 2017/05/31 06:59:52, ortuno wrote: > Because this is an enum class we need to spell out the whole name: > > WebBluetoothMacOSAPIs::WEB_BLUETOOTH_MACOS_APIS_DID_FAIL_TO_CONNECT_PERIPHERAL; > > So the WEB_BLUETOOTH_MACOS_APIS_ portion becomes redundant. Change it to just: > > DID_FAIL_TO_CONNECT_TO_PERIPHERAL = 0, > DID_DISCONNECT_PERIPHERAL, > // etc. Done. https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.h:25: WEB_BLUETOOTH_MACOS_ERRORS_CBATT_ERROR_SUCCESS = 0, On 2017/05/31 06:59:52, ortuno wrote: > Does this mean that we could get error != nil but still have the operation > succeeding? If not maybe we should consider error == nil as CBATT_ERROR_SUCCESS. > That way we could get a sense of how many operations succeed and how many fail. > What do you think? We are not supposed to receive it. This is when using -[CBPeripheralManager respondToRequest:withResult:]. This method is on peripheral side to process received request. I guess we should never receive this error as being a central. https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.h:44: WEB_BLUETOOTH_MACOS_ERRORS_CBERROR_UNKNOWN = 1000, On 2017/05/31 06:59:52, ortuno wrote: > Do CBErrors values overlap with CBATT errors? If so could we somehow avoid that > overlap? We already have most of this values in AndroidGATTStatusResult and it > would be nice if we could just reuse them. What do you mean overlap? for the integer values? or for the meaning? https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.mm (right): https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.mm:22: if ([error_domain isEqualToString:CBErrorDomain]) { On 2017/05/31 06:59:52, ortuno wrote: > We don't need all these enums. We can just histogram the integer value of the > error like we do for android[1] > > [1] > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device_androi... So I would have to do: if ([error_domain isEqualToString:CBErrorDomain]) return [error code]; if ([error_domain isEqualToString:CBATTErrorDomain]) return [error code] + 1000; return -1; https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.mm:132: void LogNSErrorToHistogram(NSError* error, On 2017/05/31 06:59:52, ortuno wrote: > Add > > if (error == nil) { > return; > } > > That way we don't have to check before calling this function. > > optional: I'm not sure which one is better: a big switch statement like we have > here or separate functions. What do you think? Done. https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/enums.before.pretty-print.xml (right): https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/enums.before.pretty-print.xml:37047: +<enum name="WebBluetoothMacOSErrors" type="int"> On 2017/05/31 06:59:52, ortuno wrote: > Remove this file. I'm not sure. I should remove this enum from here? https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6054: +<histogram name="Bluetooth.Web.MacOS.Errors" enum="WebBluetoothMacOSErrors" base="true"> On 2017/05/31 06:59:52, ortuno wrote: > If we use ATT_ERROR_SUCCESS for error == nil then we can name this just > Bluetooth.Web.MacOS to match android. Well, we should not receive ATT_ERROR_SUCCESS. But if we do, we definitely get error != nil. I think the suffix is fine. But I can remove it if you prefer. https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6056: + <owner>jlebel@chromium.org</owner> On 2017/05/31 16:17:19, scheib wrote: > On 2017/05/31 06:59:52, ortuno wrote: > > Add mailto:scheib@chromium.org or maybe just > mailto:web-bluetooth@chromium.org. > > > > scheib: Thoughts? Is there a more general mailing list that will always be > > monitored? > > How about mailto:scheib@chromium.org and mailto:device-dev@chromium.org Done. https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:94164: +<histogram_suffises name="WebBluetoothMacOSAPIs" separator="."> On 2017/05/31 06:59:52, ortuno wrote: > I think you mean suffixes instead of suffises. Done.
https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.h (right): https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.h:44: WEB_BLUETOOTH_MACOS_ERRORS_CBERROR_UNKNOWN = 1000, On 2017/06/13 at 22:31:37, jlebel wrote: > On 2017/05/31 06:59:52, ortuno wrote: > > Do CBErrors values overlap with CBATT errors? If so could we somehow avoid that > > overlap? We already have most of this values in AndroidGATTStatusResult and it > > would be nice if we could just reuse them. > > What do you mean overlap? for the integer values? or for the meaning? Yes, do the integer values overlap? https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.mm (right): https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.mm:22: if ([error_domain isEqualToString:CBErrorDomain]) { On 2017/06/13 at 22:31:37, jlebel wrote: > On 2017/05/31 06:59:52, ortuno wrote: > > We don't need all these enums. We can just histogram the integer value of the > > error like we do for android[1] > > > > [1] > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device_androi... > > So I would have to do: > if ([error_domain isEqualToString:CBErrorDomain]) > return [error code]; > if ([error_domain isEqualToString:CBATTErrorDomain]) > return [error code] + 1000; > return -1; Yup, what's the difference between CBErrorDomain and CBATTErrorDomain? Why do we +1000 the CBATTErrorDomain and not CBErrorDomain? https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/enums.before.pretty-print.xml (right): https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/enums.before.pretty-print.xml:37047: +<enum name="WebBluetoothMacOSErrors" type="int"> On 2017/06/13 at 22:31:37, jlebel wrote: > On 2017/05/31 06:59:52, ortuno wrote: > > Remove this file. > > I'm not sure. I should remove this enum from here? This file, called "enums.before.pretty-print.xml", is usually created when running a script that will pretty-print "enums.xml". It seems you added this extra temporary file by accident. We should remove it. https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6054: +<histogram name="Bluetooth.Web.MacOS.Errors" enum="WebBluetoothMacOSErrors" base="true"> On 2017/06/13 at 22:31:38, jlebel wrote: > On 2017/05/31 06:59:52, ortuno wrote: > > If we use ATT_ERROR_SUCCESS for error == nil then we can name this just > > Bluetooth.Web.MacOS to match android. > > Well, we should not receive ATT_ERROR_SUCCESS. But if we do, we definitely get error != nil. > I think the suffix is fine. But I can remove it if you prefer. What bothers me is having to maintain two huge lists of enums. If we can use the same list for both that would be great.
https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.h (right): https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.h:44: WEB_BLUETOOTH_MACOS_ERRORS_CBERROR_UNKNOWN = 1000, On 2017/06/14 03:46:28, ortuno wrote: > On 2017/06/13 at 22:31:37, jlebel wrote: > > On 2017/05/31 06:59:52, ortuno wrote: > > > Do CBErrors values overlap with CBATT errors? If so could we somehow avoid > that > > > overlap? We already have most of this values in AndroidGATTStatusResult and > it > > > would be nice if we could just reuse them. > > > > What do you mean overlap? for the integer values? or for the meaning? > > Yes, do the integer values overlap? Yes, they do. They both start at 0 and increment. https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.mm (right): https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.mm:22: if ([error_domain isEqualToString:CBErrorDomain]) { On 2017/06/14 03:46:29, ortuno wrote: > On 2017/06/13 at 22:31:37, jlebel wrote: > > On 2017/05/31 06:59:52, ortuno wrote: > > > We don't need all these enums. We can just histogram the integer value of > the > > > error like we do for android[1] > > > > > > [1] > > > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device_androi... > > > > So I would have to do: > > if ([error_domain isEqualToString:CBErrorDomain]) > > return [error code]; > > if ([error_domain isEqualToString:CBATTErrorDomain]) > > return [error code] + 1000; > > return -1; > > Yup, what's the difference between CBErrorDomain and CBATTErrorDomain? Why do we > +1000 the CBATTErrorDomain and not CBErrorDomain? Both errors from CBErrorDomain and CBATTErrorDomain starts at 0. I choose 1000, just in case Apple adds error codes. CBErrorDomain error codes can be: https://developer.apple.com/documentation/corebluetooth/cberror?language=objc CBATTErrorDomain error codes can be: https://developer.apple.com/documentation/corebluetooth/cbatterror?language=objc See (not the official version, but it is close enough): https://github.com/mstg/iOS-full-sdk/blob/master/iPhoneOS9.3.sdk/System/Libra... https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/enums.before.pretty-print.xml (right): https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/enums.before.pretty-print.xml:37047: +<enum name="WebBluetoothMacOSErrors" type="int"> On 2017/06/14 03:46:29, ortuno wrote: > On 2017/06/13 at 22:31:37, jlebel wrote: > > On 2017/05/31 06:59:52, ortuno wrote: > > > Remove this file. > > > > I'm not sure. I should remove this enum from here? > > This file, called "enums.before.pretty-print.xml", is usually created when > running a script that will pretty-print "enums.xml". It seems you added this > extra temporary file by accident. We should remove it. Done. https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2912633002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:6054: +<histogram name="Bluetooth.Web.MacOS.Errors" enum="WebBluetoothMacOSErrors" base="true"> On 2017/06/14 03:46:29, ortuno wrote: > On 2017/06/13 at 22:31:38, jlebel wrote: > > On 2017/05/31 06:59:52, ortuno wrote: > > > If we use ATT_ERROR_SUCCESS for error == nil then we can name this just > > > Bluetooth.Web.MacOS to match android. > > > > Well, we should not receive ATT_ERROR_SUCCESS. But if we do, we definitely get > error != nil. > > I think the suffix is fine. But I can remove it if you prefer. > > What bothers me is having to maintain two huge lists of enums. If we can use the > same list for both that would be great. AndroidGATTStatusResult is definitely matching CBATTErrorDomain on macOS. But I was trying to create one histogram per method which can receive an error, those histograms were going to contain all possible errors. There is no guaranty about the error domain that a method can receive. I worked on CBErrorDomain and CBATTErrorDomain, but we could definitely receive other domains. And we have to keep in mind the error code is unique per error domain. If I use Bluetooth.Web.Android, then I would have to create an histogram per pair (method, error domain). I'm guessing some API are more likely to receive errors about CBATTErrorDomain and others about CBErrorDomain. But I see nothing related to that in Apple's doc.
https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.mm (right): https://codereview.chromium.org/2912633002/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.mm:22: if ([error_domain isEqualToString:CBErrorDomain]) { On 2017/06/19 at 07:07:49, jlebel wrote: > On 2017/06/14 03:46:29, ortuno wrote: > > On 2017/06/13 at 22:31:37, jlebel wrote: > > > On 2017/05/31 06:59:52, ortuno wrote: > > > > We don't need all these enums. We can just histogram the integer value of > > the > > > > error like we do for android[1] > > > > > > > > [1] > > > > > > https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device_androi... > > > > > > So I would have to do: > > > if ([error_domain isEqualToString:CBErrorDomain]) > > > return [error code]; > > > if ([error_domain isEqualToString:CBATTErrorDomain]) > > > return [error code] + 1000; > > > return -1; > > > > Yup, what's the difference between CBErrorDomain and CBATTErrorDomain? Why do we > > +1000 the CBATTErrorDomain and not CBErrorDomain? > > Both errors from CBErrorDomain and CBATTErrorDomain starts at 0. I choose 1000, just in case Apple adds error codes. > > CBErrorDomain error codes can be: > https://developer.apple.com/documentation/corebluetooth/cberror?language=objc > > CBATTErrorDomain error codes can be: > https://developer.apple.com/documentation/corebluetooth/cbatterror?language=objc > > See (not the official version, but it is close enough): > https://github.com/mstg/iOS-full-sdk/blob/master/iPhoneOS9.3.sdk/System/Libra... I see. Let's do what you propose instead of using all these enums: > > So I would have to do: > > if ([error_domain isEqualToString:CBErrorDomain]) > > return [error code]; > > if ([error_domain isEqualToString:CBATTErrorDomain]) > > return [error code] + 1000; > > return -1; https://codereview.chromium.org/2912633002/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.h (right): https://codereview.chromium.org/2912633002/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.h:59: void LogDidFailToConnectPeripheralErrorToHistogram(NSError* error); To match web bluetooth's methods: RecordDidFailToConnectPeripheralResult(NSError* error); https://codereview.chromium.org/2912633002/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.mm (right): https://codereview.chromium.org/2912633002/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.mm:19: WebBluetoothMacOSErrors GetWebBluetoothMacOSErrorsFromNSError(NSError* error) { What do you think about renaming this to: GetMacOSOperationResultFromNSError(NSError* error)? https://codereview.chromium.org/2912633002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2912633002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:37709: +<enum name="WebBluetoothMacOSErrors" type="int"> MacOSBluetoothOperationsResult https://codereview.chromium.org/2912633002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:37710: + <int value="-1" label="UnknownErrorDomain"/> What do you think of the following: <int value="-2" label="UnknownErrorDomain"/> <int value="-1" label="NoError"/> <int value="0" label="CBATTErrorSuccess"/> And in GetWebBluetoothMacOSErrorsFromNSError: if (!error) return -1 /* NoError */ That way we can record how many operations fail and how many succeed.
Description was changed from ========== bluetooth: macOS: Adding histograms for NSError values BUG= ========== to ========== bluetooth: macOS: Adding histograms for NSError values BUG= 755590 ==========
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
Patchset #4 (id:220001) has been deleted
Patchset #4 (id:240001) has been deleted
Patchset #6 (id:300001) has been deleted
Hello Giovanni, Sorry for the delay about this patch. Also, I did update enums.xml and histograms.xml with those changes: http://uu.zoy.org/p/fgz03w#x=1bHNAKbNVABEdtMA But I can't upload because those files are too big. But it looks like I will be able to commit that according to this post: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/si2AC_VlSEM/69cIH... Thanks, Jérôme, https://codereview.chromium.org/2912633002/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.h (right): https://codereview.chromium.org/2912633002/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.h:59: void LogDidFailToConnectPeripheralErrorToHistogram(NSError* error); On 2017/06/20 05:59:40, ortuno wrote: > To match web bluetooth's methods: > > RecordDidFailToConnectPeripheralResult(NSError* error); Done. https://codereview.chromium.org/2912633002/diff/160001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.mm (right): https://codereview.chromium.org/2912633002/diff/160001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.mm:19: WebBluetoothMacOSErrors GetWebBluetoothMacOSErrorsFromNSError(NSError* error) { On 2017/06/20 05:59:40, ortuno wrote: > What do you think about renaming this to: > > GetMacOSOperationResultFromNSError(NSError* error)? Done. https://codereview.chromium.org/2912633002/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2912633002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:37709: +<enum name="WebBluetoothMacOSErrors" type="int"> On 2017/06/20 05:59:41, ortuno wrote: > MacOSBluetoothOperationsResult Done. https://codereview.chromium.org/2912633002/diff/160001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:37710: + <int value="-1" label="UnknownErrorDomain"/> On 2017/06/20 05:59:40, ortuno wrote: > What do you think of the following: > > <int value="-2" label="UnknownErrorDomain"/> > <int value="-1" label="NoError"/> > <int value="0" label="CBATTErrorSuccess"/> > > And in GetWebBluetoothMacOSErrorsFromNSError: > > if (!error) > return -1 /* NoError */ > > > That way we can record how many operations fail and how many succeed. Done.
https://codereview.chromium.org/2912633002/diff/280001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.h (right): https://codereview.chromium.org/2912633002/diff/280001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.h:10: enum class WebBluetoothMacOSAPIs : int { I don't think this enum is used, and can be removed. If it is used, it shouldn't have 'WebBluetooth' in the name, as this is recording values common to all core bluetooth & device/bluetooh clients. https://codereview.chromium.org/2912633002/diff/280001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.mm (right): https://codereview.chromium.org/2912633002/diff/280001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.mm:48: NOTREACHED(); Please create and record something like CBATT_ERROR_UNKNOWN_ERROR_CODE https://codereview.chromium.org/2912633002/diff/280001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.mm:98: return MacOSBluetoothOperationsResult::CBATT_ERROR_MAX; Instead, something named more clearly such as CBERROR_UNKNOWN_ERROR_CODE https://codereview.chromium.org/2912633002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2912633002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:100040: + <suffix name="DidUpdateValue" label="Updated value"/> Updated value for characteristic
Thanks Vincent, The diff for enums.xml and histograms.xml: http://uu.zoy.org/p/kZx9aA#x=drK/AAqzYQD/emYA You are right, I don't use the WebBluetoothMacOSAPIs enum in the code anymore. But it is still defined in histograms.xml. I'm not sure what name you want me to put if I don't put WebBluetooth in it. https://codereview.chromium.org/2912633002/diff/280001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.h (right): https://codereview.chromium.org/2912633002/diff/280001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.h:10: enum class WebBluetoothMacOSAPIs : int { On 2017/08/15 22:20:28, scheib - Prefer gerrit tool wrote: > I don't think this enum is used, and can be removed. > > If it is used, it shouldn't have 'WebBluetooth' in the name, as this is > recording values common to all core bluetooth & device/bluetooh clients. Done. https://codereview.chromium.org/2912633002/diff/280001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac_metrics.mm (right): https://codereview.chromium.org/2912633002/diff/280001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.mm:48: NOTREACHED(); On 2017/08/15 22:20:28, scheib - Prefer gerrit tool wrote: > Please create and record something like > CBATT_ERROR_UNKNOWN_ERROR_CODE Done. https://codereview.chromium.org/2912633002/diff/280001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac_metrics.mm:98: return MacOSBluetoothOperationsResult::CBATT_ERROR_MAX; On 2017/08/15 22:20:28, scheib - Prefer gerrit tool wrote: > Instead, something named more clearly such as > CBERROR_UNKNOWN_ERROR_CODE Done. https://codereview.chromium.org/2912633002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2912633002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:100040: + <suffix name="DidUpdateValue" label="Updated value"/> On 2017/08/15 22:20:28, scheib - Prefer gerrit tool wrote: > Updated value for characteristic Done.
Thanks, one nit pick left and then LGTM. https://codereview.chromium.org/2912633002/diff/320001/tools/metrics/histogra... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2912633002/diff/320001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:24151: + <int value="999" label="CBATTErrorUnknownErrorCode"/> Let's make the 'unknown' codes not look as if they are actual CBATTError codes: "unknown CBATTError code" https://codereview.chromium.org/2912633002/diff/320001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:24163: + <int value="1999" label="CBErrorUnknownErrorCode"/> ditto
Thanks, So I guess the xml files are uploaded. https://codereview.chromium.org/2912633002/diff/320001/tools/metrics/histogra... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2912633002/diff/320001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:24151: + <int value="999" label="CBATTErrorUnknownErrorCode"/> On 2017/08/15 23:39:21, scheib - Prefer gerrit tool wrote: > Let's make the 'unknown' codes not look as if they are actual CBATTError codes: > "unknown CBATTError code" Done. https://codereview.chromium.org/2912633002/diff/320001/tools/metrics/histogra... tools/metrics/histograms/enums.xml:24163: + <int value="1999" label="CBErrorUnknownErrorCode"/> On 2017/08/15 23:39:21, scheib - Prefer gerrit tool wrote: > ditto Done.
jlebel@chromium.org changed reviewers: + holte@google.com
Hello Steven, Can you review this patch to add histograms for error codes from macOS? Thanks,
holte@chromium.org changed reviewers: + holte@chromium.org
lgtm
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org Link to the patchset: https://codereview.chromium.org/2912633002/#ps340001 (title: ".")
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": 340001, "attempt_start_ts": 1503009205353220,
"parent_rev": "c6eb0d4262f940537b289699fbd28f67d430f540", "commit_rev":
"4f908508fa602b8415510a9f56e6ad9ffe8d9ff7"}
Message was sent while issue was closed.
Description was changed from ========== bluetooth: macOS: Adding histograms for NSError values BUG= 755590 ========== to ========== bluetooth: macOS: Adding histograms for NSError values BUG= 755590 Review-Url: https://codereview.chromium.org/2912633002 Cr-Commit-Position: refs/heads/master@{#495405} Committed: https://chromium.googlesource.com/chromium/src/+/4f908508fa602b8415510a9f56e6... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:340001) as https://chromium.googlesource.com/chromium/src/+/4f908508fa602b8415510a9f56e6...
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 495405 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |
