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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9bdbd81
Add useSlowDatabase feature flag to context
Nov 16, 2022
46dfbb4
Add subProtocol to WebSocketOptions interface
Nov 16, 2022
e481baa
Add `initGitPodService` function
Nov 18, 2022
46c6722
Call `initGitPodService` in App component
Nov 18, 2022
e00c277
Change feature flag name
Nov 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions components/dashboard/src/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { Login } from "./Login";
import { UserContext } from "./user-context";
import { getSelectedTeamSlug, TeamsContext } from "./teams/teams-context";
import { ThemeContext } from "./theme-context";
import { getGitpodService } from "./service/service";
import { getGitpodService, initGitPodService } from "./service/service";
import { shouldSeeWhatsNew, WhatsNew } from "./whatsnew/WhatsNew";
import gitpodIcon from "./icons/gitpod.svg";
import { ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
Expand Down Expand Up @@ -159,14 +159,18 @@ function App() {
const { user, setUser, refreshUserBillingMode } = useContext(UserContext);
const { teams, setTeams } = useContext(TeamsContext);
const { setIsDark } = useContext(ThemeContext);
const { usePublicApiTeamsService } = useContext(FeatureFlagContext);
const { usePublicApiTeamsService, useSlowDatabase } = useContext(FeatureFlagContext);

const [loading, setLoading] = useState<boolean>(true);
const [isWhatsNewShown, setWhatsNewShown] = useState(false);
const [showUserIdePreference, setShowUserIdePreference] = useState(false);
const [isSetupRequired, setSetupRequired] = useState(false);
const history = useHistory();

useEffect(() => {
initGitPodService(useSlowDatabase);
}, [useSlowDatabase]);
Comment on lines +170 to +172
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):

gitpod/components/dashboard/src/contexts/FeatureFlagContext.tsx

Lines 47 to 85 in 46c6722

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.


useEffect(() => {
(async () => {
var user: User | undefined;
Expand Down
5 changes: 5 additions & 0 deletions components/dashboard/src/contexts/FeatureFlagContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,14 @@ const FeatureFlagContext = createContext<{
showUseLastSuccessfulPrebuild: boolean;
usePublicApiTeamsService: boolean;
enablePersonalAccessTokens: boolean;
useSlowDatabase: boolean;
}>({
showPersistentVolumeClaimUI: false,
showUsageView: false,
showUseLastSuccessfulPrebuild: false,
usePublicApiTeamsService: false,
enablePersonalAccessTokens: false,
useSlowDatabase: false,
});

const FeatureFlagContextProvider: React.FC = ({ children }) => {
Expand All @@ -40,6 +42,7 @@ const FeatureFlagContextProvider: React.FC = ({ children }) => {
const [showUseLastSuccessfulPrebuild, setShowUseLastSuccessfulPrebuild] = useState<boolean>(false);
const [usePublicApiTeamsService, setUsePublicApiTeamsService] = useState<boolean>(false);
const [enablePersonalAccessTokens, setPersonalAccessTokensEnabled] = useState<boolean>(false);
const [useSlowDatabase, setUseSlowDatabase] = useState<boolean>(false);

useEffect(() => {
if (!user) return;
Expand All @@ -50,6 +53,7 @@ const FeatureFlagContextProvider: React.FC = ({ children }) => {
showUseLastSuccessfulPrebuild: { defaultValue: false, setter: setShowUseLastSuccessfulPrebuild },
publicApiExperimentalTeamsService: { defaultValue: false, setter: setUsePublicApiTeamsService },
personalAccessTokensEnabled: { defaultValue: false, setter: setPersonalAccessTokensEnabled },
slow_database: { defaultValue: false, setter: setUseSlowDatabase },
};
for (const [flagName, config] of Object.entries(featureFlags)) {
if (teams) {
Expand Down Expand Up @@ -88,6 +92,7 @@ const FeatureFlagContextProvider: React.FC = ({ children }) => {
showUseLastSuccessfulPrebuild,
usePublicApiTeamsService,
enablePersonalAccessTokens,
useSlowDatabase,
}}
>
{children}
Expand Down
14 changes: 12 additions & 2 deletions components/dashboard/src/service/service.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { log } from "@gitpod/gitpod-protocol/lib/util/logging";

export const gitpodHostUrl = new GitpodHostUrl(window.location.toString());

function createGitpodService<C extends GitpodClient, S extends GitpodServer>() {
function createGitpodService<C extends GitpodClient, S extends GitpodServer>(useSlowDatabase: boolean = false) {
if (window.top !== window.self && process.env.NODE_ENV === "production") {
const connection = createWindowMessageConnection("gitpodServer", window.parent, "*");
const factory = new JsonRpcProxyFactory<S>();
Expand Down Expand Up @@ -48,6 +48,7 @@ function createGitpodService<C extends GitpodClient, S extends GitpodServer>() {
onListening: (socket) => {
onReconnect = () => socket.reconnect();
},
subProtocol: useSlowDatabase ? "slow-database" : undefined,
});

return new GitpodServiceImpl<C, S>(proxy, { onReconnect });
Expand All @@ -64,4 +65,13 @@ function getGitpodService(): GitpodService {
return service;
}

export { getGitpodService };
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 };
Comment on lines +68 to +77
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.

7 changes: 4 additions & 3 deletions components/gitpod-protocol/src/messaging/browser/connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import ReconnectingWebSocket, { Event } from "reconnecting-websocket";
export interface WebSocketOptions {
onerror?: (event: Event) => void;
onListening?: (socket: ReconnectingWebSocket) => void;
subProtocol?: string;
}

export class WebSocketConnectionProvider {
Expand Down Expand Up @@ -62,7 +63,7 @@ export class WebSocketConnectionProvider {
*/
listen(handler: ConnectionHandler, eventHandler: ConnectionEventHandler, options?: WebSocketOptions): WebSocket {
const url = handler.path;
const webSocket = this.createWebSocket(url);
const webSocket = this.createWebSocket(url, options);

const logger = this.createLogger();
if (options && options.onerror) {
Expand All @@ -86,8 +87,8 @@ export class WebSocketConnectionProvider {
/**
* Creates a web socket for the given url
*/
createWebSocket(url: string): WebSocket {
return new ReconnectingWebSocket(url, undefined, {
createWebSocket(url: string, options?: WebSocketOptions): WebSocket {
return new ReconnectingWebSocket(url, options?.subProtocol, {
maxReconnectionDelay: 10000,
minReconnectionDelay: 1000,
reconnectionDelayGrowFactor: 1.3,
Expand Down