Skip to content

Commit

Permalink
ref(insights): Replace usage of <ReleaseSeries> in `<InsightsTimeSe…
Browse files Browse the repository at this point in the history
…riesWidget>` (#86300)

This reverts commit 3febd2b and extends
the [original PR](#86129)

It was reverted because it was causing crashes
(https://sentry.sentry.io/issues/6352301875/?project=11276&referrer=github-pr-bot)
due to a cache key conflict in react-query between `useQuery` and
`useInfiniteQuery` ([read more about it
here](https://tkdodo.eu/blog/effective-react-query-keys#caching-data)).
This would happen in Sentry if you visited an insights page (which uses
an "infinite" query) and then went to e.g. the issue details page (which
has it's own hook
[useReleaseMarkLineSeries](https://github.com/getsentry/sentry/blob/master/static/app/views/issueDetails/streamline/hooks/useReleaseMarkLineSeries.tsx#L25-L45)
that uses a "normal" query). Both queries use the same cache key because
infinite queries handles the `cursor` param for us (thus it not being
part of the query key).

I've made some additional changes to the hook: it's now moved to
`app/utils` as it'll be needed in other places. it's also more generic
now.


From the original PR:

> This will be needed for the new release bubbles feature as we will
show releases on the mini widgets instead of only on fullscreen. Using
`<ReleaseSeries>` renderer will cause duplicate requests to the API.


ref #85779
  • Loading branch information
billyvg authored Mar 5, 2025
1 parent 5023d1d commit 9218558
Show file tree
Hide file tree
Showing 13 changed files with 199 additions and 30 deletions.
4 changes: 4 additions & 0 deletions static/app/utils/queryClient.tsx
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,
];

export interface UseApiQueryOptions<TApiResponse, TError = RequestError>
Expand Down Expand Up @@ -287,9 +288,11 @@ function parsePageParam(dir: 'previous' | 'next') {
export function useInfiniteApiQuery<TResponseData>({
queryKey,
enabled,
staleTime,
}: {
queryKey: ApiQueryKey;
enabled?: boolean;
staleTime?: number;
}) {
const api = useApi({persistInFlight: PERSIST_IN_FLIGHT});
const query = useInfiniteQuery({
Expand All @@ -299,6 +302,7 @@ export function useInfiniteApiQuery<TResponseData>({
getNextPageParam: parsePageParam('next'),
initialPageParam: undefined,
enabled: enabled ?? true,
staleTime,
});

return query;
Expand Down
75 changes: 75 additions & 0 deletions static/app/utils/useReleaseStats.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import {useEffect} from 'react';

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

interface ReleaseMetaBasic {
date: string;
version: string;
}

interface UseReleaseStatsParams {
datetime: Parameters<typeof normalizeDateTimeParams>[0];
environments: readonly string[];
projects: readonly number[];

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

/**
* 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, maxPages = 10}: UseReleaseStatsParams,
queryOptions: {staleTime: number} = {staleTime: Infinity}
) {
const organization = useOrganization();

const {
isLoading,
isFetching,
fetchNextPage,
hasNextPage,
isPending,
isError,
error,
data,
} = useInfiniteApiQuery<ReleaseMetaBasic[]>({
queryKey: [
`/organizations/${organization.slug}/releases/stats/`,
{
query: {
environment: environments,
project: projects,
...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',
],
...queryOptions,
});

const currentNumberPages = data?.pages.length ?? 0;

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

return {
isLoading,
isPending,
isError,
error,
releases: data?.pages.flatMap(([pageData]) => pageData),
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ jest.mock('sentry/utils/useLocation');
jest.mock('sentry/utils/usePageFilters');
jest.mock('sentry/utils/useProjects');
jest.mock('sentry/views/insights/common/queries/useOnboardingProject');
import {useReleaseStats} from 'sentry/utils/useReleaseStats';

jest.mock('sentry/utils/useReleaseStats');

const requestMocks: Record<string, jest.Mock> = {};

Expand Down Expand Up @@ -171,6 +174,13 @@ const setupMocks = () => {
reloadProjects: jest.fn(),
placeholders: [],
});
jest.mocked(useReleaseStats).mockReturnValue({
isLoading: false,
isPending: false,
isError: false,
error: null,
releases: [],
});
};

const setupMockRequests = (organization: Organization) => {
Expand Down
11 changes: 11 additions & 0 deletions static/app/views/insights/cache/views/cacheLandingPage.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ jest.mock('sentry/utils/useLocation');
jest.mock('sentry/utils/usePageFilters');
jest.mock('sentry/utils/useProjects');
jest.mock('sentry/views/insights/common/queries/useOnboardingProject');
import {useReleaseStats} from 'sentry/utils/useReleaseStats';

jest.mock('sentry/utils/useReleaseStats');

const requestMocks = {
missRateChart: jest.fn(),
Expand Down Expand Up @@ -79,6 +82,14 @@ describe('CacheLandingPage', function () {
initiallyLoaded: false,
});

jest.mocked(useReleaseStats).mockReturnValue({
isLoading: false,
isPending: false,
isError: false,
error: null,
releases: [],
});

beforeEach(function () {
jest.clearAllMocks();
setRequestMocks(organization);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ import styled from '@emotion/styled';

import {openInsightChartModal} from 'sentry/actionCreators/modal';
import {Button} from 'sentry/components/button';
import ReleaseSeries from 'sentry/components/charts/releaseSeries';
import {CHART_PALETTE} from 'sentry/constants/chartPalette';
import {IconExpand} from 'sentry/icons';
import {t} from 'sentry/locale';
import usePageFilters from 'sentry/utils/usePageFilters';
import {useReleaseStats} from 'sentry/utils/useReleaseStats';
import {MISSING_DATA_MESSAGE} from 'sentry/views/dashboards/widgets/common/settings';
import type {Aliases} from 'sentry/views/dashboards/widgets/common/types';
import {
Expand Down Expand Up @@ -38,8 +38,12 @@ export interface InsightsTimeSeriesWidgetProps {

export function InsightsTimeSeriesWidget(props: InsightsTimeSeriesWidgetProps) {
const pageFilters = usePageFilters();
const {start, end, period, utc} = pageFilters.selection.datetime;
const {projects, environments} = 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 Expand Up @@ -108,33 +112,12 @@ export function InsightsTimeSeriesWidget(props: InsightsTimeSeriesWidgetProps) {
openInsightChartModal({
title: props.title,
children: (
<ReleaseSeries
start={start}
end={end}
queryExtra={undefined}
period={period}
utc={utc}
projects={projects}
environments={environments}
>
{({releases}) => {
return (
<ModalChartContainer>
<TimeSeriesWidgetVisualization
{...visualizationProps}
releases={
releases
? releases.map(release => ({
timestamp: release.date,
version: release.version,
}))
: []
}
/>
</ModalChartContainer>
);
}}
</ReleaseSeries>
<ModalChartContainer>
<TimeSeriesWidgetVisualization
{...visualizationProps}
releases={releases ?? []}
/>
</ModalChartContainer>
),
});
}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ jest.mock('sentry/utils/useLocation');
jest.mock('sentry/utils/usePageFilters');
jest.mock('sentry/utils/useProjects');
jest.mock('sentry/views/insights/common/queries/useOnboardingProject');
import {useReleaseStats} from 'sentry/utils/useReleaseStats';

jest.mock('sentry/utils/useReleaseStats');

describe('DatabaseLandingPage', function () {
const organization = OrganizationFixture({features: ['insights-initial-modules']});
Expand Down Expand Up @@ -60,6 +63,14 @@ describe('DatabaseLandingPage', function () {
key: '',
});

jest.mocked(useReleaseStats).mockReturnValue({
isLoading: false,
isPending: false,
isError: false,
error: null,
releases: [],
});

beforeEach(function () {
MockApiClient.addMockResponse({
url: '/organizations/org-slug/sdk-updates/',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import {DatabaseSpanSummaryPage} from 'sentry/views/insights/database/views/data

jest.mock('sentry/utils/useLocation');
jest.mock('sentry/utils/usePageFilters');
import {useReleaseStats} from 'sentry/utils/useReleaseStats';

jest.mock('sentry/utils/useReleaseStats');

describe('DatabaseSpanSummaryPage', function () {
const organization = OrganizationFixture({
Expand Down Expand Up @@ -44,6 +47,14 @@ describe('DatabaseSpanSummaryPage', function () {
key: '',
});

jest.mocked(useReleaseStats).mockReturnValue({
isLoading: false,
isPending: false,
isError: false,
error: null,
releases: [],
});

beforeEach(function () {
jest.clearAllMocks();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ import {HTTPDomainSummaryPage} from 'sentry/views/insights/http/views/httpDomain

jest.mock('sentry/utils/useLocation');
jest.mock('sentry/utils/usePageFilters');
import {useReleaseStats} from 'sentry/utils/useReleaseStats';

jest.mock('sentry/utils/useReleaseStats');

describe('HTTPSummaryPage', function () {
const organization = OrganizationFixture({features: ['insights-initial-modules']});
Expand Down Expand Up @@ -52,6 +55,14 @@ describe('HTTPSummaryPage', function () {
key: '',
});

jest.mocked(useReleaseStats).mockReturnValue({
isLoading: false,
isPending: false,
isError: false,
error: null,
releases: [],
});

beforeEach(function () {
jest.clearAllMocks();

Expand Down
11 changes: 11 additions & 0 deletions static/app/views/insights/http/views/httpLandingPage.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ jest.mock('sentry/utils/useLocation');
jest.mock('sentry/utils/usePageFilters');
jest.mock('sentry/utils/useProjects');
jest.mock('sentry/views/insights/common/queries/useOnboardingProject');
import {useReleaseStats} from 'sentry/utils/useReleaseStats';

jest.mock('sentry/utils/useReleaseStats');

describe('HTTPLandingPage', function () {
const organization = OrganizationFixture({
Expand Down Expand Up @@ -75,6 +78,14 @@ describe('HTTPLandingPage', function () {
initiallyLoaded: false,
});

jest.mocked(useReleaseStats).mockReturnValue({
isLoading: false,
isPending: false,
isError: false,
error: null,
releases: [],
});

beforeEach(function () {
jest.clearAllMocks();

Expand Down
11 changes: 11 additions & 0 deletions static/app/views/insights/queues/charts/latencyChart.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,23 @@ import {OrganizationFixture} from 'sentry-fixture/organization';

import {render, screen, waitForElementToBeRemoved} from 'sentry-test/reactTestingLibrary';

import {useReleaseStats} from 'sentry/utils/useReleaseStats';
import {LatencyChart} from 'sentry/views/insights/queues/charts/latencyChart';
import {Referrer} from 'sentry/views/insights/queues/referrers';

jest.mock('sentry/utils/useReleaseStats');

describe('latencyChart', () => {
const organization = OrganizationFixture();

jest.mocked(useReleaseStats).mockReturnValue({
isLoading: false,
isPending: false,
isError: false,
error: null,
releases: [],
});

let eventsStatsMock: jest.Mock;

beforeEach(() => {
Expand Down
11 changes: 11 additions & 0 deletions static/app/views/insights/queues/charts/throughputChart.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,25 @@ import {OrganizationFixture} from 'sentry-fixture/organization';

import {render, screen, waitForElementToBeRemoved} from 'sentry-test/reactTestingLibrary';

import {useReleaseStats} from 'sentry/utils/useReleaseStats';
import {ThroughputChart} from 'sentry/views/insights/queues/charts/throughputChart';
import {Referrer} from 'sentry/views/insights/queues/referrers';

jest.mock('sentry/utils/useReleaseStats');

describe('throughputChart', () => {
const organization = OrganizationFixture();

let eventsStatsMock!: jest.Mock;

jest.mocked(useReleaseStats).mockReturnValue({
isLoading: false,
isPending: false,
isError: false,
error: null,
releases: [],
});

beforeEach(() => {
eventsStatsMock = MockApiClient.addMockResponse({
url: `/organizations/${organization.slug}/events-stats/`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ import {render, screen, waitForElementToBeRemoved} from 'sentry-test/reactTestin
import {useLocation} from 'sentry/utils/useLocation';
import usePageFilters from 'sentry/utils/usePageFilters';
import useProjects from 'sentry/utils/useProjects';
import {useReleaseStats} from 'sentry/utils/useReleaseStats';
import PageWithProviders from 'sentry/views/insights/queues/views/destinationSummaryPage';

jest.mock('sentry/utils/useLocation');
jest.mock('sentry/utils/usePageFilters');
jest.mock('sentry/utils/useProjects');
jest.mock('sentry/utils/useReleaseStats');

describe('destinationSummaryPage', () => {
const organization = OrganizationFixture({
Expand Down Expand Up @@ -54,6 +56,14 @@ describe('destinationSummaryPage', () => {
initiallyLoaded: false,
});

jest.mocked(useReleaseStats).mockReturnValue({
isLoading: false,
isPending: false,
isError: false,
error: null,
releases: [],
});

let eventsMock: jest.Mock;
let eventsStatsMock: jest.Mock;

Expand Down
Loading

0 comments on commit 9218558

Please sign in to comment.