Skip to content

Commit 442d6df

Browse files
authored
fix(common-utils): expire cached table columns after 5 minutes (#3481)
## Summary `getMetadata()` shares one module-level `MetadataCache`, and `getOrFetch` kept every entry for the life of the process. After an `ALTER TABLE` adds a column, the API keeps rendering queries with the column list from before the change until it restarts. As #3475 shows, a new MATERIALIZED column is not used for a filter on the Map key it covers, and grouping by it fails with `Unknown expression identifier`. - `MetadataCache.getOrFetch` and `set` take an optional `ttlMs`. `get` drops an entry whose TTL has passed, so the next `getOrFetch` runs the query again. Entries stored without a TTL behave as before. - `getColumns` stores its `DESCRIBE` result with a 5 minute TTL (`COLUMNS_CACHE_TTL_MS`). `getColumn`, `getMaterializedColumnsLookupTable` and the computed metric column lookup from #3191 all read `getColumns`, so they see a new column within 5 minutes. The cost is one `DESCRIBE` per table every 5 minutes per process. Concurrent callers still share one pending query. The browser uses the same cache, so a page reload is no longer needed there either. Things to be aware of: - If the refresh fails (ClickHouse is down), the caller now gets the error instead of the old column list. The query that needs the columns would fail in that case anyway. - Other cached schema data still has no TTL: skip indices, the text index lookups, `system.tables` metadata and TimeSeries table columns. I kept this PR to the column list from the issue. I can extend the TTL to those in a follow-up if you want. - 5 minutes is a guess at "a few minutes" from the issue. Happy to change it. ### Tests In `packages/common-utils/src/__tests__/metadata.test.ts`, with `Date.now` mocked: - `MetadataCache`: a value stored with a TTL is returned from the cache until the TTL passes and fetched again after it. A value stored without a TTL is still cached a year later. - `getMaterializedColumnsLookupTable`: the first `DESCRIBE` has no MATERIALIZED column, and a second one adds `__otel_materialized_k8s.namespace.name`. The lookup stays empty just before the TTL, and has `ResourceAttributes['k8s.namespace.name']` → `__otel_materialized_k8s.namespace.name` once the TTL has passed. `DESCRIBE` runs twice. With this change all 156 tests in the file pass. With `metadata.ts` from `main`, the two TTL tests fail (the cache returns `first` instead of `second`, and the lookup stays empty). With the cache TTL but without the TTL on `getColumns`, the lookup test fails. ESLint, Prettier, `tsc` on the two changed files and `knip` for `common-utils` pass. I did not run the issue's steps against a full `yarn dev` stack. ### How to reproduce **Starting point:** `yarn dev`, with the steps from #3475 (the MCP `clickstack_timeseries` tool on the metrics source). **On `main`:** 1. Call `clickstack_timeseries` on a gauge metric with `where: ResourceAttributes.k8s.namespace.name:foo`. 2. Add the `__otel_materialized_k8s.namespace.name` and `__otel_materialized_k8s.cluster.name` MATERIALIZED columns to `otel_metrics_gauge` (SQL in the issue). 3. Repeat step 1. `system.query_log` shows the filter still reads `ResourceAttributes['k8s.namespace.name']`, and grouping by `` `__otel_materialized_k8s.cluster.name` `` fails with `Unknown expression identifier`. This keeps happening until the API restarts. **On this branch:** 1. Do the same steps, then wait 5 minutes after step 2. 2. Repeat step 1. The filter uses `__otel_materialized_k8s.namespace.name`, and the group-by works without a restart. ### How to test on Vercel preview N/A — non-UI change. ### References - Closes #3475 - Related: #3191 (computed metric columns), #3231
1 parent 821c2f9 commit 442d6df

3 files changed

Lines changed: 138 additions & 7 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@hyperdx/common-utils': patch
3+
'@hyperdx/app': patch
4+
'@hyperdx/api': patch
5+
---
6+
7+
fix: refresh cached table columns after 5 minutes
8+
9+
The shared metadata cache kept each table's `DESCRIBE` result for the life of
10+
the process, so the API did not see columns added with `ALTER TABLE`, such as
11+
new MATERIALIZED columns, until it restarted. Cached columns now expire after 5
12+
minutes. Other cached metadata is unchanged.

‎packages/common-utils/src/__tests__/metadata.test.ts‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { ColumnMeta } from '@/clickhouse';
22
import { ClickhouseClient } from '@/clickhouse/node';
33
import {
4+
COLUMNS_CACHE_TTL_MS,
45
GET_ALL_KEY_VALUES_CHUNK_SIZE,
56
Metadata,
67
MetadataCache,
@@ -157,6 +158,48 @@ describe('MetadataCache', () => {
157158
// And no value should be stored in the cache
158159
expect(metadataCache.get(key)).toBeUndefined();
159160
});
161+
162+
it('refetches a value stored with a TTL once the TTL has passed', async () => {
163+
const now = jest.spyOn(Date, 'now').mockReturnValue(1_000);
164+
const queryFn = jest
165+
.fn()
166+
.mockResolvedValueOnce('first')
167+
.mockResolvedValueOnce('second');
168+
169+
try {
170+
await expect(
171+
metadataCache.getOrFetch('key', queryFn, { ttlMs: 100 }),
172+
).resolves.toBe('first');
173+
174+
now.mockReturnValue(1_099);
175+
await expect(
176+
metadataCache.getOrFetch('key', queryFn, { ttlMs: 100 }),
177+
).resolves.toBe('first');
178+
179+
now.mockReturnValue(1_100);
180+
await expect(
181+
metadataCache.getOrFetch('key', queryFn, { ttlMs: 100 }),
182+
).resolves.toBe('second');
183+
expect(queryFn).toHaveBeenCalledTimes(2);
184+
} finally {
185+
now.mockRestore();
186+
}
187+
});
188+
189+
it('keeps a value stored without a TTL', async () => {
190+
const now = jest.spyOn(Date, 'now').mockReturnValue(1_000);
191+
const queryFn = jest.fn().mockResolvedValue('value');
192+
193+
try {
194+
await metadataCache.getOrFetch('key', queryFn);
195+
now.mockReturnValue(1_000 + 365 * 24 * 60 * 60 * 1000);
196+
await metadataCache.getOrFetch('key', queryFn);
197+
198+
expect(queryFn).toHaveBeenCalledTimes(1);
199+
} finally {
200+
now.mockRestore();
201+
}
202+
});
160203
});
161204
});
162205

