Skip to content

Commit 3a412e4

Browse files
authored
fix(app): run the row side panel query once per opened row (#3568)
## Summary Opening a row in the side panel runs the same `SELECT *` row query twice. `DBRowSidePanel` shows "Loading..." until its `useRowData` lookup settles, then mounts the Overview or Column Values tab, which calls `useRowData` again with the same query key. The app's `QueryClient` keeps React Query's default `staleTime` of 0, so the tab's new observer refetches the row that has just loaded. Both lookups share one cache entry, so the second request is a refetch of a stale query, not two different keys. - `useRowData` gives its bounded and fallback lookups a `staleTime`: `Infinity` when the row was found, `0` when it was not. The tab reads the row from the cache, so opening a row runs one `SELECT *`. Switching between Overview and Column Values does not run it again either. - A different row, source or query settings (for example the "Show materialized and alias columns" option from #3231) is a different query key, so it still runs. A row that was not found stays stale, so the next lookup tries again: the row may not have been ingested yet. `useMergeTreeSettings` keeps failures stale the same way. - The same lookups back the expanded row in the search table and the span details in the trace waterfall. Those mount their tab together with the lookup, so React Query already shared one request there. With this change, switching their tabs reuses the row too. Things to be aware of: - A found row now stays fresh while it is cached. Closing the panel and opening the same row again within 5 minutes (React Query's default `gcTime`) shows it from the cache with no request. On `main`, the cached row shows at once and is fetched again in the background. So a row that a ClickHouse `UPDATE` changes, or that `DELETE` or a TTL removes, within those 5 minutes shows its cached values until the cache entry expires. - When the time-bounded lookup of a "View Trace" frame misses and the unbounded fallback finds the row, the tab still runs the bounded lookup once more, because its empty result is stale: 3 requests instead of 4 on `main` (from reading the code; no test covers this path). I kept one rule for both lookups instead of treating a bounded miss as fresh. Happy to change that. - I fixed this in `useRowData` instead of passing the row from `DBRowSidePanel` into the tabs, so the other callers above get it too and the tab components keep their props. Happy to switch if you prefer that. ### Tests New `packages/app/src/components/__tests__/DBRowDataPanel.rowQuery.test.tsx` uses the real `useRowData` and `useQueriedChartConfig`, a `QueryClient` with default options (as in `pages/_app.tsx`) and a mocked ClickHouse client that counts `queryChartConfig` calls. A small panel mirrors `DBRowSidePanel`: it shows "Loading..." until its own lookup settles, then mounts the real `RowDataPanel`. - The panel and its tab run the lookup once, without and with a `dateRange`, and the tab renders the row. - Closing the panel and opening the same row again reuses the found row. - Another row and another source each run the lookup again, once. - A row that was not found is looked up again when the tab mounts. With `DBRowDataPanel.tsx` from `main`, 5 of the 6 tests fail: the panel and its tab run the lookup twice (`Expected number of calls: 1`, `Received number of calls: 2`), the reopened row is fetched again, and the row and source tests see two requests per row. The not-found test passes on both. Removing the `staleTime` from the bounded lookup fails only the bounded test, and using `Infinity` for misses fails only the not-found test. The existing `DBRowDataPanel.test.ts` passes, and so do the test files that render the row panels without mocking `DBRowDataPanel` (`ContextSidePanel`, `DirectTraceSidePanel`, `ErrorsTab`, `DBSearchPage.directTrace`). ESLint (no warnings), Prettier, `tsc --noEmit` for `packages/app`, `knip` and `yarn ratchet` pass. In the browser, with `yarn app:dev:local` connected to the demo server, I counted the POST requests whose SQL starts with `SELECT *,` (the row lookup) in headless Chromium: | | `main` | This branch | | :-- | :-- | :-- | | Open a log row (10 rows) | 2 per row | 1 per row | | Then Overview → Column Values → Overview (2 rows) | 2 more per row | 0 | play.hyperdx.io shows the same 2 requests per row (3 rows). The UI does not change. I did not test trace rows, "View Trace" or session replay in the browser. The unit tests cover both the bounded and the unbounded lookup. ### How to reproduce **Starting point:** play.hyperdx.io, or `yarn app:dev:local` connected to the demo server (onboarding → "Connect to Demo Server"). **On `main`:** 1. Open Search on the Logs source. 2. Open DevTools → Network and filter by Fetch/XHR. 3. Click a log row to open the side panel. 4. Two POST requests to ClickHouse have the same payload, starting with `SELECT *,Timestamp AS "__hdx_timestamp"`, about 300–700 ms apart. 5. Click "Column Values", then "Overview". Each click sends that request again. **On this branch:** 1. Do the same steps. Step 4 shows one request for the row, and step 5 sends none. 2. Click another row. It sends one request for that row. ### How to test on Vercel preview **Preview routes:** /search **Steps:** 1. Click "Connect to Demo Server" on the onboarding modal. 2. Open /search on the Logs source, and open DevTools → Network. 3. Click a log row in the results table to open the side panel. 4. Verify that exactly one POST request with a payload that starts with `SELECT *,Timestamp AS "__hdx_timestamp"` is sent for that row. 5. Click "Column Values", then "Overview", and verify that no new request with that payload is sent. 6. Click another row and verify that it sends one such request. ### References - Closes #1400 - Related: #3231
1 parent a044642 commit 3a412e4

3 files changed

Lines changed: 189 additions & 0 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
'@hyperdx/app': patch
3+
---
4+
5+
fix: run the row details query once when opening a row
6+
7+
The side panel ran the same `SELECT *` row query twice: once for the panel, and
8+
again when its Overview or Column Values tab mounted. A row that was found is
9+
now reused from the cache, so the tab and later tab switches do not query it
10+
again. A different row or source still runs the query, and a row that was not
11+
found is looked up again.

‎packages/app/src/components/DBRowDataPanel.tsx‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
TSource,
1010
} from '@hyperdx/common-utils/dist/types';
1111
import { Box } from '@mantine/core';
12+
import type { Query } from '@tanstack/react-query';
1213

