refactor(app): Collect system USB information #5482

mcous · 2020-04-23T21:24:35Z

overview

This PR closes #5358 by adding USB device information collection to the app. See ticket for acceptance criteria.

changelog

refactor(app): Collect system USB information

review requests

Functionality should be tested on all three OS's with the enableSystemInfo feature flag on. Simple test plan:

On boot, without U2E adapter plugged in
- Nothing that special happens, but you should see a list of devices in state.systemInfo.usbDevices
Plug in a Realtek U2E adapter
- Adapter should show up in state.systemInfo.usbDevices
- If Windows, device in state should have windowsDriverVersion field set to a string (null means something went wrong)
- Console log should note an analytics event and an Intercom update adding information about the U2E adapter to the user's analytics profile and to their Intercom profile
- If Windows, profile information should include driver version
Quit app and relaunch with U2E adapter still plugged in
- Adapter should show up in state.systemInfo.usbDevices
- If Windows, device in state should have windowsDriverVersion field set to a string (null means something went wrong)
- Console log should note an analytics event and an Intercom update adding information about the U2E adapter to the user's analytics profile and to their Intercom profile
- If Windows, profile information should include driver version
Remove U2E adapter
- Adapter should be removed from state.systemInfo.usbDevices

risk assessment

Low, feature is currently behind the enableSystemInfo feature flag. Existing Intercom support profile was found to be buggy and un-tested, so I refactored some of it and covered it in tests

mcous · 2020-04-23T21:31:23Z

Because the usb-detection module is a native dependency, we're running into an issue building the mac dev build on the Linux CI machine, which is what we currently do for non-release branch builds. We have a few options:

Wait and hope that build(prebuild): update prebuild dependencies to latest MadLittleMods/node-usb-detection#106 gets merged and a new release is pushed, which should mean we can download pre-built binaries at build time rather than build them ourselves
Tweak our CI config to move our branch mac builds to an actual macOS machine
Stop building macOS branch builds

codecov · 2020-04-23T21:37:59Z

Codecov Report

Merging #5482 into edge will increase coverage by 0.88%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             edge    #5482      +/-   ##
==========================================
+ Coverage   65.65%   66.54%   +0.88%     
==========================================
  Files        1084     1095      +11     
  Lines       30918    31952    +1034     
==========================================
+ Hits        20300    21262     +962     
- Misses      10618    10690      +72

Impacted Files	Coverage Δ
app-shell/src/main.js	`0.00% <ø> (ø)`
app/src/config/constants.js	`100.00% <ø> (ø)`
app/src/epic.js	`0.00% <ø> (ø)`
app/src/index.js	`0.00% <0.00%> (ø)`
app/src/reducer.js	`0.00% <ø> (ø)`
app/src/shell/index.js	`100.00% <ø> (ø)`
app/src/support/index.js	`0.00% <0.00%> (ø)`
app/src/support/profile.js	`56.00% <56.00%> (ø)`
app/src/system-info/reducer.js	`87.50% <87.50%> (ø)`
app-shell/src/system-info/usb-devices.js	`91.30% <91.30%> (ø)`
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f7fa08...37f2997. Read the comment docs.

sfoster1

Some inline comments

sfoster1 · 2020-04-24T13:28:59Z

.travis.yml

@@ -87,6 +87,8 @@ jobs:
      name: 'JS unit tests; build Protocol Designer, Labware Library, Components Library'
      # node version pulled from .nvmrc
      language: node_js
+      before_install:
+        - sudo apt-get install -y --no-install-recommends libudev-dev


this is only a build dep, right?

Yeah, but hopefully it's unnecessary now that usb-detection is publishing prebuilt binaries for the correct node versions. Still evaluating but will likely updaate the travis config once more

Looks like there's some continuing native build shenanigans. For now, it appears we need both the prebuilt binaries and these build dependencies in place for everything to work. My hunch is that there's a problem with the Linux prebuilt binaries.

The usb-detection author appears to be in the middle of a node-gyp upgrade, so I'll take a look at removing these deps when that update is released. Keeping in place right now so we can have successful builds.

