Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2458163003:
Add UMA for WebBluetooth RSSI signal strength level (Closed)
DescriptionAdd UMA for WebBluetooth RSSI signal strength level
In order to better understand the Bluetooth device signal strength
distributions, this CL adds UMA for RSSI signal strength level in
WebBluetooth.
BUG=651562
Committed: https://crrev.com/0b428c36220831fc53daf5e540fad3e8f5328c6f
Cr-Commit-Position: refs/heads/master@{#428911}
Patch Set 1 : add UMA for WebBluetooth RSSI signal strength level #
Total comments: 2
Patch Set 2 : address comments #Patch Set 3 : updated histogram_macros.h comments #
Total comments: 14
Patch Set 4 : address comments #
Messages
Total messages: 29 (19 generated)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
juncai@chromium.org changed reviewers: + scheib@chromium.org
Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Keep the current histogram as well, because it will be useful as well. But, I think the original and more important intent is to understand what RSSI values exist as they are input into CalculateSignalStrengthLevel, before we put them into buckets. This would allow us to adjust the buckets to have a better display of RSSI values. So, it's essential that we capture a non-enum histogram. Look in histogram_macros.h to find an appropriate one. You need to remap values from std::numeric_limits<int8_t>::min() .. max() [-128, 127] range into a valid range for the histograms type selected. RSSI is already in log space, so I think you want a linear histogram. So, a quick look makes me think UMA_HISTOGRAM_EXACT_LINEAR (50 buckets may be enough? Hmm, maybe UMA_HISTOGRAM_PERCENTAGE). https://codereview.chromium.org/2458163003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2458163003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:4770: + Records what the Bluetooth device RSSI signal strength distributions are. Displayed Bluetooth device RSSI signal strengths in Web Bluetooth chooser. (remove later lines)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Use the UMA_HISTOGRAM_SPARSE_SLOWLY() to record the RSSI signal strength value. It can also record negative values as well. https://codereview.chromium.org/2458163003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2458163003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:4770: + Records what the Bluetooth device RSSI signal strength distributions are. On 2016/10/29 00:03:48, scheib wrote: > Displayed Bluetooth device RSSI signal strengths in Web Bluetooth chooser. > (remove later lines) Done.
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with some comment/string changes & a DCHECK. https://codereview.chromium.org/2458163003/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right): https://codereview.chromium.org/2458163003/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:411: RecordRSSISignalStrengthLevel(kRSSISignalStrengthEnumTable[level]); DCHECK(kNumSignalStrengthLevels == arraysize(kRSSISignalStrengthEnumTable)); https://codereview.chromium.org/2458163003/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_metrics.h (right): https://codereview.chromium.org/2458163003/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_metrics.h:291: // Records the RSSI signal strength and level when "Records the raw RSSI, and processed result displayed to users, when" https://codereview.chromium.org/2458163003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2458163003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4769: + Bluetooth device RSSI signal strengths in Web Bluetooth chooser. Raw RSSI values provided to chooser, before processing them for display in the Web Bluetooth chooser. https://codereview.chromium.org/2458163003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4780: + Displayed Bluetooth device RSSI signal strengths in Web Bluetooth chooser. Displayed RSSI levels, after processing and as displayed to users in the Web Bluetooth chooser. https://codereview.chromium.org/2458163003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:103210: + <int value="0" label="Less than or equal to minimum RSSI"/> <= minimum RSSI; displayed as level 0 https://codereview.chromium.org/2458163003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:103211: + <int value="1" label="RSSI signal strength level 0"/> RSSI displayed as level 0 (Remove redundant 'signal strength', because RSSI is "received signal strength indicator") https://codereview.chromium.org/2458163003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:103216: + <int value="6" label="Greater than or equal to maximum RSSI"/> >= maximium RSSI; displayed as level 4
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
juncai@chromium.org changed reviewers: + haraken@chromium.org, isherman@chromium.org
isherman@chromium.org: Please review changes in //base/metrics/histogram_macros.h haraken@chromium.org: Please review changes in //tools/metrics/histograms/histograms.xml https://codereview.chromium.org/2458163003/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_device_chooser_controller.cc (right): https://codereview.chromium.org/2458163003/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_device_chooser_controller.cc:411: RecordRSSISignalStrengthLevel(kRSSISignalStrengthEnumTable[level]); On 2016/10/31 22:22:32, scheib wrote: > DCHECK(kNumSignalStrengthLevels == arraysize(kRSSISignalStrengthEnumTable)); Done. https://codereview.chromium.org/2458163003/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_metrics.h (right): https://codereview.chromium.org/2458163003/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_metrics.h:291: // Records the RSSI signal strength and level when On 2016/10/31 22:22:32, scheib wrote: > "Records the raw RSSI, and processed result displayed to users, when" Done. https://codereview.chromium.org/2458163003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2458163003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4769: + Bluetooth device RSSI signal strengths in Web Bluetooth chooser. On 2016/10/31 22:22:32, scheib wrote: > Raw RSSI values provided to chooser, before processing them for display in the > Web Bluetooth chooser. Done. https://codereview.chromium.org/2458163003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:4780: + Displayed Bluetooth device RSSI signal strengths in Web Bluetooth chooser. On 2016/10/31 22:22:32, scheib wrote: > Displayed RSSI levels, after processing and as displayed to users in the Web > Bluetooth chooser. Done. https://codereview.chromium.org/2458163003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:103210: + <int value="0" label="Less than or equal to minimum RSSI"/> On 2016/10/31 22:22:32, scheib wrote: > <= minimum RSSI; displayed as level 0 Done. https://codereview.chromium.org/2458163003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:103211: + <int value="1" label="RSSI signal strength level 0"/> On 2016/10/31 22:22:32, scheib wrote: > RSSI displayed as level 0 > (Remove redundant 'signal strength', because RSSI is "received signal strength > indicator") Done. https://codereview.chromium.org/2458163003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:103216: + <int value="6" label="Greater than or equal to maximum RSSI"/> On 2016/10/31 22:22:32, scheib wrote: > >= maximium RSSI; displayed as level 4 Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
metrics lgtm, thanks
The CQ bit was checked by isherman@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/2458163003/#ps60001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add UMA for WebBluetooth RSSI signal strength level In order to better understand the Bluetooth device signal strength distributions, this CL adds UMA for RSSI signal strength level in WebBluetooth. BUG=651562 ========== to ========== Add UMA for WebBluetooth RSSI signal strength level In order to better understand the Bluetooth device signal strength distributions, this CL adds UMA for RSSI signal strength level in WebBluetooth. BUG=651562 Committed: https://crrev.com/0b428c36220831fc53daf5e540fad3e8f5328c6f Cr-Commit-Position: refs/heads/master@{#428911} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0b428c36220831fc53daf5e540fad3e8f5328c6f Cr-Commit-Position: refs/heads/master@{#428911}
Message was sent while issue was closed.
LGTM |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
