Skip to content

[dashboard/protocol] Pass slow-database Sec-WebSocket-Protocol header to websocket connections from dashboard #14752

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 5 commits into from
Nov 22, 2022

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Nov 16, 2022

Description

Allow websocket connections from the dashboard to the server to specify a Sec-WebSocket-Protocol header indicating whether the connection should use a high-latency database connection, according to the value of a feature flag.

At a high level, the architecture looks like this:

ws

Together with the dependency #14778 which handles the proxying of websocket upgrade requests to the slow-server backend based on the presence of the Sec-WebSocket-Protocol header, this PR gives us a means to roll out higher-latency dashboard to server websocket connections to selected internal users.

Related Issue(s)

Part of #9198

How to test

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-slow-database
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@andrew-farries andrew-farries force-pushed the af/use-slow-database-ws-subprotocol branch from e61b8ba to 280e49e Compare November 17, 2022 21:46
@andrew-farries andrew-farries force-pushed the af/use-slow-database-ws-subprotocol branch from 280e49e to 719ec50 Compare November 18, 2022 12:28
@gitpod-io gitpod-io deleted a comment from werft-gitpod-dev-com bot Nov 18, 2022
@gitpod-io gitpod-io deleted a comment from werft-gitpod-dev-com bot Nov 18, 2022
@andrew-farries andrew-farries force-pushed the af/use-slow-database-ws-subprotocol branch from 719ec50 to 07a8d16 Compare November 18, 2022 16:19
@roboquat roboquat added size/L and removed size/M labels Nov 18, 2022
@andrew-farries andrew-farries changed the base branch from main to af/proxy-to-slow-server-based-on-ws-header November 18, 2022 16:20
@roboquat roboquat added size/M and removed size/L labels Nov 18, 2022
@andrew-farries andrew-farries force-pushed the af/use-slow-database-ws-subprotocol branch from 8297a02 to 152a132 Compare November 19, 2022 15:07
@andrew-farries andrew-farries marked this pull request as ready for review November 20, 2022 21:01
@andrew-farries andrew-farries requested a review from a team November 20, 2022 21:01
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Nov 20, 2022
@andrew-farries
Copy link
Contributor Author

/hold for dependency

Base automatically changed from af/proxy-to-slow-server-based-on-ws-header to main November 21, 2022 13:53
@roboquat roboquat added size/L and removed size/M labels Nov 21, 2022
Andrew Farries added 4 commits November 21, 2022 15:59
Pass the options into createWebSocket and use it to decide what value
should be sent as the `Sec-WebSocket-Protocol` header when creating the
websocket connection.
@andrew-farries andrew-farries force-pushed the af/use-slow-database-ws-subprotocol branch from 152a132 to 46c6722 Compare November 21, 2022 16:00
@roboquat roboquat removed the size/L label Nov 21, 2022
Comment on lines +68 to +77
function initGitPodService(useSlowDatabase: boolean) {
const w = window as any;
const _gp = w._gp || (w._gp = {});
if (window.location.search.includes("service=mock")) {
_gp.gitpodService = require("./service-mock").gitpodServiceMock;
}
_gp.gitpodService = createGitpodService(useSlowDatabase);
}

export { getGitpodService, initGitPodService };
Copy link
Member

Choose a reason for hiding this comment

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

Would you be able to explain why we're doing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function re-initializes the websocket connection - ie makes a new connection.

The getGitpodService function above lazily creates a ws connection, ie it will create on the first time it is called if there is no connection yet.

We want to be able to create a brand new connection on demand when the value of the slow-database feature flag changes.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Do we need the mock logic in there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not - I took it from getGitpodService function above. I'm inclined to keep it.

Comment on lines +170 to +172
useEffect(() => {
initGitPodService(useSlowDatabase);
}, [useSlowDatabase]);
Copy link
Member

Choose a reason for hiding this comment

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

This seems to cause a bootstrap problem. We don't know who the user is (to evaluate the feature flag), but we're creating the connection which is parametrized on the feature flag (by user). I suspect this would result in always using "false" for the slow database argument

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The value of useSlowDatabase is taken from the FeatureFlagContext. All feature flags are re-evaluated when the value of user changes (the dependency array in the effect includes user):

useEffect(() => {
if (!user) return;
(async () => {
const featureFlags: FeatureFlagConfig = {
persistent_volume_claim: { defaultValue: true, setter: setShowPersistentVolumeClaimUI },
usage_view: { defaultValue: false, setter: setShowUsageView },
showUseLastSuccessfulPrebuild: { defaultValue: false, setter: setShowUseLastSuccessfulPrebuild },
publicApiExperimentalTeamsService: { defaultValue: false, setter: setUsePublicApiTeamsService },
personalAccessTokensEnabled: { defaultValue: false, setter: setPersonalAccessTokensEnabled },
useSlowDatabase: { defaultValue: false, setter: setUseSlowDatabase },
};
for (const [flagName, config] of Object.entries(featureFlags)) {
if (teams) {
for (const team of teams) {
const flagValue = await getExperimentsClient().getValueAsync(flagName, config.defaultValue, {
user,
projectId: project?.id,
teamId: team.id,
teamName: team?.name,
});
// We got an explicit override value from ConfigCat
if (flagValue !== config.defaultValue) {
config.setter(flagValue);
return;
}
}
}
const flagValue = await getExperimentsClient().getValueAsync(flagName, config.defaultValue, {
user,
projectId: project?.id,
teamId: team?.id,
teamName: team?.name,
});
config.setter(flagValue);
}
})();
}, [user, teams, team, project]);

Whenever the context sets the value of useSlowDatabase (as it will whenever the effect runs) it causes any component that consumes the context to re-render, and the dependency array in the useEffect here ensures that we re-initialize the websocket connection whenever App renders.

Copy link
Member

Choose a reason for hiding this comment

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

I guess with this change, we might see nearly double the WS connections being created. Just something we should be aware of from ops side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we'll only see two connections for the small subset of users for which the feature flag is enabled.

For everyone else, the value of useSlowDatabase will never change and the connection will never be re-initialized.

Use the name that matches the flag as defined in ConfigCat.
@roboquat roboquat merged commit 5c5ecb1 into main Nov 22, 2022
@roboquat roboquat deleted the af/use-slow-database-ws-subprotocol branch November 22, 2022 15:36
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants