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 f54f539492..d82526e25c 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,7 @@ function QueryDisplay(props: { value={query.storage || ""} onChange={selectStorage} name="storage" - options={nodeData.map((storage) => storage.storage_name)} + 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 abd3fbcb8e..5dd239a958 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,70 @@ describe("getAllNodes", () => { }); }); +describe("getStorageOptions", () => { + // 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("keeps the given storage", () => { + expect(getStorageOptions(withSets(eap), "eap_items_downsample_8")).toEqual([ + "eap_items", + "eap_items_downsample_8", + "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(withSets(eap)); + 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 +152,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 +199,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 +296,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..374791b219 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,30 @@ 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 + +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. + +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 | +| --- | --- | --- | +| `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. + +- 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 is `foldStorages` in `clickhouse_node_utils.tsx`. + ## Words Matching works on **words**: @@ -38,10 +62,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 +88,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 +109,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..a1aae9f1c1 100644 --- a/snuba/admin/static/utils/clickhouse_node_utils.tsx +++ b/snuba/admin/static/utils/clickhouse_node_utils.tsx @@ -51,6 +51,79 @@ export function getHostsForStorage( return hosts; } +// 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 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 - 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 + */ +function foldStorages( + storageNames: string[], + storageSetOf: (name: string) => string | undefined, + keep?: string +): string[] { + 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; @@ -96,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]) => { @@ -104,15 +178,17 @@ export function getAllNodes(nodeData: ClickhouseNodeData[]): NodeOption[] { ...(entry.query ? ["query node"] : []), ]; const storages = Array.from(entry.storages).sort(); + // Listed one per storage set. + const shown = foldStorages(storages, storageSetOf); 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, }; @@ -140,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 @@ -271,9 +345,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 +359,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 +396,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; } diff --git a/tests/admin/test_api.py b/tests/admin/test_api.py index 098e78e788..2ff2f4a0ac 100644 --- a/tests/admin/test_api.py +++ b/tests/admin/test_api.py @@ -67,6 +67,16 @@ 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 +@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 + 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 (