feat(SIP-39): Websocket sidecar app #11498

robdiciuccio · 2020-10-30T01:10:33Z

SUMMARY

Node.js WebSocket server application for SIP-39 pub/sub architecture. This PR contains:

The main WebSocket application (superset-websocket/src/index.ts)
- Consumes Redis streams for event data, pushing to connected WebSocket clients
- Utilizes signed JWT for authentication from Superset frontend
- Leverages WebSocket ping/pong for connection management
- Configurable connection timeout and garbage collection settings
A new Superset Flask app config key: GLOBAL_ASYNC_QUERIES_WEBSOCKET_URL
An Express-based client application for testing and development only (superset-websocket/utils/client-ws-app/)
CI actions for running automated unit tests

Corresponding Superset web app changes are in the following PRs: #11499, #13696

To be added in future PRs:

Integration tests
Containerization

TEST PLAN

CI runs unit tests and build checks
Run the WS server locally and configure Superset to use the websocket transport for async queries

ADDITIONAL INFORMATION

Has associated issue: [SIP-39] Global Async Query Support #9190
Changes UI
Requires DB Migration.
Confirm DB Migration upgrade and downgrade tested.
Introduces new feature or API
Removes existing feature or API

codecov-io · 2020-10-30T01:18:03Z

Codecov Report

Merging #11498 (fb00d15) into master (a0a00e6) will increase coverage by 0.12%.
The diff coverage is 90.00%.

❗ Current head fb00d15 differs from pull request most recent head 3af092f. Consider uploading reports for the commit 3af092f to get more accurate results

@@            Coverage Diff             @@
##           master   #11498      +/-   ##
==========================================
+ Coverage   78.35%   78.47%   +0.12%     
==========================================
  Files         934      937       +3     
  Lines       47350    47439      +89     
  Branches     5941     5976      +35     
==========================================
+ Hits        37102    37229     +127     
+ Misses      10104    10067      -37     
+ Partials      144      143       -1

Flag	Coverage Δ
cypress	`56.03% <73.80%> (-0.01%)`	⬇️
hive	`?`
mysql	`80.56% <77.77%> (-0.01%)`	⬇️
postgres	`80.55% <77.77%> (-0.05%)`	⬇️
presto	`80.29% <77.77%> (-0.01%)`	⬇️
python	`80.82% <77.77%> (-0.33%)`	⬇️
sqlite	`80.16% <77.77%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...et-frontend/src/SqlLab/components/TableElement.jsx	`88.60% <ø> (+0.23%)`	⬆️
...end/src/common/components/DropdownButton/index.tsx	`24.00% <0.00%> (ø)`
.../components/CrossFilterScopingModal/utils/index.ts	`100.00% <ø> (ø)`
...dashboard/components/SliceHeaderControls/index.jsx	`77.89% <ø> (ø)`
...perset-frontend/src/datasource/DatasourceModal.tsx	`72.58% <ø> (ø)`
...components/controls/FilterBoxItemControl/index.jsx	`73.58% <ø> (ø)`
...onents/controls/FilterControl/AdhocFilter/index.js	`96.92% <ø> (ø)`
...ontrols/FilterControl/AdhocFilterControl/index.jsx	`60.00% <ø> (ø)`
...ols/FilterControl/AdhocFilterEditPopover/index.jsx	`66.00% <ø> (ø)`
...l/AdhocFilterEditPopoverSimpleTabContent/index.jsx	`77.48% <ø> (ø)`
... and 53 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 a0a00e6...3af092f. Read the comment docs.

superset-websocket/client-ws-app/public/javascripts/js.cookie.min.js

craig-rueda · 2020-10-30T01:40:02Z

Did you consider Typescript here?

robdiciuccio · 2020-10-30T19:06:03Z

@craig-rueda yes, the plan is to migrate this to TypeScript (added as a TODO item)

ktmud · 2020-10-30T20:01:25Z

If you are going to use TypeScript, I'd recommend look into integrating esbuild in the workflow. It's much faster than Babel and TSC and seems good enough for node.js environment.

superset-websocket/client-ws-app/public/javascripts/app.js

superset-websocket/index.js

superset-websocket/client-ws-app/public/javascripts/app.js

craig-rueda · 2021-03-26T01:15:02Z

superset-websocket/src/index.ts

+}
+
+const startServer = process.argv[2] === 'start';
+const configFile = environment === 'test' ? '../config.test.json' : '../config.json';


Can we add a CLI argument for the config file?

craig-rueda · 2021-03-26T01:22:19Z

superset-websocket/utils/client-ws-app/bin/www

+ * Module dependencies.
+ */
+
+var app = require('../app');


What does this do? Is it the main entrypoint?

This is all boilerplate Express code. Not used in production app.

superset-websocket/src/index.ts

craig-rueda · 2021-03-26T01:23:56Z

superset-websocket/utils/client-ws-app/app.js

+app.set('views', path.join(__dirname, 'views'));
+app.set('view engine', 'jade');
+
+app.use(logger('dev'));


what's dev in this context? Is that the logger level?

Boilerplate Express app code. Not part of the production app.

craig-rueda · 2021-03-26T01:25:00Z

superset-websocket/utils/client-ws-app/app.js

+app.use(function(err, req, res) {
+  // set locals, only providing error in development
+  res.locals.message = err.message;
+  res.locals.error = req.app.get('env') === 'development' ? err : {};


Can we leverage settings from the config file referenced above? I'd advise using one or the other (env vars of config file)

Test utility app only, all boilerplate.

craig-rueda

Overall, looks good. I'm pretty sure it works, as you've been working on this for some time now... I just think a few spots could use some more code comments to guide others when digging in.

benjreinhart

I think the core logic is looking good, but had a few minor comments.

Also:

Nitpick but looks like the indentation is inconsistent across files with both two and four spaces being used. Not a huge deal but might be nice to add prettier and forget about these types of things.
Do we normally check in auxiliary code into the core repository? In this case, I'm referring the the load testing application. Would it make sense to have that as a separate repo outside of the main Superset repo?

benjreinhart · 2021-03-27T00:38:27Z

superset-websocket/.gitignore

@@ -0,0 +1,18 @@
+#


Should this ignore node_modules ?

node_modules is ignored in the top-level .gitignore, but I'll make it explicit.

benjreinhart · 2021-03-27T00:40:59Z

superset-websocket/README.md

+GLOBAL_ASYNC_QUERIES_WEBSOCKET_URL = "ws://<host>:<port>/"
+```
+
+Note that the WebSocket server must be run on the same hostname (different port) for cookies to be shared between the Flask app and the WebSocket server.


