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

Emyrk · 2023-05-12T14:27:29Z

EDIT: This has been reverted and no longer true

What this does

This changes the default behavior of workspace proxies to auto select based on latency, rather than just using the name "primary".

Fixes: #7496

Continuation of: #7486

Testing

I added a suite of tests on a basic TestComponent:

coder/site/src/contexts/ProxyContext.test.tsx

Lines 139 to 165 in 1305931

    
           const TestingScreen = () => { 
        
             const { proxy, userProxy, isFetched, isLoading, clearProxy, setProxy } = 
        
               useProxy() 
        
             return ( 
        
               <> 
        
                 <div data-testid="isFetched" title={isFetched.toString()}></div> 
        
                 <div data-testid="isLoading" title={isLoading.toString()}></div> 
        
                 <div 
        
                   data-testid="preferredProxy" 
        
                   title={proxy.selectedProxy && proxy.selectedProxy.id} 
        
                 ></div> 
        
                 <div data-testid="userProxy" title={userProxy && userProxy.id}></div> 
        
                 <button data-testid="clearProxy" onClick={clearProxy}></button> 
        
                 <div data-testid="userSelectProxyData"></div> 
        
                 <button 
        
                   data-testid="userSelectProxy" 
        
                   onClick={() => { 
        
                     const data = screen.getByTestId("userSelectProxyData") 
        
                     if (data.innerText) { 
        
                       setProxy(JSON.parse(data.innerText)) 
        
                     } 
        
                   }} 
        
                 ></button> 
        
               </> 
        
             ) 
        
           }

This is so I can test the context independent of any UI/UX and styling.

Extra

The ProxyContext has been fully commented:

coder/site/src/contexts/ProxyContext.tsx

Lines 16 to 58 in 1305931

    
           export interface ProxyContextValue { 
        
             // proxy is **always** the workspace proxy that should be used. 
        
             // The 'proxy.selectedProxy' field is the proxy being used and comes from either: 
        
             //   1. The user manually selected this proxy. (saved to local storage) 
        
             //   2. The default proxy auto selected because: 
        
             //    a. The user has not selected a proxy. 
        
             //    b. The user's selected proxy is not in the list of proxies. 
        
             //    c. The user's selected proxy is not healthy. 
        
             //   3. undefined if there are no proxies. 
        
             // 
        
             // The values 'proxy.preferredPathAppURL' and 'proxy.preferredWildcardHostname' can 
        
             // always be used even if 'proxy.selectedProxy' is undefined. These values are sourced from 
        
             // the 'selectedProxy', but default to relative paths if the 'selectedProxy' is undefined. 
        
             proxy: PreferredProxy 
        
             // userProxy is always the proxy the user has selected. This value comes from local storage. 
        
             // 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. 
        
             // isFetched, isLoading, and error are used to track the state of the async call. 
        
             proxies?: Region[] 
        
             // isFetched is true when the 'proxies' api call is complete. 
        
             isFetched: boolean 
        
             isLoading: boolean 
        
             error?: Error | unknown 
        
             // proxyLatencies is a map of proxy id to latency report. If the proxyLatencies[proxy.id] is undefined 
        
             // then the latency has not been fetched yet. Calculations happen async for each proxy in the list. 
        
             // Refer to the returned report for a given proxy for more information. 
        
             proxyLatencies: Record<string, ProxyLatencyReport> 
        
             // setProxy is a function that sets the user's selected proxy. This function should 
        
             // only be called if the user is manually selecting a proxy. This value is stored in local 
        
             // storage and will persist across reloads and tabs. 
        
             setProxy: (selectedProxy: Region) => void 
        
             // clearProxy is a function that clears the user's selected proxy. 
        
             // If no proxy is selected, then the default proxy will be used. 
        
             clearProxy: () => void 
        
           }

Extra functionality:

clearProxy to remove a user's selected proxy
userProxy returns the user's selected proxy. Since their choice might be overriden.

Lotta comments

site/src/contexts/ProxyContext.test.tsx

BrunoQuaresma · 2023-05-12T16:52:35Z

site/src/contexts/ProxyContext.test.tsx

+// TestingScreen just mounts some components that we can check in the unit test.
+const TestingScreen = () => {
+  const { proxy, userProxy, isFetched, isLoading, clearProxy, setProxy } =
+    useProxy()


You probably don't need this to test a hook. You can use the renderHook from @testing-library/react. I think it can make the test way easier to reason about. More about its usage: https://kentcdodds.com/blog/how-to-test-custom-react-hooks

This looks like exactly what I was looking for.

In the example they have this:

const {result} = renderHook(() => useUndo('one'))

The only caveat I have is I also need the useDashboard provider present until this leaves experimental. So I'd need to so useProxy with the Dashboard context available 🤔

BrunoQuaresma · 2023-05-12T16:54:18Z

site/src/contexts/ProxyContext.test.tsx

+      const selectProxyButton = screen.getByTestId("userSelectProxy")
+      await user.click(selectProxyButton)
+    }
+  }


