Skip to content

feat: use sse for workspace page #4122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Sep 21, 2022
Merged

feat: use sse for workspace page #4122

merged 7 commits into from
Sep 21, 2022

Conversation

Kira-Pilot
Copy link
Member

resolves #4058

Based on @f0ssel's implementation of SSE on the BE, we are now using SSE on the FE to get workspace and resource updates (instead of polling).

Background

Previously, we fetched a workspace, template, and permissions, and then we started two separate polling states: we polled for workspace, and we polled for resources.

Since @f0ssel's implementation returns both workspace and resources in one object, we are able to simplify.

Now: we fetch a workspace, template, and permissions, and then we initialize an EventSource that returns workspace and workspace-related data as the server updates the client.

Screen Shot 2022-09-19 at 4 06 28 PM

To Test

Pull down the branch and open a workspace; make sure the workspace and resources load.
Open Chrome inspect and check out the network tab: you should be able to see a watch request that shows events streaming in:
Screen Shot 2022-09-19 at 4 20 53 PM

@Kira-Pilot Kira-Pilot requested a review from a team as a code owner September 19, 2022 20:22
@Kira-Pilot Kira-Pilot requested review from BrunoQuaresma and presleyp and removed request for a team September 19, 2022 20:22
@presleyp
Copy link
Contributor

If SSE fails and we're in the sseFailure state, should we be taken out of ready? Currently it looks like that will happen but I'm guessing we can still show the timeline (and even send build requests though that may be unwise). I wonder if you want to have a workspaceAndResources submachine inside ready with listenForEvents and sseFailure as its states.

@presleyp
Copy link
Contributor

How should we handle SSE errors? Should the page show something about failing to refresh data? Maybe a warning-style thing rather than error-style, like when your workspace is going to stop soon?

@f0ssel
Copy link
Contributor

f0ssel commented Sep 19, 2022

How should we handle SSE errors? Should the page show something about failing to refresh data? Maybe a warning-style thing rather than error-style, like when your workspace is going to stop soon?

The backend will close the connection when we get a non-recoverable error. My first thought is to console log the errors we get or something low-stakes like that, and just keep it open unless the backend closes it. A non-fatal error may be a momentary db issue, but the next message may come through fine. On the connection being closed we should retry, probably with a backoff.

@BrunoQuaresma BrunoQuaresma removed their request for review September 20, 2022 13:41
@Kira-Pilot
Copy link
Member Author

If SSE fails and we're in the sseFailure state, should we be taken out of ready?

@presleyp good catch; this should be fixed.

How should we handle SSE errors? Should the page show something about failing to refresh data? Maybe a warning-style thing rather than error-style, like when your workspace is going to stop soon?

@presleyp Right now, SSE errors will not block the user from viewing the page. I will put up a subsequent PR with a new, warning style message indicating workspace data might not be up to date if that field is populated. I want to handle that separately because there are several workspace error cleanup items to tackle and it would be nice to have them all in 1 PR.

On the connection being closed we should retry, probably with a backoff.

@f0ssel Yeah, we should add a backoff. Right now, we close, wait a second and then try again with a new connection. I would like to add that backoff when I add the warning described above.

Copy link
Contributor

@presleyp presleyp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I could've sworn I got this review in last night!

@Kira-Pilot Kira-Pilot merged commit 5698b9d into main Sep 21, 2022
@Kira-Pilot Kira-Pilot deleted the workspace-sse/kira-pilot branch September 21, 2022 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Workspace page should display real-time data
3 participants