Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

chore: Add watch workspace endpoint#1493

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
f0ssel merged 13 commits intomainfromf0ssel/watch-workspace
May 18, 2022
Merged

Conversation

f0ssel
Copy link
Contributor

@f0sself0ssel commentedMay 16, 2022
edited
Loading

To avoid polling frequently I'm adding a/watch endpoint to workspaces that will pump the data we currently poll for over a one-way websocket.

Changes:

  • Added support forsession_token query parameter on authenticated calls
  • Changedhttpmw.AuthCookie tohttpmw.SessionTokeyKey since we now use it for query parameters
  • Added watch workspace endpoint
  • AddeddialWebsocket method tocodersdk.Client

@kylecarbs
Copy link
Member

@f0ssel I don't have much information on polling, but I'm curious where you got this pattern from. I've seen server-side events before, but I haven't seen this! Seems reasonable though... so not sure why I haven't seen it.

@kylecarbs
Copy link
Member

BTW this is pretty much the exact same as server-side events.

@f0ssel
Copy link
ContributorAuthor

Yeah I'm unfamiliar with Server Side Events, my brief look seems like it's a javascript websocket standard. If there's anything I need to change to make this PR compatible with that web API I'm down to do so, would just like to be pointed in the right direction.

@f0ssel
Copy link
ContributorAuthor

As far as where this is coming from, it's just porting a trimmed down version of what we do in V1 for thewatch-stats,devurls/watch-status, etc.

@kylecarbs
Copy link
Member

Ahh, I'm more so wondering how other products attack this polling problem. I know Discord has a WebSocket that updates information

@f0ssel
Copy link
ContributorAuthor

f0ssel commentedMay 16, 2022
edited
Loading

I'm not sure but I'll break down my thoughts so far

I think there's a few things we could improve here like:

  • Multiplex updates to many different entities over the same websocket instead of opening multiple
  • Only send updates on changes or on a backoff if data has not changed
  • Gracefully handle errors instead of closing the websocket connection

But I've chosen this way so far because:

  • The workspace entity having near-real-time updates seems like it will get us pretty far, I don't think there's plans to have other resources that will use this pattern.
  • We already have code to support this pattern on the frontend and I'm optimizing for not adding more scope for community MVP.
  • It's a familiar pattern from V1 that seemed to work fine, I don't think we've had much issues around it at the api layer.

@dwahler
Copy link
Contributor

I like this approach because in the future we can optimize the backend by removing unchanged values and/or replacing the internal DB polling loop with some kind of pub/sub system, without having to change the API at all.

f0ssel and kylecarbs reacted with heart emoji

// Makes the websocket connection write-only
ctx := c.CloseRead(r.Context())

// Send a heartbeat every 15 seconds to avoid the websocket being killed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually necessary if we're already sending updates every second?

Copy link
ContributorAuthor

@f0sself0sselMay 17, 2022
edited
Loading

Choose a reason for hiding this comment

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

This is a great callout, I haven't considered it but you are correct.

That said, I'd like to have the precedent that we always do a heartbeat for websockets as a best practice. If the behavior of this websocket changes to only send updates and we go longer than 15 seconds without an update the browser could close the connection on us.

dwahler reacted with thumbs up emoji
@f0sself0sselforce-pushed thef0ssel/watch-workspace branch fromc24fac0 toec066aaCompareMay 17, 2022 20:38
@f0sself0ssel marked this pull request as ready for reviewMay 17, 2022 20:59
Comment on lines +20 to +21
//SessionTokenKey represents the name of the cookie or query paramater the API key is stored in.
constSessionTokenKey = "session_token"
Copy link
Member

Choose a reason for hiding this comment

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

Wise rename.AuthCookie was questionable misdirection anyways!

f0ssel reacted with thumbs up emoji
return nil
}

func (c *Client) WatchWorkspace(ctx context.Context, id uuid.UUID) (*WorkspaceWatcher, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Could this return a channel instead?<-chan Workspace could be nice!

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Sure thing that sounds nice

Copy link
Contributor

@greyscaledgreyscaled left a comment

Choose a reason for hiding this comment

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

This looks good to me - we're returning acodersdk.Workspace on an interval viaWS.

@f0sself0ssel requested a review fromkylecarbsMay 18, 2022 16:14
@f0sself0sselforce-pushed thef0ssel/watch-workspace branch from8db2066 tocc3f57fCompareMay 18, 2022 16:41
@f0sself0sselforce-pushed thef0ssel/watch-workspace branch fromcc3f57f to534bcdbCompareMay 18, 2022 16:44
@f0sself0ssel merged commit0706c60 intomainMay 18, 2022
@f0sself0ssel deleted the f0ssel/watch-workspace branchMay 18, 2022 21:16
kylecarbs pushed a commit that referenced this pull requestJun 10, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@dwahlerdwahlerdwahler left review comments

@greyscaledgreyscaledgreyscaled approved these changes

@kylecarbskylecarbsAwaiting requested review from kylecarbs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@f0ssel@kylecarbs@dwahler@greyscaled

[8]ページ先頭

©2009-2025 Movatter.jp