Feature/server health check #349

ascibisz · 2023-12-01T00:39:14Z

Problem

The server health check has existed in Octopus for a while now, time to take it in to the viewer!

One part of this ticket. The other piece of that ticket will include adding a health check call, similar to the one implemented here in the example viewer, to the simualrium-website in the early stages of the autoconversion form.

Also added in support for handling error messages octopus sends now, which is another new message type that has existed in Octopus for a bit.

Solution

Paired on this with with @interim17

Type of change

New feature (non-breaking change which adds functionality)

Change summary:

Added handling for new message types that come from octopus: ID_ERROR_MSG = 21 and ID_SERVER_HEALTHY_RESPONSE = 23
Added ability to request health check, using new message type ID_CHECK_HEALTH_REQUEST = 22, which octopus already knows how to handle
Added an example of using the health check request / response to the example viewer. Now, when you hit the "Load a smoldyn trajectory" button, a websocket connection will be initiated and health check request will be sent to octopus. The "submit" button will remain disabled until a server healthy response comes back
As part of the changes to the example, I updated the autoconversion request call to only initiate a new websocket connection if there isn't already one there. Previously, we just automatically created a new websocket connection, but in the case that one may already exist from the server health check, we should just reuse that one

…heck

…undant check" This reverts commit f3ae249.

…, so allow them to set the callback function

…utton when response comes in

github-actions · 2023-12-01T00:41:24Z

jest coverage report 🧪

Total coverage

Status	Category	Percentage	Covered / Total
🔴	Statements	40.77%	2013/4937
🔴	Branches	44.08%	831/1885
🔴	Functions	37.4%	413/1104
🔴	Lines	41%	1927/4700

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

Show files with reduced coverage 🔻

Reduced coverage

Status	Filename	Statements	Branches	Functions	Lines
🔴	src/controller	29.71%	15.32% (-1.21% 🔻)	17.54%	29.75%
🟡	src/simularium	62.32%	60.31%	50.25% (-1.48% 🔻)	63.29%
🔴	ClientSimulator.ts	22.11%	31.81%	11.76%	21.78%
🔴	LocalFileSimulator.ts	31.42%	50%	12.12%	31.34%
🔴	RemoteSimulator.ts	48.85% (-2.84% 🔻)	66.66%	27.27% (-4.3% 🔻)	52.1% (-2.52% 🔻)
🟢	WebsocketClient.ts	82.66% (+1.05% 🔼)	69.23%	69.69% (-1.27% 🔻)	83.56%

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

This reverts commit e253436.

ascibisz · 2023-12-01T01:08:46Z

src/simularium/WebsocketClient.ts

