Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1256793012:
bluetooth: Add histogram for WebBluetooth actions (Closed)
|
Created:
5 years, 4 months ago by ortuno Modified:
5 years, 4 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@bluetooth-origin Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Add histogram for WebBluetooth actions
This will let us know how to better allocate resources when polishing
the API
BUG=499636
Committed: https://crrev.com/eb2ba9ab7627093f3ef01a3b7cdf44be9bd8ee6c
Cr-Commit-Position: refs/heads/master@{#341772}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Change back to histograms #Patch Set 4 : Reorder function #
Total comments: 10
Patch Set 5 : Change enum name, reorder functions #Patch Set 6 : WebBluetooth -> Bluetooth.Web #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 27 (8 generated)
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org
@jyasskin: PTAL
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256793012/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256793012/20001
IIUC, this records calls to each of the Web Bluetooth API functions, but doesn't (yet?) record the outcome of the call, how long it took, etc. Recording the outcome will naturally give us the number of calls, so maybe we shouldn't record the call itself? On the other hand, we might have bugs where the whole call is dropped, and recording on both ends will catch that. I'd be happy to get guidance on this from the UMA team.
scheib@chromium.org changed reviewers: + scheib@chromium.org
I think RecordAction is designed more for actions users take on Chrome UI surface. I defer to any UMA owner of course, but I think we should be using histograms for API utilization.
On 2015/08/03 at 17:47:34, jyasskin wrote: > IIUC, this records calls to each of the Web Bluetooth API functions, but doesn't (yet?) record the outcome of the call, how long it took, etc. Recording the outcome will naturally give us the number of calls, so maybe we shouldn't record the call itself? On the other hand, we might have bugs where the whole call is dropped, and recording on both ends will catch that. > > I'd be happy to get guidance on this from the UMA team. Yes this is only half of the logging. I didn't want to submit a huge CL with all histograms and logging. You can see the planned CL here: https://docs.google.com/document/d/1dchRzqqqKnpshOrPgHuTNVd-y8k5BGc3jC0jZYLhj...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ortuno@chromium.org changed reviewers: + asvitkine@chromium.org
@asvitkine: We want to record calls to each of the Web Bluetooth API functions. Do you think "RecordAction" is the correct way of doing this or should we use histograms instead?
An enumerated histogram would be better. These are not actions directly triggered by the user - and we have better tools for histograms on the dashboards. Also, you can still see what % of users use the features via the UMA usage dashboard - which supports both actions and histogram buckets. On Mon, Aug 3, 2015 at 4:19 PM, <ortuno@chromium.org> wrote: > @asvitkine: We want to record calls to each of the Web Bluetooth API > functions. > Do you think "RecordAction" is the correct way of doing this or should we > use > histograms instead? > > https://codereview.chromium.org/1256793012/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #3 (id:40001) has been deleted
@asvitkine: Thanks! I changed it back to histograms. I will ping when it's ready for review. @jyasskin: PTAL
lgtm https://codereview.chromium.org/1256793012/diff/70001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1256793012/diff/70001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:58: enum class UMAWebBluetoothFunction { Maybe put these right next to the function that uses them, instead of putting the enums together and then the functions together. https://codereview.chromium.org/1256793012/diff/70001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:63: READ_VALUE, Similarly, mark that these are the characteristic functions. https://codereview.chromium.org/1256793012/diff/70001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1256793012/diff/70001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:71035: + <int value="4" label="readValue()"/> This should be Characteristic.readValue, since we also have BluetoothGATTDescriptor.readValue(). Similarly for writeValue.
https://codereview.chromium.org/1256793012/diff/70001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1256793012/diff/70001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48307: +<histogram name="WebBluetooth.FunctionCall.Count" enum="WebBluetoothFunction"> I suggest not introducing a new top-level prefix. How about Bluetooth.WebFunctionCall.Count?
https://codereview.chromium.org/1256793012/diff/70001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1256793012/diff/70001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:58: enum class UMAWebBluetoothFunction { On 2015/08/03 at 21:51:13, Jeffrey Yasskin wrote: > Maybe put these right next to the function that uses them, instead of putting the enums together and then the functions together. Done. https://codereview.chromium.org/1256793012/diff/70001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:63: READ_VALUE, On 2015/08/03 at 21:51:13, Jeffrey Yasskin wrote: > Similarly, mark that these are the characteristic functions. Done. https://codereview.chromium.org/1256793012/diff/70001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1256793012/diff/70001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48307: +<histogram name="WebBluetooth.FunctionCall.Count" enum="WebBluetoothFunction"> On 2015/08/03 at 21:59:06, Alexei Svitkine wrote: > I suggest not introducing a new top-level prefix. How about Bluetooth.WebFunctionCall.Count? We plan on introducing 15 new WebBluetooth histograms: https://docs.google.com/document/d/1dchRzqqqKnpshOrPgHuTNVd-y8k5BGc3jC0jZYLhj... The WebBluetooth prefix will help us distinguish from the more general Bluetooth histograms. https://codereview.chromium.org/1256793012/diff/70001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:71035: + <int value="4" label="readValue()"/> On 2015/08/03 at 21:51:13, Jeffrey Yasskin wrote: > This should be Characteristic.readValue, since we also have BluetoothGATTDescriptor.readValue(). Similarly for writeValue. Done.
lgtm % comment https://codereview.chromium.org/1256793012/diff/70001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1256793012/diff/70001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48307: +<histogram name="WebBluetooth.FunctionCall.Count" enum="WebBluetoothFunction"> On 2015/08/03 22:41:16, ortuno wrote: > On 2015/08/03 at 21:59:06, Alexei Svitkine wrote: > > I suggest not introducing a new top-level prefix. How about > Bluetooth.WebFunctionCall.Count? > > We plan on introducing 15 new WebBluetooth histograms: > https://docs.google.com/document/d/1dchRzqqqKnpshOrPgHuTNVd-y8k5BGc3jC0jZYLhj... > > The WebBluetooth prefix will help us distinguish from the more general Bluetooth > histograms. How about Bluetooth.Web.FunctionCall.Count then? You can then add other Bluetooth.Web.* histograms.
@scheib: PTAL @asvitkine: Thanks! https://codereview.chromium.org/1256793012/diff/70001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1256793012/diff/70001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:48307: +<histogram name="WebBluetooth.FunctionCall.Count" enum="WebBluetoothFunction"> On 2015/08/04 at 15:35:03, Alexei Svitkine wrote: > On 2015/08/03 22:41:16, ortuno wrote: > > On 2015/08/03 at 21:59:06, Alexei Svitkine wrote: > > > I suggest not introducing a new top-level prefix. How about > > Bluetooth.WebFunctionCall.Count? > > > > We plan on introducing 15 new WebBluetooth histograms: > > https://docs.google.com/document/d/1dchRzqqqKnpshOrPgHuTNVd-y8k5BGc3jC0jZYLhj... > > > > The WebBluetooth prefix will help us distinguish from the more general Bluetooth > > histograms. > > How about Bluetooth.Web.FunctionCall.Count then? You can then add other Bluetooth.Web.* histograms. Sounds good. Done.
LGTM https://codereview.chromium.org/1256793012/diff/110001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1256793012/diff/110001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:77: UMA_HISTOGRAM_ENUMERATION("Bluetooth.Web.FunctionCall.Count", Bluetooth.Web.* LGTM ... we'll rename the others in another patch?
https://codereview.chromium.org/1256793012/diff/110001/content/browser/blueto... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1256793012/diff/110001/content/browser/blueto... content/browser/bluetooth/bluetooth_dispatcher_host.cc:77: UMA_HISTOGRAM_ENUMERATION("Bluetooth.Web.FunctionCall.Count", On 2015/08/04 at 18:30:22, scheib wrote: > Bluetooth.Web.* LGTM ... we'll rename the others in another patch? Yup. There is only one more which is in RequestDevice which will be fixed in a follow up patch.
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyasskin@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1256793012/#ps110001 (title: "WebBluetooth -> Bluetooth.Web")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256793012/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256793012/110001
Message was sent while issue was closed.
Committed patchset #6 (id:110001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/eb2ba9ab7627093f3ef01a3b7cdf44be9bd8ee6c Cr-Commit-Position: refs/heads/master@{#341772} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