1314
import { useQueriedChartConfig } from '@/hooks/useChartConfig';
1415
import {
@@ -23,6 +24,7 @@ import {
2324
getDurationMsExpression,
2425
getEventBody,
2526
} from '@/source';
27+
import type { ChartQueryResult } from '@/types';
2628
import { getSelectExpressionsForHighlightedAttributes } from '@/utils/highlightedAttributes';
2729
import { getTimestampValueSelects } from '@/utils/rowTimestamps';
2830

@@ -48,6 +50,14 @@ export enum ROW_DATA_ALIASES {
4850
EVENT_NAME = '__hdx_event_name',
4951
}
5052

53+
// The side panel mounts its tabs once its own lookup settles, and each tab
54+
// looks the same row up again. Those lookups reuse a found row instead of
55+
// running `SELECT *` again. A miss stays stale so the next lookup retries it:
56+
// the row may not have been ingested yet.
57+
function rowLookupStaleTime(query: Query<ChartQueryResult>) {
58+
return query.state.data?.data.length ? Infinity : 0;
59+
}
60+
5161
export function useRowData({
5262
source,
5363
rowId,
@@ -289,6 +299,7 @@ export function useRowData({
289299
queryKey: [...baseQueryKey, dateRange],
290300
enabled: rowId != null && hasWindow && !isEventNameColumnPending,
291301
additionalQuerySettings,
302+
staleTime: rowLookupStaleTime,
292303
},
293304
);
294305

@@ -310,6 +321,7 @@ export function useRowData({
310321
queryKey: [...baseQueryKey, undefined],
311322
enabled: rowId != null && isFallbackActive && !isEventNameColumnPending,
312323
additionalQuerySettings,
324+
staleTime: rowLookupStaleTime,
313325
});
314326

315327
const queryResult = isFallbackActive ? fallbackResult : boundedResult;
Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
import type { ResponseJSON } from '@hyperdx/common-utils/dist/clickhouse';
2+
import { SourceKind, TLogSource } from '@hyperdx/common-utils/dist/types';
3+
import { MantineProvider } from '@mantine/core';
4+
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
5+
import { render, screen, waitFor } from '@testing-library/react';
6+
7+
import { RowDataPanel, useRowData } from '@/components/DBRowDataPanel';
8+
9+
const mockQueryChartConfig = jest.fn();
10+
jest.mock('@/clickhouse', () => ({
11+
useClickhouseClient: () => ({ queryChartConfig: mockQueryChartConfig }),
12+
}));
13+
jest.mock('@/hooks/useMetadata', () => ({
14+
useColumns: () => ({ data: undefined, isLoading: false }),
15+
useMetadataWithSettings: () => ({}),
16+
}));
17+
jest.mock('@/hooks/useMVOptimizationExplanation', () => ({
18+
useMVOptimizationExplanation: () => ({ data: undefined, isLoading: false }),
19+
}));
20+
jest.mock('@/source', () => ({
21+
...jest.requireActual('@/source'),
22+
useSource: () => ({ data: undefined, isLoading: false }),
23+
}));
24+
// Records the row the tab renders.
25+
const mockJsonViewerData = jest.fn();
26+
jest.mock('@/components/DBRowJsonViewer', () => ({
27+
DBRowJsonViewer: ({ data }: { data: unknown }) => {
28+
mockJsonViewerData(data);
29+
return null;
30+
},
31+
}));
32+
33+
const source: TLogSource = {
34+
id: 'source-id',
35+
kind: SourceKind.Log,
36+
name: 'logs',
37+
connection: 'conn-id',
38+
from: { databaseName: 'default', tableName: 'logs' },
39+
timestampValueExpression: 'Timestamp',
40+
defaultTableSelectExpression: 'Timestamp, Body',
41+
bodyExpression: 'Body',
42+
};
43+
44+
const DATE_RANGE: [Date, Date] = [
45+
new Date('2024-01-01T00:00:00Z'),
46+
new Date('2024-01-01T02:00:00Z'),
47+
];
48+
49+
const found: ResponseJSON<Record<string, unknown>> = {
50+
data: [{ Body: 'hello' }],
51+
meta: [{ name: 'Body', type: 'String' }],
52+
rows: 1,
53+
};
54+
55+
type PanelProps = {
56+
source: TLogSource;
57+
rowId: string;
58+
dateRange?: [Date, Date];
59+
};
60+
61+
// Mirrors DBRowSidePanel: the panel shows "Loading..." until its own lookup of
62+
// the row settles, then mounts the tab, which looks up the same row again.
63+
function SidePanel(props: PanelProps) {
64+
const { isLoading } = useRowData(props);
65+
if (isLoading) {
66+
return <div>Loading...</div>;
67+
}
68+
return <RowDataPanel {...props} data-testid="tab" />;
69+
}
70+
71+
describe('row lookup', () => {
72+
// The app's QueryClient uses React Query's defaults (pages/_app.tsx).
73+
let queryClient: QueryClient;
74+
75+
function renderPanel(props: PanelProps) {
76+
const tree = (panelProps: PanelProps) => (
77+
<MantineProvider>
78+
<QueryClientProvider client={queryClient}>
79+
<SidePanel {...panelProps} />
80+
</QueryClientProvider>
81+
</MantineProvider>
82+
);
83+
const result = render(tree(props));
84+
return {
85+
rerender: (next: PanelProps) => result.rerender(tree(next)),
86+
unmount: result.unmount,
87+
};
88+
}
89+
90+
async function settled() {
91+
await screen.findByTestId('tab');
92+
await waitFor(() => expect(queryClient.isFetching()).toBe(0));
93+
}
94+
95+
beforeEach(() => {
96+
queryClient = new QueryClient();
97+
mockQueryChartConfig.mockReset().mockResolvedValue(found);
98+
mockJsonViewerData.mockReset();
99+
});
100+
101+
afterEach(() => {
102+
queryClient.clear();
103+
});
104+
105+
it.each<{ lookup: string; dateRange?: [Date, Date] }>([
106+
{ lookup: 'unbounded' },
107+
{ lookup: 'bounded', dateRange: DATE_RANGE },
108+
])(
109+
'runs the $lookup lookup once for the panel and its tab',
110+
async ({ dateRange }) => {
111+
renderPanel({ source, rowId: "id='a'", dateRange });
112+
await settled();
113+
114+
expect(mockQueryChartConfig).toHaveBeenCalledTimes(1);
115+
expect(mockJsonViewerData).toHaveBeenLastCalledWith({ Body: 'hello' });
116+
},
117+
);
118+
119+
it('reuses a found row when the panel opens it again', async () => {
120+
const first = renderPanel({ source, rowId: "id='a'" });
121+
await settled();
122+
first.unmount();
123+
124+
renderPanel({ source, rowId: "id='a'" });
125+
await settled();
126+
127+
expect(mockQueryChartConfig).toHaveBeenCalledTimes(1);
128+
expect(mockJsonViewerData).toHaveBeenLastCalledWith({ Body: 'hello' });
129+
});
130+
131+
it('runs again for another row', async () => {
132+
const { rerender } = renderPanel({ source, rowId: "id='a'" });
133+
await settled();
134+
rerender({ source, rowId: "id='b'" });
135+
await settled();
136+
137+
expect(
138+
mockQueryChartConfig.mock.calls.map(([{ config }]) => config.where),
139+
).toEqual(["id='a'", "id='b'"]);
140+
});
141+
142+
it('runs again for another source', async () => {
143+
const { rerender } = renderPanel({ source, rowId: "id='a'" });
144+
await settled();
145+
rerender({
146+
source: { ...source, from: { ...source.from, tableName: 'logs_v2' } },
147+
rowId: "id='a'",
148+
});
149+
await settled();
150+
151+
expect(
152+
mockQueryChartConfig.mock.calls.map(
153+
([{ config }]) => config.from.tableName,
154+
),
155+
).toEqual(['logs', 'logs_v2']);
156+
});
157+
158+
it('runs again for a row it did not find', async () => {
159+
mockQueryChartConfig.mockResolvedValue({ data: [], meta: [], rows: 0 });
160+
161+
renderPanel({ source, rowId: "id='a'" });
162+
await settled();
163+
164+
expect(mockQueryChartConfig).toHaveBeenCalledTimes(2);
165+
});
166+
});

0 commit comments

Comments
 (0)