From 21762f85ebdb29d44dbe0a899d20ecefe5a26239 Mon Sep 17 00:00:00 2001 From: Oliver Newland Date: Fri, 6 Nov 2026 07:20:57 -0800 Subject: [PATCH 1/3] feat(admin): Fold shared-node storages and skip host words in node search In production, many storages use the same nodes, which made the storage lists long and repetitive. Every eap_items storage (downsampled, co-occurring attributes, ...) now shows as eap_items, or eap_items_ro for the read-only ones, and the outcomes storage set shows as outcomes_raw. That applies to the storage line under each node and to the storage pickers. The selected storage is always kept in the picker, and the storage sent with a query is still chosen from all of the node's storages. Hosts like snuba-events-analytics-platform-arm-1-1 and ...-mz-1-1 didn't match eap-1-1. A typed separator can now skip one whole host word that starts with a letter, counted as a boundary miss so exact hosts rank first. Number words (shards, replicas, ports) are never skipped, so their positions still count. Co-Authored-By: Claude --- .../clickhouse_queries/query_display.tsx | 6 +- .../utils/clickhouse_node_utils.spec.tsx | 95 +++++++++++++++++- snuba/admin/static/utils/NODE_SEARCH.md | 38 +++++-- .../static/utils/clickhouse_node_utils.tsx | 98 ++++++++++++++++--- 4 files changed, 216 insertions(+), 21 deletions(-) diff --git a/snuba/admin/static/clickhouse_queries/query_display.tsx b/snuba/admin/static/clickhouse_queries/query_display.tsx index f54f539492..41d7bc88c1 100644 --- a/snuba/admin/static/clickhouse_queries/query_display.tsx +++ b/snuba/admin/static/clickhouse_queries/query_display.tsx @@ -29,6 +29,7 @@ import { getAllNodes, getErrorDomElement, getHostsForStorage, + getStorageOptions, nodeMatchesSearch, nodeSearchMatch, nodeSelectItem, @@ -366,7 +367,10 @@ function QueryDisplay(props: { value={query.storage || ""} onChange={selectStorage} name="storage" - options={nodeData.map((storage) => storage.storage_name)} + options={getStorageOptions( + nodeData.map((storage) => storage.storage_name), + query.storage + )} /> {flagged === "storage" && ( diff --git a/snuba/admin/static/tests/utils/clickhouse_node_utils.spec.tsx b/snuba/admin/static/tests/utils/clickhouse_node_utils.spec.tsx index abd3fbcb8e..f1203c32f5 100644 --- a/snuba/admin/static/tests/utils/clickhouse_node_utils.spec.tsx +++ b/snuba/admin/static/tests/utils/clickhouse_node_utils.spec.tsx @@ -1,6 +1,7 @@ import { it, expect, describe } from "@jest/globals"; import { getAllNodes, + getStorageOptions, nodeMatchesSearch, nodeSearchMatch, rankNodes, @@ -52,6 +53,61 @@ describe("getAllNodes", () => { }); }); +describe("getStorageOptions", () => { + const eap = [ + "eap_items", + "eap_items_ro", + "eap_items_2", + "eap_items_2_ro", + "eap_items_downsample_8", + "eap_items_downsample_8_ro", + "eap_item_co_occurring_attrs", + "eap_item_co_occurring_attrs_ro", + ]; + + it("folds eap_items storages into eap_items and eap_items_ro", () => { + expect(getStorageOptions([...eap, "errors"])).toEqual([ + "eap_items", + "eap_items_ro", + "errors", + ]); + }); + + it("folds the outcomes storage set into outcomes_raw", () => { + expect( + getStorageOptions([ + "outcomes_raw", + "outcomes_hourly", + "outcomes_daily", + "outcomes_cross_org_hourly", + "llm_proxy_cost_raw", + "querylog", + ]) + ).toEqual(["outcomes_raw", "querylog"]); + }); + + it("keeps the given storage", () => { + expect(getStorageOptions(eap, "eap_items_downsample_8")).toEqual([ + "eap_items", + "eap_items_downsample_8", + "eap_items_ro", + ]); + }); + + it("only folds into a top-level storage that's listed", () => { + expect(getStorageOptions(["eap_items_2", "eap_items_2_ro", "eap_items_ro"])).toEqual([ + "eap_items_2", + "eap_items_ro", + ]); + }); + + it("folds a node's storage list too", () => { + const [node] = getAllNodes(eap.map((name) => storage(name, ["h"]))); + expect(node.description).toBe("eap_items, eap_items_ro"); + expect(node.storages).toHaveLength(eap.length); + }); +}); + describe("storageForNode", () => { const [node] = getAllNodes([ storage("errors_ro", ["h"]), @@ -87,7 +143,8 @@ describe("nodeMatchesSearch", () => { it("matches words shortened to their initials", () => { expect(nodeMatchesSearch("eap", host)).toBe(true); expect(nodeMatchesSearch("snuba-eap-tier1", host)).toBe(true); - expect(nodeMatchesSearch("eap-0-1", host)).toBe(false); + // tier1 is skipped as a whole word. + expect(nodeMatchesSearch("eap-0-1", host)).toBe(true); expect(nodeMatchesSearch("eapt-0-1", host)).toBe(true); expect(nodeMatchesSearch("seap", host)).toBe(true); expect(nodeMatchesSearch("gm", host)).toBe(false); @@ -133,8 +190,27 @@ describe("nodeSearchMatch", () => { expect(nodeSearchMatch("err-1", errors)?.rank).toBe(0); expect(nodeSearchMatch("err1", errors)).toBeNull(); expect(nodeSearchMatch("sn-err-1-0", errors)).not.toBeNull(); - // Only the rest of one word. - expect(nodeSearchMatch("snuba-1", errors)).toBeNull(); + // The rest of one word, then at most one whole word. + expect(nodeSearchMatch("snuba-1", errors)).not.toBeNull(); + expect(nodeSearchMatch("snuba-1", "snuba-generic-metrics-1-0:8123")).toBeNull(); + expect(nodeSearchMatch("sn-1", errors)?.boundaryMisses).toBe(2); + expect(nodeSearchMatch("sn-1", "snuba-generic-metrics-1-0:8123")).toBeNull(); + }); + + it("skips one whole word at a typed separator", () => { + const arm = "snuba-events-analytics-platform-arm-1-1:8123"; + const mz = "snuba-events-analytics-platform-mz-1-1:8123"; + expect( + nodeSearchMatch("eap-1-1", arm)?.indexes.map((idx) => arm[idx]).join("") + ).toBe("eap1-1"); + expect(nodeSearchMatch("eap-1-1", mz)?.boundaryMisses).toBe(1); + expect(nodeSearchMatch("eap-arm-1-1", arm)?.boundaryMisses).toBe(0); + // Not without a typed separator. + expect(nodeSearchMatch("eap11", arm)).toBeNull(); + // Number words aren't skipped. + expect( + nodeSearchMatch("eap-1", "snuba-events-analytics-platform-0-1:8123") + ).toBeNull(); }); it("prefers a match at the start of a word", () => { @@ -211,6 +287,19 @@ describe("rankNodes", () => { expect(order("eap-1-1")[0]).toBe("snuba-events-analytics-platform-1-1:9000"); }); + it("lists hosts without a skipped word first", () => { + const hosts = getAllNodes( + ["arm-1-1", "1-1", "mz-1-1"].map((shard) => + storage("eap", [`snuba-events-analytics-platform-${shard}`]) + ) + ); + expect(rankNodes("eap-1-1", hosts).map((node) => node.value)).toEqual([ + "snuba-events-analytics-platform-1-1:9000", + "snuba-events-analytics-platform-arm-1-1:9000", + "snuba-events-analytics-platform-mz-1-1:9000", + ]); + }); + it("lists word-start matches before mid-word ones", () => { expect(order("m")).toEqual([ "snuba-generic-metrics-0-0:9000", diff --git a/snuba/admin/static/utils/NODE_SEARCH.md b/snuba/admin/static/utils/NODE_SEARCH.md index dc860a4b9c..1b45f9637f 100644 --- a/snuba/admin/static/utils/NODE_SEARCH.md +++ b/snuba/admin/static/utils/NODE_SEARCH.md @@ -1,12 +1,12 @@ # System Queries node search -How the **Host** autocomplete in System Queries ("Search nodes" mode) finds, ranks and highlights ClickHouse nodes. The code is in [`clickhouse_node_utils.tsx`](clickhouse_node_utils.tsx) (`getAllNodes`, `nodeSearchMatch`, `rankNodes`), and the select it drives is `CustomSelect` in [`../select.tsx`](../select.tsx). +How the **Host** autocomplete in System Queries ("Search nodes" mode) finds, ranks and highlights ClickHouse nodes, and how storage lists are shortened. The code is in [`clickhouse_node_utils.tsx`](clickhouse_node_utils.tsx) (`getAllNodes`, `nodeSearchMatch`, `rankNodes`), and the select it drives is `CustomSelect` in [`../select.tsx`](../select.tsx). ## The node list - Every node of every storage in `/clickhouse_nodes` appears once, keyed by `host:port`. This includes local nodes, distributed nodes and query nodes. - The label is `host:port`, with `(distributed)` and/or `(query node)` appended where that applies. A node that is also local is never labelled distributed. -- The line under the label lists the storages whose cluster includes the node, for example `errors, errors_ro, search_issues +4 more`. +- The line under the label lists the storages whose cluster includes the node, for example `errors, errors_ro, search_issues +4 more`. Storages that share a top-level storage's nodes are folded into it (see [Storage lists](#storage-lists)). - A node counts as **distributed** for ranking only if it is distributed and nothing else: not local, and not a query node. ## Picking a node sets the storage @@ -21,6 +21,22 @@ Manual host entry still shows a storage picker, since a typed host has no other "By storage" mode is the old way of picking a host, kept as an alternative: pick a storage, its first host fills in, and the host list shows only that storage's nodes. The page remembers whether you last used "Search nodes" or "By storage". Switching between them keeps the current host and storage. +## Storage lists + +Many storages are known to use the same nodes as a top-level storage. Storage lists show only the top-level storage. This applies to the storage line under each node and to the storage pickers in "By storage" and "Manual" modes. + +| Folded storages | Shown as | Why they share nodes | +| --- | --- | --- | +| Every `eap_items*` and `eap_item_*` storage (`eap_items_2`, the `downsample_*` ones, `eap_item_co_occurring_attrs`, …), except the `_ro` ones | `eap_items` | Known to use the same cluster. | +| The `_ro` ones of those | `eap_items_ro` | Known to use the same cluster. | +| `outcomes_hourly`, `outcomes_daily`, `outcomes_cross_org_hourly`, `llm_proxy_cost_raw` | `outcomes_raw` | They're all in the `outcomes` storage set, and a storage set belongs to exactly one cluster (asserted in `snuba/clusters/cluster.py`). | + +- A storage is folded into a top-level one only if that top-level storage is in the same list. On a node that lists `eap_items_2` but not `eap_items`, `eap_items_2` is shown. +- The selected storage is always in the picker, even if it would be folded, so a shared link to `eap_items_downsample_8` still shows it. +- Folding only affects what's listed. The storage sent with a query is still chosen from every storage that lists the node. + +The folding rule is `topLevelStorage` in `clickhouse_node_utils.tsx`. + ## Words Matching works on **words**: @@ -38,10 +54,16 @@ A node matches if the search, with its separators removed, can be matched charac 2. **Initials.** Any run of two or more consecutive host words can be typed as their first letters, for example `eap` for `events-analytics-platform`. - Every word in the run has to start with a letter. Number words and the port can't be part of a run. - Only one run per match. - - `eapt` would cover `events-analytics-platform-tier1`, but `eap-0-1` can't skip a `tier1` that sits in between. -3. **A typed separator can skip the rest of a word.** If you type a separator and you're partway through a host word, the next character may jump to the start of the next host word. `err-1` matches `snuba-errors-1-0` as `err`…`1`. - - The jump covers only the rest of the current word. `snuba-1` doesn't match `snuba-errors-1-0`, because it would have to skip `errors`. - - Without a typed separator there's no jump. `err1` doesn't match `errors-1`. + - `eapt` covers `events-analytics-platform-tier1`. `eap-0-1` also matches it, but by skipping `tier1` (rule 3). +3. **A typed separator can skip ahead.** Where you type a separator, the next character may skip: + - **the rest of the current host word**: `err-1` matches `snuba-errors-1-0` as `err`…`1`; and/or + - **one whole host word that starts with a letter**: `eap-1-1` matches `…-platform-arm-1-1` and `…-platform-mz-1-1`, skipping `arm` or `mz`. + + Limits: + - At most one whole word per separator. `snuba-1` matches `snuba-errors-1-0` but not `snuba-generic-metrics-1-0`. + - Number words (shards, replicas, the port) are never skipped, so their positions still count. `eap-1` doesn't match `…-platform-0-1`. + - An initial from rule 2 stands for a word you typed, so it can't be skipped. + - Without a typed separator, nothing is skipped. `err1` doesn't match `errors-1`, and `eap11` doesn't match `…-arm-1-1`. 4. **The match can start anywhere,** including mid-word, for example `m` in `platform`. These matches rank last; see below. An empty search matches every node. @@ -58,6 +80,7 @@ The dropdown is re-sorted on every keystroke. Each node is scored by its best ma - The host starts a new word where you typed no separator. Initials typed together don't count. - You typed a separator where the host keeps going in the same word, without skipping (rule 3). - A typed separator skipped the rest of a word (rule 3). + - A typed separator skipped a whole word (rule 3). So `eap-1-1` lists `…-platform-1-1` before `…-platform-arm-1-1`. - The search ends partway through a host word. 3. **Distributed nodes last.** 4. **`host:port`, alphabetically.** Because `-` sorts before digits, `…-1-0` comes before `…-10-0`. @@ -78,7 +101,10 @@ The comparison order means a closer match of a worse rank never beats a word-sta | `eap-1-1` | `…-platform-1-1` | initials | 0 | breaks line up | | `eap-1-1` | `…-platform-1-11` | initials | 1 | ends partway through `11` | | `eap-1` | `…-platform-10-0` | initials | 1 | ends partway through `10` | +| `eap-1-1` | `…-platform-arm-1-1` | initials | 1 | skips `arm` | +| `eap-arm-1-1` | `…-platform-arm-1-1` | initials | 0 | breaks line up | | `err-1` | `snuba-errors-1-0` | word start | 1 | skips the rest of `errors` | +| `snuba-1` | `snuba-errors-1-0` | word start | 1 | skips `errors` | | `errors-1` | `snuba-errors-1-0` | word start | 0 | breaks line up | ## Highlighting diff --git a/snuba/admin/static/utils/clickhouse_node_utils.tsx b/snuba/admin/static/utils/clickhouse_node_utils.tsx index ad1d907bd2..938437c68d 100644 --- a/snuba/admin/static/utils/clickhouse_node_utils.tsx +++ b/snuba/admin/static/utils/clickhouse_node_utils.tsx @@ -51,6 +51,59 @@ export function getHostsForStorage( return hosts; } +// Storages that are known to share their nodes with a top-level one, which +// stands in for them in storage lists: every eap_items storage (downsampled, +// co-occurring attributes, ...) uses eap_items' nodes, and the read-only +// ones eap_items_ro's. +const EAP_ITEMS_STORAGE = /^eap_items?(_|$)/; + +// The outcomes storage set. A storage set belongs to exactly one cluster +// (snuba/clusters/cluster.py asserts it), so these have to share nodes. +const OUTCOMES_STORAGES = new Set([ + "outcomes_raw", + "outcomes_hourly", + "outcomes_daily", + "outcomes_cross_org_hourly", + "llm_proxy_cost_raw", +]); + +function topLevelStorage(name: string): string { + if (OUTCOMES_STORAGES.has(name)) { + return "outcomes_raw"; + } + if (!EAP_ITEMS_STORAGE.test(name)) { + return name; + } + return name.endsWith("_ro") ? "eap_items_ro" : "eap_items"; +} + +/** + * The storage names to list, with storages that share a top-level storage's + * nodes (see topLevelStorage) folded into it. A storage is only folded into + * a top-level one that's in the list, and `keep` (e.g. the selected storage) + * is always listed so a select can still show it. + * + * @param storageNames - All storage names + * @param keep - A storage name to always list + * @returns The storage names to list, sorted + */ +export function getStorageOptions( + storageNames: string[], + keep?: string +): string[] { + const known = new Set(storageNames); + const options = new Set( + storageNames.map((name) => { + const topLevel = topLevelStorage(name); + return known.has(topLevel) ? topLevel : name; + }) + ); + if (keep && known.has(keep)) { + options.add(keep); + } + return Array.from(options).sort(); +} + export type NodeOption = SelectItem & { value: string; label: string; @@ -104,15 +157,17 @@ export function getAllNodes(nodeData: ClickhouseNodeData[]): NodeOption[] { ...(entry.query ? ["query node"] : []), ]; const storages = Array.from(entry.storages).sort(); + // Listed with the top-level storages that stand in for others. + const shown = getStorageOptions(storages); return { value: hostPort, label: roles.length ? `${hostPort} (${roles.join(", ")})` : hostPort, description: - storages.length > 3 - ? `${storages.slice(0, 3).join(", ")} +${storages.length - 3} more` - : storages.join(", "), + shown.length > 3 + ? `${shown.slice(0, 3).join(", ")} +${shown.length - 3} more` + : shown.join(", "), storages, distributed: entry.dist && !entry.local && !entry.query, }; @@ -271,9 +326,12 @@ type VariantMatch = { positions: number[]; boundaryMisses: number }; /** * The closest match of the needle in the variant starting at `start`, or null. - * The needle's characters are matched in order, and where a separator was - * typed, the rest of the current host word may be skipped, so "err-1" finds - * errors-1. + * The needle's characters are matched in order. Where a separator was typed, + * the rest of the current host word may be skipped, so "err-1" finds + * errors-1, and so may one whole word that starts with a letter, so + * "eap-1-1" finds events-analytics-platform-arm-1-1. Number words (shards, + * replicas, ports) are never skipped, so "eap-1" doesn't find + * events-analytics-platform-0-1. */ function matchVariantFrom( variant: SearchVariant, @@ -282,6 +340,13 @@ function matchVariantFrom( start: number ): VariantMatch | null { const { text, wordStarts, initials } = variant; + function nextWordStart(position: number): number { + let next = position + 1; + while (next < text.length && !wordStarts[next]) { + next++; + } + return next; + } function closest(a: VariantMatch | null, b: VariantMatch | null) { return a === null || (b !== null && b.boundaryMisses < a.boundaryMisses) ? b @@ -312,13 +377,24 @@ function matchVariantFrom( const hostBreak = wordStarts[at] && !(initials[at] && initials[at - 1]); let best = take(at, hostBreak !== typedBreaks[k] ? 1 : 0); - if (typedBreaks[k] && !wordStarts[at]) { + if (typedBreaks[k]) { // Skip to the next word. Leaving a word unfinished is a miss. - let next = at + 1; - while (next < text.length && !wordStarts[next]) { - next++; + let wordStart = at; + let misses = 0; + if (!wordStarts[at]) { + wordStart = nextWordStart(at); + misses = 1; + best = closest(best, take(wordStart, misses)); + } + // Skipping a whole word is a miss too. An initial stands in for a + // word that was typed, so it can't be skipped. + if ( + wordStart < text.length && + !initials[wordStart] && + /[a-z]/.test(text[wordStart]) + ) { + best = closest(best, take(nextWordStart(wordStart), misses + 1)); } - best = closest(best, take(next, 1)); } return best; } From 60c87efa07bdc45ed60a5c7244ae0162a929702d Mon Sep 17 00:00:00 2001 From: Oliver Newland Date: Fri, 6 Nov 2026 07:29:50 -0800 Subject: [PATCH 2/3] ref(admin): Fold storage lists by storage set instead of name rules The eap_items and outcomes folding hard-coded which storages share nodes. The storage configuration already says this: a storage set belongs to exactly one cluster (snuba/clusters/cluster.py asserts it), so its storages always share their nodes. /clickhouse_nodes now reports each storage's set, and storage lists show the shortest name in each set. That gives the same result for eap_items, eap_items_ro and outcomes_raw, also folds the metrics, functions and replays sets, and picks up new storages without any change here. Co-Authored-By: Claude --- snuba/admin/clickhouse/nodes.py | 4 + .../clickhouse_queries/query_display.tsx | 5 +- .../admin/static/clickhouse_queries/types.tsx | 2 + .../utils/clickhouse_node_utils.spec.tsx | 75 +++++++------ snuba/admin/static/utils/NODE_SEARCH.md | 22 ++-- .../static/utils/clickhouse_node_utils.tsx | 105 +++++++++++------- tests/admin/test_api.py | 9 ++ 7 files changed, 135 insertions(+), 87 deletions(-) diff --git a/snuba/admin/clickhouse/nodes.py b/snuba/admin/clickhouse/nodes.py index 99e51c8f76..04f35044c3 100644 --- a/snuba/admin/clickhouse/nodes.py +++ b/snuba/admin/clickhouse/nodes.py @@ -23,6 +23,9 @@ class Node(TypedDict): class Storage(TypedDict): storage_name: str + # Storages in one storage set always share a cluster, so the admin lists + # them as one. + storage_set: str local_table_name: str local_nodes: Sequence[Node] dist_nodes: Sequence[Node] @@ -77,6 +80,7 @@ def get_storage_info() -> Sequence[Storage]: return [ { "storage_name": storage_key.value, + "storage_set": get_storage(storage_key).get_storage_set_key().value, "local_table_name": _get_local_table_name(storage_key), "local_nodes": _get_local_nodes(storage_key), "dist_nodes": _get_dist_nodes(storage_key), diff --git a/snuba/admin/static/clickhouse_queries/query_display.tsx b/snuba/admin/static/clickhouse_queries/query_display.tsx index 41d7bc88c1..d82526e25c 100644 --- a/snuba/admin/static/clickhouse_queries/query_display.tsx +++ b/snuba/admin/static/clickhouse_queries/query_display.tsx @@ -367,10 +367,7 @@ function QueryDisplay(props: { value={query.storage || ""} onChange={selectStorage} name="storage" - options={getStorageOptions( - nodeData.map((storage) => storage.storage_name), - query.storage - )} + options={getStorageOptions(nodeData, query.storage)} /> {flagged === "storage" && ( diff --git a/snuba/admin/static/clickhouse_queries/types.tsx b/snuba/admin/static/clickhouse_queries/types.tsx index 9c1c2955a8..21c805828f 100644 --- a/snuba/admin/static/clickhouse_queries/types.tsx +++ b/snuba/admin/static/clickhouse_queries/types.tsx @@ -5,6 +5,8 @@ type ClickhouseNode = { type ClickhouseNodeData = { storage_name: string; + // Storages in one storage set always share a cluster. + storage_set?: string; local_table_name: string; local_nodes: ClickhouseNode[]; dist_nodes: ClickhouseNode[]; diff --git a/snuba/admin/static/tests/utils/clickhouse_node_utils.spec.tsx b/snuba/admin/static/tests/utils/clickhouse_node_utils.spec.tsx index f1203c32f5..5dd239a958 100644 --- a/snuba/admin/static/tests/utils/clickhouse_node_utils.spec.tsx +++ b/snuba/admin/static/tests/utils/clickhouse_node_utils.spec.tsx @@ -54,55 +54,64 @@ describe("getAllNodes", () => { }); describe("getStorageOptions", () => { - const eap = [ - "eap_items", - "eap_items_ro", - "eap_items_2", - "eap_items_2_ro", - "eap_items_downsample_8", - "eap_items_downsample_8_ro", - "eap_item_co_occurring_attrs", - "eap_item_co_occurring_attrs_ro", - ]; - - it("folds eap_items storages into eap_items and eap_items_ro", () => { - expect(getStorageOptions([...eap, "errors"])).toEqual([ + // As in the storage configuration. + const storageSets: Record = { + eap_items: "events_analytics_platform", + eap_items_2: "events_analytics_platform", + eap_items_downsample_8: "events_analytics_platform", + eap_item_co_occurring_attrs: "events_analytics_platform", + eap_items_ro: "events_analytics_platform_ro", + eap_items_2_ro: "events_analytics_platform_ro", + eap_items_downsample_8_ro: "events_analytics_platform_ro", + eap_item_co_occurring_attrs_ro: "events_analytics_platform_ro", + outcomes_raw: "outcomes", + outcomes_hourly: "outcomes", + outcomes_daily: "outcomes", + outcomes_cross_org_hourly: "outcomes", + llm_proxy_cost_raw: "outcomes", + errors: "events", + }; + const withSets = (names: string[], host = "h") => + names.map((name) => ({ + ...storage(name, [host]), + storage_set: storageSets[name], + })); + const eap = Object.keys(storageSets).filter((name) => name.startsWith("eap")); + const outcomes = Object.keys(storageSets).filter( + (name) => storageSets[name] === "outcomes" + ); + + it("lists the shortest storage of each storage set", () => { + expect(getStorageOptions(withSets([...eap, ...outcomes, "errors"]))).toEqual([ "eap_items", "eap_items_ro", "errors", + "outcomes_raw", ]); }); - it("folds the outcomes storage set into outcomes_raw", () => { - expect( - getStorageOptions([ - "outcomes_raw", - "outcomes_hourly", - "outcomes_daily", - "outcomes_cross_org_hourly", - "llm_proxy_cost_raw", - "querylog", - ]) - ).toEqual(["outcomes_raw", "querylog"]); - }); - it("keeps the given storage", () => { - expect(getStorageOptions(eap, "eap_items_downsample_8")).toEqual([ + expect(getStorageOptions(withSets(eap), "eap_items_downsample_8")).toEqual([ "eap_items", "eap_items_downsample_8", "eap_items_ro", ]); }); - it("only folds into a top-level storage that's listed", () => { - expect(getStorageOptions(["eap_items_2", "eap_items_2_ro", "eap_items_ro"])).toEqual([ - "eap_items_2", - "eap_items_ro", - ]); + it("picks from the storages it's given", () => { + expect( + getStorageOptions(withSets(["eap_items_2", "eap_items_downsample_8"])) + ).toEqual(["eap_items_2"]); + }); + + it("lists storages without a known set as they are", () => { + expect( + getStorageOptions([storage("querylog", ["h"]), storage("spans", ["h"])]) + ).toEqual(["querylog", "spans"]); }); it("folds a node's storage list too", () => { - const [node] = getAllNodes(eap.map((name) => storage(name, ["h"]))); + const [node] = getAllNodes(withSets(eap)); expect(node.description).toBe("eap_items, eap_items_ro"); expect(node.storages).toHaveLength(eap.length); }); diff --git a/snuba/admin/static/utils/NODE_SEARCH.md b/snuba/admin/static/utils/NODE_SEARCH.md index 1b45f9637f..374791b219 100644 --- a/snuba/admin/static/utils/NODE_SEARCH.md +++ b/snuba/admin/static/utils/NODE_SEARCH.md @@ -23,19 +23,27 @@ Manual host entry still shows a storage picker, since a typed host has no other ## Storage lists -Many storages are known to use the same nodes as a top-level storage. Storage lists show only the top-level storage. This applies to the storage line under each node and to the storage pickers in "By storage" and "Manual" modes. +Storage lists show one storage per **storage set**. A storage set always belongs to exactly one cluster (asserted in `snuba/clusters/cluster.py`), so its storages share their nodes, and listing them all would only repeat the same nodes under different names. `/clickhouse_nodes` reports each storage's set. This applies to the storage line under each node and to the storage pickers in "By storage" and "Manual" modes. -| Folded storages | Shown as | Why they share nodes | +The shortest name in the set stands in for the rest, with ties broken alphabetically. With the current configuration: + +| Storage set | Shown as | Folded into it | | --- | --- | --- | -| Every `eap_items*` and `eap_item_*` storage (`eap_items_2`, the `downsample_*` ones, `eap_item_co_occurring_attrs`, …), except the `_ro` ones | `eap_items` | Known to use the same cluster. | -| The `_ro` ones of those | `eap_items_ro` | Known to use the same cluster. | -| `outcomes_hourly`, `outcomes_daily`, `outcomes_cross_org_hourly`, `llm_proxy_cost_raw` | `outcomes_raw` | They're all in the `outcomes` storage set, and a storage set belongs to exactly one cluster (asserted in `snuba/clusters/cluster.py`). | +| `events_analytics_platform` | `eap_items` | `eap_items_2`, the `downsample_*` storages, `eap_item_co_occurring_attrs`(`_v2`), `eap_items_dlq_replay` | +| `events_analytics_platform_ro` | `eap_items_ro` | the `_ro` versions of those | +| `outcomes` | `outcomes_raw` | `outcomes_hourly`, `outcomes_daily`, `outcomes_cross_org_hourly`, `llm_proxy_cost_raw` | +| `metrics` | `metrics_raw` | `metrics_counters`, `metrics_distributions`, `metrics_sets`, `org_metrics_counters` | +| `functions` | `functions` | `functions_raw` | +| `replays` | `replays` | `replays_aggregated` | + +New storages are folded into their set automatically; nothing here needs updating. -- A storage is folded into a top-level one only if that top-level storage is in the same list. On a node that lists `eap_items_2` but not `eap_items`, `eap_items_2` is shown. +- The representative is chosen from the storages in that list. On a node that lists `eap_items_2` but not `eap_items`, `eap_items_2` is shown. - The selected storage is always in the picker, even if it would be folded, so a shared link to `eap_items_downsample_8` still shows it. +- A storage whose set isn't reported is listed as it is. - Folding only affects what's listed. The storage sent with a query is still chosen from every storage that lists the node. -The folding rule is `topLevelStorage` in `clickhouse_node_utils.tsx`. +The folding is `foldStorages` in `clickhouse_node_utils.tsx`. ## Words diff --git a/snuba/admin/static/utils/clickhouse_node_utils.tsx b/snuba/admin/static/utils/clickhouse_node_utils.tsx index 938437c68d..a1aae9f1c1 100644 --- a/snuba/admin/static/utils/clickhouse_node_utils.tsx +++ b/snuba/admin/static/utils/clickhouse_node_utils.tsx @@ -51,59 +51,79 @@ export function getHostsForStorage( return hosts; } -// Storages that are known to share their nodes with a top-level one, which -// stands in for them in storage lists: every eap_items storage (downsampled, -// co-occurring attributes, ...) uses eap_items' nodes, and the read-only -// ones eap_items_ro's. -const EAP_ITEMS_STORAGE = /^eap_items?(_|$)/; - -// The outcomes storage set. A storage set belongs to exactly one cluster -// (snuba/clusters/cluster.py asserts it), so these have to share nodes. -const OUTCOMES_STORAGES = new Set([ - "outcomes_raw", - "outcomes_hourly", - "outcomes_daily", - "outcomes_cross_org_hourly", - "llm_proxy_cost_raw", -]); - -function topLevelStorage(name: string): string { - if (OUTCOMES_STORAGES.has(name)) { - return "outcomes_raw"; - } - if (!EAP_ITEMS_STORAGE.test(name)) { - return name; - } - return name.endsWith("_ro") ? "eap_items_ro" : "eap_items"; +// Orders storage names by how well they stand for their storage set: the +// shortest first, then alphabetically. +function compareRepresentatives(a: string, b: string): number { + return a.length - b.length || (a < b ? -1 : a > b ? 1 : 0); } /** - * The storage names to list, with storages that share a top-level storage's - * nodes (see topLevelStorage) folded into it. A storage is only folded into - * a top-level one that's in the list, and `keep` (e.g. the selected storage) - * is always listed so a select can still show it. + * The storage names to list, with one name per storage set. A storage set + * always belongs to a single cluster (snuba/clusters/cluster.py asserts it), + * so its storages share their nodes, and the set's shortest name stands in + * for the rest (eap_items for every eap_items_* storage, outcomes_raw for + * outcomes_hourly, ...). A storage without a known set is listed as is, and + * `keep` (e.g. the selected storage) is always listed so a select can still + * show it. * - * @param storageNames - All storage names + * @param storageNames - The storage names to fold + * @param storageSetOf - Each storage's storage set, if known * @param keep - A storage name to always list * @returns The storage names to list, sorted */ -export function getStorageOptions( +function foldStorages( storageNames: string[], + storageSetOf: (name: string) => string | undefined, keep?: string ): string[] { - const known = new Set(storageNames); - const options = new Set( - storageNames.map((name) => { - const topLevel = topLevelStorage(name); - return known.has(topLevel) ? topLevel : name; - }) - ); - if (keep && known.has(keep)) { + const bySet = new Map(); + const options = new Set(); + for (const name of storageNames) { + const storageSet = storageSetOf(name); + if (storageSet === undefined) { + options.add(name); + continue; + } + const current = bySet.get(storageSet); + if (current === undefined || compareRepresentatives(name, current) < 0) { + bySet.set(storageSet, name); + } + } + bySet.forEach((name) => options.add(name)); + if (keep && storageNames.includes(keep)) { options.add(keep); } return Array.from(options).sort(); } +function storageSetLookup( + nodeData: ClickhouseNodeData[] +): (name: string) => string | undefined { + const storageSets = new Map( + nodeData.map((nodeInfo) => [nodeInfo.storage_name, nodeInfo.storage_set]) + ); + return (name) => storageSets.get(name) ?? undefined; +} + +/** + * The storages to offer in a storage picker, one per storage set (see + * foldStorages). + * + * @param nodeData - Array of ClickhouseNodeData + * @param keep - A storage name to always list, e.g. the selected one + * @returns The storage names to list, sorted + */ +export function getStorageOptions( + nodeData: ClickhouseNodeData[], + keep?: string +): string[] { + return foldStorages( + nodeData.map((nodeInfo) => nodeInfo.storage_name), + storageSetLookup(nodeData), + keep + ); +} + export type NodeOption = SelectItem & { value: string; label: string; @@ -149,6 +169,7 @@ export function getAllNodes(nodeData: ClickhouseNodeData[]): NodeOption[] { } } + const storageSetOf = storageSetLookup(nodeData); return Array.from(byHostPort.entries()) .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)) .map(([hostPort, entry]) => { @@ -157,8 +178,8 @@ export function getAllNodes(nodeData: ClickhouseNodeData[]): NodeOption[] { ...(entry.query ? ["query node"] : []), ]; const storages = Array.from(entry.storages).sort(); - // Listed with the top-level storages that stand in for others. - const shown = getStorageOptions(storages); + // Listed one per storage set. + const shown = foldStorages(storages, storageSetOf); return { value: hostPort, label: roles.length @@ -195,9 +216,7 @@ export function storageForNode(node: NodeOption, preferred?: string): string { if (node.storages.includes("querylog")) { return "querylog"; } - return [...node.storages].sort( - (a, b) => a.length - b.length || (a < b ? -1 : a > b ? 1 : 0) - )[0]; + return [...node.storages].sort(compareRepresentatives)[0]; } // Searchable text without separators, where each of its characters sits in diff --git a/tests/admin/test_api.py b/tests/admin/test_api.py index 098e78e788..64838a109a 100644 --- a/tests/admin/test_api.py +++ b/tests/admin/test_api.py @@ -67,6 +67,15 @@ def get_node_for_table(admin_api: FlaskClient, storage_name: str) -> tuple[str, raise Exception(f"{storage_name} does not have a local node") +@pytest.mark.redis_db +def test_clickhouse_nodes_include_storage_set(admin_api: FlaskClient) -> None: + response = admin_api.get("/clickhouse_nodes") + assert response.status_code == 200, response + storage_sets = {node["storage_name"]: node["storage_set"] for node in json.loads(response.data)} + assert storage_sets["errors"] == "events" + assert storage_sets["outcomes_raw"] == storage_sets["outcomes_hourly"] == "outcomes" + + @pytest.mark.redis_db def test_admin_regions(admin_api: FlaskClient) -> None: with ( From 1f8b9375c6387c700aed39c8b6c0d6bbd9cad077 Mon Sep 17 00:00:00 2001 From: Oliver Newland Date: Mon, 9 Nov 2026 15:38:06 -0800 Subject: [PATCH 3/3] test(admin): Allow ClickHouse in the storage_set nodes test In distributed mode /clickhouse_nodes looks up cluster topology in ClickHouse, which the test harness blocks unless the test is marked. Co-Authored-By: Claude --- tests/admin/test_api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/admin/test_api.py b/tests/admin/test_api.py index 64838a109a..2ff2f4a0ac 100644 --- a/tests/admin/test_api.py +++ b/tests/admin/test_api.py @@ -68,6 +68,7 @@ def get_node_for_table(admin_api: FlaskClient, storage_name: str) -> tuple[str, @pytest.mark.redis_db +@pytest.mark.clickhouse_db def test_clickhouse_nodes_include_storage_set(admin_api: FlaskClient) -> None: response = admin_api.get("/clickhouse_nodes") assert response.status_code == 200, response