sfoster1 · 2020-04-24T13:31:17Z

app-shell/src/system-info/index.js

+
+const addDriverVersion = (device: Device): Promise<UsbDevice> => {
+  if (isWindows() && RE_REALTEK.test(device.manufacturer)) {
+    return getWindowsDriverVersion(device).then(windowsDriverVersion => ({


we'll be ok if this gets called for two different devices right? Like if the user has a realtek USB wifi adapter or something?

I don't see why the underlying PowerShell call would care, and the individual device states are separate in the application state, so the results of each individual call will go to the correct place

sfoster1 · 2020-04-24T13:44:13Z

app-shell/src/system-info/index.js

+import type { Action, Dispatch } from '../types'
+import type { UsbDeviceMonitor, Device } from './usb-devices'
+
+const RE_REALTEK = /realtek/i


I would love to see this filtered on vendor ID rather than a string match

I think you can even bake a VID into the usb-detector command string

Avoiding locking ourselves to the VID was a conscious decision. IMO we don't have enough information to know that the VID is stable enough to filter on. I'd like to go wide right now and use the analytics info collected (including VID) to verify that we can narrow it down. I've def seen VID / PID shenanigans with counterfeit USB devices in China and I'm sure you've seen the same

Yup fair enough

sfoster1 · 2020-04-24T13:55:04Z

app/src/system-info/utils.js

+  return result
+}
+
+export const isRealtekDevice = (device: UsbDevice) => {


We appear to check for this in a couple different places, could we check it only on the boundary to the usb detector so we can make this log whatever usb devices come through?

This utility function is used to take the wide list of USB devices in state and narrow it down for updating the user's analytics and support profiles with the Realtek information if we see it. For similar reasons to going wide w.r.t. VID, I'd like to go wide on USB devices collection at the state level

If your concern is "this function should log when it sees a Realtek device", then that logging is already happening via MixPanel and Intercom

I'm a bit uncomfortable with wide logging of every USB device a user has plugged in to their system, but I guess we don't use it anywhere

Yeah I'd really like to re-evaluate and narrow down once we do have some of that initial data. To be clear, only "realtek" devices get sent up to MixPanel and Intercom. Everything else just sits in state.

There are a few instances where having everything in state could prove useful that I can think of:

It seems like (on the dongles I have) the USB device serial number matches the MAC address of the interface, so we may be able to match up the U2E interface (feat: Add system networking interface info collection to app #5397) with an actual device, "Realtek" or not

Detecting if the device is plugged in via a hub

app-shell/src/system-info/usb-devices.js

amitlissack

Great stuff.

amitlissack · 2020-04-27T13:51:41Z

app-shell/src/system-info/usb-devices.js

+  }
+}
+
+const decToHex = (number: number) =>


Sooooo functional! ;)

amitlissack · 2020-04-27T13:56:39Z

app/src/support/profile.js

+    case RobotSettings.FETCH_SETTINGS_SUCCESS:
+    case RobotSettings.UPDATE_SETTING_SUCCESS:
+    case Pipettes.FETCH_PIPETTES_SUCCESS: {
+      const robot = getConnectedRobot(state)


Nit: This is a lot of code in a case statement. Perhaps it's justified to be it's own function?

Yeah that's a good call. This was more or less copied over from the previous support profile implementation, it's doing too much, and it's got no unit tests. I'll add a TODO comment + ticket for now

app/src/system-info/__tests__/reducer.test.js

Kadee80

🌽

code looks sane
redux work for init, add, and remove all perform as expected

sanni-t

👍Worked on windows!

dozercodes

Works as expected. Tests look good

sfoster1

Works for me on Linux

Closes #5358

mcous and others added 7 commits April 20, 2020 13:09

wip: start work on collecting usb device info

4f27b0f

wip: get usb detection working end-to-end

a6b4036

wip: get electron-rebuild task properly configured

d2a8357

wip: ensure hex codes are padded in PowerShell driver version call

1c6c639

wip: add U2E information to analytics and support profiles

c958061

wip: install libusb-dev on linux to keep make install happy

b8cb7f2

Merge branch 'edge' into app-shell_system-info-collection

b66ab00

mcous added feature Ticket is a feature request / PR introduces a feature app Affects the `app` project ready for review labels Apr 23, 2020

mcous requested a review from a team April 23, 2020 21:24

mcous requested review from a team as code owners April 23, 2020 21:24

mcous requested review from amitlissack and sanni-t and removed request for a team April 23, 2020 21:24

mcous removed the ready for review label Apr 23, 2020

fixup: upgrade usb-detection for prebuilt binaries

a9cc27e

sfoster1 reviewed Apr 24, 2020

View reviewed changes

mcous added 2 commits April 24, 2020 11:05

fixup: add build dep to js e2e test

821cc64

fixup: add build dep to js e2e test (part 2)

c2b4ca7

mcous added the ready for review label Apr 24, 2020

mcous commented Apr 24, 2020

View reviewed changes

app-shell/src/system-info/usb-devices.js Outdated Show resolved Hide resolved

fixup: clean up usb-detection event listeners on stop monitoring

3e827d3

amitlissack approved these changes Apr 27, 2020

View reviewed changes

mcous mentioned this pull request Apr 27, 2020

App: Clean-up pipette and robot support profile update logic #5490

Closed

fixup: teak support profile TODO for pipette + robot info

c2cbee6

mcous removed the request for review from a team April 27, 2020 16:23

mcous requested review from a team and Kadee80 and removed request for a team April 27, 2020 16:23

mcous commented Apr 27, 2020

View reviewed changes

app/src/system-info/__tests__/reducer.test.js Outdated Show resolved Hide resolved

fixup: remove unnecessary comment

9bc408c

Kadee80 approved these changes Apr 27, 2020

View reviewed changes

sanni-t approved these changes Apr 27, 2020

View reviewed changes

dozercodes approved these changes Apr 28, 2020

View reviewed changes

fixup: move analytics out from behind FF

37f2997

sfoster1 approved these changes Apr 28, 2020

View reviewed changes

mcous merged commit e5a448b into edge Apr 28, 2020

mcous deleted the app-shell_system-info-collection branch April 28, 2020 17:42

b-cooper pushed a commit that referenced this pull request Apr 30, 2020

refactor(app): Collect system USB information (#5482)

ddff0a3

Closes #5358

mcous mentioned this pull request May 7, 2020

ci(travis): remove unnecessary libudev build dependency #5601

Merged

1 task

mcous mentioned this pull request May 20, 2020

fix(app): tighten filter logic that identifies Realtek U2E adapters #5707

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(app): Collect system USB information #5482

refactor(app): Collect system USB information #5482

mcous commented Apr 23, 2020

mcous commented Apr 23, 2020

codecov bot commented Apr 23, 2020 •

edited

Loading

sfoster1 left a comment

sfoster1 Apr 24, 2020

mcous Apr 24, 2020

mcous Apr 24, 2020

sfoster1 Apr 24, 2020

mcous Apr 24, 2020

sfoster1 Apr 24, 2020

sfoster1 Apr 24, 2020

mcous Apr 24, 2020

sfoster1 Apr 24, 2020

sfoster1 Apr 24, 2020

mcous Apr 24, 2020

sfoster1 Apr 24, 2020

mcous Apr 24, 2020

amitlissack left a comment

amitlissack Apr 27, 2020

amitlissack Apr 27, 2020

mcous Apr 27, 2020

mcous Apr 27, 2020

Kadee80 left a comment

sanni-t left a comment

dozercodes left a comment

sfoster1 left a comment

refactor(app): Collect system USB information #5482

refactor(app): Collect system USB information #5482

Conversation

mcous commented Apr 23, 2020

overview

changelog

review requests

risk assessment

mcous commented Apr 23, 2020

codecov bot commented Apr 23, 2020 • edited Loading

Codecov Report

sfoster1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitlissack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kadee80 left a comment

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

dozercodes left a comment

Choose a reason for hiding this comment

sfoster1 left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 23, 2020 •

edited

Loading