diff --git a/.changeset/mcp-sql-tile-macro-hints.md b/.changeset/mcp-sql-tile-macro-hints.md new file mode 100644 index 0000000000..1bf70fbf95 --- /dev/null +++ b/.changeset/mcp-sql-tile-macro-hints.md @@ -0,0 +1,6 @@ +--- +"@hyperdx/api": patch +"@hyperdx/common-utils": patch +--- + +fix: Nudge agents towards macros in raw SQL tiles diff --git a/packages/api/src/mcp/__tests__/dashboards/patchDashboard.test.ts b/packages/api/src/mcp/__tests__/dashboards/patchDashboard.test.ts index dae74ab500..67bc8579f6 100644 --- a/packages/api/src/mcp/__tests__/dashboards/patchDashboard.test.ts +++ b/packages/api/src/mcp/__tests__/dashboards/patchDashboard.test.ts @@ -650,4 +650,115 @@ describe('MCP Dashboard Tools - clickstack_patch_dashboard', () => { { operator: 'gte', value: 500, color: 'chart-error' }, ]); }); + + describe('raw SQL macro warnings', () => { + // Patching a tile to a macro-less raw SQL config succeeds (non-blocking) + // but surfaces an advisory `warnings` array on the response. + it('returns a non-blocking warning when a patched tile omits macros', async () => { + const sourceId = ctx.traceSource._id.toString(); + const connectionId = ctx.connection._id.toString(); + const createResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Patch to static SQL', + tiles: [ + { + name: 'Builder Tile', + config: { + displayType: 'number', + sourceId, + select: [{ aggFn: 'count' }], + }, + }, + ], + }, + ); + const created = JSON.parse(getFirstText(createResult)); + const tileId = created.tiles[0].id; + + const patchResult = await callTool( + ctx.client!, + 'clickstack_patch_dashboard', + { + dashboardId: created.id, + tileId, + tile: { + name: 'Static SQL', + config: { + configType: 'sql', + displayType: 'table', + connectionId, + sqlTemplate: 'SELECT 1 AS value LIMIT 1', + }, + }, + }, + ); + + // Non-blocking: the patch applies and the tile is persisted. + expect(patchResult.isError).toBeFalsy(); + const output = JSON.parse(getFirstText(patchResult)); + expect(output.patchedTile.name).toBe('Static SQL'); + const dashboard = await Dashboard.findById(created.id); + const persistedConfig = dashboard?.tiles?.[0]?.config as { + sqlTemplate?: string; + }; + expect(persistedConfig?.sqlTemplate).toBe('SELECT 1 AS value LIMIT 1'); + + expect(Array.isArray(output.warnings)).toBe(true); + expect(output.warnings).toHaveLength(1); + expect(output.warnings[0]).toContain('Static SQL'); + expect(output.warnings[0]).toContain('$__timeFilter'); + expect(output.warnings[0]).toContain('strongly recommended'); + }); + + it('omits warnings when a patched raw SQL tile uses all recommended macros', async () => { + const sourceId = ctx.traceSource._id.toString(); + const connectionId = ctx.connection._id.toString(); + const createResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Patch to macro SQL', + tiles: [ + { + name: 'Builder Tile', + config: { + displayType: 'number', + sourceId, + select: [{ aggFn: 'count' }], + }, + }, + ], + }, + ); + const created = JSON.parse(getFirstText(createResult)); + const tileId = created.tiles[0].id; + + const patchResult = await callTool( + ctx.client!, + 'clickstack_patch_dashboard', + { + dashboardId: created.id, + tileId, + tile: { + name: 'Macro SQL', + config: { + configType: 'sql', + displayType: 'table', + connectionId, + sourceId, + sqlTemplate: + 'SELECT ServiceName, count() AS c FROM $__sourceTable ' + + 'WHERE $__timeFilter(Timestamp) AND $__filters GROUP BY ServiceName LIMIT 10', + }, + }, + }, + ); + + expect(patchResult.isError).toBeFalsy(); + const output = JSON.parse(getFirstText(patchResult)); + expect(output.warnings).toBeUndefined(); + }); + }); }); diff --git a/packages/api/src/mcp/__tests__/dashboards/queryTile.test.ts b/packages/api/src/mcp/__tests__/dashboards/queryTile.test.ts index 3eb44a8676..2e05ba5808 100644 --- a/packages/api/src/mcp/__tests__/dashboards/queryTile.test.ts +++ b/packages/api/src/mcp/__tests__/dashboards/queryTile.test.ts @@ -107,4 +107,88 @@ describe('MCP Dashboard Tools - clickstack_query_tile', () => { expect(result.isError).toBeFalsy(); expect(result.content).toHaveLength(1); }); + + describe('raw SQL macro warnings', () => { + it('attaches a non-blocking warning when a raw SQL tile omits macros', async () => { + const connectionId = ctx.connection._id.toString(); + const createResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Query macro-less SQL tile', + tiles: [ + { + name: 'Static SQL', + config: { + configType: 'sql', + displayType: 'table', + connectionId, + // Runs fine against ClickHouse but uses no time-range / + // filter / source macros. + sqlTemplate: 'SELECT 1 AS value LIMIT 1', + }, + }, + ], + }, + ); + const dashboard = JSON.parse(getFirstText(createResult)); + + const result = await callTool(ctx.client!, 'clickstack_query_tile', { + dashboardId: dashboard.id, + tileId: dashboard.tiles[0].id, + startTime: new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString(), + endTime: new Date().toISOString(), + }); + + // Non-blocking: the query still executes and returns its row. + expect(result.isError).toBeFalsy(); + const parsed = JSON.parse(getFirstText(result)); + expect(parsed.result).toBeDefined(); + + // The advisory rides alongside the result. + expect(Array.isArray(parsed.warnings)).toBe(true); + expect(parsed.warnings).toHaveLength(1); + expect(parsed.warnings[0]).toContain('Static SQL'); + expect(parsed.warnings[0]).toContain('$__timeFilter'); + expect(parsed.warnings[0]).toContain('strongly recommended'); + }); + + it('does not attach warnings when a raw SQL tile uses all recommended macros', async () => { + const connectionId = ctx.connection._id.toString(); + const sourceId = ctx.traceSource._id.toString(); + const createResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Query macro SQL tile', + tiles: [ + { + name: 'Macro SQL', + config: { + configType: 'sql', + displayType: 'table', + connectionId, + sourceId, + sqlTemplate: + 'SELECT ServiceName, count() AS c FROM $__sourceTable ' + + 'WHERE $__timeFilter(Timestamp) AND $__filters GROUP BY ServiceName LIMIT 10', + }, + }, + ], + }, + ); + const dashboard = JSON.parse(getFirstText(createResult)); + + const result = await callTool(ctx.client!, 'clickstack_query_tile', { + dashboardId: dashboard.id, + tileId: dashboard.tiles[0].id, + startTime: new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString(), + endTime: new Date().toISOString(), + }); + + expect(result.isError).toBeFalsy(); + const parsed = JSON.parse(getFirstText(result)); + expect(parsed.warnings).toBeUndefined(); + }); + }); }); diff --git a/packages/api/src/mcp/__tests__/dashboards/saveDashboard.test.ts b/packages/api/src/mcp/__tests__/dashboards/saveDashboard.test.ts index b329eb6ed2..5f6974eaad 100644 --- a/packages/api/src/mcp/__tests__/dashboards/saveDashboard.test.ts +++ b/packages/api/src/mcp/__tests__/dashboards/saveDashboard.test.ts @@ -1142,6 +1142,157 @@ describe('MCP Dashboard Tools - clickstack_save_dashboard', () => { }); }); + describe('raw SQL macro warnings', () => { + // These warnings are advisory: a raw SQL tile that omits the dashboard + // time-range / filter / source macros still saves successfully, but the + // response carries a `warnings` array so the agent can spot a tile that + // won't react to dashboard controls. + it('returns a non-blocking warning when a raw SQL tile omits macros on create', async () => { + const connectionId = ctx.connection._id.toString(); + const result = await callTool(ctx.client!, 'clickstack_save_dashboard', { + name: 'SQL Dashboard without macros', + tiles: [ + { + name: 'Static SQL', + config: { + configType: 'sql', + displayType: 'table', + connectionId, + sqlTemplate: 'SELECT 1 AS value LIMIT 1', + }, + }, + ], + }); + + // Non-blocking: the save succeeds and the dashboard is persisted. + expect(result.isError).toBeFalsy(); + const output = JSON.parse(getFirstText(result)); + expect(output.id).toBeDefined(); + expect(output.tiles).toHaveLength(1); + const dashboard = await Dashboard.findById(output.id); + expect(dashboard).not.toBeNull(); + + // The advisory names the tile and recommends the missing macros. + expect(Array.isArray(output.warnings)).toBe(true); + expect(output.warnings).toHaveLength(1); + expect(output.warnings[0]).toContain('Static SQL'); + expect(output.warnings[0]).toContain('$__timeFilter'); + expect(output.warnings[0]).toContain('$__filters'); + expect(output.warnings[0]).toContain('$__sourceTable'); + expect(output.warnings[0]).toContain('strongly recommended'); + }); + + it('omits warnings when a raw SQL tile uses all recommended macros', async () => { + const connectionId = ctx.connection._id.toString(); + const sourceId = ctx.traceSource._id.toString(); + const result = await callTool(ctx.client!, 'clickstack_save_dashboard', { + name: 'SQL Dashboard with macros', + tiles: [ + { + name: 'Macro SQL', + config: { + configType: 'sql', + displayType: 'table', + connectionId, + sourceId, + sqlTemplate: + 'SELECT ServiceName, count() AS c FROM $__sourceTable ' + + 'WHERE $__timeFilter(Timestamp) AND $__filters GROUP BY ServiceName LIMIT 10', + }, + }, + ], + }); + + expect(result.isError).toBeFalsy(); + const output = JSON.parse(getFirstText(result)); + expect(output.warnings).toBeUndefined(); + }); + + it('flags a missing interval macro on a time-series raw SQL tile', async () => { + const connectionId = ctx.connection._id.toString(); + const sourceId = ctx.traceSource._id.toString(); + const result = await callTool(ctx.client!, 'clickstack_save_dashboard', { + name: 'SQL line tile without interval', + tiles: [ + { + name: 'Line SQL', + config: { + configType: 'sql', + displayType: 'line', + connectionId, + sourceId, + // Has time-range + filters + source table, but buckets by a + // fixed minute instead of $__timeInterval, so granularity is + // ignored. + sqlTemplate: + 'SELECT toStartOfMinute(Timestamp) AS ts, count() AS c FROM $__sourceTable ' + + 'WHERE $__timeFilter(Timestamp) AND $__filters GROUP BY ts ORDER BY ts', + }, + }, + ], + }); + + expect(result.isError).toBeFalsy(); + const output = JSON.parse(getFirstText(result)); + expect(Array.isArray(output.warnings)).toBe(true); + expect(output.warnings).toHaveLength(1); + expect(output.warnings[0]).toContain('$__timeInterval'); + }); + + it('returns a non-blocking warning when a raw SQL tile omits macros on update', async () => { + const connectionId = ctx.connection._id.toString(); + const sourceId = ctx.traceSource._id.toString(); + + // Create with a well-formed builder tile (no warnings expected). + const createResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Update to static SQL', + tiles: [ + { + name: 'Builder Tile', + config: { + displayType: 'number', + sourceId, + select: [{ aggFn: 'count' }], + }, + }, + ], + }, + ); + const created = JSON.parse(getFirstText(createResult)); + expect(created.warnings).toBeUndefined(); + + // Update, replacing the tile with a macro-less raw SQL tile. + const updateResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + id: created.id, + name: 'Update to static SQL', + tiles: [ + { + name: 'Static SQL', + config: { + configType: 'sql', + displayType: 'table', + connectionId, + sqlTemplate: 'SELECT 1 AS value LIMIT 1', + }, + }, + ], + }, + ); + + expect(updateResult.isError).toBeFalsy(); + const updated = JSON.parse(getFirstText(updateResult)); + expect(updated.id).toBe(created.id); + expect(Array.isArray(updated.warnings)).toBe(true); + expect(updated.warnings[0]).toContain('Static SQL'); + }); + }); + describe('containers and tabs', () => { // Mirrors the v2 external-API "Containers and tabs" describe block so // the MCP path enforces the same 5 cross-field rules: container id diff --git a/packages/api/src/mcp/__tests__/dashboards/validation.test.ts b/packages/api/src/mcp/__tests__/dashboards/validation.test.ts index 88d0a1910f..5190249a0d 100644 --- a/packages/api/src/mcp/__tests__/dashboards/validation.test.ts +++ b/packages/api/src/mcp/__tests__/dashboards/validation.test.ts @@ -4,6 +4,7 @@ import type { ExternalDashboardTileWithId } from '@/utils/zod'; import { getRawSqlMissingSourceError, + getRawSqlTileMacroWarnings, getRawSqlTilesMissingRequiredSource, } from '../../tools/dashboards/validation'; @@ -14,6 +15,7 @@ function makeSqlTile(overrides: { name?: string; sqlTemplate: string; sourceId?: string; + displayType?: string; }): ExternalDashboardTileWithId { return { id: 'sql-tile', @@ -24,7 +26,7 @@ function makeSqlTile(overrides: { name: overrides.name ?? 'SQL Tile', config: { configType: 'sql', - displayType: 'table', + displayType: overrides.displayType ?? 'table', connectionId, sqlTemplate: overrides.sqlTemplate, ...(overrides.sourceId ? { sourceId: overrides.sourceId } : {}), @@ -167,3 +169,143 @@ describe('getRawSqlMissingSourceError', () => { expect(message).toContain('Errors by Service (uses $__filters)'); }); }); + +describe('getRawSqlTileMacroHints', () => { + it('flags a tile with no time-range macro', () => { + const hints = getRawSqlTileMacroWarnings([ + makeSqlTile({ + name: 'Static Count', + sqlTemplate: 'SELECT count() FROM otel_traces LIMIT 1', + }), + ]); + expect(hints).toHaveLength(1); + expect(hints[0]).toContain('Static Count'); + expect(hints[0]).toContain('$__timeFilter'); + expect(hints[0]).toContain('strongly recommended'); + }); + + it('does not flag a table tile that uses all recommended macros', () => { + expect( + getRawSqlTileMacroWarnings([ + makeSqlTile({ + sourceId, + sqlTemplate: + 'SELECT count() FROM $__sourceTable WHERE $__timeFilter(Timestamp) AND $__filters LIMIT 10', + }), + ]), + ).toEqual([]); + }); + + it('accepts any time-range macro to satisfy the time-range check', () => { + // $__fromTime / $__toTime are an acceptable substitute for $__timeFilter. + expect( + getRawSqlTileMacroWarnings([ + makeSqlTile({ + sourceId, + sqlTemplate: + 'SELECT count() FROM $__sourceTable ' + + 'WHERE Timestamp >= $__fromTime AND Timestamp <= $__toTime AND $__filters LIMIT 10', + }), + ]), + ).toEqual([]); + }); + + it('flags a time-series tile that omits an interval macro', () => { + const hints = getRawSqlTileMacroWarnings([ + makeSqlTile({ + name: 'Requests Over Time', + displayType: 'line', + sourceId, + sqlTemplate: + 'SELECT toStartOfMinute(Timestamp) AS ts, count() FROM $__sourceTable ' + + 'WHERE $__timeFilter(Timestamp) AND $__filters GROUP BY ts', + }), + ]); + expect(hints).toHaveLength(1); + expect(hints[0]).toContain('$__timeInterval'); + }); + + it('does not flag an interval macro on non-time-series tiles', () => { + // A table tile does not need $__timeInterval. + expect( + getRawSqlTileMacroWarnings([ + makeSqlTile({ + displayType: 'table', + sourceId, + sqlTemplate: + 'SELECT count() FROM $__sourceTable WHERE $__timeFilter(Timestamp) AND $__filters', + }), + ]), + ).toEqual([]); + }); + + it('does not flag a time-series tile that includes an interval macro', () => { + expect( + getRawSqlTileMacroWarnings([ + makeSqlTile({ + displayType: 'stacked_bar', + sourceId, + sqlTemplate: + 'SELECT $__timeInterval(Timestamp) AS ts, count() FROM $__sourceTable ' + + 'WHERE $__timeFilter(Timestamp) AND $__filters GROUP BY ts', + }), + ]), + ).toEqual([]); + }); + + it('flags missing $__filters and $__sourceTable when a sourceId is set', () => { + const hints = getRawSqlTileMacroWarnings([ + makeSqlTile({ + name: 'Hardcoded Table', + sourceId, + sqlTemplate: + 'SELECT count() FROM otel_traces WHERE $__timeFilter(Timestamp)', + }), + ]); + expect(hints).toHaveLength(1); + expect(hints[0]).toContain('$__filters'); + expect(hints[0]).toContain('$__sourceTable'); + }); + + it('still suggests $__filters / $__sourceTable even without a sourceId', () => { + // The advisory always checks for these macros and notes that they require + // a sourceId, so the agent can either add one or knowingly disregard it + // (e.g. for a multi-table query that intentionally omits a source). + const hints = getRawSqlTileMacroWarnings([ + makeSqlTile({ + sqlTemplate: + 'SELECT count() FROM otel_traces t JOIN otel_logs l ON t.TraceId = l.TraceId ' + + 'WHERE $__timeFilter(t.Timestamp) LIMIT 10', + }), + ]); + expect(hints).toHaveLength(1); + expect(hints[0]).toContain('$__filters'); + expect(hints[0]).toContain('$__sourceTable'); + expect(hints[0]).toContain('requires a sourceId'); + }); + + it('ignores builder (non-SQL) tiles entirely', () => { + const builderTile = { + id: 'builder', + x: 0, + y: 0, + w: 12, + h: 4, + name: 'Line', + config: { + displayType: 'line', + sourceId, + select: [{ aggFn: 'count', valueExpression: '', where: '' }], + }, + } as unknown as ExternalDashboardTileWithId; + expect(getRawSqlTileMacroWarnings([builderTile])).toEqual([]); + }); + + it('falls back to a positional label when the tile has no name', () => { + const hints = getRawSqlTileMacroWarnings([ + makeSqlTile({ name: '', sqlTemplate: 'SELECT count() FROM otel_traces' }), + ]); + expect(hints).toHaveLength(1); + expect(hints[0]).toContain('tile #1'); + }); +}); diff --git a/packages/api/src/mcp/tools/dashboards/patchDashboard.ts b/packages/api/src/mcp/tools/dashboards/patchDashboard.ts index 9cc0a8483a..a9f056200f 100644 --- a/packages/api/src/mcp/tools/dashboards/patchDashboard.ts +++ b/packages/api/src/mcp/tools/dashboards/patchDashboard.ts @@ -16,7 +16,10 @@ import { mcpError } from '../../utils/errors'; import { withToolTracing } from '../../utils/tracing'; import type { McpContext } from '../types'; import { mcpPatchDashboardSchema } from './schemas'; -import { getRawSqlMissingSourceError } from './validation'; +import { + getRawSqlMissingSourceError, + getRawSqlTileMacroWarnings, +} from './validation'; export function registerPatchDashboard( server: McpServer, @@ -273,6 +276,10 @@ export function registerPatchDashboard( output.patchedTile = patchedTile; output.hint = 'Use clickstack_query_tile to test the patched tile query.'; + const macroWarnings = getRawSqlTileMacroWarnings([patchedTile]); + if (macroWarnings.length > 0) { + output.warnings = macroWarnings; + } } return { diff --git a/packages/api/src/mcp/tools/dashboards/queryTile.ts b/packages/api/src/mcp/tools/dashboards/queryTile.ts index 5517ab54cf..74e30e81af 100644 --- a/packages/api/src/mcp/tools/dashboards/queryTile.ts +++ b/packages/api/src/mcp/tools/dashboards/queryTile.ts @@ -8,6 +8,7 @@ import { objectIdSchema } from '@/utils/zod'; import { withToolTracing } from '../../utils/tracing'; import { parseTimeRange, runConfigTile } from '../query/helpers'; import type { McpContext } from '../types'; +import { getRawSqlTileMacroWarnings } from './validation'; export function registerQueryTile( server: McpServer, @@ -83,7 +84,32 @@ export function registerQueryTile( }; } - return runConfigTile(teamId.toString(), tile, startDate, endDate); + const result = await runConfigTile( + teamId.toString(), + tile, + startDate, + endDate, + ); + + // Surface non-blocking missing macro warnings alongside + // the successful result so the agent can spot a tile that runs but + // ignores dashboard controls. + const macroWarnings = getRawSqlTileMacroWarnings([tile]); + if ( + macroWarnings.length > 0 && + !('isError' in result && result.isError) && + result.content?.[0]?.type === 'text' + ) { + try { + const parsed = JSON.parse(result.content[0].text); + parsed.warnings = macroWarnings; + result.content[0].text = JSON.stringify(parsed, null, 2); + } catch { + // leave result unmodified + } + } + + return result; }, ), ); diff --git a/packages/api/src/mcp/tools/dashboards/saveDashboard.ts b/packages/api/src/mcp/tools/dashboards/saveDashboard.ts index 725db4a8d6..803336f811 100644 --- a/packages/api/src/mcp/tools/dashboards/saveDashboard.ts +++ b/packages/api/src/mcp/tools/dashboards/saveDashboard.ts @@ -27,7 +27,10 @@ import { mcpError } from '../../utils/errors'; import { withToolTracing } from '../../utils/tracing'; import type { McpContext } from '../types'; import { mcpContainersParam, mcpFiltersParam, mcpTilesParam } from './schemas'; -import { getRawSqlMissingSourceError } from './validation'; +import { + getRawSqlMissingSourceError, + getRawSqlTileMacroWarnings, +} from './validation'; export function registerSaveDashboard( server: McpServer, @@ -194,6 +197,8 @@ async function createDashboard({ }; } + const macroWarnings = getRawSqlTileMacroWarnings(tilesWithId); + const internalTiles = convertExternalTilesToInternal(tilesWithId); const filtersWithIds = convertExternalFiltersToInternal(filters ?? []); @@ -224,6 +229,7 @@ async function createDashboard({ ? { url: `${frontendUrl}/dashboards/${newDashboard._id}` } : {}), hint: 'Use clickstack_query_tile to test individual tile queries before viewing the dashboard.', + ...(macroWarnings.length > 0 ? { warnings: macroWarnings } : {}), }, null, 2, @@ -311,6 +317,8 @@ async function updateDashboard({ }; } + const macroWarnings = getRawSqlTileMacroWarnings(tilesWithId); + const existingTileIds = new Set( (existingDashboard.tiles ?? []).map((t: { id: string }) => t.id), ); @@ -387,6 +395,7 @@ async function updateDashboard({ ? { url: `${frontendUrl}/dashboards/${updatedDashboard._id}` } : {}), hint: 'Use clickstack_query_tile to test individual tile queries before viewing the dashboard.', + ...(macroWarnings.length > 0 ? { warnings: macroWarnings } : {}), }, null, 2, diff --git a/packages/api/src/mcp/tools/dashboards/schemas.ts b/packages/api/src/mcp/tools/dashboards/schemas.ts index c12d485737..5861d0848f 100644 --- a/packages/api/src/mcp/tools/dashboards/schemas.ts +++ b/packages/api/src/mcp/tools/dashboards/schemas.ts @@ -657,21 +657,34 @@ const mcpSqlTileSchema = mcpTileLayoutSchema.extend({ 'sourceId is REQUIRED by two macros: $__filters and $__sourceTable. ' + 'The sourceId must belong to the same connection as connectionId.', ), - sqlTemplate: z - .string() - .describe( - 'Raw ClickHouse SQL query. Always include a LIMIT clause to avoid excessive data.\n' + - 'Use query parameters: {startDateMilliseconds:Int64}, {endDateMilliseconds:Int64}, ' + - '{intervalSeconds:Int64}, {intervalMilliseconds:Int64}.\n' + - 'Or use macros: $__timeFilter(col), $__timeFilter_ms(col), $__dateFilter(col), ' + - '$__fromTime, $__toTime, $__fromTime_ms, $__toTime_ms, ' + - '$__timeInterval(col), $__timeInterval_ms(col), $__interval_s, $__filters, $__sourceTable.\n' + - 'IMPORTANT: $__filters and $__sourceTable both require sourceId to be set on this tile. ' + - 'Prefer including "AND $__filters" in the WHERE clause so dashboard filters apply.\n' + - 'Example: "SELECT $__timeInterval(TimestampTime) AS ts, ServiceName, count() ' + - 'FROM otel_logs WHERE $__timeFilter(TimestampTime) AND $__filters ' + - 'GROUP BY ServiceName, ts ORDER BY ts"', - ), + sqlTemplate: z.string().describe(` +Raw ClickHouse SQL query. SQL guidelines: + +1. ALWAYS include a LIMIT clause to avoid excessive data. +2. ALWAYS include a date/time filter in the WHERE clause using either macros or raw parameters to ensure the chart responds to user selected time range. + - $__timeFilter(col) expands to col >= toDateTime(fromUnixTimestamp64Milli({startDateMilliseconds:Int64})) AND col <= toDateTime(fromUnixTimestamp64Milli({endDateMilliseconds:Int64})) + - $__timeFilter_ms(col) is the same but should be used when col has millisecond precision (DateTime64 type) + - $__dateFilter(col) is the same but should be used when col has Day granularity (Date type) + - $__dateTimeFilter(dateCol, dateTimeCol) should be used when there are both Date and DateTime columns that should be filtered on. + - NEVER hardcode a fixed time range unless the user specifically asks for it. + - $__fromTime and $__toTime can be expanded to {startDateMilliseconds:Int64} and {endDateMilliseconds:Int64}, but prefer the full filter macros for readability. +3. ALWAYS include a granularity macro or parameter for time series (line or bar charts) to ensure the chart's granularity responds to user selected time bucket size. + - $__timeInterval(col) expands to toStartOfInterval(TimestampTime, INTERVAL {intervalSeconds:Int64} second) + - $__interval_s expands to {intervalSeconds:Int64} + - These macros are only available for time-series charts; do not use them for other display types. +4. STRONGLY RECOMMENDED: use the $__filters and $__sourceTable macros to ensure the tile reacts to dashboard-level filters and source selectors. + - $__filters and $__sourceTable both require sourceId to be set on this tile. + +Example: + +SELECT + $__timeInterval(TimestampTime) AS ts, + count() +FROM $__sourceTable +WHERE $__timeFilter(TimestampTime) + AND $__filters +GROUP BY ServiceName, ts +`), fillNulls: z.boolean().optional(), alignDateRangeToGranularity: z.boolean().optional(), numberFormat: mcpNumberFormatSchema diff --git a/packages/api/src/mcp/tools/dashboards/validation.ts b/packages/api/src/mcp/tools/dashboards/validation.ts index 2cdbbab170..bd9109dd0a 100644 --- a/packages/api/src/mcp/tools/dashboards/validation.ts +++ b/packages/api/src/mcp/tools/dashboards/validation.ts @@ -1,4 +1,9 @@ -import { SOURCE_DEPENDENT_MACROS } from '@hyperdx/common-utils/dist/macros'; +import { + hasMacro, + INTERVAL_MACROS, + SOURCE_DEPENDENT_MACROS, + TIME_RANGE_MACROS, +} from '@hyperdx/common-utils/dist/macros'; import { isConfigTile, @@ -24,13 +29,15 @@ export function getRawSqlTilesMissingRequiredSource( return; } const { sqlTemplate } = tile.config; + // SOURCE_DEPENDENT_MACROS are bare names ('filters', 'sourceTable') for + // hasMacro; surface them in the user-facing `$__name` form. const macros = SOURCE_DEPENDENT_MACROS.filter(macro => - sqlTemplate.includes(macro), - ); + hasMacro(sqlTemplate, macro), + ).map(macro => `$__${macro}`); if (macros.length > 0) { offending.push({ tile: tile.name?.trim() || `tile #${index + 1}`, - macros: [...macros], + macros, }); } }); @@ -59,3 +66,65 @@ export function getRawSqlMissingSourceError( `from multiple tables: ${list}` ); } + +/** Raw SQL display types that plot a value over time. */ +const TIME_SERIES_DISPLAY_TYPES = ['line', 'stacked_bar']; + +/** + * Returns one advisory string per raw SQL tile that omits a strongly + * recommended macro: + * - a time-range macro (any of TIME_RANGE_MACROS) — all display types; + * - $__timeInterval — time-series display types only; + * - $__filters / $__sourceTable + * + * These are non-blocking warnings, not errors: a tile may legitimately omit them + * (e.g. a query that should ignore the dashboard time range), so they are + * surfaced as guidance the agent can act on or knowingly disregard. + */ +export function getRawSqlTileMacroWarnings( + tiles: ExternalDashboardTileWithId[], +): string[] { + const hints: string[] = []; + tiles.forEach((tile, index) => { + if (!isConfigTile(tile) || !isRawSqlExternalTileConfig(tile.config)) { + return; + } + const { sqlTemplate, displayType } = tile.config; + const label = tile.name?.trim() || `tile #${index + 1}`; + const missing: string[] = []; + + if (!TIME_RANGE_MACROS.some(macro => hasMacro(sqlTemplate, macro))) { + missing.push( + 'a time-range macro such as $__timeFilter(col) (so the tile follows the dashboard time picker)', + ); + } + if ( + TIME_SERIES_DISPLAY_TYPES.includes(displayType) && + !INTERVAL_MACROS.some(macro => hasMacro(sqlTemplate, macro)) + ) { + missing.push( + '$__timeInterval(col) (so time buckets match the dashboard granularity)', + ); + } + + if (!hasMacro(sqlTemplate, 'filters')) { + missing.push( + '$__filters (so dashboard filters apply to this tile; requires a sourceId on the tile)', + ); + } + + if (!hasMacro(sqlTemplate, 'sourceTable')) { + missing.push( + "$__sourceTable (so the query tracks the tile's configured source; requires a sourceId on the tile)", + ); + } + + if (missing.length > 0) { + hints.push( + `Raw SQL tile "${label}" is missing ${missing.join('; ')}. ` + + 'These macros are strongly recommended unless the query intentionally ignores the dashboard time range and filters.', + ); + } + }); + return hints; +} diff --git a/packages/common-utils/src/__tests__/macros.test.ts b/packages/common-utils/src/__tests__/macros.test.ts index d7f791f4bd..a22c1a17b4 100644 --- a/packages/common-utils/src/__tests__/macros.test.ts +++ b/packages/common-utils/src/__tests__/macros.test.ts @@ -1,4 +1,4 @@ -import { replaceMacros } from '../macros'; +import { hasMacro, replaceMacros } from '../macros'; import type { MetricTable } from '../types'; const ALL_METRIC_TABLES: MetricTable = { @@ -9,6 +9,24 @@ const ALL_METRIC_TABLES: MetricTable = { 'exponential histogram': 'otel_metrics_exponential_histogram', }; +describe('hasMacro', () => { + it('detects a $__filters macro', () => { + expect(hasMacro('SELECT * WHERE $__filters', 'filters')).toBe(true); + }); + + it('returns false when the macro is absent', () => { + expect(hasMacro('SELECT * WHERE $__timeFilter(ts)', 'filters')).toBe(false); + }); + + it('matches on a word boundary (does not match a longer token)', () => { + expect(hasMacro('SELECT $__filtersExtra', 'filters')).toBe(false); + }); + + it('detects macros that take arguments', () => { + expect(hasMacro('WHERE $__timeFilter(ts)', 'timeFilter')).toBe(true); + }); +}); + describe('replaceMacros', () => { it('should replace $__fromTime with seconds-precision DateTime', () => { expect(replaceMacros({ sqlTemplate: 'SELECT $__fromTime' })).toBe( diff --git a/packages/common-utils/src/macros.ts b/packages/common-utils/src/macros.ts index 142cb84724..c6517cd4b1 100644 --- a/packages/common-utils/src/macros.ts +++ b/packages/common-utils/src/macros.ts @@ -162,7 +162,7 @@ export const MACRO_SUGGESTIONS = [ ]; /** - * Macro tokens (in `$__name` form) whose expansion depends on the chart's + * Macros whose expansion depends on the chart's * source being configured. With no source: * - `$__filters` cannot resolve dashboard filters against the source's * columns and falls back to `(1=1)`, so the filters are silently dropped. @@ -172,9 +172,34 @@ export const MACRO_SUGGESTIONS = [ * Every other macro only needs the dashboard time range / interval and takes * its column as an argument, so it does not require a source. */ -export const SOURCE_DEPENDENT_MACROS = [ - '$__filters', - '$__sourceTable', +export const SOURCE_DEPENDENT_MACROS = ['filters', 'sourceTable'] as const; + +/** + * Time-range macros. Any single one binds a raw + * SQL query to the dashboard/query time range, so the presence of *any* of + * these satisfies a time-range check — callers should test them as a set + * rather than requiring a specific one. + */ +export const TIME_RANGE_MACROS = [ + 'timeFilter', + 'timeFilter_ms', + 'dateFilter', + 'dateTimeFilter', + 'dt', + 'fromTime', + 'toTime', + 'fromTime_ms', + 'toTime_ms', +] as const; + +/** + * Time-bucketing macros. Only relevant to time-series + * (line/bar) charts, where they align buckets to the requested granularity. + */ +export const INTERVAL_MACROS = [ + 'timeInterval', + 'timeInterval_ms', + 'interval_s', ] as const; type MacroMatch = { @@ -212,6 +237,13 @@ function parseMacroArgs(argString: string): { args: string[]; length: number } { return { args, length: closeParenIndex + 1 }; } +/** + * True if the SQL contains at least one occurrence of the `$__` macro. + */ +export function hasMacro(sql: string, name: string): boolean { + return findMacros(sql, name).length > 0; +} + function findMacros(input: string, name: string): MacroMatch[] { // eslint-disable-next-line security/detect-non-literal-regexp const pattern = new RegExp(`\\$__${name}\\b`, 'g');