fix(editor): tenant-scope the PromQL schema cache, and drop overtaken lookups
Both from external review, both reproduced before being believed.
A CROSS-TENANT CACHE. metricLabelCache was keyed on the metric name alone, and
organisations are switched IN PLACE in this SPA — no reload, so module state
survives. Metric names are not unique across organisations, and label names are
that tenant's schema. Open the same metric name in a second organisation and it
served the first one's labels, without ever asking the server. This is the same
class as the get_all_transform leak fixed earlier in this workstream, and the
field-value cache next to it was already keyed org|type|stream|field for exactly
this reason. Now keyed by organisation and metric.
STALE LOOKUPS OVERWRITING CURRENT ONES. Every completed lookup published its
result without asking whether it was still the current question. Values are a
network call with a ten-second ceiling, so `{service="` can answer after the user
has moved on to `{region="` — and the reproduction shows exactly that: region's
values on screen, replaced by service names when the abandoned request landed.
A generation counter now gates all three publish points, matching the requestSeq
guard the SQL catalog fetch already uses.
Verified by mutation, not by inspection: removing the guard from the value
branch turns the stale-lookup test red, and only that test.
Full suite: 40468 total, 40037 passed, 23 failed — 14 synthetics journey specs
and the 9 load-flaky CodeQueryEditor ones, all pre-existing. type-check, eslint
and prettier clean.
This commit is contained in:
parent
b67411343c
commit
008d259ddc
|
|
@ -43,12 +43,14 @@ vi.mock("@/composables/fieldValueStore", () => ({
|
|||
getFieldValuesForSuggestion: vi.fn().mockResolvedValue([]),
|
||||
requestFieldValues: vi.fn().mockResolvedValue([]),
|
||||
}));
|
||||
// A SINGLE mutable store, so a test can switch organisation the way the app
|
||||
// does — in place, without a page reload.
|
||||
const mockStore = vi.hoisted(() => ({
|
||||
state: { selectedOrganization: { identifier: "myorg" } },
|
||||
}));
|
||||
vi.mock("vuex", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("vuex")>();
|
||||
return {
|
||||
...actual,
|
||||
useStore: vi.fn(() => ({ state: { selectedOrganization: { identifier: "myorg" } } })),
|
||||
};
|
||||
return { ...actual, useStore: vi.fn(() => mockStore) };
|
||||
});
|
||||
|
||||
import searchService from "@/services/search";
|
||||
|
|
@ -100,6 +102,7 @@ const offered = (c: any) => c.autoCompletePromqlKeywords.value as any[];
|
|||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
mockStore.state.selectedOrganization.identifier = "myorg";
|
||||
vi.mocked(streamService.schema).mockResolvedValue(METRIC_SCHEMA as any);
|
||||
vi.mocked(getFieldValuesForSuggestion).mockResolvedValue([]);
|
||||
vi.mocked(requestFieldValues).mockResolvedValue([]);
|
||||
|
|
@ -349,6 +352,72 @@ describe("while a lookup is in flight", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("caches and races", () => {
|
||||
it("does not serve one organisation's labels to another", async () => {
|
||||
// The same class of bug as the cross-tenant transform leak fixed earlier in
|
||||
// this workstream: a cache keyed on the metric alone. Organisations are
|
||||
// switched IN PLACE in this SPA, metric names are not unique across them,
|
||||
// and label names are that tenant's schema.
|
||||
const c = await freshComposable();
|
||||
const query = "cpu_utilization_percent{";
|
||||
const ask = async () => {
|
||||
c.autoCompleteData.value.query = query;
|
||||
c.autoCompleteData.value.position.cursorIndex = query.length - 1;
|
||||
await c.getSuggestions();
|
||||
await flushPromises();
|
||||
return offered(c).map((k: any) => k.label);
|
||||
};
|
||||
|
||||
vi.mocked(streamService.schema).mockResolvedValue({
|
||||
data: { schema: [{ name: "tenant_a_only", type: "Utf8" }] },
|
||||
} as any);
|
||||
expect(await ask()).toContain("tenant_a_only");
|
||||
|
||||
mockStore.state.selectedOrganization.identifier = "otherorg";
|
||||
vi.mocked(streamService.schema).mockResolvedValue({
|
||||
data: { schema: [{ name: "tenant_b_only", type: "Utf8" }] },
|
||||
} as any);
|
||||
|
||||
const afterSwitch = await ask();
|
||||
expect(afterSwitch, "served the previous tenant's labels").not.toContain("tenant_a_only");
|
||||
expect(afterSwitch).toContain("tenant_b_only");
|
||||
expect(vi.mocked(streamService.schema).mock.calls.at(-1)?.[0]).toBe("otherorg");
|
||||
});
|
||||
|
||||
it("ignores a slow lookup that lands after a newer one", async () => {
|
||||
// Values are a network call with a ten-second ceiling. Type `{service="`,
|
||||
// change your mind to `{region="`, and the first answer can arrive last and
|
||||
// overwrite the second — offering service names for a region filter.
|
||||
const c = await freshComposable();
|
||||
let resolveSlow: (v: string[]) => void = () => {};
|
||||
vi.mocked(getFieldValuesForSuggestion)
|
||||
.mockImplementationOnce(() => new Promise((resolve) => (resolveSlow = resolve)) as any)
|
||||
.mockResolvedValue(["us-east-1"]);
|
||||
|
||||
const first = 'cpu_utilization_percent{service="';
|
||||
c.autoCompleteData.value.query = first;
|
||||
c.autoCompleteData.value.position.cursorIndex = first.length - 1;
|
||||
void c.getSuggestions();
|
||||
await flushPromises();
|
||||
|
||||
const second = 'cpu_utilization_percent{region="';
|
||||
c.autoCompleteData.value.query = second;
|
||||
c.autoCompleteData.value.position.cursorIndex = second.length - 1;
|
||||
await c.getSuggestions();
|
||||
await flushPromises();
|
||||
expect(offered(c).map((k: any) => k.label)).toEqual(["us-east-1"]);
|
||||
|
||||
// The abandoned request finally answers.
|
||||
resolveSlow(["api-gateway", "chat-service"]);
|
||||
await flushPromises();
|
||||
|
||||
expect(
|
||||
offered(c).map((k: any) => k.label),
|
||||
"a stale lookup overwrote the current one",
|
||||
).toEqual(["us-east-1"]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("what must not change", () => {
|
||||
it("still offers the catalog when the cursor is not in a label position", async () => {
|
||||
const c = await freshComposable();
|
||||
|
|
|
|||
|
|
@ -11,7 +11,12 @@ const NON_LABEL_COLUMNS = new Set(["value", "_timestamp", "__hash__", "__name__"
|
|||
|
||||
// One schema per metric per page, shared by every editor. A metrics stream is
|
||||
// named for its metric, so this is also the label list.
|
||||
//
|
||||
// Keyed by ORGANISATION and metric, because organisations are switched in place
|
||||
// in this SPA and metric names are not unique across them — the field-value
|
||||
// cache is scoped the same way, for the same reason.
|
||||
const metricLabelCache = new Map<string, string[]>();
|
||||
const metricLabelCacheKey = (org: string, metric: string) => `${org}|${metric}`;
|
||||
import { useStore } from "vuex";
|
||||
|
||||
const usePromqlSuggestions = () => {
|
||||
|
|
@ -177,7 +182,15 @@ const usePromqlSuggestions = () => {
|
|||
return labelMeta;
|
||||
}
|
||||
|
||||
// Bumped by every suggestion pass. A lookup that finishes after a newer one
|
||||
// started has been overtaken and must not publish: values are a network call
|
||||
// with a ten-second ceiling, so `{service="` can easily answer after the user
|
||||
// has moved on to `{region="`.
|
||||
let suggestionGeneration = 0;
|
||||
|
||||
const getSuggestions = async () => {
|
||||
const generation = ++suggestionGeneration;
|
||||
const isCurrent = () => generation === suggestionGeneration;
|
||||
try {
|
||||
const parsedQuery: any = parsePromQlQuery(autoCompleteData.value.query);
|
||||
const metricName = parsedQuery?.metricName || "";
|
||||
|
|
@ -229,16 +242,19 @@ const usePromqlSuggestions = () => {
|
|||
// with all of its labels for the client to dedupe — 5903 bytes where
|
||||
// the schema answers in 1699, and it is metadata, so no scan at all.
|
||||
try {
|
||||
let labels = metricLabelCache.get(metricName);
|
||||
const cacheKey = metricLabelCacheKey(org, metricName);
|
||||
let labels = metricLabelCache.get(cacheKey);
|
||||
if (!labels) {
|
||||
const response: any = await streamService.schema(org, metricName, "metrics");
|
||||
const columns = response?.data?.schema ?? response?.data?.uds_schema ?? [];
|
||||
labels = columns
|
||||
.map((column: any) => column?.name)
|
||||
.filter((name: string) => name && !NON_LABEL_COLUMNS.has(name));
|
||||
metricLabelCache.set(metricName, labels as string[]);
|
||||
metricLabelCache.set(cacheKey, labels as string[]);
|
||||
}
|
||||
|
||||
if (!isCurrent()) return;
|
||||
|
||||
const alreadyFiltered = formattedLabels.join(",");
|
||||
updatePromqlKeywords(
|
||||
(labels as string[])
|
||||
|
|
@ -254,6 +270,7 @@ const usePromqlSuggestions = () => {
|
|||
);
|
||||
} catch {
|
||||
// The labels are unavailable; the language is not.
|
||||
if (!isCurrent()) return;
|
||||
updatePromqlKeywords([]);
|
||||
autoCompleteData.value.popup.close("");
|
||||
}
|
||||
|
|
@ -271,6 +288,8 @@ const usePromqlSuggestions = () => {
|
|||
values = [];
|
||||
}
|
||||
|
||||
if (!isCurrent()) return;
|
||||
|
||||
// Quoting, decided from what is actually around the cursor. Monaco
|
||||
// auto-closes `"`, so the model is `service=""` with the cursor between
|
||||
// them; inserting a fully quoted value there produced
|
||||
|
|
|
|||
Loading…
Reference in New Issue