Repository navigation
fix(app): let the Database query drawer use the span attribute skip index #3351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@hyperdx/app': patch | ||
| --- | ||
|
|
||
| fix: Let the Services > Database query drawer use the span attribute skip index instead of scanning the whole trace window |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,27 @@ | ||
| import { useMemo } from 'react'; | ||
| import { ColumnMeta } from '@hyperdx/common-utils/dist/clickhouse'; | ||
| import { tcFromSource } from '@hyperdx/common-utils/dist/core/metadata'; | ||
| import { escapeSqlString } from '@hyperdx/common-utils/dist/core/utils'; | ||
| import { SourceKind, TTraceSource } from '@hyperdx/common-utils/dist/types'; | ||
|
|
||
| import { useColumns, useJsonColumns } from './hooks/useMetadata'; | ||
|
|
||
| const COALESCE_FIELDS_LIMIT = 100; | ||
|
|
||
| // Ordered from highest to lowest precedence | ||
| const DB_STATEMENT_KEYS = ['db.query.text', 'db.statement']; | ||
|
|
||
| /** | ||
| * Where a skip index can be made to match a database statement. The OTel | ||
| * schemas index span attributes two ways depending on the ClickHouse version: | ||
| * newer ones an `<Name>AttributeItems` column of `key=value` strings, older | ||
| * ones `mapValues(<Name>Attributes)`. | ||
| */ | ||
| type DbStatementIndexHint = { | ||
| kind: 'items' | 'mapValues'; | ||
| column: string; | ||
| }; | ||
|
|
||
| // Helper function to format field access based on column type | ||
| function formatFieldAccess( | ||
| field: string, | ||
|
|
@@ -89,18 +104,9 @@ function getDefaults({ | |
| isAttributeFieldJSON?: boolean; | ||
| } = {}) { | ||
| const dbStatement = makeCoalescedFieldsAccessQuery( | ||
| [ | ||
| formatFieldAccess( | ||
| spanAttributeField, | ||
| 'db.query.text', | ||
| isAttributeFieldJSON, | ||
| ), | ||
| formatFieldAccess( | ||
| spanAttributeField, | ||
| 'db.statement', | ||
| isAttributeFieldJSON, | ||
| ), | ||
| ], | ||
| DB_STATEMENT_KEYS.map(key => | ||
| formatFieldAccess(spanAttributeField, key, isAttributeFieldJSON), | ||
| ), | ||
| isAttributeFieldJSON, | ||
| ); | ||
|
|
||
|
|
@@ -138,6 +144,77 @@ function getDefaults({ | |
|
|
||
| const ENDPOINT_MATERIALIZED_COLUMN_NAME = 'endpoint'; | ||
|
|
||
| function getAttributeItemsColumn( | ||
| attributesField: string, | ||
| columns: ColumnMeta[], | ||
| ): string | undefined { | ||
|
Comment on lines
+147
to
+150
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The added helpers grow Context Used: AGENTS.md (source) Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| const itemsColumn = attributesField.replace(/Attributes$/, 'AttributeItems'); | ||
| const exists = | ||
| itemsColumn !== attributesField && | ||
| columns.some(column => column.name === itemsColumn); | ||
| return exists ? itemsColumn : undefined; | ||
|
Comment on lines
+151
to
+155
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Use the existing |
||
| } | ||
|
|
||
| function getDbStatementIndexHint( | ||
| attributesField: string, | ||
| isAttributeFieldJSON: boolean, | ||
| columns: ColumnMeta[], | ||
| ): DbStatementIndexHint | undefined { | ||
| if (isAttributeFieldJSON) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const itemsColumn = getAttributeItemsColumn(attributesField, columns); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 major — The Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond |
||
| if (itemsColumn) { | ||
| return { kind: 'items', column: itemsColumn }; | ||
| } | ||
|
|
||
| const isMap = columns.some( | ||
| column => | ||
| column.name === attributesField && column.type.startsWith('Map('), | ||
| ); | ||
| return isMap ? { kind: 'mapValues', column: attributesField } : undefined; | ||
| } | ||
|
|
||
| function makeIndexPrefilter( | ||
| hint: DbStatementIndexHint, | ||
| statement: string, | ||
| ): string { | ||
| if (hint.kind === 'mapValues') { | ||
| return `has(mapValues(${hint.column}), '${escapeSqlString(statement)}')`; | ||
| } | ||
| return DB_STATEMENT_KEYS.map( | ||
| key => | ||
| `has(${hint.column}, '${escapeSqlString(`${key}=${statement}`)}')`, | ||
| ).join(' OR '); | ||
| } | ||
|
|
||
| /** | ||
| * Condition selecting the spans of a single database statement. | ||
| * | ||
| * No skip index covers the coalesced attribute lookup, so on its own it makes | ||
| * ClickHouse read every granule in the time range. The prefilter matches a | ||
| * superset of those spans in a form an index does cover; the coalesced | ||
| * comparison still decides the match, so results are unchanged. It is also | ||
| * cheaper where no index applies, since it short-circuits the coalesce. | ||
| */ | ||
| export function makeDbStatementCondition({ | ||
| expressions, | ||
| statement, | ||
| }: { | ||
| expressions: Pick< | ||
| ReturnType<typeof getExpressions>, | ||
| 'dbStatement' | 'dbStatementIndexHint' | ||
| >; | ||
| statement: string; | ||
| }): string { | ||
| const equality = `${expressions.dbStatement} IN ('${escapeSqlString(statement)}')`; | ||
| const hint = expressions.dbStatementIndexHint; | ||
| return hint | ||
| ? `(${makeIndexPrefilter(hint, statement)}) AND ${equality}` | ||
| : equality; | ||
| } | ||
|
|
||
| export function getExpressions( | ||
| source: TTraceSource, | ||
| columns: ColumnMeta[], | ||
|
|
@@ -172,6 +249,11 @@ export function getExpressions( | |
|
|
||
| // Database | ||
| dbStatement: defaults.dbStatement, | ||
| dbStatementIndexHint: getDbStatementIndexHint( | ||
| spanAttributeField, | ||
| isAttributeFieldJSON, | ||
| columns, | ||
| ), | ||
| }; | ||
|
|
||
| const auxExpressions = { | ||
|
|
||
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.
🔵 minor — Service filter is still built from an unescaped string
This PR escapes the statement, but the service condition it also edits still interpolates
serviceraw. A service name containing'or\produces broken SQL, and every tile in the drawer errors. Wrap it as'${escapeSqlString(service)}'(from@hyperdx/common-utils/dist/core/utils), asmakeDbStatementConditiondoes.Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond