Repository navigation
feat(admin): Search all nodes in System Queries instead of picking a storage #8609
+1,088
−82
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import React, { useEffect, useMemo, useState } from "react"; | ||
| import Client from "SnubaAdmin/api_client"; | ||
| import QueryEditor from "SnubaAdmin/query_editor"; | ||
| import ExecuteButton from "SnubaAdmin/utils/execute_button"; | ||
|
|
@@ -24,7 +24,17 @@ import { | |
| import { COLORS, SECTION_HEADING_STYLE } from "SnubaAdmin/theme"; | ||
| import { PrefillWarning } from "SnubaAdmin/editor_panel"; | ||
| import { CustomSelect, getParamFromStorage } from "SnubaAdmin/select"; | ||
| import { | ||
| NodeOption, | ||
| getAllNodes, | ||
| getErrorDomElement, | ||
| getHostsForStorage, | ||
| nodeMatchesSearch, | ||
| nodeSearchMatch, | ||
| nodeSelectItem, | ||
| rankNodes, | ||
| storageForNode, | ||
| } from "SnubaAdmin/utils/clickhouse_node_utils"; | ||
| import { QueryHistoryEntry } from "SnubaAdmin/clickhouse_queries/query_history_entry"; | ||
| import { | ||
| clearHashParams, | ||
|
|
@@ -42,13 +52,41 @@ import { | |
| type QueryState = Partial<QueryRequest>; | ||
|
|
||
| const HISTORY_KEY = "clickhouse_queries"; | ||
| // Whether hosts are picked by storage, as before the node search. | ||
| const BY_STORAGE_KEY = "system-queries-by-storage"; | ||
|
|
||
| function getSavedByStorage(): boolean { | ||
| try { | ||
| return localStorage.getItem(BY_STORAGE_KEY) === "true"; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| function saveByStorage(value: boolean) { | ||
| try { | ||
| localStorage.setItem(BY_STORAGE_KEY, String(value)); | ||
| } catch { | ||
| // Not remembering the mode is fine. | ||
| } | ||
| } | ||
|
|
||
| // The storage's first node, to have a host ready as soon as a storage is | ||
| // picked. | ||
| function firstHost(nodeData: ClickhouseNodeData[], storage?: string) { | ||
| const [first] = getHostsForStorage(nodeData, storage); | ||
| return first ? parseHostPort(first.value) : {}; | ||
| } | ||
|
|
||
| // What picking a node sets on the query: the node, and a storage that lists | ||
| // it so the backend can find its cluster. | ||
| function queryForNode(node: NodeOption, preferredStorage?: string) { | ||
| return { | ||
| ...parseHostPort(node.value), | ||
| storage: storageForNode(node, preferredStorage), | ||
| }; | ||
| } | ||
|
|
||
| function QueryDisplay(props: { | ||
| api: Client; | ||
| resultDataPopulator: (queryResult: QueryResult) => JSX.Element; | ||
|
|
@@ -62,10 +100,12 @@ function QueryDisplay(props: { | |
| prefill !== null | ||
| ); | ||
| const [nodeData, setNodeData] = useState<ClickhouseNodeData[]>([]); | ||
| const nodeOptions = useMemo(() => getAllNodes(nodeData), [nodeData]); | ||
| // The host is filled in once the nodes arrive, so until then it isn't | ||
| // flagged as missing. | ||
| const [nodesLoaded, setNodesLoaded] = useState(false); | ||
| const [manualHostInput, setManualHostInput] = useState<string>(""); | ||
| const [byStorage, setByStorage] = useState(getSavedByStorage); | ||
| const [query, setQuery] = useState<QueryState>({ | ||
| storage: prefill?.storage ?? getParamFromStorage("storage"), | ||
| host: prefill?.host, | ||
|
|
@@ -97,43 +137,61 @@ function QueryDisplay(props: { | |
| props.api.getClickhouseNodes().then((res) => { | ||
| setNodeData(res); | ||
| setNodesLoaded(true); | ||
| const nodes = getAllNodes(res); | ||
| // A linked host no storage knows about can only be shown (and checked) | ||
| // via manual host entry. | ||
| if (prefill?.host) { | ||
| const hostPort = `${prefill.host}:${prefill.port}`; | ||
| const node = nodes.find((option) => option.value === hostPort); | ||
| if (node) { | ||
| setQuery((prevQuery) => ({ | ||
| ...prevQuery, | ||
| ...queryForNode(node, prefill.storage), | ||
| })); | ||
| } else { | ||
| setManualHostInput(hostPort); | ||
| setClusterlessHost(true); | ||
| } | ||
| } else if (!prefill) { | ||
| // The node picked last time (the host select saves it). | ||
| const saved = getParamFromStorage("host"); | ||
| const node = nodes.find((option) => option.value === saved); | ||
| if (node) { | ||
| setQuery((prevQuery) => ({ | ||
| ...prevQuery, | ||
| ...queryForNode(node, prevQuery.storage), | ||
| })); | ||
| } | ||
| } | ||
| }); | ||
| }, []); | ||
|
|
||
| // For picking hosts by storage, and for manual host entry, where the | ||
| // storage picks the cluster whose credentials are used. | ||
| function selectStorage(storage: string) { | ||
| setQuery((prevQuery) => { | ||
| if (prevQuery.clusterless) { | ||
| // A typed host isn't replaced; it's up to the user. | ||
| return { ...prevQuery, storage }; | ||
| } | ||
| const { host, port, ...rest } = prevQuery; | ||
| return { ...rest, storage, ...firstHost(nodeData, storage) }; | ||
| }); | ||
| } | ||
|
|
||
| function setHostEntry(value: string) { | ||
| setClusterlessHost(value === "manual"); | ||
| if (value !== "manual") { | ||
| setByStorage(value === "storage"); | ||
| saveByStorage(value === "storage"); | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Manual-to-search drops storageMedium Severity
Additional Locations (1)  Reviewed by Cursor Bugbot for commit b1d7c6d. Configure here. |
||
|
|
||
| // A host picked from the storage's own list keeps that storage. | ||
| function selectStorageHost(hostPort: string) { | ||
| setQuery((prevQuery) => ({ ...prevQuery, ...parseHostPort(hostPort) })); | ||
| } | ||
|
|
||
| function setSudo(value: boolean) { | ||
| setQuery((prevQuery) => { | ||
| return { | ||
|
|
@@ -168,16 +226,15 @@ function QueryDisplay(props: { | |
| }); | ||
| } | ||
|
|
||
| function selectNode(hostPort: string) { | ||
| const node = nodeOptions.find((option) => option.value === hostPort); | ||
| if (!node) { | ||
| return; | ||
| } | ||
| setQuery((prevQuery) => ({ | ||
| ...prevQuery, | ||
| ...queryForNode(node, prevQuery.storage), | ||
| })); | ||
| } | ||
|
|
||
| function updateQuerySql(sql: string) { | ||
|
|
@@ -230,16 +287,23 @@ function QueryDisplay(props: { | |
|
|
||
| // The first thing still needed before the query can run, if any. It's | ||
| // flagged on its field and explains why Execute is disabled. | ||
| // Searching nodes also sets the node's storage, so only the other ways of | ||
| // picking a host ask for one. | ||
| const missing = | ||
| (query.clusterless || byStorage) && !query.storage | ||
| ? "storage" | ||
| : !query.host || !query.port | ||
| ? "host" | ||
| : !query.storage | ||
| ? "storage" | ||
| : !query.sql | ||
| ? "query" | ||
| : null; | ||
| const missingHint = { | ||
| storage: "Select a storage to run the query.", | ||
| host: query.clusterless | ||
| ? "Enter a host to run the query." | ||
| : "Select a host to run the query.", | ||
| query: "Write or pick a query to run.", | ||
| }; | ||
| // What to flag on the page; Execute stays disabled either way. | ||
|
|
@@ -292,22 +356,24 @@ function QueryDisplay(props: { | |
| </PrefillWarning> | ||
| )} | ||
| <div style={targetRowStyle}> | ||
| {(query.clusterless || byStorage) && ( | ||
| <div style={targetFieldStyle}> | ||
| <Text size="sm" fw={600}> | ||
| Storage | ||
| </Text> | ||
| <div style={flagged === "storage" ? needsInputStyle : undefined}> | ||
| <CustomSelect | ||
| value={query.storage || ""} | ||
| onChange={selectStorage} | ||
| name="storage" | ||
| options={nodeData.map((storage) => storage.storage_name)} | ||
| /> | ||
| </div> | ||
| {flagged === "storage" && ( | ||
| <p style={needsInputHintStyle}>{missingHint.storage}</p> | ||
| )} | ||
| </div> | ||
| )} | ||
| <div style={targetFieldStyle}> | ||
| {/* The manual-entry rules live in a tooltip so they don't | ||
| make the row taller. */} | ||
|
|
@@ -348,17 +414,39 @@ function QueryDisplay(props: { | |
| ) : ( | ||
| <> | ||
| <div style={flagged === "host" ? needsInputStyle : undefined}> | ||
| {byStorage ? ( | ||
| <CustomSelect | ||
| disabled={!query.storage} | ||
| value={ | ||
| query.host && query.port | ||
| ? `${query.host}:${query.port}` | ||
| : "" | ||
| } | ||
| onChange={selectStorageHost} | ||
| name="host" | ||
| options={getHostsForStorage(nodeData, query.storage)} | ||
| /> | ||
| ) : ( | ||
| <CustomSelect | ||
| value={ | ||
| query.host && query.port | ||
| ? `${query.host}:${query.port}` | ||
| : "" | ||
| } | ||
| onChange={selectNode} | ||
| name="host" | ||
| options={nodeOptions.map(nodeSelectItem)} | ||
| filter={(search, item) => | ||
| nodeMatchesSearch(search, item.value) | ||
| } | ||
| highlight={(search, value) => | ||
| nodeSearchMatch(search, value)?.indexes ?? null | ||
| } | ||
| sortOptions={(search) => | ||
| rankNodes(search, nodeOptions).map(nodeSelectItem) | ||
| } | ||
| /> | ||
| )} | ||
| </div> | ||
| {flagged === "host" && ( | ||
| <p style={needsInputHintStyle}>{missingHint.host}</p> | ||
|
|
@@ -372,10 +460,13 @@ function QueryDisplay(props: { | |
| </Text> | ||
| <SegmentedControl | ||
| aria-label="Host entry" | ||
| value={ | ||
| query.clusterless ? "manual" : byStorage ? "storage" : "search" | ||
| } | ||
| onChange={setHostEntry} | ||
| data={[ | ||
| { label: "Search nodes", value: "search" }, | ||
| { label: "By storage", value: "storage" }, | ||
| { label: "Manual", value: "manual" }, | ||
| ]} | ||
| /> | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By-storage host not restored
Medium Severity
Picking a storage in by-storage mode fills the first host in query state only, and never writes
select-host. Reload always restores that key throughqueryForNode, so an auto-filled host is lost, and an older search-mode host can overwrite the current storage.Additional Locations (1)
snuba/admin/static/clickhouse_queries/query_display.tsx#L170-L179Reviewed by Cursor Bugbot for commit b1d7c6d. Configure here.