remove info request

[blakeroberts-wk]

Motivation

The initial /info XHR tends to add ~200 ms to websocket connections in released/deployed environments due to public internet round trip latency.

Changes

Removed the /info request replacing its response with sane defaults.

Release Notes

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review:

QA Checklist

• Tests were updated and provide good coverage of the changeset and other affected code
• Manual testing was performed if needed
  • Steps from PR author:
  • Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

• A Frontend Architecture member has reviewed these changes
• There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
• For release PRs - Version metadata in Rosie comment is correct

[blakeroberts-wk]

QA +1 the test app produces the following output in the console:

js_primitives.dart:30 Starting example
js_primitives.dart:30 OPEN: websocket http://localhost:9009/echo http://localhost:9009/echo/703/pwtrcgtj
js_primitives.dart:30 MSG: test
js_primitives.dart:30 MSG: test 2
js_primitives.dart:30 CLOSE: 4002 Normal closure (wasClean true)

I also pulled this change into our internal wdesk_sdk test app and verified that we could establish a websocket connection with our frontend.

[brettkail-wk]

The initial /info XHR tends to add ~200 ms

Out of curiosity, how did 100 get picked as the default (vs 200 mentioned in the description)?

[blakeroberts-wk]

The p75 of window.navigator.connection?.rtt for prod users is 100 ms.

And the 200 ms was the p75 of the /info request for prod users prior to some other optimizations that have since caused the value to increase to ~350 ms. That increase is most likely due to client side resource contention because the optimizations tend to be things that further saturate the CPU by allowing execution of more async tasks. So the browser context switches to a bunch of stuff then finally comes back to resolving the request at a later time than it would have had there not been as much other stuff to do.

Also worth noting that connection is null for firefox but will contain a value for chrome/edge.

[brettkail-wk]

Thanks, the || 100 makes sense. Out of curiosity, would (rtt || 100) * 2 or 4 more closely match the current semantics?

(I haven't looked at how this value is actually used, so I'm just learning. Feel free to ignore me :-).)

[travissmith-wf]

The rtt value is used to create the transport timeout in the countRTO() method: https://github.com/Workiva/sockjs_client_wrapper/blob/master/lib/sockjs.js#L976-L987

This is what triggers the SockJS fallback – if the websocket connect request takes longer than the RTO value, then it will simply try the next transport option(s).

[blakeroberts-wk]

The way this rtt value is currently used is:

1. rto = rtt > 100 ? 4 * rtt : 300 + rtt here
2. ws connect timeout = rto * 2 here

For our users, even the p50 of /info is just over 100. So most of the time, the connect timeout is set to something a bit over 100 * 4 * 2 = 800. So by defaulting to 100, firefox users will see a reduction to their timeout of about 40 ms most of the time. For chrome/edge users I wouldn't think there'd be much difference because I would assume the browser's estimated rtt will tend to match the rtt of the /info request.

TBH, it's probably advisable to set a hardcoded timeout using the options passed to the constructor of SockJS. But even that can be overridden at runtime by (2) because it actually does timeout = max(options.timeout, ~800).

[travissmith-wf]

Do we also need to make the change in /lib/sockjs_prod.js? I'm unsure how we bundle things and push to our CDN...

[blakeroberts-wk]

Do we also need to make the change in /lib/sockjs_prod.js? I'm unsure how we bundle things and push to our CDN...

Right now, our app points to sockjs.js, not sockjs_prod.js. But it still seems appropriate to regenerate the minified version. I added an npm script to minify the js and generate a new source map. I deleted the old source map for the unminified code since the unminified code is now the source of truth.

[evanweible-wf]

Changes LGTM, but since we're making an edit to sockjs.js (which was previously just vendored from sockjs-client) and treating it as our source file now, could we add a comment either at the top of this file or in the readme to that effect? I noticed we removed the sockjs.js.map file (makes sense given this change), but we should also remove the //# sourceMappingURL=sockjs.js.map at the bottom of this file.

[blakeroberts-wk]

QA +1 successfully established a websocket connection on chrome, firefox, and safari

[blakeroberts-wk]

@Workiva/release-management-p

[rmconsole-wf]

+1 from RM