Skip to content

feat: Auto select workspace proxy based on lowest latency #7515

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 16 commits into from
May 22, 2023
Merged

feat: Auto select workspace proxy based on lowest latency #7515

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
159538c
WIP: Proxy auto select and user selection state
Emyrk May 11, 2023
1305931
chore: Auto select based on latency
Emyrk May 12, 2023
d1c9f3f
Add extra test for unknown latencies
Emyrk May 12, 2023
c739f24
Move arg inside the funcs
Emyrk May 12, 2023
ae26777
Fmt
Emyrk May 12, 2023
523e492
Duplicate import
Emyrk May 12, 2023
877c0c9
PR feedback
Emyrk May 12, 2023
8d26643
Merge remote-tracking branch 'origin/main' into stevenmasley/proxy_la…
Emyrk May 12, 2023
5c534fa
Refactor
Emyrk May 12, 2023
a3703e1
Fix useEffect dep
Emyrk May 18, 2023
c210e8d
Merge remote-tracking branch 'origin/main' into stevenmasley/proxy_la…
Emyrk May 18, 2023
a358258
Fix deps comment
Emyrk May 18, 2023
ec860c3
Remove comment
Emyrk May 22, 2023
3197eb1
Merge remote-tracking branch 'origin/main' into stevenmasley/proxy_la…
Emyrk May 22, 2023
ebb6a76
Mock the hook with a hook
Emyrk May 22, 2023
549093e
Remove unused dep
Emyrk May 22, 2023
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
Prev Previous commit
Next Next commit
Refactor
  • Loading branch information
@Emyrk
Emyrk committed May 12, 2023
commit 5c534fa260a270da8a56edcf9512199bf9e40e01
90 changes: 30 additions & 60 deletions site/src/contexts/ProxyContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,6 @@ export interface ProxyContextValue {
// The value `proxy` should always be used instead of `userProxy`. `userProxy` is only exposed
// so the caller can determine if the proxy being used is the user's selected proxy, or if it
// was auto selected based on some other criteria.
//
// if(proxy.selectedProxy.id === userProxy.id) { /* user selected proxy */ }
// else { /* proxy was auto selected */ }
userProxy?: Region

// proxies is the list of proxies returned by coderd. This is fetched async.
Expand Down Expand Up @@ -79,27 +76,18 @@ export const ProxyContext = createContext<ProxyContextValue | undefined>(
* ProxyProvider interacts with local storage to indicate the preferred workspace proxy.
*/
export const ProxyProvider: FC<PropsWithChildren> = ({ children }) => {
// Try to load the preferred proxy from local storage.
const [savedProxy, setUserProxy] = useState<Region | undefined>(
loadUserSelectedProxy(),
)

// As the proxies are being loaded, default to using the saved proxy.
// If the saved proxy is not valid when the async fetch happens, the
// proxy will be updated accordingly.
let defaultPreferredProxy = computeUsableURLS(savedProxy)
const dashboard = useDashboard()
const experimentEnabled = dashboard?.experiments.includes("moons")

if (!savedProxy) {
// If no preferred proxy is saved, then default to using relative paths
// and no subdomain support until the proxies are properly loaded.
// This is the same as a user not selecting any proxy.
defaultPreferredProxy = getPreferredProxy([])
}
// Using a useState so the caller always has the latest user saved
// proxy.
const [userSavedProxy, setUserSavedProxy] = useState(loadUserSelectedProxy())

const [proxy, setProxy] = useState<PreferredProxy>(defaultPreferredProxy)
// Load the initial state from local storage.
const [proxy, setProxy] = useState<PreferredProxy>(
computeUsableURLS(userSavedProxy),
)

const dashboard = useDashboard()
const experimentEnabled = dashboard?.experiments.includes("moons")
const queryKey = ["get-proxies"]
const {
data: proxiesResp,
Expand All @@ -109,57 +97,39 @@ export const ProxyProvider: FC<PropsWithChildren> = ({ children }) => {
} = useQuery({
queryKey,
queryFn: getWorkspaceProxies,
// This onSuccess ensures the local storage is synchronized with the
// proxies returned by coderd. If the selected proxy is not in the list,
// then the user selection is ignored.
onSuccess: (resp) => {
// Always pass in the user's choice.
setAndSaveProxy(savedProxy, resp.regions)
},
})

// Every time we get a new proxiesResponse, update the latency check
// to each workspace proxy.
const proxyLatencies = useProxyLatency(proxiesResp)

// If the proxies change or latencies change, we need to update the
// callback function.
const setAndSaveProxy = useCallback(
(
selectedProxy?: Region,
// By default the proxies come from the api call above.
// Allow the caller to override this if they have a more up
// to date list of proxies.
proxies: Region[] = proxiesResp?.regions || [],
) => {
if (!proxies) {
throw new Error(
"proxies are not yet loaded, so selecting a proxy makes no sense. How did you get here?",
)
}

// The preferred proxy attempts to use the user's selection if it is valid.
const preferred = getPreferredProxy(
proxies,
selectedProxy,
// updateProxy is a helper function that when called will
// update the proxy being used.
const updateProxy = useCallback(() => {
// Update the saved user proxy for the caller.
setUserSavedProxy(loadUserSelectedProxy())
setProxy(
getPreferredProxy(
proxiesResp?.regions ?? [],
// For some reason if I use 'userSavedProxy' here,
// the tests fail. It does not load "undefined" after a
// clear, but takes the previous value.
loadUserSelectedProxy(),
Copy link
Member

@Kira-Pilot Kira-Pilot May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

setStates are async - when you set a value, it isn't set instantly. So userSavedProxy may not be updated by the time you try to grab it here. That is probably why you're seeing the previous value. On top of that, there's a possibility we might be overwriting what the user is trying to set in localStorage by resetting to an older value on ln 110.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried adding an await setUserSaveProxy and still had this behavior.

Ln 110 only loads from local storage, it does not do any writes.

Copy link
Member

Choose a reason for hiding this comment

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

My bad about ln 110 - was confusing setUserSavedProxy and saveUserSelectedProxy as mentioned above.

Regarding your await: you can't await a setState because, although asynchronous, it doesn't return a promise. You could write a callback but I think what you have here is best. Do you mind removing the comment? I think this is expected behavior.

proxyLatencies,
)
// Set the state for the current context.
setProxy(preferred)
},
[proxiesResp, proxyLatencies],
)
),
)
}, [userSavedProxy, proxiesResp, proxyLatencies])

// This useEffect ensures the proxy to be used is updated whenever the state changes.
// This includes proxies being loaded, latencies being calculated, and the user selecting a proxy.
useEffect(() => {
setAndSaveProxy(savedProxy)
}, [savedProxy, proxiesResp, proxyLatencies, setAndSaveProxy])
updateProxy()
}, [proxiesResp, proxyLatencies])

return (
<ProxyContext.Provider
value={{
userProxy: savedProxy,
userProxy: userSavedProxy,
proxyLatencies: proxyLatencies,
proxy: experimentEnabled
? proxy
Expand All @@ -177,13 +147,13 @@ export const ProxyProvider: FC<PropsWithChildren> = ({ children }) => {
setProxy: (proxy: Region) => {
// Save to local storage to persist the user's preference across reloads
saveUserSelectedProxy(proxy)
// Set the state for the current context.
setUserProxy(proxy)
// Update the selected proxy
updateProxy()
},
clearProxy: () => {
// Clear the user's selection from local storage.
clearUserSelectedProxy()
setUserProxy(undefined)
updateProxy()
},
}}
>
Expand Down