-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[server] add http endpoint workspacePageClose for beacon #11643
Conversation
started the job as gitpod-build-hw-hb-server.1 because the annotations in the pull request description changed |
Please make it sure to update the PR description to explain why we do it instead of using Gitpod API, i.e. it is not about general heartbeating, but making sure that we report close signal always. General heartbeating still happens using Gitpod API, but it is not reliable to use while page is unloading. |
]); | ||
if (!workspace || instance?.status.phase !== "running") { | ||
res.sendStatus(404); | ||
log.warn("attempted to send heartbeat for non-existent workspace instance", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be log.warn(<context>, <message>)
? [1]
I think userId
, istanceId
are part of LogContext
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andreafalzetti See here
gitpod/components/server/src/user/user-controller.ts
Lines 205 to 208 in 9b46ff3
log.warn("blocked user attempted to fetch workspace cookie", { | |
instanceId: req.params.instanceID, | |
userId: user.id, | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought log
support a lot of param pairs, and we used to make log string as first param in most cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure if the order of params will affect the query in gcloud, I unresolved this conversation and maybe webApp team can answer us @andreafalzetti
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure if the order of params will affect the query in gcloud
@mustard-mh It does indeed.
we used to make log string as first param in most cases
That's not good. The expected order is: logContext, message, error, payload.
We need to test it coupled with supervisor-frontend. If it does not work there reliably, then this PR won't make sense. |
server.initialize( | ||
undefined, | ||
user, | ||
new OwnerResourceGuard(user.id), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This line ensures that only the owner can access the sendHeartbeat
method for their instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ouch, it is a valid point in case of live sharing cc @mustard-mh we need to check how construct proper guard similar how we do for web socket connections, i think there should be a service fro HTTP connections as well already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ouch, it is a valid point in case of live sharing
💡
This is the code we use for the websocket connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM
res.sendStatus(200); | ||
} catch (e) { | ||
log.error("workspacePageClose failed", e); | ||
res.sendStatus(500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mustard-mh I just realised that it is wrong, i.e. if a user tries to access a workspace to which they don't have access it will pop up in our metrics like 500 (i.e. our failure), then it should be 403. Could you have a look sendHeartbeat method and translate exceptions properly? 🙏 cc @gitpod-io/engineering-webapp
Description
Add HTTP endpoint for supervisor frontend to send heartbeat when user's leaving the browser
In the current implementation of supervisor-frontend, we will send some requests before/during browser unload, which may cause some data loss since requests will be canceled after 1 second once user close the tab.
gitpod/components/supervisor/frontend/src/ide/heart-beat.ts
Lines 45 to 58 in 00d5108
We will move those requests to use Navigator.sendBeacon to avoid data loss.
So this PR is going to add HTTP post endpoint for sendBeacon
see also internal chat
Related Issue(s)
Fixes #
How to test
gpctl workspaces last-heartbeat <instance_id>
to see if heartbeat sentkubectl get pods ws-<instance_id> -oyaml | yq4 .metadata.annotations | grep closed
to see ifwasClosed
worksRelease Notes
Documentation
Werft options: