Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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/discover/eventView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ export type MetaType = Record<string, any> & {
export type EventsMetaType = {fields: Record<string, ColumnType>} & {
units: Record<string, string>;
} & {
bytesScanned?: number | null;
dataScanned?: 'full' | 'partial';
discoverSplitDecision?: WidgetType;
isMetricsData?: boolean;
Expand Down
38 changes: 34 additions & 4 deletions static/app/views/explore/logs/tables/logsInfiniteTable.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type {CSSProperties} from 'react';
import {Fragment, useCallback, useEffect, useMemo, useRef, useState} from 'react';
import {useTheme} from '@emotion/react';
import styled from '@emotion/styled';
Expand All @@ -8,6 +9,7 @@ import {Button} from 'sentry/components/core/button';
import {ExternalLink} from 'sentry/components/core/link';
import {Tooltip} from 'sentry/components/core/tooltip';
import EmptyStateWarning from 'sentry/components/emptyStateWarning';
import FileSize from 'sentry/components/fileSize';
import LoadingIndicator from 'sentry/components/loadingIndicator';
import JumpButtons from 'sentry/components/replays/jumpButtons';
import useJumpButtons from 'sentry/components/replays/useJumpButtons';
Expand Down Expand Up @@ -105,6 +107,7 @@ export function LogsInfiniteTable({
const {infiniteLogsQueryResult} = useLogsPageData();
const lastFetchTime = useRef<number | null>(null);
const {
bytesScanned,
isPending,
isEmpty,
meta,
Expand Down Expand Up @@ -383,7 +386,7 @@ export function LogsInfiniteTable({
</TableRow>
)}
{/* Only render these in table for non-replay contexts */}
{!hasReplay && isPending && <LoadingRenderer />}
{!hasReplay && isPending && <LoadingRenderer bytesScanned={bytesScanned} />}
{!hasReplay && isError && <ErrorRenderer />}
{!hasReplay && isEmpty && (emptyRenderer ? emptyRenderer() : <EmptyRenderer />)}
{!autoRefresh && !isPending && isFetchingPreviousPage && (
Expand Down Expand Up @@ -571,14 +574,41 @@ function ErrorRenderer() {
);
}

export function LoadingRenderer({size}: {size?: number}) {
export function LoadingRenderer({bytesScanned}: {bytesScanned?: number | null}) {
return (
<TableStatus size={size}>
<LoadingIndicator size={size} />
<TableStatus>
<LoadingStateContainer>
<EmptyStateText size="md" textAlign="center">
<StyledLoadingIndicator margin="1em auto" />
{defined(bytesScanned) && bytesScanned > 0 && (
<Fragment>
{t('Searching for a needle in a haystack. This could take a while.')}
<br />
<span>
{tct('[bytesScanned] scanned', {
bytesScanned: <FileSize bytes={bytesScanned} base={2} />,
})}
</span>
</Fragment>
)}
</EmptyStateText>
</LoadingStateContainer>
</TableStatus>
);
}

const LoadingStateContainer = styled('div')`
display: flex;
flex-direction: column;
align-items: center;
`;

const StyledLoadingIndicator = styled(LoadingIndicator)<{
margin: CSSProperties['margin'];
}>`
${p => p.margin && `margin: ${p.margin}`};
`;

function HoveringRowLoadingRenderer({
position,
isEmbedded,
Expand Down
22 changes: 22 additions & 0 deletions static/app/views/explore/logs/useLogsQuery.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,27 @@ export function useInfiniteLogsQuery({

const {virtualStreamedTimestamp} = useVirtualStreaming({data, highFidelity});

// Due to the way we prune empty pages, we cannot simply compute the sum of bytes scanned
// for all pages as most empty pages would have been evicted already.
//
// Instead, we watch the last page loaded, and keep a running sum that is reset when
// the last page is falsey which corresponds to a query change.
const [totalBytesScanned, setTotalBytesScanned] = useState(0);
const lastPage = data?.pages?.[data?.pages?.length - 1];
useEffect(() => {
if (!lastPage) {
setTotalBytesScanned(0);
return;
}

const bytesScanned = lastPage[0].meta?.bytesScanned;
if (!defined(bytesScanned)) {
return;
}

setTotalBytesScanned(previousBytesScanned => previousBytesScanned + bytesScanned);
}, [lastPage]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Bug: Stale Dependency Causes Duplicate Scans

The useEffect for totalBytesScanned uses lastPage as a dependency. Its reference changes on re-renders even when the underlying page data is the same, causing the effect to re-run. This adds the same page's bytesScanned multiple times, leading to an inflated and incorrect total displayed to users.

Fix in Cursor Fix in Web

Comment on lines +596 to +606
Copy link

Choose a reason for hiding this comment

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

Bug: The useEffect accumulates bytesScanned multiple times for the same page due to lastPage reference changes on React Query state updates.
Severity: CRITICAL | Confidence: 1.00

🔍 Detailed Analysis

The useEffect hook responsible for accumulating bytesScanned re-runs unnecessarily because its dependency, lastPage, receives a new object reference on every React Query state update, even if the underlying page data has not changed. This causes the bytesScanned value from the same page to be added multiple times to totalBytesScanned, leading to an inflated and incorrect total displayed to the user. This occurs whenever React Query updates its internal state, such as when isPending changes, triggering the effect with a new lastPage reference that points to the same data.

💡 Suggested Fix

Modify the useEffect to ensure bytesScanned is added only once per unique lastPage data object. This can be achieved by tracking a stable identifier from lastPage (e.g., lastPage.id or a unique hash of its content) and only performing the accumulation if this identifier has not been processed before.

🤖 Prompt for AI Agent
Fix this bug. In static/app/views/explore/logs/useLogsQuery.tsx at lines 587-606: The
`useEffect` hook responsible for accumulating `bytesScanned` re-runs unnecessarily
because its dependency, `lastPage`, receives a new object reference on every React Query
state update, even if the underlying page data has not changed. This causes the
`bytesScanned` value from the same page to be added multiple times to
`totalBytesScanned`, leading to an inflated and incorrect total displayed to the user.
This occurs whenever React Query updates its internal state, such as when `isPending`
changes, triggering the effect with a new `lastPage` reference that points to the same
data.

Did we get this right? 👍 / 👎 to inform future reviews.


const _data = useMemo(() => {
const usedRowIds = new Set();
const filteredData =
Expand Down Expand Up @@ -694,6 +715,7 @@ export function useInfiniteLogsQuery({
isFetchingNextPage: _data.length > 0 && (waitingToAutoFetch || isFetchingNextPage),
isFetchingPreviousPage,
lastPageLength,
bytesScanned: totalBytesScanned,
};
}

Expand Down
3 changes: 3 additions & 0 deletions static/app/views/explore/tables/tracesTable/styles.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type {CSSProperties} from 'react';
import {css} from '@emotion/react';
import styled from '@emotion/styled';

Expand Down Expand Up @@ -89,10 +90,12 @@ export const BreakdownPanelItem = styled(StyledPanelItem)<{highlightedSliceName:

export const EmptyStateText = styled('div')<{
size: 'xl' | 'md';
textAlign?: CSSProperties['textAlign'];
}>`
color: ${p => p.theme.subText};
font-size: ${p => p.theme.fontSize[p.size]};
padding-bottom: ${space(1)};
${p => p.textAlign && `text-align: ${p.textAlign}`};
`;

export const EmptyValueContainer = styled('span')`
Expand Down
Loading