Nice helpers!

site/src/contexts/ProxyContext.test.tsx

BrunoQuaresma · 2023-05-12T16:57:53Z

site/src/contexts/ProxyContext.tsx

+  // This includes proxies being loaded, latencies being calculated, and the user selecting a proxy.
+  useEffect(() => {
+    setAndSaveProxy(savedProxy)
+  }, [savedProxy, proxiesResp, proxyLatencies, setAndSaveProxy])


Not sure if I'm getting this correctly but let's say the latency changes while I'm on the page, would the proxy be changed automatically?

Yes it would.

BrunoQuaresma · 2023-05-12T17:00:47Z

site/src/testHelpers/localstorage.ts

+  }
+}
+
+Object.defineProperty(window, "localStorage", { value: localStorageMock() })


Since this is being executed when the module is imported, maybe can be interesting to move all this code into the test setup file and mock it globally. We are doing that for a few default window interfaces.

maybe, I just wanted to avoid putting things in the global if my test was the only one using it atm.

BrunoQuaresma

I just reviewed the code and left a few comments but nothing that should be considered as a blocker. Let me know if you need a QA.

site/src/contexts/ProxyContext.tsx

Emyrk · 2023-05-12T18:14:44Z

Just having some CI issues with the test. Seems to be hanging and timing out?

Kira-Pilot · 2023-05-12T18:49:41Z

I had a little trouble following the flow of logic in the ProxyContext, and so I tried to simplify it a bit. I could easily be missing something - it's a complicated feature and you've done a great job with it - but I'm wondering if we can simplify our use of React state a little bit. This might make testing easier, as well. I'm happy to walk through this on a call!

interface Proxy {
  selectedProxy: Region | undefined
  preferredPathAppURL: string
  preferredWildcardHostname: string
}

export interface ProxyContextValue {
  proxy: Proxy
  proxies?: Region[]
  isFetched: boolean
  isLoading: boolean
  error?: Error | unknown
  getSavedProxy: () => void
  proxyLatencies: Record<string, ProxyLatencyReport>
  setProxy: (selectedProxy: Region) => void
  clearProxy: () => void
}

export const ProxyContext = createContext<ProxyContextValue | undefined>(
  undefined,
)

/**
 * ProxyProvider interacts with local storage to indicate the preferred workspace proxy.
 */
export const ProxyProvider: FC<PropsWithChildren> = ({ children }) => {
  const dashboard = useDashboard()
  const experimentEnabled = dashboard?.experiments.includes("moons")

  const [proxy, setProxy] = useState<Proxy>(
    getPreferredProxy([], loadUserSelectedProxy()),
  )

  const {
    data: proxiesResp,
    error: proxiesError,
    isLoading: proxiesLoading,
    isFetched: proxiesFetched,
  } = useQuery({
    queryKey: ["get-proxies"],
    queryFn: getWorkspaceProxies,
  })

  const proxyLatencies = useProxyLatency(proxiesResp)

  // 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(() => {
    setProxy(
      getPreferredProxy(
        proxiesResp?.regions ?? [],
        loadUserSelectedProxy(),
        proxyLatencies,
      ),
    )
  }, [proxiesResp, proxyLatencies])

  return (
    <ProxyContext.Provider
      value={{
        getSavedProxy: () => loadUserSelectedProxy(),
        proxyLatencies: proxyLatencies,
        proxy: experimentEnabled
          ? proxy
          : {
              // If the experiment is disabled, then call 'getPreferredProxy' with the regions from
              // the api call. The default behavior is to use the `primary` proxy.
              ...getPreferredProxy(proxiesResp?.regions || []),
            },
        proxies: proxiesResp?.regions,
        isLoading: proxiesLoading,
        isFetched: proxiesFetched,
        error: proxiesError,
        // These functions are exposed to allow the user to select a proxy.
        setProxy: (proxy: Region) => {
          // Save to local storage to persist the user's preference across reloads
          saveUserSelectedProxy(proxy)
          // Set the state for the current context.
          getPreferredProxy(proxiesResp?.regions ?? [], loadUserSelectedProxy())
        },
        clearProxy: () => {
          // Clear the user's selection from local storage.
          clearUserSelectedProxy()
          // Set the state for the current context.
          getPreferredProxy(proxiesResp?.regions ?? [])
        },
      }}
    >
      {children}
    </ProxyContext.Provider>
  )
}

…tency_pick

Emyrk · 2023-05-12T18:57:04Z