Is the JWT the only thing needed from the cookies? If so, have we considered passing that value explicitly? Seems like if we could do that, we could remove this assumption/constraint.

We could pass it explicitly, though we'd have to make the cookie readable by JS (currently it's httponly), so there is a bit of a tradeoff here.

Keeping the cookie httponly is a good move!

benjreinhart · 2021-03-27T00:50:28Z

superset-websocket/package.json

+    "uuid": "^8.3.2",
+    "ws": "^7.4.2"
+  },
+  "devDependencies": {


This is not important now, but I think it would be good to eventually add prettier to this application.

benjreinhart · 2021-03-27T00:54:35Z

superset-websocket/src/index.ts

+    try {
+      socketInstance.ws.send(strData);
+    } catch(err) {
+      console.debug('Error sending to socket', err);


Nit: Is this error indicative of a real problem? If so, should this be console.error?

This can happen for a number of reasons, including a disconnected socket. I believe this was a console.error and was downgraded to a debug due to noise.

benjreinhart · 2021-03-29T17:15:54Z

superset-websocket/src/index.ts

+
+export const sendToChannel = (channel: string, value: EventValue): void => {
+  const strData = JSON.stringify(value);
+  if(!channels[channel]) {


What is the behavior when we pull an event off the redis stream but for one reason or another we have no channel to send it to? Is that event lost? Or does the event stay in the redis stream and we expect the client to have the right state to connect to the event server and pull it in the future?

Events are persisted in the Redis streams until the (configurable) maximum number of items is reached. For the global stream, yes, the event is dropped if there is no available channel/socket connected. The reconnection logic then handles replaying any events that were lost while disconnected.

benjreinhart · 2021-03-29T17:26:55Z

superset-websocket/src/index.ts

+
+export const subscribeToGlobalStream = async (stream: string, listener: ListenerFunction) => {
+  /*eslint no-constant-condition: ["error", { "checkLoops": false }]*/
+  while (true) {


It's probably premature to worry about performance here and I think this is likely not an issue, but I'm curious if there are any potential concerns you foresee with an infinite loop calling redis? It looks like the BLOCK option should prevent too many requests in too short a time since if no events are available it'll wait, but not sure what would happen if one or two events regularly trickle in within short timeframes?

When events are received, it returns immediately. More info here: https://redis.io/commands/xread#blocking-for-data

When the BLOCK command is passed, but there is data to return at least in one of the streams passed, the command is executed synchronously exactly like if the BLOCK option would be missing.

"Block" could be a confusing term here. In typical Node parlance, "block" means synchronous code, which this is not. It might be helpful to add a comment clarifying what "blocking" means in this context.

benjreinhart

benjreinhart · 2021-04-05T19:56:52Z

superset-websocket/src/index.ts

+};
+
+type ListenerFunction = (results: StreamResult[]) => void;
+interface EventValue {


If I'm reading this correctly, it looks like under the polling implementation, the response is in the format:

type PollingResponse = { result: EventValue; }

Whereas the response here is simply:

type WebSocketResponse = EventValue;

I'm not opinionated either way, but I think we should consider having a standard structure

Also, it looks like the initial call to /api/v1/chart/data returns EventValue without the { result: ... } wrapper, so maybe we should change the polling response as that seems to be the outlier?

Ahh, nevermind, I missed that the polling response returns a list given there could be multiple events

suddjian

Left a few minor-medium suggestions. Looks great, excited for this feature (and to get a node server in Superset! 😁)

suddjian · 2021-04-05T22:33:27Z

superset-websocket/README.md

+GLOBAL_ASYNC_QUERIES_REDIS_CONFIG
+GLOBAL_ASYNC_QUERIES_REDIS_STREAM_PREFIX
+GLOBAL_ASYNC_QUERIES_JWT_COOKIE_NAME
+GLOBAL_ASYNC_QUERIES_JWT_SECRET


idea, non-blocking: If these are not configured correctly, can we have the service detect it and provide hints to help devs/admins get it working?

suddjian · 2021-04-05T22:35:52Z

superset-websocket/config.test.json

+    "db": 10,
+    "ssl": false
+  },
+  "streamPrefix": "test-async-events-",


change request: Can we make this more specific? Not clear what a streamPrefix is used for.

suddjian · 2021-04-05T22:39:09Z

superset-websocket/package.json

+    "lint": "eslint . --ext .js,.jsx,.ts,.tsx && npm run type",
+    "dev-server": "ts-node src/index.ts start",
+    "build": "tsc",
+    "prettier-check": "prettier --check .",


change request: Could this be named format-check instead?

This was named prettier-check for consistency with superset-frontend.

Happy to rename, but curious what people think about keeping these kinds of things consistent.

suddjian · 2021-04-05T23:02:52Z

superset/config.py

@@ -1124,6 +1125,7 @@ class CeleryConfig:  # pylint: disable=too-few-public-methods
 GLOBAL_ASYNC_QUERIES_JWT_SECRET = "test-secret-change-me"
 GLOBAL_ASYNC_QUERIES_TRANSPORT = "polling"
 GLOBAL_ASYNC_QUERIES_POLLING_DELAY = 500
+GLOBAL_ASYNC_QUERIES_WEBSOCKET_URL = "ws://127.0.0.1:8080/"


Is there potential for the websocket server to be used for purposes other than async queries in the future? If so, this config should be given a more general name.

Possibly, yes, but no immediate use cases have been defined.

suddjian · 2021-04-05T23:03:54Z

superset/utils/async_query_manager.py

-                reset_token = True
-            elif user_id != session["async_user_id"]:
-                reset_token = True
+            reset_token = (


Small change but I like how much easier to read this is 🎉

suddjian · 2021-04-05T23:12:00Z

superset-websocket/src/index.ts

+};
+
+type ListenerFunction = (results: StreamResult[]) => void;
+interface EventValue {


If this server might eventually deliver multiple kinds of events, it would be desirable to separate the id and channel_id, which are used by the server, from the schema for the other fields, which are specific to the async query event type.

So, an interface like:

interface EventValue<T extends object> { id: string; channel_id: string; data: T; // everything else }

Unclear what future use cases might be, but the existing properties are fairly generic, IMO.

Future use cases could include a wide variety of things, some that I can think of:

real-time updates to entities such as dashboards

comments or other user activity

notifications

streaming query results

any other real-time state change

The schema of different types of messages seems quite likely to vary from that of async queries, imo.

Agree that there are many potential use cases, but my point is that these should not hold up this PR. The interface in question is already in use, introduced in 1.0, and would require some additional planning and semver gymnastics to refactor.

Agreed that this and the config comment seem somewhat out of scope for this PR, just wanted to raise it as it seems like the kind of thing that might be significantly more difficult to change later.

suddjian · 2021-04-06T15:51:34Z

superset-websocket/src/index.ts

+
+export const subscribeToGlobalStream = async (stream: string, listener: ListenerFunction) => {
+  /*eslint no-constant-condition: ["error", { "checkLoops": false }]*/
+  while (true) {


"Block" could be a confusing term here. In typical Node parlance, "block" means synchronous code, which this is not. It might be helpful to add a comment clarifying what "blocking" means in this context.

suddjian · 2021-04-06T16:10:42Z

superset-websocket/src/index.ts

+ * Reads a range of events from a channel-specific Redis event stream.
+ * Invoked in the client re-connection flow.
+ */
+export const fetchRangeFromStream = async ({


Should this return the reply in a promise instead of using a listener?

Yeah, good call. I'll look at refactoring.

Thinking about this more, there's an argument to be made for keeping this consistent with the interface for subscribeToGlobalStream.

superset-websocket/src/index.ts

superset-websocket/tsconfig.json

Co-authored-by: David Aaron Suddjian <1858430+suddjian@users.noreply.github.com>

villebro

Nothing to add to the already extensive comments from others, so I focused on testing this and I must say this is really exciting stuff; chart loading times seem to be way down and it almost feels like one of the main bottlenecks right now for pre-cached data is the slow rendering times of the NVD3 charts 🎉 Tested the following:

install based on documentation - very straight forward, had it up and running in a few mins ✅
regular dashboard loading, comparing non-DAQ, polling DAQ and WS DAQ - WS DAQ loaded - WS DAQ by far the quickest to load ✅
tested rendering of native filters (both filter tab and filter config modal) ✅
triggering chart queries with native filters and cross filters ✅

villebro · 2021-04-08T11:50:30Z

superset-websocket/utils/client-ws-app/routes/index.js

+const express = require('express');
+const router = express.Router();
+const jwt = require('jsonwebtoken');
+const config = require('../../../config.json');


nit:

Suggested change

const config = require('../../../config.json');

const config = require('/config.json');

I'm not sure this will actually work, node might try to get config.json from the root dir.

I was unsure, too, but it did seem to work when I tested it.

Doesn't seem to work for me:

Error: Cannot find module '/config.json'

* WIP node.js websocket app * Load testing * Multi-stream publish with blocking reads * Use JWT for auth and channel ID * Update ws jwt cookie name * Typescript * Frontend WebSocket transport support * ws server ping/pong and GC logic * ws server unit tests * GC interval config, debug logging * Cleanup JWT cookie logic * Refactor asyncEvents.ts to support non-Redux use cases * Update tests for refactored asyncEvents * Add eslint, write READMEs, reorg files * CI workflow * Moar Apache license headers * pylint found something * adjust GH actions workflow * Improve documentation & comments * Prettier * Add configurable logging via Winston * Add SSL support for Redis connections * Fix incompatible logger statements * Apply suggestions from code review Co-authored-by: David Aaron Suddjian <1858430+suddjian@users.noreply.github.com> * rename streamPrefix config Co-authored-by: David Aaron Suddjian <1858430+suddjian@users.noreply.github.com>

superset-github-bot bot added the preset-io label Oct 30, 2020

pull-request-size bot added the size/L label Oct 30, 2020

robdiciuccio mentioned this pull request Oct 30, 2020

feat(SIP-39): Async query support for charts #11499

Merged

6 tasks

craig-rueda reviewed Oct 30, 2020

View reviewed changes

superset-websocket/client-ws-app/public/javascripts/js.cookie.min.js Outdated Show resolved Hide resolved

etr2460 reviewed Nov 9, 2020

View reviewed changes

superset-websocket/client-ws-app/public/javascripts/app.js Outdated Show resolved Hide resolved

etr2460 reviewed Nov 9, 2020

View reviewed changes

superset-websocket/index.js Outdated Show resolved Hide resolved

rusackas reviewed Nov 12, 2020

View reviewed changes

superset-websocket/client-ws-app/public/javascripts/app.js Outdated Show resolved Hide resolved

robdiciuccio force-pushed the rd/async-websocket-app branch from 14b4b2c to cf6e3a7 Compare January 20, 2021 21:17

pull-request-size bot added size/XL and removed size/L labels Jan 23, 2021

robdiciuccio added the v1.1 label Jan 27, 2021

robdiciuccio force-pushed the rd/async-websocket-app branch from 9ab49d4 to 7f0dafc Compare February 2, 2021 02:16

robdiciuccio mentioned this pull request Feb 18, 2021

Global Async Queries doesn't work #12766

Closed

3 tasks

robdiciuccio added 9 commits March 4, 2021 20:09

WIP node.js websocket app

e476ca5

Load testing

881e18d

Multi-stream publish with blocking reads

01e9f1f

Use JWT for auth and channel ID

a1954a3

Update ws jwt cookie name

3823bba

Typescript

c42cbc2

Frontend WebSocket transport support

08531f5

ws server ping/pong and GC logic

1ecf689

ws server unit tests

dc968bc

robdiciuccio force-pushed the rd/async-websocket-app branch from 7f0dafc to dc968bc Compare March 13, 2021 01:26

robdiciuccio added 3 commits March 15, 2021 13:46

GC interval config, debug logging

63c765f

Cleanup JWT cookie logic

5eab894

Refactor asyncEvents.ts to support non-Redux use cases

04e8d85

robdiciuccio force-pushed the rd/async-websocket-app branch from 9518ebf to f5087ed Compare March 26, 2021 00:22

robdiciuccio requested a review from ktmud March 26, 2021 00:27

craig-rueda reviewed Mar 26, 2021

View reviewed changes

superset-websocket/src/index.ts Outdated Show resolved Hide resolved

craig-rueda reviewed Mar 26, 2021

View reviewed changes

benjreinhart reviewed Mar 29, 2021

View reviewed changes

robdiciuccio removed the v1.1 label Mar 29, 2021

villebro added the v1.2 label Mar 30, 2021

robdiciuccio added 2 commits March 31, 2021 15:18

Improve documentation & comments

8eadc3a

Prettier

fedf0f3

benjreinhart approved these changes Apr 1, 2021

View reviewed changes

robdiciuccio added 4 commits April 2, 2021 12:41

Add configurable logging via Winston

d44742d

Merge branch 'master' into rd/async-websocket-app

23d17bb

Add SSL support for Redis connections

863b503

Fix incompatible logger statements

20b563a

benjreinhart reviewed Apr 5, 2021

View reviewed changes

suddjian reviewed Apr 6, 2021

View reviewed changes

Rob DiCiuccio and others added 2 commits April 6, 2021 10:31

Apply suggestions from code review

3be73e4

Co-authored-by: David Aaron Suddjian <1858430+suddjian@users.noreply.github.com>

rename streamPrefix config

3af092f

suddjian approved these changes Apr 7, 2021

View reviewed changes

villebro approved these changes Apr 8, 2021

View reviewed changes

robdiciuccio merged commit 806fb73 into apache:master Apr 8, 2021

robdiciuccio deleted the rd/async-websocket-app branch April 8, 2021 18:12

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

	const config = require('../../../config.json');
	const config = require('/config.json');

feat(SIP-39): Websocket sidecar app #11498

feat(SIP-39): Websocket sidecar app #11498

Conversation

robdiciuccio commented Oct 30, 2020 • edited Loading

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

codecov-io commented Oct 30, 2020 • edited by codecov bot Loading

Codecov Report

craig-rueda commented Oct 30, 2020

robdiciuccio commented Oct 30, 2020

ktmud commented Oct 30, 2020

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

craig-rueda left a comment

Choose a reason for hiding this comment

benjreinhart 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

benjreinhart left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjreinhart Apr 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suddjian left a comment • edited Loading

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

suddjian Apr 6, 2021 • edited Loading

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

villebro 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

robdiciuccio commented Oct 30, 2020 •

edited

Loading

codecov-io commented Oct 30, 2020 •

edited by codecov bot

Loading

benjreinhart left a comment •

edited

Loading

benjreinhart Apr 5, 2021 •

edited

Loading

suddjian left a comment •

edited

Loading

suddjian Apr 6, 2021 •

edited

Loading