@@ -313,7 +316,7 @@ export class WebsocketClient {
        jsonData: Record<string, unknown>,
        requestDescription: string
    ): void {
-        if (this.socketIsValid()) {
+        if (this.socketIsConnected()) {


I ran into this issue when testing where the socket can be "valid" (which means the websocket's state isn't CLOSED), but not "connected" (which means the websocket's state is OPEN) if the state is PENDING, in which case sending a message throws an error. I feel like this check is trying to prevent that error, so might as well cover the PENDING case too?

interim17 · 2023-12-01T20:10:57Z

Can confirm I just got this working in a preliminary way in the website! More work to clean up the front end before PR but the viewer side is doing what it should do. LGTM, thanks @ascibisz !

interim17 · 2023-12-01T20:13:26Z

src/controller/index.ts

+        netConnectionConfig: NetConnectionParams
+    ): void {
+        if (
+            !this.simulator ||
+            !(this.simulator instanceof RemoteSimulator) ||
+            !this.simulator.socketIsValid()
+        ) {
+            // Only configure network if we aren't already connected to the remote server
+            this.configureNetwork(netConnectionConfig);


It feels a bit funny to have to pass the config in every time we make a check, it looks redundant on the front-end, but it does ensure we aren't querying an un-configured controller that won't respond.

Yeah it does sort of make sense that you might want to check health before a connection was made.

src/controller/index.ts

toloudis · 2023-12-01T22:37:22Z

src/controller/index.ts

+        }
+        if (this.simulator && this.simulator instanceof RemoteSimulator) {
+            this.simulator.setHealthCheckHandler(handler);
+            this.simulator.checkServerHealth();


should checkServerHealth accept a callback as an arg instead of having a separate setter? Are they always called together like this?

I see why they are separate now, because of the message passing and receiving.

src/simularium/RemoteSimulator.ts

toloudis · 2023-12-01T22:42:11Z

src/simularium/WebsocketClient.ts

@@ -47,6 +47,9 @@ export const enum NetMessageEnum {
    ID_AVAILABLE_METRICS_RESPONSE = 18,
    ID_PLOT_DATA_REQUEST = 19,
    ID_PLOT_DATA_RESPONSE = 20,
+    ID_ERROR_MSG = 21,
+    ID_CHECK_HEALTH_REQUEST = 22,
+    ID_SERVER_HEALTHY_RESPONSE = 23,


is there any documentation in this repo for what these messages are supposed to mean, or their payload schema? (One thing that is nice to know is whether it's intended to go from client to server or vice versa)

There isn't... I do have it fairly well documentation in the Octopus repo though, so I should probably copy that over

I realize it's not great that it would have to be kept up in two places but it's also true that if it changes in octopus, then something would have to change over here too.

Yeah and that's true of the message types themselves. We're already in a slightly weird situation, may as well have it be a better documented weird situation!

toloudis

I left a bunch of comments but basically this seems ok. I would love to clean up the connection logic but probably out of scope for this.

interim17 · 2023-12-04T19:55:49Z

src/simularium/RemoteSimulator.ts

+            .catch((e) => {
+                throw new FrontEndError(e.message, ErrorLevel.ERROR);
+            });


Questions about error handling.

UX request is that if "server is down" we throw a styled modal.

But if the viewer can't connect this error also generates up a full screen React runtime error.

Removing this error handling yields the behavior UX asked for (just a modal), but is that not proper error handling?

ascibisz added 8 commits November 29, 2023 10:05

update net connection settings to use wss for octopus

e253436

Add new message types, request and handle server health check

436714c

even if the socket is valid, if it isn't connected we can throw errors

2ee0d87

we ensure socket is valid in sendWebSocketRequest, remove redundant c…

e0a10ca

…heck

add handling for error messages from octopus

095ee7a

Revert "we ensure socket is valid in sendWebSocketRequest, remove red…

ac00c06

…undant check" This reverts commit f3ae249.

whoever is requesting the health check will want to know the response…

02a736e

…, so allow them to set the callback function

when loading smoldyn form, initiate health check, and enable submit b…

087dbf5

…utton when response comes in

ascibisz changed the base branch from main to admin/wss-for-octopus December 1, 2023 00:41

ascibisz force-pushed the admin/wss-for-octopus branch from 075236e to 3002aed Compare December 1, 2023 00:41

Revert "update net connection settings to use wss for octopus"

31c8236

This reverts commit e253436.

ascibisz changed the base branch from admin/wss-for-octopus to main December 1, 2023 00:48

ascibisz marked this pull request as ready for review December 1, 2023 01:04

ascibisz requested a review from a team as a code owner December 1, 2023 01:04

ascibisz requested review from toloudis, meganrm, frasercl, blairlyons, interim17 and ShrimpCryptid and removed request for a team December 1, 2023 01:04

ascibisz commented Dec 1, 2023

View reviewed changes

interim17 reviewed Dec 1, 2023

View reviewed changes

interim17 approved these changes Dec 1, 2023

View reviewed changes

toloudis reviewed Dec 1, 2023

View reviewed changes

src/controller/index.ts Show resolved Hide resolved

toloudis reviewed Dec 1, 2023

View reviewed changes

src/simularium/RemoteSimulator.ts Outdated Show resolved Hide resolved

toloudis reviewed Dec 1, 2023

View reviewed changes

toloudis approved these changes Dec 1, 2023

View reviewed changes

ascibisz added 3 commits December 1, 2023 15:08

add error message type

6b0634e

add check for connection to remove server to avoid redundant code

060baca

add error message params to NetMessage

e1b37ad

interim17 reviewed Dec 4, 2023

View reviewed changes

ascibisz added 4 commits December 4, 2023 14:37

create new ErrorMessage type

77a319a

we know that in onErrorMessage, the specific type will be ErrorMessage

d114550

handle message instead of throwing

b24e4a6

code cleanup

99f2e1a

ascibisz merged commit e3ab563 into main Dec 6, 2023
6 checks passed

ascibisz deleted the feature/server-health-check branch December 6, 2023 17:19

interim17 mentioned this pull request Dec 8, 2023

Feature/conversion check server simularium/simularium-website#452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/server health check #349

Feature/server health check #349

ascibisz commented Dec 1, 2023

github-actions bot commented Dec 1, 2023 •

edited

Loading

Reduced coverage

ascibisz Dec 1, 2023

interim17 commented Dec 1, 2023

interim17 Dec 1, 2023

toloudis Dec 1, 2023

toloudis Dec 1, 2023

toloudis Dec 1, 2023

toloudis Dec 1, 2023

ascibisz Dec 1, 2023

toloudis Dec 1, 2023

ascibisz Dec 1, 2023

toloudis left a comment

interim17 Dec 4, 2023

Feature/server health check #349

Feature/server health check #349

Conversation

ascibisz commented Dec 1, 2023

Problem

Solution

Type of change

Change summary:

github-actions bot commented Dec 1, 2023 • edited Loading

jest coverage report 🧪

Total coverage

Reduced coverage

Choose a reason for hiding this comment

interim17 commented Dec 1, 2023

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

toloudis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 1, 2023 •

edited

Loading