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..60a089c37f 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,39 @@ describe('where / whereLanguage descriptions', () => { ); }); }); + +describe('defaultOrderBy', () => { + it('orders a grouped table by the first select item, descending', () => { + 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, SpanName ASC', + ); + expect( + defaultOrderBy([{ alias: 'P95 Latency' }], 'SpanName', 'table'), + ).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(); + 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..28765758a7 100644 --- a/packages/api/src/mcp/__tests__/queryTool.int.test.ts +++ b/packages/api/src/mcp/__tests__/queryTool.int.test.ts @@ -364,6 +364,132 @@ 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']); + }); + + 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']); + }, + ); + }); }); // ─── 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..5c1a1f035f 100644 --- a/packages/api/src/mcp/tools/query/table.ts +++ b/packages/api/src/mcp/tools/query/table.ts @@ -59,7 +59,32 @@ const tableSchema = z.object({ ), whereLanguage: whereLanguageSchema, groupBy: groupBySchema, - orderBy: orderBySchema, + having: z + .string() + .trim() + .max(10000) + .optional() + .describe( + 'Post-aggregation SQL HAVING condition, applied after grouping. ' + + '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, ' + + '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'.`, + ), + limit: z + .number() + .int() + .positive() + .max(10000) + .optional() + .describe( + '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, }); @@ -196,6 +221,38 @@ 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. + * + * 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. + */ +export function defaultOrderBy( + selectItems: { alias?: string; metricType?: string }[], + groupBy: string | undefined, + 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; + } + // 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`; +} + // ─── Tool registration ─────────────────────────────────────────────────────── export function registerTable({ context, registerTool }: ToolRegistrar) { @@ -270,12 +327,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 +343,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