I had a little trouble following the flow of logic in the ProxyContext, and so I tried to simplify it a bit. I could easily be missing something - it's a complicated feature and you've done a great job with it - but I'm wondering if we can simplify our use of React state a little bit. This might make testing easier, as well. I'm happy to walk through this on a call!

interface Proxy {
  selectedProxy: Region | undefined
  preferredPathAppURL: string
  preferredWildcardHostname: string
}

export interface ProxyContextValue {
  proxy: Proxy
  proxies?: Region[]
  isFetched: boolean
  isLoading: boolean
  error?: Error | unknown
  getSavedProxy: () => void
  proxyLatencies: Record<string, ProxyLatencyReport>
  setProxy: (selectedProxy: Region) => void
  clearProxy: () => void
}

export const ProxyContext = createContext<ProxyContextValue | undefined>(
  undefined,
)

/**
 * ProxyProvider interacts with local storage to indicate the preferred workspace proxy.
 */
export const ProxyProvider: FC<PropsWithChildren> = ({ children }) => {
  const dashboard = useDashboard()
  const experimentEnabled = dashboard?.experiments.includes("moons")

  const [proxy, setProxy] = useState<Proxy>(
    getPreferredProxy([], loadUserSelectedProxy()),
  )

  const {
    data: proxiesResp,
    error: proxiesError,
    isLoading: proxiesLoading,
    isFetched: proxiesFetched,
  } = useQuery({
    queryKey: ["get-proxies"],
    queryFn: getWorkspaceProxies,
  })

  const proxyLatencies = useProxyLatency(proxiesResp)

  // 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(() => {
    setProxy(
      getPreferredProxy(
        proxiesResp?.regions ?? [],
        loadUserSelectedProxy(),
        proxyLatencies,
      ),
    )
  }, [proxiesResp, proxyLatencies])

  return (
    <ProxyContext.Provider
      value={{
        getSavedProxy: () => loadUserSelectedProxy(),
        proxyLatencies: proxyLatencies,
        proxy: experimentEnabled
          ? proxy
          : {
              // If the experiment is disabled, then call 'getPreferredProxy' with the regions from
              // the api call. The default behavior is to use the `primary` proxy.
              ...getPreferredProxy(proxiesResp?.regions || []),
            },
        proxies: proxiesResp?.regions,
        isLoading: proxiesLoading,
        isFetched: proxiesFetched,
        error: proxiesError,
        // These functions are exposed to allow the user to select a proxy.
        setProxy: (proxy: Region) => {
          // Save to local storage to persist the user's preference across reloads
          saveUserSelectedProxy(proxy)
          // Set the state for the current context.
          getPreferredProxy(proxiesResp?.regions ?? [], loadUserSelectedProxy())
        },
        clearProxy: () => {
          // Clear the user's selection from local storage.
          clearUserSelectedProxy()
          // Set the state for the current context.
          getPreferredProxy(proxiesResp?.regions ?? [])
        },
      }}
    >
      {children}
    </ProxyContext.Provider>
  )
}

Will take a look! The nice thing is if the tests pass, then it's all still good

site/src/contexts/ProxyContext.tsx

Kira-Pilot · 2023-05-15T15:40:20Z

site/src/contexts/ProxyContext.tsx

+        // 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(),


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.

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

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

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.

site/src/contexts/ProxyContext.tsx

…tency_pick

Kira-Pilot · 2023-05-19T13:35:29Z

We do not need to tackle this in your PR - can totally be future work, but just a heads up that we now have a useLocal hook that might be fun to add here some point down the line.

…tency_pick

Emyrk added 2 commits May 11, 2023 15:43

WIP: Proxy auto select and user selection state

159538c

Lotta comments

chore: Auto select based on latency

1305931

github-actions bot assigned Emyrk May 12, 2023

Emyrk added 4 commits May 12, 2023 09:32

Add extra test for unknown latencies

d1c9f3f

Move arg inside the funcs

c739f24

Fmt

ae26777

Duplicate import

523e492

Emyrk requested review from Kira-Pilot and BrunoQuaresma May 12, 2023 15:59

BrunoQuaresma reviewed May 12, 2023

View reviewed changes

site/src/contexts/ProxyContext.test.tsx Show resolved Hide resolved

BrunoQuaresma reviewed May 12, 2023

View reviewed changes

site/src/contexts/ProxyContext.test.tsx Show resolved Hide resolved

BrunoQuaresma reviewed May 12, 2023

View reviewed changes

site/src/contexts/ProxyContext.test.tsx Outdated Show resolved Hide resolved

BrunoQuaresma reviewed May 12, 2023

View reviewed changes

BrunoQuaresma approved these changes May 12, 2023

View reviewed changes