From fb0c73625ef80b7bc63835e7899e37ef2acf99b7 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:08:17 -0700 Subject: [PATCH 01/12] fix(llm): use mapContains for attribute-presence filters The dashboard tested attribute presence with SpanAttributes['key'] != '', which no skip index can serve. ClickHouse's text index condition supports mapContainsKey and has() over a mapKeys() index, and a bloom_filter over mapKeys() answers the same shape; != '' normalizes to notEmpty(), which neither supports. Every tile scanned all granules as a result. Map subscripts are also subcolumn references, so on ClickHouse 26.3+ each distinct key adds a per-part size lookup during PREWHERE planning. Measured on a staging SharedMergeTree cluster, 162 active parts, EXPLAIN only so no rows are read: 10-key detection predicate, subcolumn sizes on old 36,111 ms 1,480 S3 GETs TIMEOUT_EXCEEDED before start new 7 ms 0 S3 GETs 2-key predicate over 6h, granules surviving the skip index old 1,306 granules, no Skip section in the plan new 3 granules via idx_span_attr_key Both forms return identical counts over 7 days (66 = 66, 248M rows scanned), so the presence-vs-non-empty difference is theoretical on real instrumentation. Value expressions still read subscripts; they need the values. JSON attribute columns keep the comparison, since their paths are real subcolumns and mapContains does not apply. Time-to-first-token keeps its > 0 check because it feeds a latency percentile that a zero would skew, and gains a presence conjunct only so the index can prune. --- .changeset/great-hoops-shave.md | 20 +++++++ packages/app/src/llm/__tests__/detect.test.ts | 14 ++++- .../app/src/llm/__tests__/expressions.test.ts | 33 ++++++++--- packages/app/src/llm/lib/detect.ts | 56 ++++++++++++++++--- packages/app/src/llm/lib/expressions.ts | 41 +++++++++----- 5 files changed, 133 insertions(+), 31 deletions(-) create mode 100644 .changeset/great-hoops-shave.md diff --git a/.changeset/great-hoops-shave.md b/.changeset/great-hoops-shave.md new file mode 100644 index 0000000000..9d2b859e51 --- /dev/null +++ b/.changeset/great-hoops-shave.md @@ -0,0 +1,20 @@ +--- +'@hyperdx/app': patch +--- + +fix: use mapContains for LLM dashboard attribute-presence filters + +The LLM dashboard tested attribute presence with `SpanAttributes['key'] != ''`, +which no skip index can serve — the trace schema's `mapKeys(SpanAttributes)` +index only answers `mapContains`, and `!= ''` normalizes to `notEmpty()`. Every +tile therefore scanned all granules. Map subscripts are also subcolumn +references, so on ClickHouse 26.3+ each one adds a per-part size lookup during +PREWHERE planning. + +Presence checks now use `mapContains`, which the index serves and which costs no +per-part lookups. Value expressions are unchanged. JSON attribute columns keep +the previous form, since their paths are real subcolumns. + +Detection is now presence-based rather than presence-and-non-empty: an attribute +explicitly set to an empty string counts as present. Time-to-first-token keeps +its `> 0` comparison so zero-valued rows stay out of the latency percentiles. diff --git a/packages/app/src/llm/__tests__/detect.test.ts b/packages/app/src/llm/__tests__/detect.test.ts index 783c538528..d035de414c 100644 --- a/packages/app/src/llm/__tests__/detect.test.ts +++ b/packages/app/src/llm/__tests__/detect.test.ts @@ -44,19 +44,26 @@ describe('isLLMSpan', () => { }); describe('buildLLMSpanSqlPredicate', () => { - it('builds a map-column predicate', () => { + // mapContains, not `col['k'] != ''`: only the former can be served by a + // mapKeys() skip index, and it is not a subcolumn reference so it stays out + // of the per-part size lookups PREWHERE planning does on CH 26.3+. + it('builds a map-column predicate with mapContains', () => { const predicate = buildLLMSpanSqlPredicate({ attributeField: 'SpanAttributes', isJsonColumn: false, }); expect(predicate).toContain( - "SpanAttributes['gen_ai.operation.name'] != ''", + "mapContains(SpanAttributes, 'gen_ai.operation.name')", ); - expect(predicate).toContain("SpanAttributes['llm.model_name'] != ''"); + expect(predicate).toContain( + "mapContains(SpanAttributes, 'llm.model_name')", + ); + expect(predicate).not.toContain("!= ''"); expect(predicate.startsWith('(')).toBe(true); expect(predicate.endsWith(')')).toBe(true); }); + // JSON paths are real subcolumns; mapContains does not apply to them. it('builds a JSON-column predicate with backtick paths', () => { const predicate = buildLLMSpanSqlPredicate({ attributeField: 'SpanAttributes', @@ -65,5 +72,6 @@ describe('buildLLMSpanSqlPredicate', () => { expect(predicate).toContain( "toString(SpanAttributes.`gen_ai.operation.name`) != ''", ); + expect(predicate).not.toContain('mapContains'); }); }); diff --git a/packages/app/src/llm/__tests__/expressions.test.ts b/packages/app/src/llm/__tests__/expressions.test.ts index 3f04ce58d5..93964d2101 100644 --- a/packages/app/src/llm/__tests__/expressions.test.ts +++ b/packages/app/src/llm/__tests__/expressions.test.ts @@ -30,7 +30,7 @@ describe('getLLMExpressions', () => { `${expressions.effectiveInputTokens} + ${expressions.outputTokens}`, ); expect(expressions.isLLMSpan).toContain( - "SpanAttributes['gen_ai.operation.name'] != ''", + "mapContains(SpanAttributes, 'gen_ai.operation.name')", ); expect(expressions.isError).toBe("lower(StatusCode) = 'error'"); expect(expressions.durationInMillis).toBe('Duration/1e6'); @@ -51,6 +51,9 @@ describe('getLLMExpressions', () => { expect(expressions.isLLMSpan).toContain( "toString(SpanAttributes.`gen_ai.operation.name`) != ''", ); + // mapContains does not apply to JSON paths. + expect(expressions.isLLMSpan).not.toContain('mapContains'); + expect(expressions.hasReportedTokens).not.toContain('mapContains'); }); it('derives session id and reported-token gate expressions', () => { @@ -65,15 +68,21 @@ describe('getLLMExpressions', () => { expect(expressions.sessionId).toContain( "nullif(SpanAttributes['ai.telemetry.metadata.sessionId'], '')", ); - expect(expressions.hasSessionId).toContain("!= ''"); + // The gate tests key presence so a mapKeys() index can serve it; the + // value expression above still coalesces. + expect(expressions.hasSessionId).toBe( + "(mapContains(SpanAttributes, 'gen_ai.conversation.id') OR " + + "mapContains(SpanAttributes, 'session.id') OR " + + "mapContains(SpanAttributes, 'ai.telemetry.metadata.sessionId'))", + ); // The gate keys on authoritative usage reporters only — wrapper spans // carrying just ai.usage.* must not satisfy it. expect(expressions.hasReportedTokens).toContain( - "SpanAttributes['gen_ai.usage.input_tokens'] != ''", + "mapContains(SpanAttributes, 'gen_ai.usage.input_tokens')", ); expect(expressions.hasReportedTokens).toContain( - "SpanAttributes['llm.token_count.total'] != ''", + "mapContains(SpanAttributes, 'llm.token_count.total')", ); expect(expressions.hasReportedTokens).not.toContain('ai.usage.inputTokens'); }); @@ -126,7 +135,12 @@ describe('getLLMExpressions', () => { expect(expressions.ttftMs).toContain( "SpanAttributes['copilot_chat.time_to_first_token']", ); + // Keeps its value comparison (a zero TTFT would skew the percentiles) and + // adds a presence conjunct purely so the key index can prune. expect(expressions.hasTtft).toContain('> 0'); + expect(expressions.hasTtft).toContain( + "mapContains(SpanAttributes, 'ttft_ms')", + ); // Tool name coalesce includes the flat form. expect(expressions.toolName).toContain("SpanAttributes['tool_name']"); @@ -136,7 +150,10 @@ describe('getLLMExpressions', () => { "SpanAttributes['gen_ai.agent.name']", ); expect(expressions.agentName).toContain("SpanAttributes['agent.name']"); - expect(expressions.hasAgentName).toContain("!= ''"); + expect(expressions.hasAgentName).toBe( + "(mapContains(SpanAttributes, 'gen_ai.agent.name') OR " + + "mapContains(SpanAttributes, 'agent.name'))", + ); // Finish reasons normalized out of their JSON-array encoding. expect(expressions.finishReason).toMatch(/^replaceRegexpAll\(/); @@ -145,7 +162,9 @@ describe('getLLMExpressions', () => { // User attribution coalesce. expect(expressions.userId).toContain("SpanAttributes['user.email']"); expect(expressions.userId).toContain("SpanAttributes['enduser.id']"); - expect(expressions.hasUserId).toContain("!= ''"); + expect(expressions.hasUserId).toContain( + "mapContains(SpanAttributes, 'user.email')", + ); expect(expressions.statusMessage).toBe('StatusMessage'); }); @@ -247,7 +266,7 @@ describe('getLLMLogExpressions', () => { "nullif(LogAttributes['session.id'], '')", ); expect(expressions.isLLMSpan).toContain( - "LogAttributes['gen_ai.provider.name'] != ''", + "mapContains(LogAttributes, 'gen_ai.provider.name')", ); // LLM-related = LLM markers OR any session id, so tool_result / // lifecycle log events that only carry session.id are included. diff --git a/packages/app/src/llm/lib/detect.ts b/packages/app/src/llm/lib/detect.ts index d658ad9627..48b02dd3c5 100644 --- a/packages/app/src/llm/lib/detect.ts +++ b/packages/app/src/llm/lib/detect.ts @@ -81,10 +81,53 @@ export function isLLMSpan( return (events ?? []).some(event => event.name?.startsWith('gen_ai.')); } +/** + * SQL testing whether an attribute key is present on a row. + * + * For map columns this is `mapContains`, not `col['key'] != ''`, for two + * reasons. It is one of the few shapes a `mapKeys(col)` skip index can serve + * (`MergeTreeIndexConditionText::isSupportedFunction`) — `!= ''` normalizes to + * `notEmpty()`, which no index supports, so it scans every granule. And a map + * subscript is a subcolumn reference, which on ClickHouse 26.3+ costs one + * per-part size lookup during PREWHERE planning; `mapContains` costs none. + * + * JSON columns keep the comparison: their paths are real subcolumns, and + * `mapContains` does not apply. + * + * Note this tests presence, where `!= ''` tested presence *and* non-emptiness. + * A key explicitly set to '' now counts as present. + */ +export function buildKeyExistsSql({ + attributeField, + key, + isJsonColumn, +}: { + attributeField: string; + key: string; + isJsonColumn: boolean; +}): string { + return isJsonColumn + ? `toString(${attributeField}.\`${key}\`) != ''` + : `mapContains(${attributeField}, '${key}')`; +} + +/** SQL matching rows carrying any of the given attribute keys. */ +export function buildAnyKeyExistsSql(args: { + attributeField: string; + keys: readonly string[]; + isJsonColumn: boolean; +}): string { + const { attributeField, keys, isJsonColumn } = args; + const conditions = keys.map(key => + buildKeyExistsSql({ attributeField, key, isJsonColumn }), + ); + return `(${conditions.join(' OR ')})`; +} + /** * Build a SQL predicate matching LLM spans, for use in search filters and * dashboard chart configs. Handles both `Map(String, String)` attribute - * columns (missing key reads as '') and JSON-typed columns. + * columns and JSON-typed columns. */ export function buildLLMSpanSqlPredicate({ attributeField, @@ -94,10 +137,9 @@ export function buildLLMSpanSqlPredicate({ attributeField: string; isJsonColumn: boolean; }): string { - const conditions = LLM_MARKER_ATTRIBUTE_KEYS.map(key => - isJsonColumn - ? `toString(${attributeField}.\`${key}\`) != ''` - : `${attributeField}['${key}'] != ''`, - ); - return `(${conditions.join(' OR ')})`; + return buildAnyKeyExistsSql({ + attributeField, + keys: LLM_MARKER_ATTRIBUTE_KEYS, + isJsonColumn, + }); } diff --git a/packages/app/src/llm/lib/expressions.ts b/packages/app/src/llm/lib/expressions.ts index 6b5494753a..e14b2ef9f3 100644 --- a/packages/app/src/llm/lib/expressions.ts +++ b/packages/app/src/llm/lib/expressions.ts @@ -1,7 +1,7 @@ import { TLogSource, TTraceSource } from '@hyperdx/common-utils/dist/types'; import { generateCostSqlExpression } from './cost'; -import { buildLLMSpanSqlPredicate } from './detect'; +import { buildAnyKeyExistsSql, buildLLMSpanSqlPredicate } from './detect'; /** * SQL expression derivation for the LLM dashboard, mirroring @@ -222,6 +222,10 @@ function getLLMAttributeExpressions({ attributeField: string; isJsonColumn: boolean; }) { + /** Rows carrying any of these keys. Index-friendly — see buildKeyExistsSql. */ + const anyKeyExists = (keys: readonly string[]) => + buildAnyKeyExistsSql({ attributeField, keys, isJsonColumn }); + const model = coalesceString(attributeField, MODEL_KEYS, isJsonColumn); const inputTokens = greatestNumber( attributeField, @@ -284,7 +288,7 @@ function getLLMAttributeExpressions({ ttftMs: greatestNumber(attributeField, TTFT_MS_KEYS, isJsonColumn), toolName: coalesceString(attributeField, TOOL_NAME_KEYS, isJsonColumn), agentName: coalesceString(attributeField, AGENT_NAME_KEYS, isJsonColumn), - hasAgentName: `${coalesceString(attributeField, AGENT_NAME_KEYS, isJsonColumn)} != ''`, + hasAgentName: anyKeyExists(AGENT_NAME_KEYS), // Emitters disagree on encoding ('stop' vs '["stop"]'); strip the JSON // array wrapper so the group-by buckets align. // The char class is written backslash-free (leading ] in an RE2 class @@ -312,20 +316,29 @@ function getLLMAttributeExpressions({ */ hasProvidedCost: `(${providedCost} > 0)`, /** See REPORTED_TOKEN_KEYS: gates sums so wrapper spans don't double count. */ - hasReportedTokens: `(${REPORTED_TOKEN_KEYS.map(key => - isJsonColumn - ? `toString(${attributeField}.\`${key}\`) != ''` - : `${attributeField}['${key}'] != ''`, - ).join(' OR ')})`, - hasSessionId: `${coalesceString(attributeField, SESSION_ID_KEYS, isJsonColumn)} != ''`, - hasTtft: `${greatestNumber(attributeField, TTFT_MS_KEYS, isJsonColumn)} > 0`, - hasUserId: `${coalesceString(attributeField, USER_ID_KEYS, isJsonColumn)} != ''`, - hasFinishReason: `${coalesceString(attributeField, FINISH_REASON_KEYS, isJsonColumn)} != ''`, + hasReportedTokens: anyKeyExists(REPORTED_TOKEN_KEYS), + hasSessionId: anyKeyExists(SESSION_ID_KEYS), + // Unlike the other gates this keeps its value comparison: it feeds a + // latency percentile, and a zero would drag p50/p95 down. The presence + // conjunct is redundant for correctness and there only to give the + // attribute-key index something to prune on. + hasTtft: `(${anyKeyExists(TTFT_MS_KEYS)} AND ${greatestNumber( + attributeField, + TTFT_MS_KEYS, + isJsonColumn, + )} > 0)`, + hasUserId: anyKeyExists(USER_ID_KEYS), + hasFinishReason: anyKeyExists(FINISH_REASON_KEYS), + // The `= 'TOOL'` term stays a value comparison — it discriminates one + // OpenInference span kind from the others, and the query builder already + // rewrites equality onto the attribute-items index. isToolSpan: `(${[ `${fieldAccess(attributeField, 'openinference.span.kind', isJsonColumn)} = 'TOOL'`, - `${fieldAccess(attributeField, 'gen_ai.tool.name', isJsonColumn)} != ''`, - `${fieldAccess(attributeField, 'gen_ai.tool.call.id', isJsonColumn)} != ''`, - `${fieldAccess(attributeField, 'ai.toolCall.name', isJsonColumn)} != ''`, + buildAnyKeyExistsSql({ + attributeField, + keys: ['gen_ai.tool.name', 'gen_ai.tool.call.id', 'ai.toolCall.name'], + isJsonColumn, + }), ].join(' OR ')})`, }; } From d3eb7ad5fb4e9fade159fcb24a30ce38088bcfcc Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:16:41 -0700 Subject: [PATCH 02/12] fix(llm): unexport buildKeyExistsSql Only used via buildAnyKeyExistsSql in this module; knip flags the unused export. --- packages/app/src/llm/lib/detect.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/app/src/llm/lib/detect.ts b/packages/app/src/llm/lib/detect.ts index 48b02dd3c5..d1917bc0d6 100644 --- a/packages/app/src/llm/lib/detect.ts +++ b/packages/app/src/llm/lib/detect.ts @@ -97,7 +97,7 @@ export function isLLMSpan( * Note this tests presence, where `!= ''` tested presence *and* non-emptiness. * A key explicitly set to '' now counts as present. */ -export function buildKeyExistsSql({ +function buildKeyExistsSql({ attributeField, key, isJsonColumn, From 1dc9a78b2ae983112ba70f92ebf95347517765a5 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:36:54 -0700 Subject: [PATCH 03/12] fix(llm): keep the non-empty check on gates that feed a group-by MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Converting these gates to bare mapContains let an attribute explicitly set to '' pass. Each one is paired with a coalesced value expression the dashboard groups by, so such a row surfaced as a blank bar or table row — and in SessionsTab the blank row's link carried an empty session id, so clicking it did nothing. Gates now emit presence AND non-empty: the presence term prunes granules via the mapKeys index, the value term preserves the meaning. This adds no per-part size lookups, since the value term reads the same keys the group-by expression already reads. On JSON columns presence and non-emptiness are the same test, so only the value term is emitted rather than doubling the subcolumn reads — this restores the exact pre-PR expression there. isLLMSpan stays presence-only: a span carrying gen_ai.system at all was emitted by LLM instrumentation whatever the value, and nothing groups by it. That is where the planning win comes from. Tests pin hasTtft and isToolSpan exactly, so swapping their AND for an OR (re-admitting zero-TTFT rows to the percentiles, or widening the tool charts to every OpenInference span) now fails. --- .changeset/great-hoops-shave.md | 22 +++++-- .../app/src/llm/__tests__/expressions.test.ts | 65 ++++++++++++++---- packages/app/src/llm/lib/detect.ts | 9 ++- packages/app/src/llm/lib/expressions.ts | 66 ++++++++++++------- 4 files changed, 121 insertions(+), 41 deletions(-) diff --git a/.changeset/great-hoops-shave.md b/.changeset/great-hoops-shave.md index 9d2b859e51..6b8d48b6ff 100644 --- a/.changeset/great-hoops-shave.md +++ b/.changeset/great-hoops-shave.md @@ -11,10 +11,20 @@ tile therefore scanned all granules. Map subscripts are also subcolumn references, so on ClickHouse 26.3+ each one adds a per-part size lookup during PREWHERE planning. -Presence checks now use `mapContains`, which the index serves and which costs no -per-part lookups. Value expressions are unchanged. JSON attribute columns keep -the previous form, since their paths are real subcolumns. +These filters now lead with `mapContains`, which the index serves and which +costs no per-part lookups. On a staging trace table the LLM span predicate went +from a 36s planning stall to 7ms, and a two-key filter dropped from 1,306 +granules to 3. -Detection is now presence-based rather than presence-and-non-empty: an attribute -explicitly set to an empty string counts as present. Time-to-first-token keeps -its `> 0` comparison so zero-valued rows stay out of the latency percentiles. +Gates that pair with a value expression the dashboard groups by keep their +non-empty check as a second conjunct, so an attribute set to `''` still cannot +appear as a blank row. The presence term prunes granules; the value term +preserves the meaning. It costs no extra per-part lookups, since it reads the +same keys the group-by already reads. + +The one behavior change is LLM span detection, which is now presence-based: a +span carrying `gen_ai.system` at all is treated as an LLM span whatever the +value. Nothing groups by that predicate. + +JSON attribute columns are unchanged — their paths are real subcolumns, there is +no key index to prune with, and a presence term would only duplicate reads. diff --git a/packages/app/src/llm/__tests__/expressions.test.ts b/packages/app/src/llm/__tests__/expressions.test.ts index 93964d2101..219a815dbd 100644 --- a/packages/app/src/llm/__tests__/expressions.test.ts +++ b/packages/app/src/llm/__tests__/expressions.test.ts @@ -54,6 +54,22 @@ describe('getLLMExpressions', () => { // mapContains does not apply to JSON paths. expect(expressions.isLLMSpan).not.toContain('mapContains'); expect(expressions.hasReportedTokens).not.toContain('mapContains'); + expect(expressions.hasSessionId).not.toContain('mapContains'); + expect(expressions.isToolSpan).not.toContain('mapContains'); + + // On JSON there is no key index to prune with, so the gates carry the + // value test alone rather than doubling their subcolumn reads with a + // presence term that says the same thing. + expect(expressions.hasTtft).toBe( + 'greatest(toFloat64OrZero(toString(SpanAttributes.`ttft_ms`)), ' + + 'toFloat64OrZero(toString(SpanAttributes.`ai.response.msToFirstChunk`)), ' + + 'toFloat64OrZero(toString(SpanAttributes.`copilot_chat.time_to_first_token`))) > 0', + ); + expect(expressions.hasSessionId).toBe( + "coalesce(nullif(toString(SpanAttributes.`gen_ai.conversation.id`), ''), " + + "nullif(toString(SpanAttributes.`session.id`), ''), " + + "nullif(toString(SpanAttributes.`ai.telemetry.metadata.sessionId`), ''), '') != ''", + ); }); it('derives session id and reported-token gate expressions', () => { @@ -68,12 +84,16 @@ describe('getLLMExpressions', () => { expect(expressions.sessionId).toContain( "nullif(SpanAttributes['ai.telemetry.metadata.sessionId'], '')", ); - // The gate tests key presence so a mapKeys() index can serve it; the - // value expression above still coalesces. + // Presence terms for the index, value term for the meaning: a key set to + // '' must not pass, or SessionsTab groups it as a blank row whose link + // carries no session id. expect(expressions.hasSessionId).toBe( - "(mapContains(SpanAttributes, 'gen_ai.conversation.id') OR " + + "((mapContains(SpanAttributes, 'gen_ai.conversation.id') OR " + "mapContains(SpanAttributes, 'session.id') OR " + - "mapContains(SpanAttributes, 'ai.telemetry.metadata.sessionId'))", + "mapContains(SpanAttributes, 'ai.telemetry.metadata.sessionId')) AND " + + "coalesce(nullif(SpanAttributes['gen_ai.conversation.id'], ''), " + + "nullif(SpanAttributes['session.id'], ''), " + + "nullif(SpanAttributes['ai.telemetry.metadata.sessionId'], ''), '') != '')", ); // The gate keys on authoritative usage reporters only — wrapper spans @@ -135,11 +155,16 @@ describe('getLLMExpressions', () => { expect(expressions.ttftMs).toContain( "SpanAttributes['copilot_chat.time_to_first_token']", ); - // Keeps its value comparison (a zero TTFT would skew the percentiles) and - // adds a presence conjunct purely so the key index can prune. - expect(expressions.hasTtft).toContain('> 0'); - expect(expressions.hasTtft).toContain( - "mapContains(SpanAttributes, 'ttft_ms')", + // Pinned exactly: the terms must be ANDed. With OR, zero-TTFT rows reach + // the p50/p95 the value check exists to keep them out of, and a + // toContain-style assertion would not notice. + expect(expressions.hasTtft).toBe( + "((mapContains(SpanAttributes, 'ttft_ms') OR " + + "mapContains(SpanAttributes, 'ai.response.msToFirstChunk') OR " + + "mapContains(SpanAttributes, 'copilot_chat.time_to_first_token')) AND " + + "greatest(toFloat64OrZero(SpanAttributes['ttft_ms']), " + + "toFloat64OrZero(SpanAttributes['ai.response.msToFirstChunk']), " + + "toFloat64OrZero(SpanAttributes['copilot_chat.time_to_first_token'])) > 0)", ); // Tool name coalesce includes the flat form. @@ -151,8 +176,10 @@ describe('getLLMExpressions', () => { ); expect(expressions.agentName).toContain("SpanAttributes['agent.name']"); expect(expressions.hasAgentName).toBe( - "(mapContains(SpanAttributes, 'gen_ai.agent.name') OR " + - "mapContains(SpanAttributes, 'agent.name'))", + "((mapContains(SpanAttributes, 'gen_ai.agent.name') OR " + + "mapContains(SpanAttributes, 'agent.name')) AND " + + "coalesce(nullif(SpanAttributes['gen_ai.agent.name'], ''), " + + "nullif(SpanAttributes['agent.name'], ''), '') != '')", ); // Finish reasons normalized out of their JSON-array encoding. @@ -165,6 +192,22 @@ describe('getLLMExpressions', () => { expect(expressions.hasUserId).toContain( "mapContains(SpanAttributes, 'user.email')", ); + // The value term keeps a blank user out of the Top Users group-by. + expect(expressions.hasUserId).toContain( + "AND coalesce(nullif(SpanAttributes['user.email'], '')", + ); + + // The span-kind term must stay an equality: widening it to a presence + // check would pull every OpenInference span into the tool charts. + expect(expressions.isToolSpan).toBe( + "(SpanAttributes['openinference.span.kind'] = 'TOOL' OR " + + "((mapContains(SpanAttributes, 'gen_ai.tool.name') OR " + + "mapContains(SpanAttributes, 'gen_ai.tool.call.id') OR " + + "mapContains(SpanAttributes, 'ai.toolCall.name')) AND " + + "coalesce(nullif(SpanAttributes['gen_ai.tool.name'], ''), " + + "nullif(SpanAttributes['gen_ai.tool.call.id'], ''), " + + "nullif(SpanAttributes['ai.toolCall.name'], ''), '') != ''))", + ); expect(expressions.statusMessage).toBe('StatusMessage'); }); diff --git a/packages/app/src/llm/lib/detect.ts b/packages/app/src/llm/lib/detect.ts index d1917bc0d6..2d868f08bd 100644 --- a/packages/app/src/llm/lib/detect.ts +++ b/packages/app/src/llm/lib/detect.ts @@ -94,8 +94,9 @@ export function isLLMSpan( * JSON columns keep the comparison: their paths are real subcolumns, and * `mapContains` does not apply. * - * Note this tests presence, where `!= ''` tested presence *and* non-emptiness. - * A key explicitly set to '' now counts as present. + * This tests presence only. Callers that pair a gate with a value expression + * they group by need non-emptiness as well, or a key set to '' becomes a blank + * row — see `anyKeyHasValue` in expressions.ts. */ function buildKeyExistsSql({ attributeField, @@ -128,6 +129,10 @@ export function buildAnyKeyExistsSql(args: { * Build a SQL predicate matching LLM spans, for use in search filters and * dashboard chart configs. Handles both `Map(String, String)` attribute * columns and JSON-typed columns. + * + * Presence is the right test here: a span carrying `gen_ai.system` at all was + * emitted by LLM instrumentation, whatever the value. Nothing groups by this + * predicate, so an empty value cannot produce a blank row. */ export function buildLLMSpanSqlPredicate({ attributeField, diff --git a/packages/app/src/llm/lib/expressions.ts b/packages/app/src/llm/lib/expressions.ts index e14b2ef9f3..d714956f06 100644 --- a/packages/app/src/llm/lib/expressions.ts +++ b/packages/app/src/llm/lib/expressions.ts @@ -21,7 +21,7 @@ function fieldAccess( /** First non-empty string among the given attribute keys ('' when none). */ function coalesceString( field: string, - keys: string[], + keys: readonly string[], isJsonColumn: boolean, ): string { const args = keys.map( @@ -37,7 +37,7 @@ function coalesceString( */ function greatestNumber( field: string, - keys: string[], + keys: readonly string[], isJsonColumn: boolean, ): string { const args = keys.map( @@ -226,6 +226,27 @@ function getLLMAttributeExpressions({ const anyKeyExists = (keys: readonly string[]) => buildAnyKeyExistsSql({ attributeField, keys, isJsonColumn }); + /** + * Gate for "any of these keys holds a non-empty value". + * + * The presence term is there so a mapKeys() index can prune granules; the + * value term is what preserves the meaning. Both are needed: presence alone + * would admit a key explicitly set to '', and every caller pairs this gate + * with a coalesced value expression it groups by, so such a row would show + * up as a blank bar/row whose drill-in link goes nowhere. + * + * The value term costs no extra per-part size lookups during planning: it + * reads the same keys the paired group-by expression already reads. + * + * On JSON columns presence and non-emptiness are the same test, so the + * presence term would only duplicate subcolumn reads — emit the value term + * alone. + */ + const anyKeyHasValue = (keys: readonly string[]) => { + const hasValue = `${coalesceString(attributeField, keys, isJsonColumn)} != ''`; + return isJsonColumn ? hasValue : `(${anyKeyExists(keys)} AND ${hasValue})`; + }; + const model = coalesceString(attributeField, MODEL_KEYS, isJsonColumn); const inputTokens = greatestNumber( attributeField, @@ -288,7 +309,7 @@ function getLLMAttributeExpressions({ ttftMs: greatestNumber(attributeField, TTFT_MS_KEYS, isJsonColumn), toolName: coalesceString(attributeField, TOOL_NAME_KEYS, isJsonColumn), agentName: coalesceString(attributeField, AGENT_NAME_KEYS, isJsonColumn), - hasAgentName: anyKeyExists(AGENT_NAME_KEYS), + hasAgentName: anyKeyHasValue(AGENT_NAME_KEYS), // Emitters disagree on encoding ('stop' vs '["stop"]'); strip the JSON // array wrapper so the group-by buckets align. // The char class is written backslash-free (leading ] in an RE2 class @@ -316,29 +337,30 @@ function getLLMAttributeExpressions({ */ hasProvidedCost: `(${providedCost} > 0)`, /** See REPORTED_TOKEN_KEYS: gates sums so wrapper spans don't double count. */ - hasReportedTokens: anyKeyExists(REPORTED_TOKEN_KEYS), - hasSessionId: anyKeyExists(SESSION_ID_KEYS), - // Unlike the other gates this keeps its value comparison: it feeds a - // latency percentile, and a zero would drag p50/p95 down. The presence - // conjunct is redundant for correctness and there only to give the - // attribute-key index something to prune on. - hasTtft: `(${anyKeyExists(TTFT_MS_KEYS)} AND ${greatestNumber( - attributeField, - TTFT_MS_KEYS, - isJsonColumn, - )} > 0)`, - hasUserId: anyKeyExists(USER_ID_KEYS), - hasFinishReason: anyKeyExists(FINISH_REASON_KEYS), + hasReportedTokens: anyKeyHasValue(REPORTED_TOKEN_KEYS), + hasSessionId: anyKeyHasValue(SESSION_ID_KEYS), + // Numeric rather than non-empty: it feeds a latency percentile, and a + // zero would drag p50/p95 down. Same presence-plus-value shape as + // anyKeyHasValue, including skipping the presence term on JSON. + hasTtft: (() => { + const isPositive = `${greatestNumber(attributeField, TTFT_MS_KEYS, isJsonColumn)} > 0`; + return isJsonColumn + ? isPositive + : `(${anyKeyExists(TTFT_MS_KEYS)} AND ${isPositive})`; + })(), + hasUserId: anyKeyHasValue(USER_ID_KEYS), + hasFinishReason: anyKeyHasValue(FINISH_REASON_KEYS), // The `= 'TOOL'` term stays a value comparison — it discriminates one // OpenInference span kind from the others, and the query builder already - // rewrites equality onto the attribute-items index. + // rewrites equality onto the attribute-items index. The remaining terms + // gate a group-by on toolName, so they keep their value check too. isToolSpan: `(${[ `${fieldAccess(attributeField, 'openinference.span.kind', isJsonColumn)} = 'TOOL'`, - buildAnyKeyExistsSql({ - attributeField, - keys: ['gen_ai.tool.name', 'gen_ai.tool.call.id', 'ai.toolCall.name'], - isJsonColumn, - }), + anyKeyHasValue([ + 'gen_ai.tool.name', + 'gen_ai.tool.call.id', + 'ai.toolCall.name', + ]), ].join(' OR ')})`, }; } From ffb508d56dc9248362dd3f59003f379f500c8409 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:51:13 -0700 Subject: [PATCH 04/12] perf(llm): hint the pruning terms instead of re-evaluating them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The presence terms added in the previous commit are implied by the value terms they sit beside — a non-empty value requires the key — so they can never change the result set and exist only to give idx_span_attr_key something to prune on. As plain AND conjuncts ClickHouse still evaluated them for every surviving row: 9 extra mapContains calls per row for hasReportedTokens, which appears twice inside every gated aggregate. Wrapping them in indexHint keeps them in skip-index analysis without the per-row cost. This is the same idiom queryParser.ts already uses for mapKeyIndexExpression. isLLMSpan is deliberately not wrapped: there the presence check is the whole filter, and indexHint returns true at execution, so hinting it would match every span. anyKeyExists stays unwrapped for that reason and the wrapper is applied at the call sites that pair it with a value term. Also pins hasReportedTokens and hasFinishReason, which were the two converted gates a presence-only revert could still slip past. --- .changeset/great-hoops-shave.md | 9 +++-- .../app/src/llm/__tests__/expressions.test.ts | 40 +++++++++++++++---- packages/app/src/llm/lib/expressions.ts | 36 +++++++++++------ 3 files changed, 61 insertions(+), 24 deletions(-) diff --git a/.changeset/great-hoops-shave.md b/.changeset/great-hoops-shave.md index 6b8d48b6ff..1e0069fa01 100644 --- a/.changeset/great-hoops-shave.md +++ b/.changeset/great-hoops-shave.md @@ -17,10 +17,11 @@ from a 36s planning stall to 7ms, and a two-key filter dropped from 1,306 granules to 3. Gates that pair with a value expression the dashboard groups by keep their -non-empty check as a second conjunct, so an attribute set to `''` still cannot -appear as a blank row. The presence term prunes granules; the value term -preserves the meaning. It costs no extra per-part lookups, since it reads the -same keys the group-by already reads. +non-empty check, so an attribute set to `''` still cannot appear as a blank row. +There the value term defines the result and the presence term is pruning only, +so it is wrapped in `indexHint` — it reaches skip-index analysis without being +re-evaluated per surviving row. The value term costs no extra per-part lookups, +since it reads the same keys the group-by already reads. The one behavior change is LLM span detection, which is now presence-based: a span carrying `gen_ai.system` at all is treated as an LLM span whatever the diff --git a/packages/app/src/llm/__tests__/expressions.test.ts b/packages/app/src/llm/__tests__/expressions.test.ts index 219a815dbd..629420103b 100644 --- a/packages/app/src/llm/__tests__/expressions.test.ts +++ b/packages/app/src/llm/__tests__/expressions.test.ts @@ -56,6 +56,9 @@ describe('getLLMExpressions', () => { expect(expressions.hasReportedTokens).not.toContain('mapContains'); expect(expressions.hasSessionId).not.toContain('mapContains'); expect(expressions.isToolSpan).not.toContain('mapContains'); + // No key index to hint at either. + expect(expressions.hasTtft).not.toContain('indexHint'); + expect(expressions.hasReportedTokens).not.toContain('indexHint'); // On JSON there is no key index to prune with, so the gates carry the // value test alone rather than doubling their subcolumn reads with a @@ -88,9 +91,9 @@ describe('getLLMExpressions', () => { // '' must not pass, or SessionsTab groups it as a blank row whose link // carries no session id. expect(expressions.hasSessionId).toBe( - "((mapContains(SpanAttributes, 'gen_ai.conversation.id') OR " + + "(indexHint((mapContains(SpanAttributes, 'gen_ai.conversation.id') OR " + "mapContains(SpanAttributes, 'session.id') OR " + - "mapContains(SpanAttributes, 'ai.telemetry.metadata.sessionId')) AND " + + "mapContains(SpanAttributes, 'ai.telemetry.metadata.sessionId'))) AND " + "coalesce(nullif(SpanAttributes['gen_ai.conversation.id'], ''), " + "nullif(SpanAttributes['session.id'], ''), " + "nullif(SpanAttributes['ai.telemetry.metadata.sessionId'], ''), '') != '')", @@ -105,6 +108,14 @@ describe('getLLMExpressions', () => { "mapContains(SpanAttributes, 'llm.token_count.total')", ); expect(expressions.hasReportedTokens).not.toContain('ai.usage.inputTokens'); + // This gate decides which rows enter every token and cost aggregate, so + // pin the ANDed shape: dropping the value term back to presence-only + // would let a key set to '' inflate the call count. + expect(expressions.hasReportedTokens).toContain( + "AND coalesce(nullif(SpanAttributes['gen_ai.usage.input_tokens'], '')", + ); + expect(expressions.hasReportedTokens.startsWith('(indexHint(')).toBe(true); + expect(expressions.hasReportedTokens).toContain("), '') != '')"); }); it('derives efficiency, attribution, and agent expressions', () => { @@ -159,9 +170,9 @@ describe('getLLMExpressions', () => { // the p50/p95 the value check exists to keep them out of, and a // toContain-style assertion would not notice. expect(expressions.hasTtft).toBe( - "((mapContains(SpanAttributes, 'ttft_ms') OR " + + "(indexHint((mapContains(SpanAttributes, 'ttft_ms') OR " + "mapContains(SpanAttributes, 'ai.response.msToFirstChunk') OR " + - "mapContains(SpanAttributes, 'copilot_chat.time_to_first_token')) AND " + + "mapContains(SpanAttributes, 'copilot_chat.time_to_first_token'))) AND " + "greatest(toFloat64OrZero(SpanAttributes['ttft_ms']), " + "toFloat64OrZero(SpanAttributes['ai.response.msToFirstChunk']), " + "toFloat64OrZero(SpanAttributes['copilot_chat.time_to_first_token'])) > 0)", @@ -176,8 +187,8 @@ describe('getLLMExpressions', () => { ); expect(expressions.agentName).toContain("SpanAttributes['agent.name']"); expect(expressions.hasAgentName).toBe( - "((mapContains(SpanAttributes, 'gen_ai.agent.name') OR " + - "mapContains(SpanAttributes, 'agent.name')) AND " + + "(indexHint((mapContains(SpanAttributes, 'gen_ai.agent.name') OR " + + "mapContains(SpanAttributes, 'agent.name'))) AND " + "coalesce(nullif(SpanAttributes['gen_ai.agent.name'], ''), " + "nullif(SpanAttributes['agent.name'], ''), '') != '')", ); @@ -186,6 +197,19 @@ describe('getLLMExpressions', () => { expect(expressions.finishReason).toMatch(/^replaceRegexpAll\(/); expect(expressions.finishReason).toContain("SpanAttributes['stop_reason']"); + // Drives the finish-reason breakdown, so it needs the value term to keep + // a blank slice out of the chart. + expect(expressions.hasFinishReason).toBe( + "(indexHint((mapContains(SpanAttributes, 'gen_ai.response.finish_reasons') OR " + + "mapContains(SpanAttributes, 'stop_reason') OR " + + "mapContains(SpanAttributes, 'llm.finish_reason') OR " + + "mapContains(SpanAttributes, 'ai.response.finishReason'))) AND " + + "coalesce(nullif(SpanAttributes['gen_ai.response.finish_reasons'], ''), " + + "nullif(SpanAttributes['stop_reason'], ''), " + + "nullif(SpanAttributes['llm.finish_reason'], ''), " + + "nullif(SpanAttributes['ai.response.finishReason'], ''), '') != '')", + ); + // User attribution coalesce. expect(expressions.userId).toContain("SpanAttributes['user.email']"); expect(expressions.userId).toContain("SpanAttributes['enduser.id']"); @@ -201,9 +225,9 @@ describe('getLLMExpressions', () => { // check would pull every OpenInference span into the tool charts. expect(expressions.isToolSpan).toBe( "(SpanAttributes['openinference.span.kind'] = 'TOOL' OR " + - "((mapContains(SpanAttributes, 'gen_ai.tool.name') OR " + + "(indexHint((mapContains(SpanAttributes, 'gen_ai.tool.name') OR " + "mapContains(SpanAttributes, 'gen_ai.tool.call.id') OR " + - "mapContains(SpanAttributes, 'ai.toolCall.name')) AND " + + "mapContains(SpanAttributes, 'ai.toolCall.name'))) AND " + "coalesce(nullif(SpanAttributes['gen_ai.tool.name'], ''), " + "nullif(SpanAttributes['gen_ai.tool.call.id'], ''), " + "nullif(SpanAttributes['ai.toolCall.name'], ''), '') != ''))", diff --git a/packages/app/src/llm/lib/expressions.ts b/packages/app/src/llm/lib/expressions.ts index d714956f06..9d36d567ff 100644 --- a/packages/app/src/llm/lib/expressions.ts +++ b/packages/app/src/llm/lib/expressions.ts @@ -226,25 +226,37 @@ function getLLMAttributeExpressions({ const anyKeyExists = (keys: readonly string[]) => buildAnyKeyExistsSql({ attributeField, keys, isJsonColumn }); + /** + * Wrap a presence term that exists only to prune granules. + * + * `indexHint` feeds its argument to skip-index analysis but returns true at + * execution, so the keys are never re-tested per surviving row. This is the + * same thing queryParser.ts does with `mapKeyIndexExpression`. + * + * Only safe when a value term follows that already implies presence. A bare + * presence filter must not be wrapped — `indexHint` alone matches every row. + */ + const prunableKeys = (keys: readonly string[]) => + `indexHint(${anyKeyExists(keys)})`; + /** * Gate for "any of these keys holds a non-empty value". * - * The presence term is there so a mapKeys() index can prune granules; the - * value term is what preserves the meaning. Both are needed: presence alone - * would admit a key explicitly set to '', and every caller pairs this gate - * with a coalesced value expression it groups by, so such a row would show - * up as a blank bar/row whose drill-in link goes nowhere. + * Presence alone would admit a key explicitly set to '', and every caller + * pairs this gate with a coalesced value expression it groups by, so such a + * row would show up as a blank bar/row whose drill-in link goes nowhere. + * The value term is therefore what defines the result; the presence term is + * pruning only, and is implied by it (a non-empty value requires the key). * * The value term costs no extra per-part size lookups during planning: it * reads the same keys the paired group-by expression already reads. * - * On JSON columns presence and non-emptiness are the same test, so the - * presence term would only duplicate subcolumn reads — emit the value term - * alone. + * On JSON columns presence and non-emptiness are the same test and there is + * no key index to prune with, so emit the value term alone. */ const anyKeyHasValue = (keys: readonly string[]) => { const hasValue = `${coalesceString(attributeField, keys, isJsonColumn)} != ''`; - return isJsonColumn ? hasValue : `(${anyKeyExists(keys)} AND ${hasValue})`; + return isJsonColumn ? hasValue : `(${prunableKeys(keys)} AND ${hasValue})`; }; const model = coalesceString(attributeField, MODEL_KEYS, isJsonColumn); @@ -340,13 +352,13 @@ function getLLMAttributeExpressions({ hasReportedTokens: anyKeyHasValue(REPORTED_TOKEN_KEYS), hasSessionId: anyKeyHasValue(SESSION_ID_KEYS), // Numeric rather than non-empty: it feeds a latency percentile, and a - // zero would drag p50/p95 down. Same presence-plus-value shape as - // anyKeyHasValue, including skipping the presence term on JSON. + // zero would drag p50/p95 down. Same shape as anyKeyHasValue otherwise — + // `> 0` implies the key is present, so presence is pruning only. hasTtft: (() => { const isPositive = `${greatestNumber(attributeField, TTFT_MS_KEYS, isJsonColumn)} > 0`; return isJsonColumn ? isPositive - : `(${anyKeyExists(TTFT_MS_KEYS)} AND ${isPositive})`; + : `(${prunableKeys(TTFT_MS_KEYS)} AND ${isPositive})`; })(), hasUserId: anyKeyHasValue(USER_ID_KEYS), hasFinishReason: anyKeyHasValue(FINISH_REASON_KEYS), From 8fce066bfcdf9ebfcd7c0282e0f86dbe436a6c8f Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:07:48 -0700 Subject: [PATCH 05/12] fix(llm): hoist isToolSpan's hint, drop the inert one, escape keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isToolSpan's hint sat inside an OR whose other arm is a span-kind value comparison. A skip index can only drop a granule when every arm of an OR is decidable by that index, so the hint could never fire and the two tool charts still scanned every granule — the case this PR exists to fix. Hoisted to a top-level conjunct covering the span-kind key as well, which is sound because both arms imply one of the hinted keys. hasReportedTokens loses its hint entirely: perServiceElectionExpr only embeds it inside select-list aggregates, and skip-index analysis never looks there, so the hint pruned nothing while adding ~600 characters twice per gated aggregate to queries this file already keeps under max_query_size. hasTtft now shares anyKeyHasValue's branching via withKeyPruning rather than restating it, so the JSON and hinted shapes cannot drift apart. Keys are escaped with SqlString rather than interpolated, since buildAnyKeyExistsSql is exported and takes an arbitrary key list. The field stays raw: it holds a source's eventAttributesExpression, which may be a compound expression rather than an identifier. --- .changeset/great-hoops-shave.md | 4 +- .../app/src/llm/__tests__/expressions.test.ts | 26 ++++--- packages/app/src/llm/lib/detect.ts | 10 ++- packages/app/src/llm/lib/expressions.ts | 69 +++++++++++++------ 4 files changed, 74 insertions(+), 35 deletions(-) diff --git a/.changeset/great-hoops-shave.md b/.changeset/great-hoops-shave.md index 1e0069fa01..95806e4975 100644 --- a/.changeset/great-hoops-shave.md +++ b/.changeset/great-hoops-shave.md @@ -21,7 +21,9 @@ non-empty check, so an attribute set to `''` still cannot appear as a blank row. There the value term defines the result and the presence term is pruning only, so it is wrapped in `indexHint` — it reaches skip-index analysis without being re-evaluated per surviving row. The value term costs no extra per-part lookups, -since it reads the same keys the group-by already reads. +since it reads the same keys the group-by already reads. Gates used only inside +select-list aggregates are left unhinted, since skip-index analysis does not +reach them. The one behavior change is LLM span detection, which is now presence-based: a span carrying `gen_ai.system` at all is treated as an LLM span whatever the diff --git a/packages/app/src/llm/__tests__/expressions.test.ts b/packages/app/src/llm/__tests__/expressions.test.ts index 629420103b..1f20657f03 100644 --- a/packages/app/src/llm/__tests__/expressions.test.ts +++ b/packages/app/src/llm/__tests__/expressions.test.ts @@ -102,20 +102,20 @@ describe('getLLMExpressions', () => { // The gate keys on authoritative usage reporters only — wrapper spans // carrying just ai.usage.* must not satisfy it. expect(expressions.hasReportedTokens).toContain( - "mapContains(SpanAttributes, 'gen_ai.usage.input_tokens')", + "nullif(SpanAttributes['gen_ai.usage.input_tokens'], '')", ); expect(expressions.hasReportedTokens).toContain( - "mapContains(SpanAttributes, 'llm.token_count.total')", + "nullif(SpanAttributes['llm.token_count.total'], '')", ); expect(expressions.hasReportedTokens).not.toContain('ai.usage.inputTokens'); // This gate decides which rows enter every token and cost aggregate, so - // pin the ANDed shape: dropping the value term back to presence-only - // would let a key set to '' inflate the call count. - expect(expressions.hasReportedTokens).toContain( - "AND coalesce(nullif(SpanAttributes['gen_ai.usage.input_tokens'], '')", - ); - expect(expressions.hasReportedTokens.startsWith('(indexHint(')).toBe(true); - expect(expressions.hasReportedTokens).toContain("), '') != '')"); + // pin the value test: reverting it to presence-only would let a key set + // to '' inflate the call count. + expect(expressions.hasReportedTokens).toMatch(/^coalesce\(nullif\(/); + expect(expressions.hasReportedTokens).toContain("!= ''"); + // Only ever embedded in select-list aggregates, where a hint cannot prune + // and would double the length of an already-long expression. + expect(expressions.hasReportedTokens).not.toContain('indexHint'); }); it('derives efficiency, attribution, and agent expressions', () => { @@ -223,11 +223,15 @@ describe('getLLMExpressions', () => { // The span-kind term must stay an equality: widening it to a presence // check would pull every OpenInference span into the tool charts. + // The hint covers the span-kind key and sits above the OR: a skip index + // cannot prune an OR whose other arm it can't decide, so nesting it inside + // would silently stop pruning the tool charts. expect(expressions.isToolSpan).toBe( - "(SpanAttributes['openinference.span.kind'] = 'TOOL' OR " + - "(indexHint((mapContains(SpanAttributes, 'gen_ai.tool.name') OR " + + "(indexHint((mapContains(SpanAttributes, 'openinference.span.kind') OR " + + "mapContains(SpanAttributes, 'gen_ai.tool.name') OR " + "mapContains(SpanAttributes, 'gen_ai.tool.call.id') OR " + "mapContains(SpanAttributes, 'ai.toolCall.name'))) AND " + + "(SpanAttributes['openinference.span.kind'] = 'TOOL' OR " + "coalesce(nullif(SpanAttributes['gen_ai.tool.name'], ''), " + "nullif(SpanAttributes['gen_ai.tool.call.id'], ''), " + "nullif(SpanAttributes['ai.toolCall.name'], ''), '') != ''))", diff --git a/packages/app/src/llm/lib/detect.ts b/packages/app/src/llm/lib/detect.ts index 2d868f08bd..b818df0617 100644 --- a/packages/app/src/llm/lib/detect.ts +++ b/packages/app/src/llm/lib/detect.ts @@ -1,3 +1,5 @@ +import SqlString from 'sqlstring'; + import { hasKeyWithPrefix } from './attributeUtils'; import { LLMSpanEvent, SpanAttributeMap } from './types'; @@ -97,6 +99,12 @@ export function isLLMSpan( * This tests presence only. Callers that pair a gate with a value expression * they group by need non-emptiness as well, or a key set to '' becomes a blank * row — see `anyKeyHasValue` in expressions.ts. + * + * queryParser's private `buildMapContains` emits the same call, but it takes a + * rendered `col['key']` subscript and parses it back apart; the field and key + * are already separate here. `attributeField` stays raw because it holds a + * source's `eventAttributesExpression`, which may be a compound expression + * rather than an identifier — the same reason `fieldAccess` interpolates it. */ function buildKeyExistsSql({ attributeField, @@ -109,7 +117,7 @@ function buildKeyExistsSql({ }): string { return isJsonColumn ? `toString(${attributeField}.\`${key}\`) != ''` - : `mapContains(${attributeField}, '${key}')`; + : `mapContains(${attributeField}, ${SqlString.escape(key)})`; } /** SQL matching rows carrying any of the given attribute keys. */ diff --git a/packages/app/src/llm/lib/expressions.ts b/packages/app/src/llm/lib/expressions.ts index 9d36d567ff..0fbb6dfc84 100644 --- a/packages/app/src/llm/lib/expressions.ts +++ b/packages/app/src/llm/lib/expressions.ts @@ -239,25 +239,38 @@ function getLLMAttributeExpressions({ const prunableKeys = (keys: readonly string[]) => `indexHint(${anyKeyExists(keys)})`; + /** + * Pair a value term with a pruning hint over the keys it reads. + * + * `valueTerm` alone defines the result; the hint only lets the index drop + * granules, so it must be implied by `valueTerm` to be sound. + * + * Only worth adding in WHERE position — skip-index analysis does not look at + * the select list, so a gate used solely inside an aggregate should pass its + * value term through unhinted rather than pay the query-length cost. + * + * On JSON columns there is no key index to prune with, so the value term is + * emitted alone. + */ + const withKeyPruning = (keys: readonly string[], valueTerm: string) => + isJsonColumn ? valueTerm : `(${prunableKeys(keys)} AND ${valueTerm})`; + + /** "Any of these keys holds a non-empty value", before any pruning hint. */ + const anyKeyNonEmpty = (keys: readonly string[]) => + `${coalesceString(attributeField, keys, isJsonColumn)} != ''`; + /** * Gate for "any of these keys holds a non-empty value". * * Presence alone would admit a key explicitly set to '', and every caller * pairs this gate with a coalesced value expression it groups by, so such a * row would show up as a blank bar/row whose drill-in link goes nowhere. - * The value term is therefore what defines the result; the presence term is - * pruning only, and is implied by it (a non-empty value requires the key). * * The value term costs no extra per-part size lookups during planning: it * reads the same keys the paired group-by expression already reads. - * - * On JSON columns presence and non-emptiness are the same test and there is - * no key index to prune with, so emit the value term alone. */ - const anyKeyHasValue = (keys: readonly string[]) => { - const hasValue = `${coalesceString(attributeField, keys, isJsonColumn)} != ''`; - return isJsonColumn ? hasValue : `(${prunableKeys(keys)} AND ${hasValue})`; - }; + const anyKeyHasValue = (keys: readonly string[]) => + withKeyPruning(keys, anyKeyNonEmpty(keys)); const model = coalesceString(attributeField, MODEL_KEYS, isJsonColumn); const inputTokens = greatestNumber( @@ -348,32 +361,44 @@ function getLLMAttributeExpressions({ * an app's own authoritative per-call reporters — see llmGatedSumExpr. */ hasProvidedCost: `(${providedCost} > 0)`, - /** See REPORTED_TOKEN_KEYS: gates sums so wrapper spans don't double count. */ - hasReportedTokens: anyKeyHasValue(REPORTED_TOKEN_KEYS), + /** + * See REPORTED_TOKEN_KEYS: gates sums so wrapper spans don't double count. + * Unhinted — perServiceElectionExpr only ever embeds this inside select-list + * aggregates, where a hint cannot prune and would just lengthen a query this + * file already keeps under max_query_size (see LLM_COST_SQL_ALIAS). + */ + hasReportedTokens: anyKeyNonEmpty(REPORTED_TOKEN_KEYS), hasSessionId: anyKeyHasValue(SESSION_ID_KEYS), // Numeric rather than non-empty: it feeds a latency percentile, and a // zero would drag p50/p95 down. Same shape as anyKeyHasValue otherwise — // `> 0` implies the key is present, so presence is pruning only. - hasTtft: (() => { - const isPositive = `${greatestNumber(attributeField, TTFT_MS_KEYS, isJsonColumn)} > 0`; - return isJsonColumn - ? isPositive - : `(${prunableKeys(TTFT_MS_KEYS)} AND ${isPositive})`; - })(), + hasTtft: withKeyPruning( + TTFT_MS_KEYS, + `${greatestNumber(attributeField, TTFT_MS_KEYS, isJsonColumn)} > 0`, + ), hasUserId: anyKeyHasValue(USER_ID_KEYS), hasFinishReason: anyKeyHasValue(FINISH_REASON_KEYS), // The `= 'TOOL'` term stays a value comparison — it discriminates one // OpenInference span kind from the others, and the query builder already // rewrites equality onto the attribute-items index. The remaining terms // gate a group-by on toolName, so they keep their value check too. - isToolSpan: `(${[ - `${fieldAccess(attributeField, 'openinference.span.kind', isJsonColumn)} = 'TOOL'`, - anyKeyHasValue([ + // + // The hint sits above the OR, not inside it: a skip index can only drop a + // granule when every arm of an OR is decidable by that same index, and the + // span-kind arm is not. Hoisting is sound because both arms imply one of + // the hinted keys. + isToolSpan: (() => { + const toolKeys = [ 'gen_ai.tool.name', 'gen_ai.tool.call.id', 'ai.toolCall.name', - ]), - ].join(' OR ')})`, + ]; + const kindIsTool = `${fieldAccess(attributeField, 'openinference.span.kind', isJsonColumn)} = 'TOOL'`; + return withKeyPruning( + ['openinference.span.kind', ...toolKeys], + `(${kindIsTool} OR ${anyKeyNonEmpty(toolKeys)})`, + ); + })(), }; } From c1402976d5d4de3c98093ac9747e534deaaeb5c1 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:24:08 -0700 Subject: [PATCH 06/12] fix(llm): expose isToolSpan unhinted for select-list use isToolSpan is the one gate used in both positions: a WHERE filter in AgentToolCharts, where the hint prunes, and an aggCondition in SessionsTab, where renderChartConfig emits it inside countIf and skip-index analysis never reaches it. One string cannot serve both, so the hint shipped ~180 characters of constant-folded SQL in every Sessions query. isToolSpan stays hinted, since WHERE is the position where getting it wrong costs a full scan, and isToolSpanUnhinted is the bare match for aggregate use. A test asserts the hinted form still wraps exactly the unhinted predicate, so the two cannot drift. --- .changeset/great-hoops-shave.md | 7 ++- .../app/src/llm/__tests__/expressions.test.ts | 5 ++ .../app/src/llm/dashboard/SessionsTab.tsx | 3 +- packages/app/src/llm/lib/expressions.ts | 56 ++++++++++++------- 4 files changed, 46 insertions(+), 25 deletions(-) diff --git a/.changeset/great-hoops-shave.md b/.changeset/great-hoops-shave.md index 95806e4975..57848276c0 100644 --- a/.changeset/great-hoops-shave.md +++ b/.changeset/great-hoops-shave.md @@ -21,9 +21,10 @@ non-empty check, so an attribute set to `''` still cannot appear as a blank row. There the value term defines the result and the presence term is pruning only, so it is wrapped in `indexHint` — it reaches skip-index analysis without being re-evaluated per surviving row. The value term costs no extra per-part lookups, -since it reads the same keys the group-by already reads. Gates used only inside -select-list aggregates are left unhinted, since skip-index analysis does not -reach them. +since it reads the same keys the group-by already reads. Gates that land in a +select-list aggregate are left unhinted, since skip-index analysis does not +reach the select list; the tool-call gate is used in both positions and so is +exposed in both forms. The one behavior change is LLM span detection, which is now presence-based: a span carrying `gen_ai.system` at all is treated as an LLM span whatever the diff --git a/packages/app/src/llm/__tests__/expressions.test.ts b/packages/app/src/llm/__tests__/expressions.test.ts index 1f20657f03..0e66bacd90 100644 --- a/packages/app/src/llm/__tests__/expressions.test.ts +++ b/packages/app/src/llm/__tests__/expressions.test.ts @@ -236,6 +236,11 @@ describe('getLLMExpressions', () => { "nullif(SpanAttributes['gen_ai.tool.call.id'], ''), " + "nullif(SpanAttributes['ai.toolCall.name'], ''), '') != ''))", ); + // SessionsTab uses this one as an aggCondition, where a hint folds to a + // constant. It must stay the bare match, and must still be the same + // predicate the hinted form wraps. + expect(expressions.isToolSpanUnhinted).not.toContain('indexHint'); + expect(expressions.isToolSpan).toContain(expressions.isToolSpanUnhinted); expect(expressions.statusMessage).toBe('StatusMessage'); }); diff --git a/packages/app/src/llm/dashboard/SessionsTab.tsx b/packages/app/src/llm/dashboard/SessionsTab.tsx index 9106e982fa..a0f3f3ee1e 100644 --- a/packages/app/src/llm/dashboard/SessionsTab.tsx +++ b/packages/app/src/llm/dashboard/SessionsTab.tsx @@ -114,7 +114,8 @@ export function SessionsTab(props: LLMChartProps) { alias: 'Tool Calls', aggFn: 'count', valueExpression: '', - aggCondition: expressions.isToolSpan, + // Select-list position: the hinted form cannot prune here. + aggCondition: expressions.isToolSpanUnhinted, aggConditionLanguage: 'sql', numberFormat: INTEGER_NUMBER_FORMAT, }, diff --git a/packages/app/src/llm/lib/expressions.ts b/packages/app/src/llm/lib/expressions.ts index 0fbb6dfc84..51f1663231 100644 --- a/packages/app/src/llm/lib/expressions.ts +++ b/packages/app/src/llm/lib/expressions.ts @@ -272,6 +272,17 @@ function getLLMAttributeExpressions({ const anyKeyHasValue = (keys: readonly string[]) => withKeyPruning(keys, anyKeyNonEmpty(keys)); + const TOOL_SPAN_KEYS = [ + 'gen_ai.tool.name', + 'gen_ai.tool.call.id', + 'ai.toolCall.name', + ]; + const isToolSpanMatch = `(${fieldAccess( + attributeField, + 'openinference.span.kind', + isJsonColumn, + )} = 'TOOL' OR ${anyKeyNonEmpty(TOOL_SPAN_KEYS)})`; + const model = coalesceString(attributeField, MODEL_KEYS, isJsonColumn); const inputTokens = greatestNumber( attributeField, @@ -378,27 +389,30 @@ function getLLMAttributeExpressions({ ), hasUserId: anyKeyHasValue(USER_ID_KEYS), hasFinishReason: anyKeyHasValue(FINISH_REASON_KEYS), - // The `= 'TOOL'` term stays a value comparison — it discriminates one - // OpenInference span kind from the others, and the query builder already - // rewrites equality onto the attribute-items index. The remaining terms - // gate a group-by on toolName, so they keep their value check too. - // - // The hint sits above the OR, not inside it: a skip index can only drop a - // granule when every arm of an OR is decidable by that same index, and the - // span-kind arm is not. Hoisting is sound because both arms imply one of - // the hinted keys. - isToolSpan: (() => { - const toolKeys = [ - 'gen_ai.tool.name', - 'gen_ai.tool.call.id', - 'ai.toolCall.name', - ]; - const kindIsTool = `${fieldAccess(attributeField, 'openinference.span.kind', isJsonColumn)} = 'TOOL'`; - return withKeyPruning( - ['openinference.span.kind', ...toolKeys], - `(${kindIsTool} OR ${anyKeyNonEmpty(toolKeys)})`, - ); - })(), + /** + * Tool-call rows, for WHERE position. + * + * The `= 'TOOL'` term stays a value comparison — it discriminates one + * OpenInference span kind from the others, and the query builder already + * rewrites equality onto the attribute-items index. The remaining terms + * gate a group-by on toolName, so they keep their value check too. + * + * The hint sits above the OR, not inside it: a skip index can only drop a + * granule when every arm of an OR is decidable by that same index, and the + * span-kind arm is not. Hoisting is sound because both arms imply one of + * the hinted keys. + */ + isToolSpan: withKeyPruning( + ['openinference.span.kind', ...TOOL_SPAN_KEYS], + isToolSpanMatch, + ), + /** + * isToolSpan for select-list aggregate positions (`aggCondition`), which + * renderChartConfig emits inside countIf/sumIf. Skip-index analysis never + * reaches the select list, so the hint would fold to a constant while + * lengthening every query — see withKeyPruning. + */ + isToolSpanUnhinted: isToolSpanMatch, }; } From cd725f5741c0af778d26cd66e73419e463263bb6 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:41:00 -0700 Subject: [PATCH 07/12] test: cover the indexHint shape through the kv-items rewrite The LLM gates are the first caller to put a bare boolean OR inside a function argument, and rewriteSqlFilterWithKvItems bails to its input verbatim on a parse error. If node-sql-parser rejected that shape the sibling span-kind equality would quietly stop being rewritten onto the attribute-items index, with nothing failing. Verified it round-trips and pinned it. Also replaces two assertions in the JSON test that held on both branches and so could not catch a JSON regression: hasReportedTokens is unhinted either way, so its JSON form is now pinned exactly. --- .../app/src/llm/__tests__/expressions.test.ts | 19 ++++++++++---- .../rewriteSqlFilterWithKvItems.test.ts | 26 +++++++++++++++++++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/packages/app/src/llm/__tests__/expressions.test.ts b/packages/app/src/llm/__tests__/expressions.test.ts index 0e66bacd90..e65ccea704 100644 --- a/packages/app/src/llm/__tests__/expressions.test.ts +++ b/packages/app/src/llm/__tests__/expressions.test.ts @@ -53,16 +53,12 @@ describe('getLLMExpressions', () => { ); // mapContains does not apply to JSON paths. expect(expressions.isLLMSpan).not.toContain('mapContains'); - expect(expressions.hasReportedTokens).not.toContain('mapContains'); - expect(expressions.hasSessionId).not.toContain('mapContains'); expect(expressions.isToolSpan).not.toContain('mapContains'); - // No key index to hint at either. - expect(expressions.hasTtft).not.toContain('indexHint'); - expect(expressions.hasReportedTokens).not.toContain('indexHint'); // On JSON there is no key index to prune with, so the gates carry the // value test alone rather than doubling their subcolumn reads with a // presence term that says the same thing. + expect(expressions.isToolSpan).not.toContain('indexHint'); expect(expressions.hasTtft).toBe( 'greatest(toFloat64OrZero(toString(SpanAttributes.`ttft_ms`)), ' + 'toFloat64OrZero(toString(SpanAttributes.`ai.response.msToFirstChunk`)), ' + @@ -73,6 +69,19 @@ describe('getLLMExpressions', () => { "nullif(toString(SpanAttributes.`session.id`), ''), " + "nullif(toString(SpanAttributes.`ai.telemetry.metadata.sessionId`), ''), '') != ''", ); + // Unhinted on both branches, so pin the JSON form itself — a + // not.toContain('indexHint') here would hold either way and prove nothing. + expect(expressions.hasReportedTokens).toBe( + "coalesce(nullif(toString(SpanAttributes.`gen_ai.usage.input_tokens`), ''), " + + "nullif(toString(SpanAttributes.`gen_ai.usage.prompt_tokens`), ''), " + + "nullif(toString(SpanAttributes.`gen_ai.usage.output_tokens`), ''), " + + "nullif(toString(SpanAttributes.`gen_ai.usage.completion_tokens`), ''), " + + "nullif(toString(SpanAttributes.`llm.token_count.prompt`), ''), " + + "nullif(toString(SpanAttributes.`llm.token_count.completion`), ''), " + + "nullif(toString(SpanAttributes.`llm.token_count.total`), ''), " + + "nullif(toString(SpanAttributes.`input_tokens`), ''), " + + "nullif(toString(SpanAttributes.`output_tokens`), ''), '') != ''", + ); }); it('derives session id and reported-token gate expressions', () => { diff --git a/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts b/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts index fb81d16a91..ae7624f26a 100644 --- a/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts +++ b/packages/common-utils/src/__tests__/rewriteSqlFilterWithKvItems.test.ts @@ -425,4 +425,30 @@ describe('rewriteSqlFilterWithKvItems', () => { expect(result).not.toContain('hasAny('); }); }); + + describe('indexHint', () => { + // The LLM dashboard gates pair a value term with indexHint(a OR b), which + // is the first caller to put a bare boolean OR in an argument position. + // This function bails to the input verbatim on a parse error, so a grammar + // that rejected that shape would silently stop rewriting the rest of the + // condition rather than fail — hence asserting the sibling rewrite here. + it('rewrites a sibling equality and leaves the hint intact', () => { + const result = rewriteSqlFilterWithKvItems( + "(indexHint((mapContains(LogAttributes, 'openinference.span.kind') OR " + + "mapContains(LogAttributes, 'gen_ai.tool.name'))) AND " + + "(LogAttributes['openinference.span.kind'] = 'TOOL' OR " + + "coalesce(nullif(LogAttributes['gen_ai.tool.name'], ''), '') != ''))", + defaultLookup, + ); + expect(result).toContain( + "has(`LogAttributeItems`, concat('openinference.span.kind', '=', 'TOOL'))", + ); + // The hint's own mapContains calls must survive untouched — rewriting + // them to has() would point the hint at the wrong index. + expect(result).toContain( + "indexHint((mapContains(LogAttributes, 'openinference.span.kind') OR " + + "mapContains(LogAttributes, 'gen_ai.tool.name')))", + ); + }); + }); }); From 871137f489d7499c3d0d9ad77aaa765beda2e405 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:57:43 -0700 Subject: [PATCH 08/12] fix(llm): derive tool-span keys from the tool-name keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The detection list omitted the flat `tool_name` that TOOL_NAME_KEYS carries for Claude Code and opencode, so those spans resolved a tool name the dashboard could display but never passed isToolSpan — absent from both tool charts. Pre-existing since #2990, but naming the list in this PR made the divergence look deliberate. Deriving it from TOOL_NAME_KEYS fixes that and removes the sync hazard; gen_ai.tool.call.id stays an explicit addition, since it marks a tool call without naming one. The pruning hint is built from the same constant, so the two cannot drift and the hint stays implied by the filter. Tool-call counts rise slightly where tool_name is emitted; noted in the changeset. A test asserts every key toolName can resolve also satisfies the gate. --- .changeset/great-hoops-shave.md | 12 +++++++++--- .../app/src/llm/__tests__/expressions.test.ts | 19 ++++++++++++++----- packages/app/src/llm/lib/expressions.ts | 11 ++++++----- 3 files changed, 29 insertions(+), 13 deletions(-) diff --git a/.changeset/great-hoops-shave.md b/.changeset/great-hoops-shave.md index 57848276c0..a55205ad19 100644 --- a/.changeset/great-hoops-shave.md +++ b/.changeset/great-hoops-shave.md @@ -26,9 +26,15 @@ select-list aggregate are left unhinted, since skip-index analysis does not reach the select list; the tool-call gate is used in both positions and so is exposed in both forms. -The one behavior change is LLM span detection, which is now presence-based: a -span carrying `gen_ai.system` at all is treated as an LLM span whatever the -value. Nothing groups by that predicate. +Two behavior changes. LLM span detection is now presence-based: a span carrying +`gen_ai.system` at all is treated as an LLM span whatever the value. Nothing +groups by that predicate. + +Tool-call detection now also accepts the flat `tool_name` key (Claude Code, +opencode). The tool-name expression already resolved it, so such spans had a +name the dashboard could display but were missing from the tool charts; the +detection key set is now derived from the tool-name key set so the two cannot +drift apart. Expect slightly higher tool-call counts where that key is emitted. JSON attribute columns are unchanged — their paths are real subcolumns, there is no key index to prune with, and a presence term would only duplicate reads. diff --git a/packages/app/src/llm/__tests__/expressions.test.ts b/packages/app/src/llm/__tests__/expressions.test.ts index e65ccea704..5852854ccc 100644 --- a/packages/app/src/llm/__tests__/expressions.test.ts +++ b/packages/app/src/llm/__tests__/expressions.test.ts @@ -238,13 +238,22 @@ describe('getLLMExpressions', () => { expect(expressions.isToolSpan).toBe( "(indexHint((mapContains(SpanAttributes, 'openinference.span.kind') OR " + "mapContains(SpanAttributes, 'gen_ai.tool.name') OR " + - "mapContains(SpanAttributes, 'gen_ai.tool.call.id') OR " + - "mapContains(SpanAttributes, 'ai.toolCall.name'))) AND " + + "mapContains(SpanAttributes, 'ai.toolCall.name') OR " + + "mapContains(SpanAttributes, 'tool_name') OR " + + "mapContains(SpanAttributes, 'gen_ai.tool.call.id'))) AND " + "(SpanAttributes['openinference.span.kind'] = 'TOOL' OR " + "coalesce(nullif(SpanAttributes['gen_ai.tool.name'], ''), " + - "nullif(SpanAttributes['gen_ai.tool.call.id'], ''), " + - "nullif(SpanAttributes['ai.toolCall.name'], ''), '') != ''))", - ); + "nullif(SpanAttributes['ai.toolCall.name'], ''), " + + "nullif(SpanAttributes['tool_name'], ''), " + + "nullif(SpanAttributes['gen_ai.tool.call.id'], ''), '') != ''))", + ); + // Every key the tool-name expression can resolve must also satisfy the + // gate, or the span is dropped from the tool charts under a name the + // dashboard could have shown. + for (const key of ['gen_ai.tool.name', 'ai.toolCall.name', 'tool_name']) { + expect(expressions.toolName).toContain(`SpanAttributes['${key}']`); + expect(expressions.isToolSpan).toContain(`SpanAttributes['${key}']`); + } // SessionsTab uses this one as an aggCondition, where a hint folds to a // constant. It must stay the bare match, and must still be the same // predicate the hinted form wraps. diff --git a/packages/app/src/llm/lib/expressions.ts b/packages/app/src/llm/lib/expressions.ts index 51f1663231..623ad6195f 100644 --- a/packages/app/src/llm/lib/expressions.ts +++ b/packages/app/src/llm/lib/expressions.ts @@ -272,11 +272,12 @@ function getLLMAttributeExpressions({ const anyKeyHasValue = (keys: readonly string[]) => withKeyPruning(keys, anyKeyNonEmpty(keys)); - const TOOL_SPAN_KEYS = [ - 'gen_ai.tool.name', - 'gen_ai.tool.call.id', - 'ai.toolCall.name', - ]; + // Derived from TOOL_NAME_KEYS rather than restated: a span whose tool name + // resolves is a tool call, so the two must not drift. Until now the flat + // `tool_name` (Claude Code, opencode) was missing here, so those spans got a + // resolvable toolName but never passed the gate. gen_ai.tool.call.id is the + // one addition — it marks a tool call without naming it. + const TOOL_SPAN_KEYS = [...TOOL_NAME_KEYS, 'gen_ai.tool.call.id']; const isToolSpanMatch = `(${fieldAccess( attributeField, 'openinference.span.kind', From d43af928de96c35729364751df10867cc1f6bf62 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 9 Sep 2026 19:14:15 -0700 Subject: [PATCH 09/12] fix(llm): correct an overclaimed changeset, note the materialized-column trade-off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tool_name derivation cannot change any counts: that key is not in LLM_MARKER_ATTRIBUTE_KEYS and every caller ANDs isLLMSpan, so a span carrying only the flat key is dropped before the tool gate is reached. The Claude Code fixture also carries gen_ai.tool.call.id, so those spans always passed. Dropped the "expect slightly higher tool-call counts" claim, which would have shipped verbatim into release notes. The derivation stays for the sync hazard it removes. Also records a real trade-off the diff was silent about: fastifySQL rewrites a Col['key'] subscript onto a materialized column by matching the subscript AST node, so it cannot see mapContains. Tables with those columns never had the planning cost the PR fixes, since a rewritten subscript is not a subcolumn reference — so this swaps that rewrite for skip-index pruning, which only pays off where the columns are absent. --- .changeset/great-hoops-shave.md | 17 +++++++++-------- packages/app/src/llm/lib/detect.ts | 9 +++++++++ packages/app/src/llm/lib/expressions.ts | 11 +++++++---- 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/.changeset/great-hoops-shave.md b/.changeset/great-hoops-shave.md index a55205ad19..52b23549a4 100644 --- a/.changeset/great-hoops-shave.md +++ b/.changeset/great-hoops-shave.md @@ -26,15 +26,16 @@ select-list aggregate are left unhinted, since skip-index analysis does not reach the select list; the tool-call gate is used in both positions and so is exposed in both forms. -Two behavior changes. LLM span detection is now presence-based: a span carrying -`gen_ai.system` at all is treated as an LLM span whatever the value. Nothing -groups by that predicate. +The one behavior change is LLM span detection, which is now presence-based: a +span carrying `gen_ai.system` at all is treated as an LLM span whatever the +value. Nothing groups by that predicate. -Tool-call detection now also accepts the flat `tool_name` key (Claude Code, -opencode). The tool-name expression already resolved it, so such spans had a -name the dashboard could display but were missing from the tool charts; the -detection key set is now derived from the tool-name key set so the two cannot -drift apart. Expect slightly higher tool-call counts where that key is emitted. +One caveat for tables with materialized columns: a `SpanAttributes['key']` +subscript gets rewritten onto a materialized column when an operator has created +one, and `mapContains` is not matched by that rewrite. Such tables were never +affected by the planning cost either, since a rewritten subscript is no longer a +subcolumn reference — so this trades that rewrite for skip-index pruning, which +is the better deal only where those columns do not exist. JSON attribute columns are unchanged — their paths are real subcolumns, there is no key index to prune with, and a presence term would only duplicate reads. diff --git a/packages/app/src/llm/lib/detect.ts b/packages/app/src/llm/lib/detect.ts index b818df0617..a429f1d874 100644 --- a/packages/app/src/llm/lib/detect.ts +++ b/packages/app/src/llm/lib/detect.ts @@ -96,6 +96,15 @@ export function isLLMSpan( * JSON columns keep the comparison: their paths are real subcolumns, and * `mapContains` does not apply. * + * Trade-off: `fastifySQL` rewrites a `Col['key']` subscript onto a + * materialized column when one exists, and it matches on the subscript AST + * node, so it cannot see this form. On a table where an operator materialized + * one of these keys, the subscript would have read that column instead. The + * subscript is also not a subcolumn reference once rewritten, so such tables + * never had the planning problem — this form trades that rewrite for skip-index + * pruning, which is the better deal only where the columns do not exist (the + * default). + * * This tests presence only. Callers that pair a gate with a value expression * they group by need non-emptiness as well, or a key set to '' becomes a blank * row — see `anyKeyHasValue` in expressions.ts. diff --git a/packages/app/src/llm/lib/expressions.ts b/packages/app/src/llm/lib/expressions.ts index 623ad6195f..72f901a18a 100644 --- a/packages/app/src/llm/lib/expressions.ts +++ b/packages/app/src/llm/lib/expressions.ts @@ -273,10 +273,13 @@ function getLLMAttributeExpressions({ withKeyPruning(keys, anyKeyNonEmpty(keys)); // Derived from TOOL_NAME_KEYS rather than restated: a span whose tool name - // resolves is a tool call, so the two must not drift. Until now the flat - // `tool_name` (Claude Code, opencode) was missing here, so those spans got a - // resolvable toolName but never passed the gate. gen_ai.tool.call.id is the - // one addition — it marks a tool call without naming it. + // resolves is a tool call, so the two must not drift. gen_ai.tool.call.id is + // the one addition — it marks a tool call without naming it. + // + // Deriving currently changes nothing observable: the flat `tool_name` it + // picks up is not in LLM_MARKER_ATTRIBUTE_KEYS, and every caller ANDs + // isLLMSpan, so a span carrying only that key is dropped before this gate is + // reached. Widening LLM detection to a key that generic is its own decision. const TOOL_SPAN_KEYS = [...TOOL_NAME_KEYS, 'gen_ai.tool.call.id']; const isToolSpanMatch = `(${fieldAccess( attributeField, From cacc7d506617d8d5a1b4c6be31c4c9d82010d739 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 9 Sep 2026 19:29:48 -0700 Subject: [PATCH 10/12] test: pin the Tool Calls aggCondition to the unhinted gate The hinted/unhinted split was enforced only by a call-site comment, and isToolSpan is the default-looking name, so reverting SessionsTab to it would have passed CI while putting a fold-to-constant indexHint back inside countIf. Verified the assertion fails on that exact revert. --- packages/app/src/llm/__tests__/SessionsTab.test.tsx | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/app/src/llm/__tests__/SessionsTab.test.tsx b/packages/app/src/llm/__tests__/SessionsTab.test.tsx index 659dacab69..08dc3810b8 100644 --- a/packages/app/src/llm/__tests__/SessionsTab.test.tsx +++ b/packages/app/src/llm/__tests__/SessionsTab.test.tsx @@ -46,6 +46,16 @@ describe('SessionsTab', () => { expect(conditions).toContain(expressions.isLLMSpan); expect(conditions).toContain(expressions.hasSessionId); + // aggCondition is a select-list position, so it must use the unhinted + // gate: skip-index analysis never reaches the select list, and the hinted + // form would fold to a constant inside countIf while lengthening every + // Sessions query. + const toolCallsSelect = config.select.find( + (s: any) => s.alias === 'Tool Calls', + ); + expect(toolCallsSelect.aggCondition).toBe(expressions.isToolSpanUnhinted); + expect(toolCallsSelect.aggCondition).not.toContain('indexHint'); + // Token/cost sums use the per-service provided-cost election so apps // emitting several instrumentation dialects in parallel don't double // count (and token-only apps aren't dropped by cost-reporting ones). From 5cc4b76ac0bcdbfe85430a8402b54b944fe4146d Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 9 Sep 2026 21:44:08 -0700 Subject: [PATCH 11/12] feat(llm): filter the dashboard by end user Adds a user filter beside the session filter, following the same pattern: a dropdown of the distinct values seen in the searched range, applied centrally in baseLLMChartConfig so every tab picks it up, and held in the URL rather than the search form so it can be set programmatically without two-way sync. Users resolve through the same cross-dialect expression Top Users groups by, so a value from the dropdown always matches the rows that produced it. The Errors tab's log query is scoped too, matching against the log source's own attribute column. SessionSelect and UserSelect are now thin wrappers over a shared DistinctValueSelect rather than two near-identical files. buildSessionCondition becomes buildScopeCondition: it was already generic and there are two scopes now. LatencyTab needed both a destructure and a useMemo dep, since it passes scopes in by hand instead of spreading props; a test covers that, and fails if either is dropped. --- .changeset/olive-cameras-repeat.md | 15 +++ .../app/src/llm/__tests__/ErrorsTab.test.tsx | 10 ++ .../app/src/llm/__tests__/LatencyTab.test.tsx | 10 +- .../app/src/llm/__tests__/chartConfig.test.ts | 58 ++++++++++- .../src/llm/dashboard/DistinctValueSelect.tsx | 96 +++++++++++++++++++ packages/app/src/llm/dashboard/ErrorsTab.tsx | 13 ++- .../src/llm/dashboard/LLMDashboardPage.tsx | 12 +++ packages/app/src/llm/dashboard/LatencyTab.tsx | 4 +- .../app/src/llm/dashboard/SessionSelect.tsx | 75 +++------------ packages/app/src/llm/dashboard/UserSelect.tsx | 40 ++++++++ packages/app/src/llm/dashboard/chartConfig.ts | 31 +++--- packages/app/src/llm/dashboard/types.ts | 6 ++ 12 files changed, 289 insertions(+), 81 deletions(-) create mode 100644 .changeset/olive-cameras-repeat.md create mode 100644 packages/app/src/llm/dashboard/DistinctValueSelect.tsx create mode 100644 packages/app/src/llm/dashboard/UserSelect.tsx diff --git a/.changeset/olive-cameras-repeat.md b/.changeset/olive-cameras-repeat.md new file mode 100644 index 0000000000..6d8385f665 --- /dev/null +++ b/.changeset/olive-cameras-repeat.md @@ -0,0 +1,15 @@ +--- +'@hyperdx/app': minor +--- + +feat: filter the LLM dashboard by end user + +Adds a user filter alongside the existing session filter. It lists the distinct +users seen on LLM spans in the searched range and scopes every tab to the one +selected, including the Errors tab's correlated log events. + +Users are resolved with the same cross-dialect expression the "Top Users" chart +groups by (`user.email`, `enduser.id`, `user.id`, +`ai.telemetry.metadata.userId`), so a value picked from the dropdown always +matches the rows that produced it. The selection lives in the URL, so a filtered +view can be shared. diff --git a/packages/app/src/llm/__tests__/ErrorsTab.test.tsx b/packages/app/src/llm/__tests__/ErrorsTab.test.tsx index 819bc2e79a..211bcd9275 100644 --- a/packages/app/src/llm/__tests__/ErrorsTab.test.tsx +++ b/packages/app/src/llm/__tests__/ErrorsTab.test.tsx @@ -41,6 +41,7 @@ describe('ErrorsTab', () => { logSource={LOG_SOURCE} logExpressions={logExpressions} sessionId="ses_123" + userId="alice@x.com" />, ); @@ -56,6 +57,9 @@ describe('ErrorsTab', () => { expect(traceConditions.some((c: string) => c.includes("'ses_123'"))).toBe( true, ); + expect( + traceConditions.some((c: string) => c.includes("'alice@x.com'")), + ).toBe(true); expect(traceConfig.orderBy[0].ordering).toBe('DESC'); const logConfig = rowTableProps[1].config; @@ -70,6 +74,12 @@ describe('ErrorsTab', () => { c.includes("'ses_123'"), ); expect(sessionCondition).toContain("LogAttributes['session.id']"); + // Same for the user scope: matched against the log source's own column, + // not the trace source's. + const userCondition = logConditions.find((c: string) => + c.includes("'alice@x.com'"), + ); + expect(userCondition).toContain("LogAttributes['user.email']"); }); it('renders only the trace tile without a log source', () => { diff --git a/packages/app/src/llm/__tests__/LatencyTab.test.tsx b/packages/app/src/llm/__tests__/LatencyTab.test.tsx index 60e21fc88e..abb4c2de15 100644 --- a/packages/app/src/llm/__tests__/LatencyTab.test.tsx +++ b/packages/app/src/llm/__tests__/LatencyTab.test.tsx @@ -38,8 +38,13 @@ describe('LatencyTab', () => { jest.clearAllMocks(); }); - it('scopes the heatmap to LLM spans (and session) without the cost alias', () => { - renderWithMantine(); + it('scopes the heatmap to LLM spans (and session/user) without the cost alias', () => { + // This tab passes the scopes into baseLLMChartConfig by hand rather than + // spreading props, and memoizes on them — a scope missing from either + // list leaves the heatmap silently unscoped. + renderWithMantine( + , + ); expect(screen.getByTestId('search-heatmap-chart')).toBeInTheDocument(); const { chartConfig, source } = heatmapChartProps[0]; @@ -47,6 +52,7 @@ describe('LatencyTab', () => { const conditions = chartConfig.filters.map((f: any) => f.condition); expect(conditions[0]).toBe(expressions.isLLMSpan); expect(conditions[1]).toContain("'ses_123'"); + expect(conditions[2]).toContain("'alice@x.com'"); // Delta sampling queries never reference cost; the WITH binding is // skipped to keep them small. expect(chartConfig.with).toBeUndefined(); diff --git a/packages/app/src/llm/__tests__/chartConfig.test.ts b/packages/app/src/llm/__tests__/chartConfig.test.ts index 3ed89460d8..cec84ae563 100644 --- a/packages/app/src/llm/__tests__/chartConfig.test.ts +++ b/packages/app/src/llm/__tests__/chartConfig.test.ts @@ -3,7 +3,7 @@ import { appendWhereClause, baseLLMChartConfig, buildDeltaFilterClause, - buildSessionCondition, + buildScopeCondition, buildTrimmedDeltaSelect, DELTA_ATTRIBUTE_VALUE_MAX_LENGTH, } from '@/llm/dashboard/chartConfig'; @@ -21,15 +21,24 @@ const baseProps = { whereLanguage: 'sql' as const, }; -describe('buildSessionCondition', () => { +describe('buildScopeCondition', () => { it('escapes the session id value', () => { - const condition = buildSessionCondition( + const condition = buildScopeCondition( expressions.sessionId, "ses_1'; DROP TABLE x --", ); expect(condition).toContain(expressions.sessionId); expect(condition).toContain("'ses_1\\'; DROP TABLE x --'"); }); + + it('escapes the user value', () => { + const condition = buildScopeCondition( + expressions.userId, + "alice'; DROP TABLE x --", + ); + expect(condition).toContain(expressions.userId); + expect(condition).toContain("'alice\\'; DROP TABLE x --'"); + }); }); describe('buildDeltaFilterClause', () => { @@ -123,7 +132,7 @@ describe('baseLLMChartConfig session scoping', () => { expect(config.filters).toHaveLength(2); expect(config.filters[1]).toEqual({ type: 'sql', - condition: buildSessionCondition(expressions.sessionId, 'ses_123'), + condition: buildScopeCondition(expressions.sessionId, 'ses_123'), }); }); @@ -136,6 +145,47 @@ describe('baseLLMChartConfig session scoping', () => { expect(config.filters).toHaveLength(3); expect(config.filters[2]).toEqual({ type: 'sql', condition: '1=1' }); }); +}); + +describe('baseLLMChartConfig user scoping', () => { + it('adds no user filter by default', () => { + const config = baseLLMChartConfig(baseProps); + expect(config.filters).toEqual([ + { type: 'sql', condition: expressions.isLLMSpan }, + ]); + }); + + it('appends the user filter to every chart when userId is set', () => { + const config = baseLLMChartConfig({ ...baseProps, userId: 'alice@x.com' }); + expect(config.filters).toHaveLength(2); + expect(config.filters[1]).toEqual({ + type: 'sql', + condition: buildScopeCondition(expressions.userId, 'alice@x.com'), + }); + }); + + // The two scopes are independent, so both must survive together and keep a + // stable order — extra filters are appended relative to them. + it('applies session and user together, session first, extras last', () => { + const config = baseLLMChartConfig({ + ...baseProps, + sessionId: 'ses_123', + userId: 'alice@x.com', + extraFilters: [{ type: 'sql', condition: '1=1' }], + }); + expect(config.filters).toEqual([ + { type: 'sql', condition: expressions.isLLMSpan }, + { + type: 'sql', + condition: buildScopeCondition(expressions.sessionId, 'ses_123'), + }, + { + type: 'sql', + condition: buildScopeCondition(expressions.userId, 'alice@x.com'), + }, + { type: 'sql', condition: '1=1' }, + ]); + }); it('binds the cost expression once as a WITH expression alias when opted in', () => { // The cost expression embeds the model price catalog (~70 KiB of SQL); diff --git a/packages/app/src/llm/dashboard/DistinctValueSelect.tsx b/packages/app/src/llm/dashboard/DistinctValueSelect.tsx new file mode 100644 index 0000000000..8a47d6373a --- /dev/null +++ b/packages/app/src/llm/dashboard/DistinctValueSelect.tsx @@ -0,0 +1,96 @@ +import { useMemo } from 'react'; +import { SourceKind, TTraceSource } from '@hyperdx/common-utils/dist/types'; +import { Select, SelectProps } from '@mantine/core'; + +import { useQueriedChartConfig } from '@/hooks/useChartConfig'; + +/** + * Dropdown over the distinct values of one LLM span expression in the + * searched range — the shape both dashboard-wide scope filters (session, + * user) share. + * + * Plain value/onChange: the dashboard keeps these scopes in the URL rather + * than in the search form, so they can also be set programmatically (e.g. the + * session drawer's "Filter dashboard") without two-way form sync. + */ +export function DistinctValueSelect({ + source, + valueExpression, + gateExpression, + queryKey, + allLabel, + value, + onChange, + dateRange, + ...props +}: { + source: TTraceSource | undefined; + /** Expression whose distinct values populate the dropdown. */ + valueExpression: string | undefined; + /** Scope predicate, ANDed with the LLM-span filter (e.g. hasUserId). */ + gateExpression: string | undefined; + /** Distinguishes this select's cached query from its siblings'. */ + queryKey: string; + /** Label for the "no filter" option, e.g. 'All users'. */ + allLabel: string; + value: string; + onChange: (value: string) => void; + dateRange: [Date, Date]; +} & Omit) { + const enabled = + source?.kind === SourceKind.Trace && !!valueExpression && !!gateExpression; + + const queriedConfig = { + source: source?.id, + timestampValueExpression: source?.timestampValueExpression || '', + from: { + databaseName: source?.from.databaseName || '', + tableName: source?.from.tableName || '', + }, + connection: source?.connection || '', + select: [ + { alias: 'value', valueExpression: `distinct(${valueExpression})` }, + ], + where: gateExpression || '', + whereLanguage: 'sql' as const, + limit: { limit: 10000 }, + dateRange, + }; + + const { data, isLoading, isError } = useQueriedChartConfig(queriedConfig, { + placeholderData: (prev: any) => prev, + queryKey: [queryKey, queriedConfig], + enabled, + }); + + const values = useMemo(() => { + const found = + data?.data + ?.map((d: any) => d.value) + .filter(Boolean) + .sort() || []; + return [{ value: '', label: allLabel }, ...found]; + }, [data, allLabel]); + + // Mantine does not clear the select if the value disappears from data + // (e.g. the searched window changed and the value is no longer in it). + const selected = values.some(d => + typeof d === 'string' ? d === value : d.value === value, + ); + + return ( + onChange(v ?? '')} - disabled={isLoading || isError} - comboboxProps={{ withinPortal: false }} - searchable - clearable - placeholder="All sessions" - maxDropdownHeight={280} - nothingFoundMessage={isLoading ? 'Loading more...' : 'No matches found'} + source={source} + valueExpression={expressions?.sessionId} + gateExpression={ + expressions && + `${expressions.isLLMSpan} AND ${expressions.hasSessionId}` + } + queryKey="llm-session-select" + allLabel="All sessions" /> ); } diff --git a/packages/app/src/llm/dashboard/UserSelect.tsx b/packages/app/src/llm/dashboard/UserSelect.tsx new file mode 100644 index 0000000000..b1e2ccf045 --- /dev/null +++ b/packages/app/src/llm/dashboard/UserSelect.tsx @@ -0,0 +1,40 @@ +import { TTraceSource } from '@hyperdx/common-utils/dist/types'; +import { SelectProps } from '@mantine/core'; + +import { LLMExpressions } from '@/llm/lib/expressions'; + +import { DistinctValueSelect } from './DistinctValueSelect'; + +/** + * End-user filter dropdown: distinct users seen on LLM spans in the searched + * range, resolved via the cross-dialect user expression (user.email, + * enduser.id, user.id, ai.telemetry.metadata.userId). + * + * Values come from the same expression the "Top Users" chart groups by, so a + * selection always matches the rows that produced it. + */ +export function UserSelect({ + source, + expressions, + ...props +}: { + /** Trace source: users are resolved from LLM spans. */ + source: TTraceSource | undefined; + expressions: LLMExpressions | undefined; + dateRange: [Date, Date]; + value: string; + onChange: (userId: string) => void; +} & Omit) { + return ( + + ); +} diff --git a/packages/app/src/llm/dashboard/chartConfig.ts b/packages/app/src/llm/dashboard/chartConfig.ts index 96fe69089d..188eaee50f 100644 --- a/packages/app/src/llm/dashboard/chartConfig.ts +++ b/packages/app/src/llm/dashboard/chartConfig.ts @@ -9,18 +9,21 @@ import { LLM_COST_SQL_ALIAS } from '@/llm/lib/expressions'; import { LLMChartProps } from './types'; -/** `sessionIdExpr = 'value'` condition, SQL-escaped. */ -export function buildSessionCondition( - sessionIdExpr: string, - sessionId: string, -): string { - return SqlString.format('? = ?', [SqlString.raw(sessionIdExpr), sessionId]); +/** + * `scopeExpr = 'value'` condition, SQL-escaped. + * + * Used for the dashboard-wide scope filters (session, user), whose values are + * matched exactly against a cross-dialect coalesced expression — the same + * expression the corresponding dropdown selected them from. + */ +export function buildScopeCondition(scopeExpr: string, value: string): string { + return SqlString.format('? = ?', [SqlString.raw(scopeExpr), value]); } /** * Base chart-config fields shared by every LLM dashboard chart: source * binding, the user's where clause, the LLM-span scope filter, the optional - * session-id scope, and the searched date range. + * session-id and user scopes, and the searched date range. */ export function baseLLMChartConfig({ source, @@ -29,6 +32,7 @@ export function baseLLMChartConfig({ where, whereLanguage, sessionId, + userId, extraFilters = [], withCostAlias = IS_LLM_COST_ENABLED, }: LLMChartProps & { @@ -72,10 +76,15 @@ export function baseLLMChartConfig({ ? [ { type: 'sql' as const, - condition: buildSessionCondition( - expressions.sessionId, - sessionId, - ), + condition: buildScopeCondition(expressions.sessionId, sessionId), + }, + ] + : []), + ...(userId + ? [ + { + type: 'sql' as const, + condition: buildScopeCondition(expressions.userId, userId), }, ] : []), diff --git a/packages/app/src/llm/dashboard/types.ts b/packages/app/src/llm/dashboard/types.ts index d78eeb74f0..82ddd05e00 100644 --- a/packages/app/src/llm/dashboard/types.ts +++ b/packages/app/src/llm/dashboard/types.ts @@ -18,6 +18,12 @@ export interface LLMChartProps { * via the cross-dialect session expression). */ sessionId?: string; + /** + * When set, every chart is additionally scoped to this end user (matched + * via the cross-dialect user expression, which coalesces user.email, + * enduser.id, user.id and ai.telemetry.metadata.userId). + */ + userId?: string; /** Correlated log source for LLM log events, when one is selected. */ logSource?: TLogSource; logExpressions?: LLMLogExpressions; From 32e9837ec56c1b023429704ab485af95437dd7c2 Mon Sep 17 00:00:00 2001 From: Warren Lee <5959690+wrn14897@users.noreply.github.com> Date: Wed, 9 Sep 2026 21:54:11 -0700 Subject: [PATCH 12/12] fix(llm): keep an applied scope value selected when it is not in the options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The select fell back to its placeholder whenever the applied value was absent from the fetched options, so the control read "All users" while every chart stayed filtered — an apparently unfiltered dashboard returning nothing. Absence is not only staleness. The value is also missing before the distinct-value query resolves (deep-linking into a filtered URL) and when it falls off the end of the row limit, so clearing the parent would discard a filter the user still has applied — and would write to the URL from a render driven by fetch timing, which is what keeping these selects out of the search form avoids. The applied value is appended to the options instead. Pre-existing in SessionSelect; extracting DistinctValueSelect in the previous commit made it shared, so this fixes both filters at once. Tests cover present, absent, and still-loading; the absent and loading cases fail against the old logic. --- .../__tests__/DistinctValueSelect.test.tsx | 95 +++++++++++++++++++ .../src/llm/dashboard/DistinctValueSelect.tsx | 25 +++-- 2 files changed, 110 insertions(+), 10 deletions(-) create mode 100644 packages/app/src/llm/__tests__/DistinctValueSelect.test.tsx diff --git a/packages/app/src/llm/__tests__/DistinctValueSelect.test.tsx b/packages/app/src/llm/__tests__/DistinctValueSelect.test.tsx new file mode 100644 index 0000000000..8565aaf639 --- /dev/null +++ b/packages/app/src/llm/__tests__/DistinctValueSelect.test.tsx @@ -0,0 +1,95 @@ +import { screen } from '@testing-library/react'; + +import { makeTraceSource } from '@/llm/__fixtures__/sources'; +import { DistinctValueSelect } from '@/llm/dashboard/DistinctValueSelect'; + +let mockRows: Record[] | undefined = []; +let mockLoading = false; +jest.mock('@/hooks/useChartConfig', () => ({ + useQueriedChartConfig: () => ({ + data: mockRows == null ? undefined : { data: mockRows }, + isLoading: mockLoading, + isError: false, + }), +})); + +const TRACE_SOURCE = makeTraceSource(); + +const baseProps = { + source: TRACE_SOURCE, + valueExpression: "SpanAttributes['user.email']", + gateExpression: '1=1', + queryKey: 'llm-user-select', + allLabel: 'All users', + dateRange: [new Date(0), new Date(1000)] as [Date, Date], +}; + +describe('DistinctValueSelect', () => { + beforeEach(() => { + mockRows = []; + mockLoading = false; + }); + + it('shows the applied value when it is present in the fetched options', () => { + mockRows = [{ value: 'alice@x.com' }, { value: 'bob@x.com' }]; + const onChange = jest.fn(); + renderWithMantine( + , + ); + + expect(screen.getByRole('combobox')).toHaveValue('alice@x.com'); + expect(onChange).not.toHaveBeenCalled(); + }); + + // The charts read the value from the URL, so a control that falls back to + // its placeholder here would read "All users" over a filtered dashboard — + // which looks like an unfiltered view returning nothing. + it('keeps the applied value selected when it is absent from the options', () => { + mockRows = [{ value: 'bob@x.com' }]; + const onChange = jest.fn(); + renderWithMantine( + , + ); + + expect(screen.getByRole('combobox')).toHaveValue('alice@x.com'); + // Clearing the parent here would discard a filter the user still has + // applied, so absence must never write back. + expect(onChange).not.toHaveBeenCalled(); + }); + + // Deep-linking into a filtered URL renders once before the distinct-value + // query resolves; the value is not stale then, just not fetched yet. + it('keeps the applied value selected while the options are still loading', () => { + mockRows = undefined; + mockLoading = true; + const onChange = jest.fn(); + renderWithMantine( + , + ); + + expect(screen.getByRole('combobox')).toHaveValue('alice@x.com'); + expect(onChange).not.toHaveBeenCalled(); + }); + + it('falls back to the placeholder when no value is applied', () => { + mockRows = [{ value: 'bob@x.com' }]; + renderWithMantine( + , + ); + + expect(screen.getByRole('combobox')).toHaveValue(''); + expect(screen.getByPlaceholderText('All users')).toBeInTheDocument(); + }); +}); diff --git a/packages/app/src/llm/dashboard/DistinctValueSelect.tsx b/packages/app/src/llm/dashboard/DistinctValueSelect.tsx index 8a47d6373a..fe947d79b0 100644 --- a/packages/app/src/llm/dashboard/DistinctValueSelect.tsx +++ b/packages/app/src/llm/dashboard/DistinctValueSelect.tsx @@ -64,25 +64,30 @@ export function DistinctValueSelect({ }); const values = useMemo(() => { - const found = + const found: string[] = data?.data ?.map((d: any) => d.value) .filter(Boolean) .sort() || []; - return [{ value: '', label: allLabel }, ...found]; - }, [data, allLabel]); - - // Mantine does not clear the select if the value disappears from data - // (e.g. the searched window changed and the value is no longer in it). - const selected = values.some(d => - typeof d === 'string' ? d === value : d.value === value, - ); + // Keep the applied value selectable even when it is not in `found`, so the + // control can never read "all" while the charts are still filtered. It is + // missing in three cases, only one of which is staleness: the query has + // not resolved yet (deep link into a filtered URL), the value fell off the + // end of the row limit, or the searched window no longer contains it. + // + // Clearing the parent instead would silently discard a filter in the first + // two, and would mean writing to the URL from a render driven by fetch + // timing — which is what keeping these selects out of the search form is + // meant to avoid. + const applied = value && !found.includes(value) ? [value] : []; + return [{ value: '', label: allLabel }, ...applied, ...found]; + }, [data, allLabel, value]); return (