@@ -168,6 +211,63 @@ describe('Metadata', () => {
168211
jest.clearAllMocks();
169212
});
170213

214+
describe('getMaterializedColumnsLookupTable', () => {
215+
it('sees a MATERIALIZED column added after the first read once the cached columns expire', async () => {
216+
const now = jest.spyOn(Date, 'now').mockReturnValue(0);
217+
const client = { query: jest.fn() } as unknown as ClickhouseClient;
218+
const md = new Metadata(client, new MetadataCache());
219+
const describeResult = (columns: Partial<ColumnMeta>[]) => ({
220+
json: jest.fn().mockResolvedValue({ data: columns }),
221+
});
222+
const resourceAttributes = {
223+
name: 'ResourceAttributes',
224+
type: 'Map(LowCardinality(String), String)',
225+
default_type: '',
226+
default_expression: '',
227+
};
228+
const materialized = {
229+
name: '__otel_materialized_k8s.namespace.name',
230+
type: 'LowCardinality(String)',
231+
default_type: 'MATERIALIZED',
232+
default_expression: "ResourceAttributes['k8s.namespace.name']",
233+
};
234+
(client.query as jest.Mock)
235+
.mockResolvedValueOnce(describeResult([resourceAttributes]))
236+
.mockResolvedValueOnce(
237+
describeResult([resourceAttributes, materialized]),
238+
);
239+
const table = {
240+
databaseName: 'otel',
241+
tableName: 'otel_metrics_gauge',
242+
connectionId: 'conn-1',
243+
};
244+
245+
try {
246+
expect(await md.getMaterializedColumnsLookupTable(table)).toEqual(
247+
new Map(),
248+
);
249+
250+
now.mockReturnValue(COLUMNS_CACHE_TTL_MS - 1);
251+
expect(await md.getMaterializedColumnsLookupTable(table)).toEqual(
252+
new Map(),
253+
);
254+
255+
now.mockReturnValue(COLUMNS_CACHE_TTL_MS);
256+
expect(await md.getMaterializedColumnsLookupTable(table)).toEqual(
257+
new Map([
258+
[
259+
"ResourceAttributes['k8s.namespace.name']",
260+
'__otel_materialized_k8s.namespace.name',
261+
],
262+
]),
263+
);
264+
expect(client.query).toHaveBeenCalledTimes(2);
265+
} finally {
266+
now.mockRestore();
267+
}
268+
});
269+
});
270+
171271
describe('getTableMetadata', () => {
172272
beforeEach(() => {
173273
mockCache.getOrFetch.mockImplementation((key, queryFn) => queryFn());

‎packages/common-utils/src/core/metadata.ts‎

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -251,17 +251,27 @@ const renderJsonStringSubcolumn = (
251251
};
252252

253253
export class MetadataCache {
254-
private cache = new Map<string, any>();
254+
private cache = new Map<string, { value: any; expiresAt?: number }>();
255255
private pendingQueries = new Map<string, Promise<any>>();
256256

257257
// this should be getOrUpdate... or just query to follow react query
258258
get<T>(key: string): T | undefined {
259-
return this.cache.get(key);
259+
const entry = this.cache.get(key);
260+
if (entry?.expiresAt != null && Date.now() >= entry.expiresAt) {
261+
this.cache.delete(key);
262+
return undefined;
263+
}
264+
return entry?.value;
260265
}
261266

262-
async getOrFetch<T>(key: string, query: () => Promise<T>): Promise<T> {
267+
/** Entries stored without `ttlMs` are kept for the life of the cache. */
268+
async getOrFetch<T>(
269+
key: string,
270+
query: () => Promise<T>,
271+
{ ttlMs }: { ttlMs?: number } = {},
272+
): Promise<T> {
263273
// Check if value exists in cache
264-
const cachedValue: T | undefined = this.cache.get(key);
274+
const cachedValue: T | undefined = this.get(key);
265275
if (cachedValue != null) {
266276
return cachedValue;
267277
}
@@ -279,16 +289,19 @@ export class MetadataCache {
279289

280290
try {
281291
const result = await queryPromise;
282-
this.cache.set(key, result);
292+
this.set(key, result, ttlMs);
283293
return result;
284294
} finally {
285295
// Clean up the pending query map
286296
this.pendingQueries.delete(key);
287297
}
288298
}
289299

290-
set<T>(key: string, value: T) {
291-
return this.cache.set(key, value);
300+
set<T>(key: string, value: T, ttlMs?: number) {
301+
this.cache.set(key, {
302+
value,
303+
expiresAt: ttlMs == null ? undefined : Date.now() + ttlMs,
304+
});
292305
}
293306

294307
// TODO: This needs to be async, and use tanstack query on frontend for cache
@@ -353,6 +366,11 @@ export type SkipIndexMetadata = {
353366

354367
const FIELD_METADATA_LOOKBACK_MS = 24 * 60 * 60 * 1000;
355368

369+
// The API keeps one cache for the life of the process, so without a TTL a
370+
// column added by ALTER TABLE (such as a MATERIALIZED one) is only seen after a
371+
// restart. #3475
372+
export const COLUMNS_CACHE_TTL_MS = 5 * 60 * 1000;
373+
356374
// Hour-aligned so a range-keyed cache rotates hourly instead of every call.
357375
export function defaultFieldMetadataDateRange(): [Date, Date] {
358376
const now = new Date();
@@ -619,6 +637,7 @@ export class Metadata {
619637
.then(d => d.data);
620638
return columns;
621639
},
640+
{ ttlMs: COLUMNS_CACHE_TTL_MS },
622641
);
623642
}
624643

0 commit comments

Comments
 (0)