feat(starfish): Adds useSpansQuery to handle querying from prod or local#48664
Conversation
github-actions
Bot
added
the
Scope: Frontend
…tarfish/prod-spans-queries
gggritso
left a comment
gggritso
left a comment
There was a problem hiding this comment.
Awesome, nice work! Having two methods to fetch is really confusing, so I hope we don't have to live in this state for too long, but for now this is a really good step forward
| }): ReturnType { | ||
| const {options} = useStarfishOptions(); | ||
| const {useDiscover} = options; | ||
| const queryFunction = getQueryFunction({useDiscover}); | ||
| if (isDiscoverFunction(queryFunction)) { | ||
| if (eventView) { | ||
| return queryFunction(eventView, initialData); | ||
| } | ||
| throw new Error( | ||
| 'eventView argument must be defined when Starfish useDiscover is true' | ||
| ); | ||
| } | ||
|
|
||
| if (queryString) { | ||
| return queryFunction(queryString, initialData); | ||
| } | ||
| throw new Error( | ||
| 'queryString argument must be defined when Starfish useDiscover is false, ie when using scraped data via fetch API' | ||
| ); | ||
| } |
There was a problem hiding this comment.
This part is really confusing because it's checking useDiscover, creating a function based on useDiscover, then checking the types of the functions to implement the behaviour. It'd be way simple to just do the conditional in one spot
if (useDiscover) {
const {isLoading, data} = useDiscoverQuery({
eventView,
orgSlug: organization.slug,
location,
});
return {isLoading, data: isLoading && initialData ? initialData : data?.data};
} else {
const {isLoading, data} = useQuery({
queryKey: [queryString],
queryFn: () => fetch(`${HOST}/?query=${queryString}`).then(res => res.json()),
retry: false,
initialData,
});
return {isLoading, data};
}
There was a problem hiding this comment.
Yeah I can see this being confusing. Fwiw, I did this so I could use a type guard on isDiscoverFunction to get typescript from complaining about arg types. I can definitely correct this though, I'll follow up in another pr!
Thanks! Yeah ideally we move exclusively onto |
2 participants
Adds a
useSpansQueryhook that handles querying from either prod via discover apis, or local scraped data.Updates some areas of API module to query via
useSpansQueryfor testing.Note: There's no ui toggle at the moment to switch between prod and local data. Just update
useStarfishOptionsto defaultuseDiscoverto true.TODO: Need to add support for events-stats queries