Skip to content

perf(logs): Fix memoization on log row content #110310

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

Merged
k-fish merged 1 commit into from
Mar 10, 2026
Merged

perf(logs): Fix memoization on log row content #110310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion static/app/views/explore/logs/tables/logsInfiniteTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,13 +246,15 @@ export function LogsInfiniteTable({
[expandedLogRowsHeights, data]
);

const searchString = search.formatString();
const highlightTerms = useMemo(() => {
const terms = getLogBodySearchTerms(search);
if (localOnlyItemFilters?.filterText) {
terms.push(localOnlyItemFilters.filterText);
}
return terms;
}, [search, localOnlyItemFilters?.filterText]);
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Member Author

@k-fish k-fish Mar 10, 2026

Choose a reason for hiding this comment

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

Don't feel like boxing here, it's just more hooks to hide the fake deps.

}, [searchString, localOnlyItemFilters?.filterText]);

const windowVirtualizer = useWindowVirtualizer({
count: data?.length ?? 0,
Expand Down
4 changes: 3 additions & 1 deletion static/app/views/explore/logs/useLogsQuery.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,7 @@ export function useInfiniteLogsQuery({
return filteredData;
}, [data, virtualStreamedTimestamp]);

const pageCount = data?.pages?.length;
const _meta = useMemo<EventsMetaType>(() => {
return (
data?.pages.reduce(
Expand All @@ -655,7 +656,8 @@ export function useInfiniteLogsQuery({
{fields: {}, units: {}}
) ?? {fields: {}, units: {}}
);
}, [data]);
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Member Author

@k-fish k-fish Mar 10, 2026

Choose a reason for hiding this comment

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

Page count should be fine since new pages only are added upon a request firing, and are reset when changing the query.

}, [pageCount]);

const _fetchPreviousPage = useCallback(() => {
if (autoRefresh || hasPreviousPage) {
Comment on lines 656 to 663
Copy link
Contributor

@sentry sentry bot Mar 10, 2026

Choose a reason for hiding this comment

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

Bug: The useMemo hook for _meta has an incomplete dependency array (pageCount), causing it to serve stale metadata when data.pages content changes without altering the page count.
Severity: HIGH

Suggested Fix

The dependency array for the _meta useMemo hook should be corrected to include the data it actually depends on. Instead of [pageCount], it should be [data?.pages] to ensure the memo re-calculates whenever the page data changes.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: static/app/views/explore/logs/useLogsQuery.tsx#L656-L663

Potential issue: The `_meta` variable in `useLogsQuery.tsx` is calculated using a
`useMemo` hook that only has `pageCount` in its dependency array. However, the memo's
callback function accesses the full `data?.pages` object to aggregate metadata. This
creates a staleness bug. When existing page data is updated (e.g., during a refetch)
without changing the total number of pages, the memo will not re-execute. This results
in `_meta` holding stale values for field types and units, which can lead to incorrect
column alignment and data formatting in the logs table. The presence of an
`eslint-disable-next-line react-hooks/exhaustive-deps` comment indicates this was an
intentional but incorrect optimization.

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

Expand Down
Loading