Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/db-statement-index-prefilter.md
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
141 changes: 141 additions & 0 deletions packages/app/src/__tests__/serviceDashboard.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { ColumnMeta } from '@hyperdx/common-utils/dist/clickhouse';
import { parseQuery } from '@hyperdx/common-utils/dist/filters';
import type { TTraceSource } from '@hyperdx/common-utils/dist/types';
import { SourceKind } from '@hyperdx/common-utils/dist/types';
import { renderHook } from '@testing-library/react';
Expand All @@ -7,6 +8,7 @@ import * as metadataModule from '@/hooks/useMetadata';
import {
getExpressions,
makeCoalescedFieldsAccessQuery,
makeDbStatementCondition,
useServiceDashboardExpressions,
} from '@/serviceDashboard';

Expand Down Expand Up @@ -146,6 +148,145 @@ describe('Service Dashboard', () => {
});
});

describe('dbStatementIndexHint', () => {
const attributesColumn = {
name: 'SpanAttributes',
type: 'Map(LowCardinality(String), String)',
} as ColumnMeta;
const itemsColumn = { name: 'SpanAttributeItems' } as ColumnMeta;

it('should prefer the attribute items column when the table has one', () => {
expect(
getExpressions(mockSource, [attributesColumn, itemsColumn], [])
.dbStatementIndexHint,
).toEqual({ kind: 'items', column: 'SpanAttributeItems' });
});

it('should fall back to the attribute map, which older schemas index by value', () => {
expect(
getExpressions(mockSource, [attributesColumn], []).dbStatementIndexHint,
).toEqual({ kind: 'mapValues', column: 'SpanAttributes' });
});

it('should be undefined when the attribute field is not a map', () => {
expect(
getExpressions(
mockSource,
[{ name: 'SpanAttributes', type: 'String' } as ColumnMeta],
[],
).dbStatementIndexHint,
).toBeUndefined();
});

it('should be undefined for JSON attribute columns', () => {
expect(
getExpressions(
mockSource,
[attributesColumn, itemsColumn],
['SpanAttributes'],
).dbStatementIndexHint,
).toBeUndefined();
});

it('should not mistake the attribute field itself for an items column', () => {
expect(
getExpressions(
{ ...mockSource, eventAttributesExpression: 'Attrs' },
[{ name: 'Attrs', type: 'Map(String, String)' } as ColumnMeta],
[],
).dbStatementIndexHint,
).toEqual({ kind: 'mapValues', column: 'Attrs' });
});
});

describe('makeDbStatementCondition', () => {
const dbStatement =
"coalesce(nullif(SpanAttributes['db.query.text'], ''), nullif(SpanAttributes['db.statement'], ''))";
const equality = `${dbStatement} IN ('SELECT 1')`;

it('should only compare the statement when nothing is indexed', () => {
expect(
makeDbStatementCondition({
expressions: { dbStatement, dbStatementIndexHint: undefined },
statement: 'SELECT 1',
}),
).toBe(equality);
});

it('should prefilter on the items column, which newer schemas index', () => {
expect(
makeDbStatementCondition({
expressions: {
dbStatement,
dbStatementIndexHint: {
kind: 'items',
column: 'SpanAttributeItems',
},
},
statement: 'SELECT 1',
}),
).toBe(
"(has(SpanAttributeItems, 'db.query.text=SELECT 1') OR " +
"has(SpanAttributeItems, 'db.statement=SELECT 1')) " +
`AND ${equality}`,
);
});

it('should prefilter on the attribute values, which older schemas index', () => {
expect(
makeDbStatementCondition({
expressions: {
dbStatement,
dbStatementIndexHint: {
kind: 'mapValues',
column: 'SpanAttributes',
},
},
statement: 'SELECT 1',
}),
).toBe(
`(has(mapValues(SpanAttributes), 'SELECT 1')) AND ${equality}`,
);
});

it.each([
['items' as const, 'SpanAttributeItems'],
['mapValues' as const, 'SpanAttributes'],
])('should escape quotes in the %s prefilter', (kind, column) => {
const condition = makeDbStatementCondition({
expressions: { dbStatement, dbStatementIndexHint: { kind, column } },
statement: "SELECT 'a'",
});

expect(condition).toContain("SELECT ''a''");
expect(condition).not.toContain("SELECT 'a'");
});

it.each([
[undefined],
[{ kind: 'items' as const, column: 'SpanAttributeItems' }],
[{ kind: 'mapValues' as const, column: 'SpanAttributes' }],
])(
'should be read back by the search page as the same filter (%p)',
dbStatementIndexHint => {
const statement = "SELECT 'a' FROM t WHERE b IN (1) AND c = ?";

const { filters } = parseQuery([
{
type: 'sql',
condition: makeDbStatementCondition({
expressions: { dbStatement, dbStatementIndexHint },
statement,
}),
},
]);

expect(Object.keys(filters)).toEqual([dbStatement]);
expect([...filters[dbStatement].included]).toEqual([statement]);
},
);
});

