Skip to content
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

ref(insights): Replace usage of <ReleaseSeries> in <InsightsTimeSeriesWidget> #86300

Merged
merged 5 commits into from
Mar 5, 2025
Merged

ref(insights): Replace usage of <ReleaseSeries> in <InsightsTimeSeriesWidget> #86300

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a28ede2
Reapply "ref(insights): Replace usage of `<ReleaseSeries>` in `<Insig…
billyvg Mar 4, 2025
ca8cc8e
make `useReleasesStats` a bit more generic so we can use it in other …
billyvg Mar 4, 2025
aa324ac
move to utils
billyvg Mar 4, 2025
8f4edca
move to utils
billyvg Mar 4, 2025
60d4d0f
review clean-up
billyvg Mar 5, 2025
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
1 change: 1 addition & 0 deletions static/app/utils/queryClient.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export type ApiQueryKey =
Record<string, any>,
Record<string, any>
>,
additionalKey?: string,
Copy link
Member Author

Choose a reason for hiding this comment

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

See ca8cc8e (#86300)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the better fix would be to have useInfiniteApiQuery always append an additional ’infinite’ key. This would avoid future conflicts if the same endpoint is used as query and infinite query. I think there’s only one other usage of useInfiniteApiQuery so it shouldn’t be a huge impact.

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 agree, though it gets kind of messy because queryKey can be 1 or 2 elements

I suppose we can check the queryKey length and do something like:

  const query = useInfiniteQuery({
    queryKey:
      queryKey.length === 1
        ? ([...queryKey, {}, 'infinite'] as const)
        : ([...queryKey, 'infinite'] as const),

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah that’s not too bad. What it would then take is to make an InfiniteApiQueryKey type that we would use in fetchInfiniteQuery. useInfiniteApiQuery would still accept an ApiQueryKey, and we would do the transformation internally.

I split up the types a bit to get there:

type UrlOnly = readonly [url: string];
type UrlWithOptions = readonly [
  url: string,
  options: QueryKeyEndpointOptions<
    Record<string, string>,
    Record<string, any>,
    Record<string, any>
  >,
];

export type ApiQueryKey = UrlOnly | UrlWithOptions;

type InfiniteApiQueryKey = readonly [...UrlWithOptions, {type: 'infinite'}];

Copy link
Member Author

Choose a reason for hiding this comment

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

sgtm, will do this in a followup

];

export interface UseApiQueryOptions<TApiResponse, TError = RequestError>
Expand Down
46 changes: 33 additions & 13 deletions static/app/views/dashboards/widgets/timeSeriesWidget/useReleaseStats.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {useEffect, useRef} from 'react';

import {normalizeDateTimeParams} from 'sentry/components/organizations/pageFilters/parse';
import type {PageFilters} from 'sentry/types/core';
import {useInfiniteApiQuery} from 'sentry/utils/queryClient';
import useOrganization from 'sentry/utils/useOrganization';

Expand All @@ -8,11 +9,28 @@ interface ReleaseMetaBasic {
version: string;
}

interface UseReleaseStatsParams {
datetime: Parameters<typeof normalizeDateTimeParams>[0];
environments: readonly string[];
projects: readonly number[];
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 decided to change this from using PageFilters because it was annoying to use this when you don't give it pagefilter selection params directly (e.g. issue details uses eventView). Instead, since we pass the datetime options to normalizeDateTimeParams, use its param type.


/**
* Max number of pages to fetch. Default is 10 pages, which should be
* sufficient to fetch "all" releases.
*/
maxPages?: number;
Copy link
Member Author

Choose a reason for hiding this comment

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

Also decided to add this so we can control the # of pages to fetch and to limit the damage if something were to go wrong or if an org has more releases than we expect. For reference, Sentry fetches ~3 pages for a 90d window.

}

/**
* Fetches *ALL* releases (e.g. all pages worth)
* This is intended to fetch "all" releases, we have a default limit of
* 10 pages (of 1000 results) to be slightly cautious.
*/
export function useReleaseStats({datetime, environments, projects}: PageFilters) {
export function useReleaseStats(
{datetime, environments, projects, maxPages = 10}: UseReleaseStatsParams,
queryOptions: {staleTime: number} = {staleTime: Infinity}
) {
const organization = useOrganization();
const currentNumberPages = useRef(0);

const {
isLoading,
Expand All @@ -33,24 +51,26 @@ export function useReleaseStats({datetime, environments, projects}: PageFilters)
...normalizeDateTimeParams(datetime),
},
},
// This is here to prevent a cache key conflict between normal queries and
// "infinite" queries. Read more here: https://tkdodo.eu/blog/effective-react-query-keys#caching-data
'load-all',
],
staleTime: Infinity,
...queryOptions,
enabled: currentNumberPages.current < maxPages,
});

if (!isFetching && hasNextPage) {
fetchNextPage();
}

const releases =
data?.pages.flatMap(([pageData]) =>
pageData.map(({date, version}) => ({timestamp: date, version}))
) ?? [];
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this mapping to where it's needed

useEffect(() => {
if (!isFetching && hasNextPage && currentNumberPages.current + 1 < maxPages) {
fetchNextPage();
currentNumberPages.current++;
}
}, [isFetching, hasNextPage, fetchNextPage, maxPages]);

return {
isLoading,
isPending,
isError,
error,
releases,
releases: data?.pages.flatMap(([pageData]) => pageData),
};
}
7 changes: 6 additions & 1 deletion static/app/views/insights/common/components/insightsTimeSeriesWidget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,12 @@ export interface InsightsTimeSeriesWidgetProps {

export function InsightsTimeSeriesWidget(props: InsightsTimeSeriesWidgetProps) {
const pageFilters = usePageFilters();
const {releases} = useReleaseStats(pageFilters.selection);
const {releases: releasesWithDate} = useReleaseStats(pageFilters.selection);
const releases =
releasesWithDate?.map(({date, version}) => ({
timestamp: date,
version,
})) ?? [];

const visualizationProps: TimeSeriesWidgetVisualizationProps = {
visualizationType: props.visualizationType,
Expand Down