Skip to content

Commit

Permalink
fix(discover): Should reload when payload changes while loading (#28653)
Browse files Browse the repository at this point in the history
If the eventView changes while the request is in-flight/loading, we should
cancel the in-flight request and fetch again. This is problematic in cases where
the request takes a little longer to load and the user changes the query while
it is still in-flight. If a reload is not triggered at this time, the results
are actually incorrect since it's showing the results from the previous (stale)
in-flight request.
  • Loading branch information
Zylphrex authored and vuluongj20 committed Sep 30, 2021
1 parent 6309653 commit 0a244f8
Show file tree
Hide file tree
Showing 7 changed files with 5 additions and 25 deletions.
7 changes: 5 additions & 2 deletions static/app/utils/discover/genericDiscoverQuery.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,8 @@ class _GenericDiscoverQuery<T, P> extends React.Component<Props<T, P>, State<T>>
}

componentDidUpdate(prevProps: Props<T, P>) {
// Reload data if we aren't already loading,
const refetchCondition = !this.state.isLoading && this._shouldRefetchData(prevProps);
// Reload data if the payload changes
const refetchCondition = this._shouldRefetchData(prevProps);

// or if we've moved from an invalid view state to a valid one,
const eventViewValidation =
Expand Down Expand Up @@ -199,6 +199,9 @@ class _GenericDiscoverQuery<T, P> extends React.Component<Props<T, P>, State<T>>

beforeFetch?.(api);

// clear any inflight requests since they are now stale
api.clear();

try {
const [data, , resp] = await doDiscoverQuery<T>(api, url, apiPayload);
if (this.state.tableFetchID !== tableFetchID) {
Expand Down
6 changes: 0 additions & 6 deletions static/app/utils/performance/histogram/histogramQuery.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import * as React from 'react';
import omit from 'lodash/omit';

import {Client} from 'app/api';
import GenericDiscoverQuery, {
DiscoverQueryProps,
GenericChildrenProps,
Expand Down Expand Up @@ -51,10 +50,6 @@ function getHistogramRequestPayload(props: RequestProps) {
return apiPayload;
}

function beforeFetch(api: Client) {
api.clear();
}

function HistogramQuery(props: Props) {
const {children, fields, didReceiveMultiAxis} = props;

Expand Down Expand Up @@ -88,7 +83,6 @@ function HistogramQuery(props: Props) {
<GenericDiscoverQuery<Histograms, HistogramProps>
route="events-histogram"
getRequestPayload={getHistogramRequestPayload}
beforeFetch={beforeFetch}
didFetch={didFetch}
{...omit(props, 'children')}
>
Expand Down
2 changes: 0 additions & 2 deletions static/app/utils/performance/quickTrace/traceFullQuery.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
TraceRequestProps,
} from 'app/utils/performance/quickTrace/types';
import {
beforeFetch,
getTraceRequestPayload,
makeEventView,
} from 'app/utils/performance/quickTrace/utils';
Expand Down Expand Up @@ -80,7 +79,6 @@ function GenericTraceFullQuery<T>({
<GenericDiscoverQuery<T, AdditionalQueryProps>
route={`events-trace/${traceId}`}
getRequestPayload={getTraceFullRequestPayload}
beforeFetch={beforeFetch}
eventView={eventView}
{...props}
>
Expand Down
2 changes: 0 additions & 2 deletions static/app/utils/performance/quickTrace/traceLiteQuery.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
TraceRequestProps,
} from 'app/utils/performance/quickTrace/types';
import {
beforeFetch,
getTraceRequestPayload,
makeEventView,
} from 'app/utils/performance/quickTrace/utils';
Expand Down Expand Up @@ -69,7 +68,6 @@ function TraceLiteQuery({
<GenericDiscoverQuery<TraceLite, AdditionalQueryProps>
route={`events-trace-light/${traceId}`}
getRequestPayload={getTraceLiteRequestPayload}
beforeFetch={beforeFetch}
eventView={eventView}
{...props}
>
Expand Down
2 changes: 0 additions & 2 deletions static/app/utils/performance/quickTrace/traceMetaQuery.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import {
TraceRequestProps,
} from 'app/utils/performance/quickTrace/types';
import {
beforeFetch,
getTraceRequestPayload,
makeEventView,
} from 'app/utils/performance/quickTrace/utils';
Expand Down Expand Up @@ -46,7 +45,6 @@ function TraceMetaQuery({
return (
<GenericDiscoverQuery<TraceMeta, {}>
route={`events-trace-meta/${traceId}`}
beforeFetch={beforeFetch}
getRequestPayload={getTraceRequestPayload}
eventView={eventView}
{...props}
Expand Down
5 changes: 0 additions & 5 deletions static/app/utils/performance/quickTrace/utils.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import omit from 'lodash/omit';
import moment from 'moment-timezone';

import {Client} from 'app/api';
import {getTraceDateTimeRange} from 'app/components/events/interfaces/spans/utils';
import {ALL_ACCESS_PROJECTS} from 'app/constants/globalSelectionHeader';
import {OrganizationSummary} from 'app/types';
Expand Down Expand Up @@ -231,10 +230,6 @@ function sortTraceLite(trace: TraceLite): TraceLite {
return trace.sort((a, b) => b['transaction.duration'] - a['transaction.duration']);
}

export function beforeFetch(api: Client) {
api.clear();
}

export function getTraceRequestPayload({eventView, location}: DiscoverQueryProps) {
return omit(eventView.getEventsAPIPayload(location), ['field', 'sort', 'per_page']);
}
Expand Down
6 changes: 0 additions & 6 deletions static/app/utils/performance/vitals/hasMeasurementsQuery.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import omit from 'lodash/omit';
import pick from 'lodash/pick';

import {Client} from 'app/api';
import {escapeDoubleQuotes} from 'app/utils';
import GenericDiscoverQuery, {
DiscoverQueryProps,
Expand Down Expand Up @@ -41,16 +40,11 @@ function getHasMeasurementsRequestPayload(props: RequestProps) {
return Object.assign(baseApiPayload, additionalApiPayload);
}

function beforeFetch(api: Client) {
api.clear();
}

function HasMeasurementsQuery(props: Props) {
return (
<GenericDiscoverQuery<HasMeasurements, HasMeasurementsProps>
route="events-has-measurements"
getRequestPayload={getHasMeasurementsRequestPayload}
beforeFetch={beforeFetch}
{...omit(props, 'children')}
>
{({tableData, ...rest}) => {
Expand Down

0 comments on commit 0a244f8

Please sign in to comment.