From d8942c205c1d4190ef7113f99c8af31aed928c3e Mon Sep 17 00:00:00 2001 From: Baris Can Vural Date: Wed, 4 Nov 2026 19:49:15 +0300 Subject: [PATCH 1/3] feat(mcp): limit, having and a default order for clickstack_table clickstack_table had no limit or having, and without orderBy a grouped result came back in arbitrary order, so trimming kept arbitrary groups. Add optional limit and having (passed into the chart config through runConfigTile options, MCP only), and order grouped tables by the first select item, descending, when no orderBy is given. Dashboard tiles and the app are unchanged. Refs #3555. --- .changeset/mcp-table-limit-having-order.md | 5 ++ packages/api/src/mcp/__tests__/query.test.ts | 27 ++++++- .../src/mcp/__tests__/queryTool.int.test.ts | 75 +++++++++++++++++++ packages/api/src/mcp/tools/query/helpers.ts | 18 +++++ packages/api/src/mcp/tools/query/table.ts | 56 +++++++++++++- 5 files changed, 177 insertions(+), 4 deletions(-) create mode 100644 .changeset/mcp-table-limit-having-order.md diff --git a/.changeset/mcp-table-limit-having-order.md b/.changeset/mcp-table-limit-having-order.md new file mode 100644 index 0000000000..b03351d002 --- /dev/null +++ b/.changeset/mcp-table-limit-having-order.md @@ -0,0 +1,5 @@ +--- +'@hyperdx/api': patch +--- + +feat(mcp): `clickstack_table` takes optional `limit` and `having`, and orders grouped results by the first select item, descending, when no `orderBy` is given diff --git a/packages/api/src/mcp/__tests__/query.test.ts b/packages/api/src/mcp/__tests__/query.test.ts index 8d4ea7a421..797f1c5327 100644 --- a/packages/api/src/mcp/__tests__/query.test.ts +++ b/packages/api/src/mcp/__tests__/query.test.ts @@ -31,7 +31,7 @@ import { whereLanguageSchema, whereSchema, } from '@/mcp/tools/query/schemas'; -import { resolveOrderBy } from '@/mcp/tools/query/table'; +import { defaultOrderBy, resolveOrderBy } from '@/mcp/tools/query/table'; import { getErrorCategory } from '@/mcp/utils/errors'; describe('parseTimeRange', () => { @@ -1430,3 +1430,28 @@ describe('where / whereLanguage descriptions', () => { ); }); }); + +describe('defaultOrderBy', () => { + it('orders a grouped table by the first select item, descending', () => { + expect(defaultOrderBy([{}, {}], 'SpanName', 'table')).toBe('1 DESC'); + }); + + it('uses the first select item alias when it has one', () => { + expect(defaultOrderBy([{ alias: 'Errors' }], 'SpanName', 'table')).toBe( + 'Errors DESC', + ); + expect( + defaultOrderBy([{ alias: 'P95 Latency' }], 'SpanName', 'table'), + ).toBe('"P95 Latency" DESC'); + }); + + it('adds no order without a groupBy, for other shapes, or for metric selects', () => { + expect(defaultOrderBy([{}], undefined, 'table')).toBeUndefined(); + expect(defaultOrderBy([{}], ' ', 'table')).toBeUndefined(); + expect(defaultOrderBy([{}], 'SpanName', 'pie')).toBeUndefined(); + expect(defaultOrderBy([{}], 'SpanName', 'number')).toBeUndefined(); + expect( + defaultOrderBy([{ metricType: 'gauge' }], 'SpanName', 'table'), + ).toBeUndefined(); + }); +}); diff --git a/packages/api/src/mcp/__tests__/queryTool.int.test.ts b/packages/api/src/mcp/__tests__/queryTool.int.test.ts index c481324a44..7c0754a625 100644 --- a/packages/api/src/mcp/__tests__/queryTool.int.test.ts +++ b/packages/api/src/mcp/__tests__/queryTool.int.test.ts @@ -364,6 +364,81 @@ describe('MCP Query Tools', () => { expect(result.isError).toBeFalsy(); expect(result.content).toHaveLength(1); }); + + describe('having, limit and the default order', () => { + const now = Date.now(); + const tableArgs = () => ({ + sourceId: logSource._id.toString(), + select: [{ aggFn: 'count', alias: 'n' }], + groupBy: 'ServiceName', + startTime: new Date(now - 60 * 60 * 1000).toISOString(), + endTime: new Date(now + 60 * 1000).toISOString(), + }); + const rows = (result: Awaited>) => { + const parsed: { result: { data: { ServiceName: string }[] } } = + JSON.parse(getFirstText(result)); + return parsed.result.data; + }; + + beforeEach(async () => { + // a: 1 row, b: 5 rows, c: 3 rows + const logs: Parameters[0] = []; + for (const [service, count] of [ + ['a', 1], + ['b', 5], + ['c', 3], + ] as const) { + for (let i = 0; i < count; i++) { + logs.push({ + Body: `event ${i}`, + ServiceName: service, + SeverityText: 'INFO', + Timestamp: new Date(now - 60 * 1000 + i), + }); + } + } + await bulkInsertLogs(logs); + }); + + it('orders by the first select item, descending, when orderBy is omitted', async () => { + const result = await callTool(client, 'clickstack_table', tableArgs()); + expect(result.isError).toBeFalsy(); + expect(rows(result).map(r => r.ServiceName)).toEqual(['b', 'c', 'a']); + }); + + it('orders by position when the first select item has no alias', async () => { + const result = await callTool(client, 'clickstack_table', { + ...tableArgs(), + select: [{ aggFn: 'count' }], + }); + expect(result.isError).toBeFalsy(); + expect(rows(result).map(r => r.ServiceName)).toEqual(['b', 'c', 'a']); + }); + + it('keeps an explicit orderBy', async () => { + const result = await callTool(client, 'clickstack_table', { + ...tableArgs(), + orderBy: 'ServiceName ASC', + }); + expect(rows(result).map(r => r.ServiceName)).toEqual(['a', 'b', 'c']); + }); + + it('applies limit after ordering', async () => { + const result = await callTool(client, 'clickstack_table', { + ...tableArgs(), + limit: 2, + }); + expect(rows(result).map(r => r.ServiceName)).toEqual(['b', 'c']); + }); + + it('applies having to the groups', async () => { + const result = await callTool(client, 'clickstack_table', { + ...tableArgs(), + having: 'n > 1', + }); + expect(rows(result).map(r => r.ServiceName)).toEqual(['b', 'c']); + }); + }); }); // ─── clickstack_search ───────────────────────────────────────────────────────── diff --git a/packages/api/src/mcp/tools/query/helpers.ts b/packages/api/src/mcp/tools/query/helpers.ts index dd3dde0d4e..0ea157aef4 100644 --- a/packages/api/src/mcp/tools/query/helpers.ts +++ b/packages/api/src/mcp/tools/query/helpers.ts @@ -423,6 +423,10 @@ export async function runConfigTile( variables?: ChartVariable[]; /** Serialized size the result is trimmed to. Defaults to trimToolResponse's limit. */ maxResponseChars?: number; + /** Post-aggregation SQL HAVING condition for builder tiles (MCP table tool). */ + having?: string; + /** SQL LIMIT for builder tiles that aren't searches (MCP table tool). */ + limit?: number; /** Source lookup for raw SQL tiles, so a batch can share one per-call cache. */ lookupSource?: (sourceId: string) => ReturnType; }, @@ -583,9 +587,23 @@ export async function runConfigTile( ? { granularity: options?.granularity ?? 'auto' } : {}; + // having/limit come from the MCP table tool's own inputs, not from the + // tile, so dashboard tiles and the app keep their current behaviour. + const tableToolOverrides = isSearch + ? {} + : { + ...(options?.having + ? { having: options.having, havingLanguage: 'sql' as const } + : {}), + ...(options?.limit + ? { limit: { limit: options.limit, offset: 0 } } + : {}), + }; + const chartConfig = { ...builderConfig, ...searchOverrides, + ...tableToolOverrides, ...metricSelectOverrides, ...granularityOverride, from: { diff --git a/packages/api/src/mcp/tools/query/table.ts b/packages/api/src/mcp/tools/query/table.ts index 6c757bd33e..804042a7a8 100644 --- a/packages/api/src/mcp/tools/query/table.ts +++ b/packages/api/src/mcp/tools/query/table.ts @@ -59,7 +59,29 @@ const tableSchema = z.object({ ), whereLanguage: whereLanguageSchema, groupBy: groupBySchema, - orderBy: orderBySchema, + having: z + .string() + .max(10000) + .optional() + .describe( + 'Post-aggregation SQL HAVING condition, applied after grouping. ' + + 'Example: "count() > 100" to drop small groups. ' + + 'Reference a select item by its alias (e.g. "Errors > 10") or repeat the aggregate.', + ), + orderBy: orderBySchema.describe( + 'Sort results by this column. Defaults to the first select item, descending, ' + + 'when groupBy is set. ' + + 'When ordering by an alias that contains spaces or special characters, ' + + `wrap the alias in quotes: e.g. '"P95 Latency" DESC'.`, + ), + limit: z + .number() + .int() + .positive() + .optional() + .describe( + 'Maximum number of rows (SQL LIMIT), applied after ordering. Omit to return every group.', + ), startTime: startTimeSchema, endTime: endTimeSchema, }); @@ -196,6 +218,30 @@ export function resolveOrderBy( return orderBy; } +/** + * Default ORDER BY for grouped tables when the caller sets none: the first + * select item, descending, so trimmed or limited results keep the largest + * groups. MCP only; dashboard tiles and the app are unchanged. + * + * Unaliased select items have no name to refer to, so the first one is + * referenced by position (`1`): the rendered SELECT lists the select items + * first, then the groupBy columns. Metric sources are skipped: their SQL is + * built from CTEs, and `increase` already orders its top-N groups. + */ +/** @internal Exported for testing only. */ +export function defaultOrderBy( + selectItems: { alias?: string; metricType?: string }[], + groupBy: string | undefined, + displayType: string, +): string | undefined { + if (displayType !== 'table' || !groupBy?.trim()) return undefined; + if (selectItems.length === 0 || selectItems.some(s => s.metricType)) { + return undefined; + } + const alias = selectItems[0].alias?.trim(); + return alias ? `${quoteAliasForOrderBy(alias)} DESC` : '1 DESC'; +} + // ─── Tool registration ─────────────────────────────────────────────────────── export function registerTable({ context, registerTool }: ToolRegistrar) { @@ -270,12 +316,15 @@ export function registerTable({ context, registerTool }: ToolRegistrar) { const { items: selectItems, warnings: mergeWarnings } = mergeWhereIntoSelectItems(select, input.where, input.whereLanguage); + const groupBy = displayType === 'number' ? undefined : input.groupBy; const tile = buildTile('MCP Table', 12, 4, { displayType, sourceId: input.sourceId, select: selectItems, - groupBy: displayType === 'number' ? undefined : input.groupBy, - orderBy: resolveOrderBy(input.orderBy, selectItems), + groupBy, + orderBy: + resolveOrderBy(input.orderBy, selectItems) ?? + defaultOrderBy(selectItems, groupBy, displayType), }); const result = await runConfigTile( @@ -283,6 +332,7 @@ export function registerTable({ context, registerTool }: ToolRegistrar) { tile, startDate, endDate, + { having: input.having, limit: input.limit }, ); // Surface language-mismatch warnings so the agent knows the top-level From e3f65a122a081eb7499d8ec567fe583ab50de0b6 Mon Sep 17 00:00:00 2001 From: Baris Can Vural Date: Wed, 4 Nov 2026 22:02:38 +0300 Subject: [PATCH 2/3] fix(mcp): stable tiebreak for the default table order, narrower orderBy/having descriptions, trim having Add the groupBy columns as an ASC tiebreak after the default first-item DESC, as convertToCategoricalChartConfig does, so a limit keeps the same groups. Say the default applies to shape "table" on non-metric sources, recommend referencing select aliases in having, and trim having so a blank value is dropped. --- packages/api/src/mcp/__tests__/query.test.ts | 8 +++++--- packages/api/src/mcp/tools/query/table.ts | 18 ++++++++++++------ 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/packages/api/src/mcp/__tests__/query.test.ts b/packages/api/src/mcp/__tests__/query.test.ts index 797f1c5327..6e3a027876 100644 --- a/packages/api/src/mcp/__tests__/query.test.ts +++ b/packages/api/src/mcp/__tests__/query.test.ts @@ -1433,16 +1433,18 @@ describe('where / whereLanguage descriptions', () => { describe('defaultOrderBy', () => { it('orders a grouped table by the first select item, descending', () => { - expect(defaultOrderBy([{}, {}], 'SpanName', 'table')).toBe('1 DESC'); + expect(defaultOrderBy([{}, {}], 'SpanName', 'table')).toBe( + '1 DESC, SpanName ASC', + ); }); it('uses the first select item alias when it has one', () => { expect(defaultOrderBy([{ alias: 'Errors' }], 'SpanName', 'table')).toBe( - 'Errors DESC', + 'Errors DESC, SpanName ASC', ); expect( defaultOrderBy([{ alias: 'P95 Latency' }], 'SpanName', 'table'), - ).toBe('"P95 Latency" DESC'); + ).toBe('"P95 Latency" DESC, SpanName ASC'); }); it('adds no order without a groupBy, for other shapes, or for metric selects', () => { diff --git a/packages/api/src/mcp/tools/query/table.ts b/packages/api/src/mcp/tools/query/table.ts index 804042a7a8..dc98bd5d65 100644 --- a/packages/api/src/mcp/tools/query/table.ts +++ b/packages/api/src/mcp/tools/query/table.ts @@ -61,16 +61,17 @@ const tableSchema = z.object({ groupBy: groupBySchema, having: z .string() + .trim() .max(10000) .optional() .describe( 'Post-aggregation SQL HAVING condition, applied after grouping. ' + - 'Example: "count() > 100" to drop small groups. ' + - 'Reference a select item by its alias (e.g. "Errors > 10") or repeat the aggregate.', + 'Reference a select item by its alias, e.g. "Errors > 10" to drop small groups. ' + + 'A bare aggregate such as count() ignores the top-level where, so prefer the alias.', ), orderBy: orderBySchema.describe( - 'Sort results by this column. Defaults to the first select item, descending, ' + - 'when groupBy is set. ' + + 'Sort results by this column. For shape "table" on a non-metric source with groupBy set, ' + + 'defaults to the first select item descending, then the groupBy columns. ' + 'When ordering by an alias that contains spaces or special characters, ' + `wrap the alias in quotes: e.g. '"P95 Latency" DESC'.`, ), @@ -227,8 +228,12 @@ export function resolveOrderBy( * referenced by position (`1`): the rendered SELECT lists the select items * first, then the groupBy columns. Metric sources are skipped: their SQL is * built from CTEs, and `increase` already orders its top-N groups. + * + * The groupBy columns follow as a tiebreak, so a `limit` keeps the same groups + * on every call, as convertToCategoricalChartConfig does for pie/bar. + * + * @internal Exported for testing only. */ -/** @internal Exported for testing only. */ export function defaultOrderBy( selectItems: { alias?: string; metricType?: string }[], groupBy: string | undefined, @@ -239,7 +244,8 @@ export function defaultOrderBy( return undefined; } const alias = selectItems[0].alias?.trim(); - return alias ? `${quoteAliasForOrderBy(alias)} DESC` : '1 DESC'; + const first = alias ? `${quoteAliasForOrderBy(alias)} DESC` : '1 DESC'; + return `${first}, ${groupBy.trim()} ASC`; } // ─── Tool registration ─────────────────────────────────────────────────────── From b3a2660f782fa3bdfdaa7cfd28b9bdebb9b43d64 Mon Sep 17 00:00:00 2001 From: Baris Can Vural Date: Wed, 4 Nov 2026 22:13:45 +0300 Subject: [PATCH 3/3] fix(mcp): keep the alias as SELECT renders it in the default order; test limit/having on pie and bar; cap limit Quote the first select alias untrimmed so ORDER BY names the rendered column. Add integration tests for limit and having on the pie/bar shapes (through convertToCategoricalChartConfig) and for a quoted alias with a space. Cap limit at 10,000, say metric sources need an explicit orderBy with limit, and type defaultOrderBy's displayType as the shape union. --- packages/api/src/mcp/__tests__/query.test.ts | 9 ++++ .../src/mcp/__tests__/queryTool.int.test.ts | 51 +++++++++++++++++++ packages/api/src/mcp/tools/query/table.ts | 17 ++++--- 3 files changed, 71 insertions(+), 6 deletions(-) diff --git a/packages/api/src/mcp/__tests__/query.test.ts b/packages/api/src/mcp/__tests__/query.test.ts index 6e3a027876..60a089c37f 100644 --- a/packages/api/src/mcp/__tests__/query.test.ts +++ b/packages/api/src/mcp/__tests__/query.test.ts @@ -1447,6 +1447,15 @@ describe('defaultOrderBy', () => { ).toBe('"P95 Latency" DESC, SpanName ASC'); }); + it('quotes an alias with surrounding spaces as SELECT renders it', () => { + expect(defaultOrderBy([{ alias: ' Errors' }], 'SpanName', 'table')).toBe( + '" Errors" DESC, SpanName ASC', + ); + expect(defaultOrderBy([{ alias: ' ' }], 'SpanName', 'table')).toBe( + '1 DESC, SpanName ASC', + ); + }); + it('adds no order without a groupBy, for other shapes, or for metric selects', () => { expect(defaultOrderBy([{}], undefined, 'table')).toBeUndefined(); expect(defaultOrderBy([{}], ' ', 'table')).toBeUndefined(); diff --git a/packages/api/src/mcp/__tests__/queryTool.int.test.ts b/packages/api/src/mcp/__tests__/queryTool.int.test.ts index 7c0754a625..28765758a7 100644 --- a/packages/api/src/mcp/__tests__/queryTool.int.test.ts +++ b/packages/api/src/mcp/__tests__/queryTool.int.test.ts @@ -438,6 +438,57 @@ describe('MCP Query Tools', () => { }); expect(rows(result).map(r => r.ServiceName)).toEqual(['b', 'c']); }); + + it('orders by a quoted alias with a space, by default and explicitly', async () => { + const select = [{ aggFn: 'count', alias: 'Line Count' }]; + const byDefault = await callTool(client, 'clickstack_table', { + ...tableArgs(), + select, + }); + expect(byDefault.isError).toBeFalsy(); + expect(rows(byDefault).map(r => r.ServiceName)).toEqual([ + 'b', + 'c', + 'a', + ]); + const explicit = await callTool(client, 'clickstack_table', { + ...tableArgs(), + select, + orderBy: '"Line Count" ASC', + }); + expect(explicit.isError).toBeFalsy(); + expect(rows(explicit).map(r => r.ServiceName)).toEqual(['a', 'c', 'b']); + }); + + it.each(['pie', 'bar'] as const)( + 'keeps the largest groups with limit on shape "%s"', + async shape => { + const result = await callTool(client, 'clickstack_table', { + ...tableArgs(), + shape, + limit: 2, + }); + expect(result.isError).toBeFalsy(); + expect(rows(result).map(r => r.ServiceName)).toEqual(['b', 'c']); + }, + ); + + it.each(['pie', 'bar'] as const)( + 'applies having on shape "%s"', + async shape => { + const result = await callTool(client, 'clickstack_table', { + ...tableArgs(), + shape, + having: 'n > 1', + }); + expect(result.isError).toBeFalsy(); + expect( + rows(result) + .map(r => r.ServiceName) + .sort(), + ).toEqual(['b', 'c']); + }, + ); }); }); diff --git a/packages/api/src/mcp/tools/query/table.ts b/packages/api/src/mcp/tools/query/table.ts index dc98bd5d65..5c1a1f035f 100644 --- a/packages/api/src/mcp/tools/query/table.ts +++ b/packages/api/src/mcp/tools/query/table.ts @@ -66,8 +66,8 @@ const tableSchema = z.object({ .optional() .describe( 'Post-aggregation SQL HAVING condition, applied after grouping. ' + - 'Reference a select item by its alias, e.g. "Errors > 10" to drop small groups. ' + - 'A bare aggregate such as count() ignores the top-level where, so prefer the alias.', + 'Give the select item an alias and reference it, e.g. alias "Errors" and having "Errors > 10". ' + + 'A bare aggregate such as count() ignores the top-level where.', ), orderBy: orderBySchema.describe( 'Sort results by this column. For shape "table" on a non-metric source with groupBy set, ' + @@ -79,9 +79,11 @@ const tableSchema = z.object({ .number() .int() .positive() + .max(10000) .optional() .describe( - 'Maximum number of rows (SQL LIMIT), applied after ordering. Omit to return every group.', + 'Maximum number of rows (SQL LIMIT), applied after ordering. Omit to return every group. ' + + 'Metric sources get no default order, so set orderBy as well, or the kept groups are arbitrary.', ), startTime: startTimeSchema, endTime: endTimeSchema, @@ -237,14 +239,17 @@ export function resolveOrderBy( export function defaultOrderBy( selectItems: { alias?: string; metricType?: string }[], groupBy: string | undefined, - displayType: string, + displayType: 'table' | 'number' | 'pie' | 'bar', ): string | undefined { if (displayType !== 'table' || !groupBy?.trim()) return undefined; if (selectItems.length === 0 || selectItems.some(s => s.metricType)) { return undefined; } - const alias = selectItems[0].alias?.trim(); - const first = alias ? `${quoteAliasForOrderBy(alias)} DESC` : '1 DESC'; + // Quote the alias exactly as SELECT renders it; trim only to test emptiness. + const alias = selectItems[0].alias; + const first = alias?.trim() + ? `${quoteAliasForOrderBy(alias)} DESC` + : '1 DESC'; return `${first}, ${groupBy.trim()} ASC`; }