describe('useServiceDashboardExpressions', () => {
const mockColumns: ColumnMeta[] = [
{ name: 'Duration', type: 'UInt64' },
Expand Down
16 changes: 13 additions & 3 deletions packages/app/src/components/ServiceDashboardDbQuerySidePanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ import { ChartCard } from '@/components/charts/ChartCard';
import { DBTimeChart } from '@/components/DBTimeChart';
import { DrawerBody, DrawerHeader } from '@/components/DrawerUtils';
import SlowestEventsTile from '@/components/ServiceDashboardSlowestEventsTile';
import { useServiceDashboardExpressions } from '@/serviceDashboard';
import {
makeDbStatementCondition,
useServiceDashboardExpressions,
} from '@/serviceDashboard';
import { useSource } from '@/source';
import { useZIndex, ZIndexContext } from '@/zIndex';

Expand Down Expand Up @@ -46,16 +49,23 @@ export default function ServiceDashboardDbQuerySidePanel({
const drawerZIndex = contextZIndex + 10;

const dbQueryFilters = useMemo(() => {
if (!expressions || !dbQuery) {
return [];
}

const filters: Filter[] = [
{
type: 'sql',
condition: `${expressions?.dbStatement} IN ('${dbQuery}')`,
condition: makeDbStatementCondition({
expressions,
statement: dbQuery,
}),
},
];
if (service) {
filters.push({
type: 'sql',
condition: `${expressions?.service} IN ('${service}')`,
condition: `${expressions.service} IN ('${service}')`,

Copy link
Copy Markdown
Contributor

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 service raw. 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), as makeDbStatementCondition does.

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

});
}
return filters;
Expand Down
106 changes: 94 additions & 12 deletions packages/app/src/serviceDashboard.ts
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,
Expand Down Expand Up @@ -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,
);

Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Dashboard file exceeds its limit

The added helpers grow serviceDashboard.ts to 308 lines. The repository guide requires files to stay under 300 lines, with an exception only for tests. Move the new statement-condition helpers into a sibling module to satisfy this requirement before merging.

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!

Fix in Claude Code Fix in Conductor Fix in Cursor Fix in Codex

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Custom query details disappear

getAttributeItemsColumn selects a column by name alone, but makeIndexPrefilter assumes its values use = and come from the selected attribute map. If a custom SpanAttributeItems column uses :, it stores db.statement:SELECT 1. The new condition looks for db.statement=SELECT 1 and removes matching spans from every drawer query.

Use the existing parseKvItemsExpression and parseKvItemsCastExpression helpers to check the source map and separator before selecting the column, or fall back to mapValues.

Fix in Claude Code Fix in Conductor Fix in Cursor Fix in Codex

}

function getDbStatementIndexHint(
attributesField: string,
isAttributeFieldJSON: boolean,
columns: ColumnMeta[],
): DbStatementIndexHint | undefined {
if (isAttributeFieldJSON) {
return undefined;
}

const itemsColumn = getAttributeItemsColumn(attributesField, columns);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 major — items prefilter skips the version, column-kind and index checks that the existing KV-items rewrite applies

The items branch is picked whenever a column named <X>AttributeItems exists. It does not check the server version, whether the column is an ALIAS/MATERIALIZED key=value column, or whether it has a text(tokenizer='array') index. populateValidKvTextIndices in packages/common-utils/src/queryParser.ts:1239 does check these, and supportsDirectReadMap in core/clickhouseVersion.ts:121 deliberately turns has(<ALIAS items>, …) off on 26.2 < 26.2.19.43, 26.3 < 26.3.12.3 and 26.4 < 26.4.3.37. On those versions an ALIAS items column has no direct_read, so ClickHouse recomputes the arrayMap for every row. The stock traces schema can exist there, since 26.2 can create it, but the PR only measured 26.8 and 26.1. The separator is also hardcoded as = instead of being parsed from the column expression. Fix: drop getAttributeItemsColumn and the items kind. Emit the prefilter as (SpanAttributes['db.query.text'] = '…' OR SpanAttributes['db.statement'] = '…') and let rewriteSqlFilterWithKvItems (core/renderChartConfig.ts:391, already run on every sql filter in renderWhere) turn it into has(items, concat(…)) only when the lookup allows it. That also picks up hasAny on 26.5+. Keep the mapValues form only for schemas without a KV items index.

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[],
Expand Down Expand Up @@ -172,6 +249,11 @@ export function getExpressions(

// Database
dbStatement: defaults.dbStatement,
dbStatementIndexHint: getDbStatementIndexHint(
spanAttributeField,
isAttributeFieldJSON,
columns,
),
};

const auxExpressions = {
Expand Down
Loading