Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Emyrk merged 16 commits intomainfromstevenmasley/proxy_latency_pick
May 22, 2023

Conversation

Emyrk
Copy link
Member

@EmyrkEmyrk commentedMay 12, 2023
edited
Loading

EDIT: This has been reverted and no longer true

What this does

This changes thedefault 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:

constTestingScreen=()=>{
const{ proxy, userProxy, isFetched, isLoading, clearProxy, setProxy}=
useProxy()
return(
<>
<divdata-testid="isFetched"title={isFetched.toString()}></div>
<divdata-testid="isLoading"title={isLoading.toString()}></div>
<div
data-testid="preferredProxy"
title={proxy.selectedProxy&&proxy.selectedProxy.id}
></div>
<divdata-testid="userProxy"title={userProxy&&userProxy.id}></div>
<buttondata-testid="clearProxy"onClick={clearProxy}></button>
<divdata-testid="userSelectProxyData"></div>
<button
data-testid="userSelectProxy"
onClick={()=>{
constdata=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

TheProxyContext has been fully commented:

exportinterfaceProxyContextValue{
// 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.

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

Choose a reason for hiding this comment

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

You probably don't need this to test a hook. You can use therenderHook 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

Kira-Pilot reacted with eyes emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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 theuseDashboard provider present until this leaves experimental. So I'd need to souseProxy with the Dashboard context available 🤔

const selectProxyButton = screen.getByTestId("userSelectProxy")
await user.click(selectProxyButton)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice helpers!

Emyrk reacted with thumbs up emoji
// This includes proxies being loaded, latencies being calculated, and the user selecting a proxy.
useEffect(() => {
setAndSaveProxy(savedProxy)
}, [savedProxy, proxiesResp, proxyLatencies, setAndSaveProxy])
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yes it would.

}
}

Object.defineProperty(window, "localStorage", { value: localStorageMock() })
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma left a comment

Choose a reason for hiding this comment

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

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.

Emyrk reacted with thumbs up emoji
@Emyrk
Copy link
MemberAuthor

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

@Kira-Pilot
Copy link
Member

Kira-Pilot commentedMay 12, 2023
edited
Loading

I had a little trouble following the flow of logic in theProxyContext, 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>  )}

@Emyrk
Copy link
MemberAuthor

Emyrk commentedMay 12, 2023
edited
Loading

I had a little trouble following the flow of logic in theProxyContext, 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!

interfaceProxy{selectedProxy:Region|undefinedpreferredPathAppURL:stringpreferredWildcardHostname:string}exportinterfaceProxyContextValue{proxy:Proxyproxies?:Region[]isFetched:booleanisLoading:booleanerror?:Error|unknowngetSavedProxy:()=>voidproxyLatencies:Record<string,ProxyLatencyReport>setProxy:(selectedProxy:Region)=>voidclearProxy:()=>void}exportconstProxyContext=createContext<ProxyContextValue|undefined>(undefined,)/** * ProxyProvider interacts with local storage to indicate the preferred workspace proxy. */exportconstProxyProvider:FC<PropsWithChildren>=({ children})=>{constdashboard=useDashboard()constexperimentEnabled=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,})constproxyLatencies=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.Providervalue={{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 reloadssaveUserSelectedProxy(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

// 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-PilotKira-PilotMay 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. SouserSavedProxy 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 onln 110.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I tried adding anawait 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 confusingsetUserSavedProxy andsaveUserSelectedProxy as mentioned above.

Regarding yourawait: you can't await asetState 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.

Emyrk reacted with thumbs up emoji
@Kira-Pilot
Copy link
Member

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

Emyrk reacted with thumbs up emoji

@EmyrkEmyrk merged commitb8c07ff intomainMay 22, 2023
@EmyrkEmyrk deleted the stevenmasley/proxy_latency_pick branchMay 22, 2023 14:56
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsMay 22, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@BrunoQuaresmaBrunoQuaresmaBrunoQuaresma approved these changes

@Kira-PilotKira-PilotKira-Pilot approved these changes

Assignees

@EmyrkEmyrk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

workspace proxies: auto-pick proxy with the best latency
3 participants
@Emyrk@Kira-Pilot@BrunoQuaresma

[8]ページ先頭

©2009-2025 Movatter.jp