diff --git a/.changeset/mcp-dashboard-variables.md b/.changeset/mcp-dashboard-variables.md new file mode 100644 index 0000000000..4ec7c3da64 --- /dev/null +++ b/.changeset/mcp-dashboard-variables.md @@ -0,0 +1,6 @@ +--- +'@hyperdx/api': patch +'@hyperdx/common-utils': patch +--- + +feat: Support dashboard variables in the MCP server diff --git a/packages/api/src/mcp/__tests__/dashboards/patchDashboard.int.test.ts b/packages/api/src/mcp/__tests__/dashboards/patchDashboard.int.test.ts index 48b22acd7b..e50b12f86e 100644 --- a/packages/api/src/mcp/__tests__/dashboards/patchDashboard.int.test.ts +++ b/packages/api/src/mcp/__tests__/dashboards/patchDashboard.int.test.ts @@ -875,6 +875,149 @@ describe('MCP Dashboard Tools - clickstack_patch_dashboard', () => { }); }); + describe('dashboard variable warnings', () => { + // A patch cannot change the filters, so the patched tile's variable + // references are checked against the dashboard's persisted set. This is + // the tool clickstack_save_dashboard points agents at for single-tile + // edits, so it has to catch what a full save would. + const variableFilter = (sourceId: string) => ({ + type: 'QUERY_EXPRESSION' as const, + name: 'Service', + expression: 'ServiceName', + sourceId, + whereLanguage: 'sql' as const, + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'service', + }); + + const createVariableDashboard = async (name: string) => { + const sourceId = ctx.traceSource._id.toString(); + const createResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name, + tiles: [ + { + name: 'Builder Tile', + config: { + displayType: 'number', + sourceId, + select: [{ aggFn: 'count' }], + }, + }, + ], + filters: [variableFilter(sourceId)], + }, + ); + expect(createResult.isError).toBeFalsy(); + return JSON.parse(getFirstText(createResult)); + }; + + const macroSql = (variable: string) => + 'SELECT ServiceName, count() AS c FROM $__sourceTable ' + + 'WHERE $__timeFilter(Timestamp) AND $__filters ' + + `AND $__filter(ServiceName, $${variable}) GROUP BY ServiceName LIMIT 10`; + + it('warns when a patched tile names a variable no filter declares', async () => { + const sourceId = ctx.traceSource._id.toString(); + const connectionId = ctx.connection._id.toString(); + const created = await createVariableDashboard('Patch unknown variable'); + + const patchResult = await callTool( + ctx.client!, + 'clickstack_patch_dashboard', + { + dashboardId: created.id, + tileId: created.tiles[0].id, + tile: { + name: 'Errors', + config: { + configType: 'sql', + displayType: 'table', + connectionId, + sourceId, + sqlTemplate: macroSql('tenant'), + }, + }, + }, + ); + + // Non-blocking, like the macro warnings above. + expect(patchResult.isError).toBeFalsy(); + const output = JSON.parse(getFirstText(patchResult)); + const warnings: string[] = output.warnings ?? []; + expect(warnings.join('\n')).toContain('Tile "Errors"'); + expect(warnings.join('\n')).toContain("unknown variable 'tenant'"); + expect(warnings.join('\n')).toContain('Available variables: service'); + }); + + it('warns about an unguarded bare reference in a patched builder tile', async () => { + const sourceId = ctx.traceSource._id.toString(); + const created = await createVariableDashboard('Patch bare reference'); + + const patchResult = await callTool( + ctx.client!, + 'clickstack_patch_dashboard', + { + dashboardId: created.id, + tileId: created.tiles[0].id, + tile: { + name: 'Requests', + config: { + displayType: 'number', + sourceId, + select: [ + { + aggFn: 'count', + alias: 'Requests', + where: 'ServiceName IN ($service)', + whereLanguage: 'sql', + }, + ], + }, + }, + }, + ); + + expect(patchResult.isError).toBeFalsy(); + const output = JSON.parse(getFirstText(patchResult)); + const warnings: string[] = output.warnings ?? []; + expect(warnings.join('\n')).toContain('Tile "Requests"'); + expect(warnings.join('\n')).toContain('renders as NULL'); + }); + + it('omits warnings when the patched tile references a declared variable', async () => { + const sourceId = ctx.traceSource._id.toString(); + const connectionId = ctx.connection._id.toString(); + const created = await createVariableDashboard('Patch declared variable'); + + const patchResult = await callTool( + ctx.client!, + 'clickstack_patch_dashboard', + { + dashboardId: created.id, + tileId: created.tiles[0].id, + tile: { + name: 'Errors', + config: { + configType: 'sql', + displayType: 'table', + connectionId, + sourceId, + sqlTemplate: macroSql('service'), + }, + }, + }, + ); + + expect(patchResult.isError).toBeFalsy(); + const output = JSON.parse(getFirstText(patchResult)); + expect(output.warnings).toBeUndefined(); + }); + }); + // Patches replace a single tile without going through the save_dashboard // body schemas, so the handler re-runs the external tile schema's // cross-field refinements itself — otherwise a patch could persist a diff --git a/packages/api/src/mcp/__tests__/dashboards/queryTile.int.test.ts b/packages/api/src/mcp/__tests__/dashboards/queryTile.int.test.ts index ea5162d0cd..9d93ff7d05 100644 --- a/packages/api/src/mcp/__tests__/dashboards/queryTile.int.test.ts +++ b/packages/api/src/mcp/__tests__/dashboards/queryTile.int.test.ts @@ -696,6 +696,335 @@ describe('MCP Dashboard Tools - clickstack_query_tile', () => { }); }); + describe('dashboard variables', () => { + const now = new Date(); + const tenMinAgo = new Date(now.getTime() - 10 * 60 * 1000); + const startTime = new Date(now.getTime() - 60 * 60 * 1000).toISOString(); + const endTime = new Date(now.getTime() + 60 * 1000).toISOString(); + + let logSourceId: string; + + beforeEach(async () => { + const logSource = await Source.create({ + kind: SourceKind.Log, + team: ctx.team._id, + from: { + databaseName: DEFAULT_DATABASE, + tableName: DEFAULT_LOGS_TABLE, + }, + timestampValueExpression: 'Timestamp', + connection: ctx.connection._id, + name: 'Variable Logs', + bodyExpression: 'Body', + severityTextExpression: 'SeverityText', + }); + logSourceId = logSource._id.toString(); + + await bulkInsertLogs([ + { + Body: 'checkout one', + ServiceName: 'checkout', + SeverityText: 'INFO', + Timestamp: tenMinAgo, + }, + { + Body: 'checkout two', + ServiceName: 'checkout', + SeverityText: 'INFO', + Timestamp: new Date(tenMinAgo.getTime() + 10), + }, + { + Body: 'payments one', + ServiceName: 'payments', + SeverityText: 'INFO', + Timestamp: new Date(tenMinAgo.getTime() + 20), + }, + ]); + }); + + const variableFilter = () => ({ + type: 'QUERY_EXPRESSION' as const, + name: 'Service', + expression: 'ServiceName', + sourceId: logSourceId, + whereLanguage: 'sql' as const, + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'service', + }); + + const saveDashboard = async (tiles: unknown[]) => { + const result = await callTool(ctx.client!, 'clickstack_save_dashboard', { + name: 'Variable tiles', + tiles, + filters: [variableFilter()], + }); + expect(result.isError).toBeFalsy(); + return JSON.parse(getFirstText(result)); + }; + + const rowsOf = (text: string) => { + const parsed = JSON.parse(text); + const result = parsed.result; + return Array.isArray(result) ? result : (result?.data ?? []); + }; + + const sqlTile = () => ({ + name: 'Rows by service', + config: { + configType: 'sql', + displayType: 'table', + connectionId: ctx.connection._id.toString(), + sourceId: logSourceId, + sqlTemplate: + 'SELECT ServiceName, count() AS c FROM $__sourceTable ' + + 'WHERE $__timeFilter(Timestamp) AND $__filters ' + + 'AND $__filter(ServiceName, $service) ' + + 'GROUP BY ServiceName ORDER BY ServiceName LIMIT 10', + }, + }); + + const builderTile = () => ({ + name: 'Rows by service (builder)', + config: { + displayType: 'table', + sourceId: logSourceId, + groupBy: 'ServiceName', + select: [ + { + aggFn: 'count', + alias: 'Count', + where: '$__filter(ServiceName, $service)', + whereLanguage: 'sql', + }, + ], + }, + }); + + const eventPatternsTile = ( + where = '$__filter(ServiceName, $service)', + whereLanguage: 'sql' | 'lucene' = 'sql', + ) => ({ + name: 'Log patterns', + config: { + displayType: 'event_patterns', + sourceId: logSourceId, + select: 'Body', + where, + whereLanguage, + }, + }); + + const patternsOf = (text: string) => { + const parsed = JSON.parse(text); + return { + totalCount: Number(parsed.summary.totalCount), + sampledCount: Number(parsed.summary.sampledCount), + bodies: Array.isArray(parsed.patterns) + ? parsed.patterns.flatMap(p => ('samples' in p ? p.samples : [])) + : [], + }; + }; + + it('expands $__filter to a no-op on an event_patterns tile with nothing selected', async () => { + const dashboard = await saveDashboard([eventPatternsTile()]); + + const result = await callTool(ctx.client!, 'clickstack_query_tile', { + dashboardId: dashboard.id, + tileId: dashboard.tiles[0].id, + startTime, + endTime, + }); + + expect(result.isError).toBeFalsy(); + const { totalCount, bodies } = patternsOf(getFirstText(result)); + // Nothing selected means the macro contributes no condition, so both + // services are still mined. + expect(totalCount).toBe(3); + expect(bodies.some(b => b.includes('checkout'))).toBe(true); + expect(bodies.some(b => b.includes('payments'))).toBe(true); + }); + + it('narrows an event_patterns tile when variableValues supplies a selection', async () => { + const dashboard = await saveDashboard([eventPatternsTile()]); + + const result = await callTool(ctx.client!, 'clickstack_query_tile', { + dashboardId: dashboard.id, + tileId: dashboard.tiles[0].id, + startTime, + endTime, + variableValues: [{ name: 'service', values: ['checkout'] }], + }); + + expect(result.isError).toBeFalsy(); + const { totalCount, sampledCount, bodies } = patternsOf( + getFirstText(result), + ); + // The selection narrows BOTH the sampled rows and the total count the + // estimates are extrapolated from — the payments row is gone from each. + expect(totalCount).toBe(2); + expect(sampledCount).toBe(2); + expect(bodies.some(b => b.includes('checkout'))).toBe(true); + expect(bodies.some(b => b.includes('payments'))).toBe(false); + }); + + it('expands a Lucene variable reference on an event_patterns tile', async () => { + // event_patterns tiles default to Lucene, where a reference renders as a + // value list rather than a SQL predicate. + const dashboard = await saveDashboard([ + eventPatternsTile('ServiceName:$service', 'lucene'), + ]); + + const result = await callTool(ctx.client!, 'clickstack_query_tile', { + dashboardId: dashboard.id, + tileId: dashboard.tiles[0].id, + startTime, + endTime, + variableValues: [{ name: 'service', values: ['payments'] }], + }); + + expect(result.isError).toBeFalsy(); + const { totalCount, bodies } = patternsOf(getFirstText(result)); + expect(totalCount).toBe(1); + expect(bodies.some(b => b.includes('checkout'))).toBe(false); + expect(bodies.some(b => b.includes('payments'))).toBe(true); + }); + + it('reports an event_patterns macro naming a variable the dashboard does not declare', async () => { + const dashboard = await saveDashboard([ + eventPatternsTile('$__filter(ServiceName, $tenant)'), + ]); + + const result = await callTool(ctx.client!, 'clickstack_query_tile', { + dashboardId: dashboard.id, + tileId: dashboard.tiles[0].id, + startTime, + endTime, + }); + + // Expansion throws before ClickHouse sees anything, and the mining path + // surfaces the same named error and hint as every other tile type. + expect(result.isError).toBe(true); + const text = getFirstText(result); + expect(text).toContain("references unknown variable 'tenant'"); + expect(text).toContain('isVariableEnabled'); + }); + + it('expands $__filter to a no-op on a raw SQL tile with nothing selected', async () => { + // Before variables were threaded through, the macro text reached + // ClickHouse verbatim and the tile died with a syntax error. + const dashboard = await saveDashboard([sqlTile()]); + + const result = await callTool(ctx.client!, 'clickstack_query_tile', { + dashboardId: dashboard.id, + tileId: dashboard.tiles[0].id, + startTime, + endTime, + }); + + expect(result.isError).toBeFalsy(); + // Nothing selected means the macro contributes no condition, so every + // service is still counted. + expect(rowsOf(getFirstText(result))).toHaveLength(2); + }); + + it('narrows a raw SQL tile when variableValues supplies a selection', async () => { + const dashboard = await saveDashboard([sqlTile()]); + + const result = await callTool(ctx.client!, 'clickstack_query_tile', { + dashboardId: dashboard.id, + tileId: dashboard.tiles[0].id, + startTime, + endTime, + variableValues: [{ name: 'service', values: ['checkout'] }], + }); + + expect(result.isError).toBeFalsy(); + const rows = rowsOf(getFirstText(result)); + expect(rows).toHaveLength(1); + expect(rows[0].ServiceName).toBe('checkout'); + }); + + it('expands $__filter in a builder tile per-series where', async () => { + const dashboard = await saveDashboard([builderTile()]); + + const unselected = await callTool(ctx.client!, 'clickstack_query_tile', { + dashboardId: dashboard.id, + tileId: dashboard.tiles[0].id, + startTime, + endTime, + }); + expect(unselected.isError).toBeFalsy(); + expect(rowsOf(getFirstText(unselected))).toHaveLength(2); + + const selected = await callTool(ctx.client!, 'clickstack_query_tile', { + dashboardId: dashboard.id, + tileId: dashboard.tiles[0].id, + startTime, + endTime, + variableValues: [{ name: 'service', values: ['payments'] }], + }); + expect(selected.isError).toBeFalsy(); + const rows = rowsOf(getFirstText(selected)); + // countIf leaves the other group at zero rather than dropping it. + const payments = rows.find( + (r: Record) => r.ServiceName === 'payments', + ); + const checkout = rows.find( + (r: Record) => r.ServiceName === 'checkout', + ); + expect(Number(payments?.Count)).toBe(1); + expect(Number(checkout?.Count ?? 0)).toBe(0); + }); + + it('reports a macro naming a variable the dashboard does not declare', async () => { + const dashboard = await saveDashboard([ + { + name: 'Typo', + config: { + configType: 'sql', + displayType: 'table', + connectionId: ctx.connection._id.toString(), + sourceId: logSourceId, + sqlTemplate: + 'SELECT count() AS c FROM $__sourceTable ' + + 'WHERE $__timeFilter(Timestamp) AND $__filter(ServiceName, $tenant) LIMIT 1', + }, + }, + ]); + + const result = await callTool(ctx.client!, 'clickstack_query_tile', { + dashboardId: dashboard.id, + tileId: dashboard.tiles[0].id, + startTime, + endTime, + }); + + // Expansion throws before ClickHouse sees anything, and the message + // names the variable rather than surfacing as a SQL syntax error. + expect(result.isError).toBe(true); + const text = getFirstText(result); + expect(text).toContain("references unknown variable 'tenant'"); + expect(text).toContain('HINT:'); + expect(text).toContain('isVariableEnabled'); + }); + + it('rejects a variableValues entry the dashboard does not declare', async () => { + const dashboard = await saveDashboard([sqlTile()]); + + const result = await callTool(ctx.client!, 'clickstack_query_tile', { + dashboardId: dashboard.id, + tileId: dashboard.tiles[0].id, + startTime, + endTime, + variableValues: [{ name: 'tenant', values: ['acme'] }], + }); + + expect(result.isError).toBe(true); + expect(getFirstText(result)).toContain('Available variables: service'); + }); + }); + describe('categorical (bar) tile series limit', () => { const now = new Date(); const tenMinAgo = new Date(now.getTime() - 10 * 60 * 1000); diff --git a/packages/api/src/mcp/__tests__/dashboards/queryTiles.int.test.ts b/packages/api/src/mcp/__tests__/dashboards/queryTiles.int.test.ts index a8b67b4611..019161cd71 100644 --- a/packages/api/src/mcp/__tests__/dashboards/queryTiles.int.test.ts +++ b/packages/api/src/mcp/__tests__/dashboards/queryTiles.int.test.ts @@ -398,4 +398,157 @@ describe('MCP Dashboard Tools - clickstack_query_tiles', () => { expect(result.isError).toBe(true); }); + + describe('dashboard variables', () => { + it('runs a $__filter tile in the batch', async () => { + const sourceId = ctx.traceSource._id.toString(); + const createResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Batch variable test', + tiles: [ + { + name: 'Rows', + config: { + configType: 'sql', + displayType: 'table', + connectionId: ctx.connection._id.toString(), + sourceId, + sqlTemplate: + 'SELECT count() AS c FROM $__sourceTable ' + + 'WHERE $__timeFilter(Timestamp) AND $__filters ' + + 'AND $__filter(ServiceName, $service) LIMIT 1', + }, + }, + ], + filters: [ + { + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + sourceId, + whereLanguage: 'sql', + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'service', + }, + ], + }, + ); + expect(createResult.isError).toBeFalsy(); + const dashboard = JSON.parse(getFirstText(createResult)); + + const result = await callTool(ctx.client!, 'clickstack_query_tiles', { + dashboardId: dashboard.id, + ...wideRange(), + }); + + expect(result.isError).toBeFalsy(); + const parsed = JSON.parse(getFirstText(result)); + // Before variables were threaded in, the macro text reached ClickHouse + // verbatim and this came back as a syntax error. + expect(parsed.summary).toMatchObject({ total: 1, ok: 1, error: 0 }); + }); + + it('runs an event_patterns tile with a variable macro in the batch', async () => { + const logSource = await Source.create({ + kind: SourceKind.Log, + team: ctx.team._id, + from: { + databaseName: DEFAULT_DATABASE, + tableName: DEFAULT_LOGS_TABLE, + }, + timestampValueExpression: 'Timestamp', + connection: ctx.connection._id, + name: 'Batch Pattern Logs', + bodyExpression: 'Body', + severityTextExpression: 'SeverityText', + }); + const sourceId = logSource._id.toString(); + await bulkInsertLogs([ + { + Body: 'checkout one', + ServiceName: 'checkout', + SeverityText: 'INFO', + Timestamp: new Date(Date.now() - 10 * 60 * 1000), + }, + { + Body: 'payments one', + ServiceName: 'payments', + SeverityText: 'INFO', + Timestamp: new Date(Date.now() - 10 * 60 * 1000 + 10), + }, + ]); + + const createResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Batch pattern variable test', + tiles: [ + { + name: 'Log patterns', + config: { + displayType: 'event_patterns', + sourceId, + select: 'Body', + where: '$__filter(ServiceName, $service)', + whereLanguage: 'sql', + }, + }, + ], + filters: [ + { + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + sourceId, + whereLanguage: 'sql', + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'service', + }, + ], + }, + ); + expect(createResult.isError).toBeFalsy(); + const dashboard = JSON.parse(getFirstText(createResult)); + + const result = await callTool(ctx.client!, 'clickstack_query_tiles', { + dashboardId: dashboard.id, + ...wideRange(), + variableValues: [{ name: 'service', values: ['checkout'] }], + }); + + expect(result.isError).toBeFalsy(); + const parsed = JSON.parse(getFirstText(result)); + // The batch path threads variables into the mining pipeline too — before + // it did, this tile came back as an error entry. + expect(parsed.summary).toMatchObject({ total: 1, ok: 1, error: 0 }); + }); + + it('rejects a variableValues name the dashboard does not declare', async () => { + const sourceId = ctx.traceSource._id.toString(); + const dashboard = await saveDashboard([ + { + name: 'Count', + config: { + displayType: 'number', + sourceId, + select: [{ aggFn: 'count' }], + }, + }, + ]); + + const result = await callTool(ctx.client!, 'clickstack_query_tiles', { + dashboardId: dashboard.id, + ...wideRange(), + variableValues: [{ name: 'service', values: ['checkout'] }], + }); + + expect(result.isError).toBe(true); + expect(getFirstText(result)).toContain('Available variables: (none)'); + }); + }); }); diff --git a/packages/api/src/mcp/__tests__/dashboards/queryTilesHandler.test.ts b/packages/api/src/mcp/__tests__/dashboards/queryTilesHandler.test.ts index be12d5bdd4..3b270d960e 100644 --- a/packages/api/src/mcp/__tests__/dashboards/queryTilesHandler.test.ts +++ b/packages/api/src/mcp/__tests__/dashboards/queryTilesHandler.test.ts @@ -177,3 +177,88 @@ describe('clickstack_query_tiles handler — per-tile failure isolation', () => } }); }); + +describe('clickstack_query_tiles handler — dashboard variables', () => { + const variableFilter = { + id: 'f1', + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + sourceId: 'src-1', + whereLanguage: 'sql', + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'service', + }; + + it('runs every tile with the declared variables emptied by default', async () => { + mockConvertToExternalDashboard.mockReturnValue({ + id: 'dash-1', + tiles: [tile('t1', 'Requests')], + filters: [variableFilter], + }); + mockRunConfigTile.mockResolvedValue(okResult); + + const handler = buildHandler(); + await handler({ + dashboardId: '000000000000000000000000', + }); + + expect(mockRunConfigTile).toHaveBeenCalledWith( + 'team-1', + expect.objectContaining({ id: 't1' }), + expect.any(Date), + expect.any(Date), + expect.objectContaining({ + variables: [{ name: 'service', expression: 'ServiceName', values: [] }], + }), + ); + }); + + it('applies a supplied selection', async () => { + mockConvertToExternalDashboard.mockReturnValue({ + id: 'dash-1', + tiles: [tile('t1', 'Requests')], + filters: [variableFilter], + }); + mockRunConfigTile.mockResolvedValue(okResult); + + const handler = buildHandler(); + await handler({ + dashboardId: '000000000000000000000000', + variableValues: [{ name: 'service', values: ['checkout'] }], + }); + + expect(mockRunConfigTile).toHaveBeenCalledWith( + 'team-1', + expect.anything(), + expect.any(Date), + expect.any(Date), + expect.objectContaining({ + variables: [ + { name: 'service', expression: 'ServiceName', values: ['checkout'] }, + ], + }), + ); + }); + + it('rejects a variableValues name the dashboard does not declare', async () => { + mockConvertToExternalDashboard.mockReturnValue({ + id: 'dash-1', + tiles: [tile('t1', 'Requests')], + filters: [variableFilter], + }); + + const handler = buildHandler(); + const result = await handler({ + dashboardId: '000000000000000000000000', + variableValues: [{ name: 'tenant', values: ['acme'] }], + }); + + expect(result.isError).toBe(true); + expect(textOf(result)).toContain('tenant'); + // A typo must not run the query with the variable still empty and look + // like a data problem. + expect(mockRunConfigTile).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/api/src/mcp/__tests__/dashboards/saveDashboard.int.test.ts b/packages/api/src/mcp/__tests__/dashboards/saveDashboard.int.test.ts index c4240711a8..9397acfddb 100644 --- a/packages/api/src/mcp/__tests__/dashboards/saveDashboard.int.test.ts +++ b/packages/api/src/mcp/__tests__/dashboards/saveDashboard.int.test.ts @@ -2043,6 +2043,534 @@ describe('MCP Dashboard Tools - clickstack_save_dashboard', () => { expect(envFilter.id).not.toBe(existingFilterId); }); + describe('variable-enabled filters', () => { + // The three variable fields were missing from mcpDashboardFilterSchema, + // and a plain z.object drops unknown keys, so an agent that read a + // variable-enabled dashboard and saved it back silently wiped the + // variable configuration. These guard the round trip and the + // validations the filter configuration form applies. + + const variableFilter = ( + sourceId: string, + overrides: Record = {}, + ) => ({ + type: 'QUERY_EXPRESSION' as const, + name: 'Service', + expression: 'ServiceName', + sourceId, + whereLanguage: 'sql' as const, + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'service', + ...overrides, + }); + + it('round-trips the variable fields through create, get, and save', async () => { + const sourceId = ctx.traceSource._id.toString(); + + const createResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Variable round-trip', + tiles: [traceTile(sourceId)], + filters: [variableFilter(sourceId)], + }, + ); + expect(createResult.isError).toBeFalsy(); + const created = JSON.parse(getFirstText(createResult)); + expect(created.filters[0]).toMatchObject({ + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'service', + }); + + const getResult = await callTool( + ctx.client!, + 'clickstack_get_dashboard', + { id: created.id }, + ); + const fetched = JSON.parse(getFirstText(getResult)); + expect(fetched.filters[0]).toMatchObject({ + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'service', + }); + + // The whole point: feed the get response straight back in. + const updateResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + id: created.id, + name: fetched.name, + tiles: [traceTile(sourceId)], + filters: fetched.filters, + }, + ); + expect(updateResult.isError).toBeFalsy(); + const updated = JSON.parse(getFirstText(updateResult)); + expect(updated.filters[0]).toMatchObject({ + id: fetched.filters[0].id, + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'service', + }); + }); + + it('derives the variable name from the display name when omitted', async () => { + const sourceId = ctx.traceSource._id.toString(); + const result = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Derived variable name', + tiles: [traceTile(sourceId)], + filters: [ + variableFilter(sourceId, { + name: 'Service Name', + variableName: undefined, + }), + ], + }, + ); + + expect(result.isError).toBeFalsy(); + const created = JSON.parse(getFirstText(result)); + // Nothing is persisted under variableName: the renderer derives + // $Service_Name from the display name at read time. The response + // still echoes that resolved name so a caller doesn't have to + // reimplement the derivation to know what token ($Service_Name) + // actually resolves at query time. + expect(created.filters[0].variableName).toBe('Service_Name'); + }); + + it('round-trips a derived variable name through get and save unchanged', async () => { + const sourceId = ctx.traceSource._id.toString(); + const createResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Derived variable name round-trip', + tiles: [traceTile(sourceId)], + filters: [ + variableFilter(sourceId, { + name: 'Service Name', + variableName: undefined, + }), + ], + }, + ); + expect(createResult.isError).toBeFalsy(); + const created = JSON.parse(getFirstText(createResult)); + expect(created.filters[0].variableName).toBe('Service_Name'); + + const getResult = await callTool( + ctx.client!, + 'clickstack_get_dashboard', + { id: created.id }, + ); + const fetched = JSON.parse(getFirstText(getResult)); + expect(fetched.filters[0].variableName).toBe('Service_Name'); + + // Feed the derived name straight back in, unmodified, and confirm + // it comes back the same rather than drifting or being dropped. + const updateResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + id: created.id, + name: fetched.name, + tiles: [traceTile(sourceId)], + filters: fetched.filters, + }, + ); + expect(updateResult.isError).toBeFalsy(); + const updated = JSON.parse(getFirstText(updateResult)); + expect(updated.filters[0].variableName).toBe('Service_Name'); + }); + + it('rejects a filter that neither broadcasts nor exposes a variable', async () => { + const sourceId = ctx.traceSource._id.toString(); + const result = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Inert filter', + tiles: [traceTile(sourceId)], + filters: [ + variableFilter(sourceId, { + isVariableEnabled: false, + variableName: undefined, + }), + ], + }, + ); + + expect(result.isError).toBe(true); + expect(getFirstText(result)).toContain( + 'must broadcast its value, be available as a variable, or both', + ); + }); + + it('rejects two variable-enabled filters claiming the same name', async () => { + const sourceId = ctx.traceSource._id.toString(); + const result = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Duplicate variable names', + tiles: [traceTile(sourceId)], + filters: [ + variableFilter(sourceId), + variableFilter(sourceId, { + name: 'Service (alt)', + expression: 'SpanName', + }), + ], + }, + ); + + expect(result.isError).toBe(true); + expect(getFirstText(result)).toContain( + 'Variable names must be unique: "service"', + ); + }); + + it('rejects a variableName on a filter that is not variable-enabled', async () => { + const sourceId = ctx.traceSource._id.toString(); + const result = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Stray variable name', + tiles: [traceTile(sourceId)], + filters: [ + variableFilter(sourceId, { + isBroadcastEnabled: true, + isVariableEnabled: false, + }), + ], + }, + ); + + expect(result.isError).toBe(true); + expect(getFirstText(result)).toContain( + 'sets variableName but is not available as a variable', + ); + }); + + it('rejects a variable-enabled filter whose name derives nothing usable', async () => { + const sourceId = ctx.traceSource._id.toString(); + const result = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Non-derivable variable name', + tiles: [traceTile(sourceId)], + filters: [ + variableFilter(sourceId, { + name: '\u73af\u5883', + variableName: undefined, + }), + ], + }, + ); + + expect(result.isError).toBe(true); + expect(getFirstText(result)).toContain('Variable name is required'); + }); + + it('reports the validation failure as readable lines, not raw Zod JSON', async () => { + const sourceId = ctx.traceSource._id.toString(); + const result = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Readable errors', + tiles: [traceTile(sourceId)], + filters: [ + variableFilter(sourceId), + variableFilter(sourceId, { name: 'Service (alt)' }), + ], + }, + ); + + const text = getFirstText(result); + expect(text).toContain('filters[1].variableName:'); + expect(text).not.toContain('"code":'); + }); + + it('warns about an unguarded bare reference without blocking the save', async () => { + const sourceId = ctx.traceSource._id.toString(); + const result = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Unguarded reference', + tiles: [ + { + name: 'Requests', + x: 0, + y: 0, + w: 6, + h: 3, + config: { + displayType: 'line' as const, + sourceId, + select: [ + { + aggFn: 'count' as const, + alias: 'Requests', + where: 'ServiceName IN ($service)', + whereLanguage: 'sql' as const, + }, + ], + }, + }, + ], + filters: [variableFilter(sourceId)], + }, + ); + + expect(result.isError).toBeFalsy(); + const created = JSON.parse(getFirstText(result)); + const warnings: string[] = created.warnings ?? []; + expect(warnings.join('\n')).toContain('Tile "Requests"'); + expect(warnings.join('\n')).toContain('renders as NULL'); + }); + + it('warns when a tile macro names a variable no filter declares', async () => { + const sourceId = ctx.traceSource._id.toString(); + const result = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Unknown variable macro', + tiles: [ + { + name: 'Errors', + x: 0, + y: 0, + w: 6, + h: 3, + config: { + configType: 'sql' as const, + displayType: 'table' as const, + connectionId: ctx.connection._id.toString(), + sourceId, + sqlTemplate: + 'SELECT count() FROM $__sourceTable WHERE $__timeFilter(Timestamp) AND $__filters AND $__filter(ServiceName, $tenant) LIMIT 10', + }, + }, + ], + filters: [variableFilter(sourceId)], + }, + ); + + expect(result.isError).toBeFalsy(); + const created = JSON.parse(getFirstText(result)); + const warnings: string[] = created.warnings ?? []; + expect(warnings.join('\n')).toContain("unknown variable 'tenant'"); + }); + + // A dependent filter's own `where` is the one variable-carrying + // expression no other check reaches: it is not a tile, and no MCP tool + // runs the dropdown query, so save is the only chance to catch it. + it('warns about a dependent filter whose upstream publishes no variable', async () => { + const sourceId = ctx.traceSource._id.toString(); + const result = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Dependent filter', + tiles: [ + { + name: 'Requests', + x: 0, + y: 0, + w: 6, + h: 3, + config: { + displayType: 'line' as const, + sourceId, + select: [{ aggFn: 'count' as const, alias: 'Requests' }], + }, + }, + ], + filters: [ + // Broadcast-only: it collects a service but never publishes + // $service, so the Endpoint dropdown below cannot depend on it. + { + type: 'QUERY_EXPRESSION' as const, + name: 'Service', + expression: 'ServiceName', + sourceId, + whereLanguage: 'sql' as const, + }, + { + type: 'QUERY_EXPRESSION' as const, + name: 'Endpoint', + expression: 'SpanName', + sourceId, + whereLanguage: 'sql' as const, + where: '$__filter(ServiceName, $service)', + }, + ], + }, + ); + + expect(result.isError).toBeFalsy(); + const created = JSON.parse(getFirstText(result)); + const warnings: string[] = created.warnings ?? []; + const joined = warnings.join('\n'); + expect(joined).toContain('Filter "Endpoint"'); + expect(joined).toContain('$service is not published as a variable'); + expect(joined).toContain('The filter named "Service"'); + // Advisory only: the dashboard still saved with both filters. + expect(created.filters).toHaveLength(2); + }); + + it('accepts a dependent filter wired to a variable-enabled upstream', async () => { + const sourceId = ctx.traceSource._id.toString(); + const result = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Dependent filter, wired', + tiles: [ + { + name: 'Requests', + x: 0, + y: 0, + w: 6, + h: 3, + config: { + displayType: 'line' as const, + sourceId, + select: [{ aggFn: 'count' as const, alias: 'Requests' }], + }, + }, + ], + filters: [ + variableFilter(sourceId, { isBroadcastEnabled: true }), + { + type: 'QUERY_EXPRESSION' as const, + name: 'Endpoint', + expression: 'SpanName', + sourceId, + whereLanguage: 'sql' as const, + where: '$__filter(ServiceName, $service)', + }, + ], + }, + ); + + expect(result.isError).toBeFalsy(); + const created = JSON.parse(getFirstText(result)); + expect(created.warnings).toBeUndefined(); + expect(created.filters).toHaveLength(2); + }); + + // `filters` is optional on update and omitting it PRESERVES the + // persisted set rather than clearing it, so the variable check has to + // run against those. Checking against the absent request field instead + // reported every tile on a variable dashboard as referencing something + // unknown, and an agent acting on that would rewrite working tiles. + describe('an update that omits filters', () => { + const macroTile = ( + sourceId: string, + connectionId: string, + variable: string, + ) => ({ + name: 'Errors', + x: 0, + y: 0, + w: 6, + h: 3, + config: { + configType: 'sql' as const, + displayType: 'table' as const, + connectionId, + sourceId, + sqlTemplate: + 'SELECT count() FROM $__sourceTable WHERE $__timeFilter(Timestamp) ' + + `AND $__filters AND $__filter(ServiceName, $${variable}) LIMIT 10`, + }, + }); + + it('checks tiles against the preserved filters, not an empty set', async () => { + const sourceId = ctx.traceSource._id.toString(); + const connectionId = ctx.connection._id.toString(); + const tile = macroTile(sourceId, connectionId, 'service'); + + const createResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Preserved variable filters', + tiles: [tile], + filters: [variableFilter(sourceId)], + }, + ); + expect(createResult.isError).toBeFalsy(); + const created = JSON.parse(getFirstText(createResult)); + expect(created.warnings).toBeUndefined(); + + const updateResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + id: created.id, + name: 'Preserved variable filters', + tiles: [tile], + }, + ); + + expect(updateResult.isError).toBeFalsy(); + const updated = JSON.parse(getFirstText(updateResult)); + expect(updated.warnings).toBeUndefined(); + // The filters really were preserved, so there was something to + // check against — this is not passing because nothing ran. + expect(updated.filters).toHaveLength(1); + expect(updated.filters[0]).toMatchObject({ variableName: 'service' }); + }); + + it('still reports a variable the preserved filters do not declare', async () => { + const sourceId = ctx.traceSource._id.toString(); + const connectionId = ctx.connection._id.toString(); + + const createResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + name: 'Preserved filters, bad reference', + tiles: [macroTile(sourceId, connectionId, 'service')], + filters: [variableFilter(sourceId)], + }, + ); + expect(createResult.isError).toBeFalsy(); + const created = JSON.parse(getFirstText(createResult)); + + const updateResult = await callTool( + ctx.client!, + 'clickstack_save_dashboard', + { + id: created.id, + name: 'Preserved filters, bad reference', + tiles: [macroTile(sourceId, connectionId, 'tenant')], + }, + ); + + expect(updateResult.isError).toBeFalsy(); + const updated = JSON.parse(getFirstText(updateResult)); + const warnings: string[] = updated.warnings ?? []; + expect(warnings.join('\n')).toContain("unknown variable 'tenant'"); + expect(warnings.join('\n')).toContain('Available variables: service'); + }); + }); + }); + it('should round-trip a table tile that uses a having clause', async () => { // mcpTableTileSchema exposes `having` so the service_detail // example's "Top Error Messages" pattern (groupBy StatusMessage diff --git a/packages/api/src/mcp/__tests__/dashboards/validation.test.ts b/packages/api/src/mcp/__tests__/dashboards/validation.test.ts index bf0b45fc55..54eb2f8a0d 100644 --- a/packages/api/src/mcp/__tests__/dashboards/validation.test.ts +++ b/packages/api/src/mcp/__tests__/dashboards/validation.test.ts @@ -1,15 +1,22 @@ +import { DASHBOARD_VARIABLE_NAME_MAX_LENGTH } from '@hyperdx/common-utils/dist/types'; import mongoose from 'mongoose'; import { + mcpFiltersParam, mcpPatchDashboardSchema, mcpTilesParam, } from '@/mcp/tools/dashboards/schemas'; import { + getFilterVariableWarnings, getRawSqlMissingSourceError, getRawSqlTileMacroWarnings, getRawSqlTilesMissingRequiredSource, + getTileVariableWarnings, } from '@/mcp/tools/dashboards/validation'; -import type { ExternalDashboardTileWithId } from '@/utils/zod'; +import type { + ExternalDashboardFilterWithId, + ExternalDashboardTileWithId, +} from '@/utils/zod'; const connectionId = new mongoose.Types.ObjectId().toString(); const sourceId = new mongoose.Types.ObjectId().toString(); @@ -437,3 +444,456 @@ describe('getRawSqlTileMacroHints', () => { expect(hints[0]).toContain('tile #1'); }); }); + +// ─── Variable-enabled filters ──────────────────────────────────────────────── + +function makeFilter( + overrides: Partial = {}, +): ExternalDashboardFilterWithId { + return { + id: new mongoose.Types.ObjectId().toString(), + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + sourceId, + whereLanguage: 'sql', + ...overrides, + }; +} + +function makeSearchTile(overrides: { + name?: string; + where: string; + whereLanguage: 'sql' | 'lucene'; +}): ExternalDashboardTileWithId { + return { + id: 'search-tile', + x: 0, + y: 0, + w: 12, + h: 4, + name: overrides.name ?? 'Search Tile', + config: { + displayType: 'search', + sourceId, + select: '', + where: overrides.where, + whereLanguage: overrides.whereLanguage, + }, + }; +} + +function makeSeriesTile(overrides: { + name?: string; + where: string; + whereLanguage: 'sql' | 'lucene'; +}): ExternalDashboardTileWithId { + return { + id: 'series-tile', + x: 0, + y: 0, + w: 12, + h: 4, + name: overrides.name ?? 'Series Tile', + config: { + displayType: 'line', + sourceId, + select: [ + { + aggFn: 'count', + alias: 'Count', + where: overrides.where, + whereLanguage: overrides.whereLanguage, + }, + ], + }, + }; +} + +function makeTableTile(overrides: { + name?: string; + selectWhere: string; + selectWhereLanguage: 'sql' | 'lucene'; + having?: string; + groupBy?: string; +}): ExternalDashboardTileWithId { + return { + id: 'table-tile', + x: 0, + y: 0, + w: 12, + h: 4, + name: overrides.name ?? 'Table Tile', + config: { + displayType: 'table', + sourceId, + select: [ + { + aggFn: 'count', + alias: 'Count', + where: overrides.selectWhere, + whereLanguage: overrides.selectWhereLanguage, + }, + ], + groupBy: overrides.groupBy, + having: overrides.having, + }, + }; +} + +describe('mcpFiltersParam variable fields', () => { + it('preserves the variable fields so a filter round-trips', () => { + const parsed = mcpFiltersParam.parse([ + { + type: 'QUERY_EXPRESSION', + name: 'Service Name', + expression: 'ServiceName', + sourceId, + whereLanguage: 'sql', + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'service', + }, + ]); + + // The MCP schema used to omit these three, and a plain z.object silently + // drops unknown keys, so reading a variable-enabled dashboard and saving + // it back wiped the variable configuration. + expect(parsed[0]).toMatchObject({ + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'service', + }); + }); + + it.each([ + ['a space', 'has space'], + ['a leading digit', '1service'], + ['a leading underscore', '_service'], + ['a dash', 'service-name'], + ])('rejects a variableName with %s', (_label, variableName) => { + const result = mcpFiltersParam.safeParse([ + { + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + sourceId, + whereLanguage: 'sql', + isVariableEnabled: true, + variableName, + }, + ]); + expect(result.success).toBe(false); + }); + + it('rejects a variableName over the length limit', () => { + const result = mcpFiltersParam.safeParse([ + { + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + sourceId, + whereLanguage: 'sql', + isVariableEnabled: true, + variableName: `v${'a'.repeat(DASHBOARD_VARIABLE_NAME_MAX_LENGTH)}`, + }, + ]); + expect(result.success).toBe(false); + }); + + it('accepts a filter that omits every variable field', () => { + const parsed = mcpFiltersParam.parse([ + { + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + sourceId, + whereLanguage: 'sql', + }, + ]); + expect(parsed[0].isVariableEnabled).toBeUndefined(); + expect(parsed[0].isBroadcastEnabled).toBeUndefined(); + }); +}); + +describe('getTileVariableWarnings', () => { + const variableFilter = makeFilter({ + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'service', + }); + + it('returns nothing for a tile that references no variable', () => { + expect( + getTileVariableWarnings( + [ + makeSqlTile({ + sourceId, + sqlTemplate: 'SELECT count() FROM $__sourceTable', + }), + ], + [variableFilter], + ), + ).toEqual([]); + }); + + it('returns nothing for a raw SQL tile using $__filter on a declared variable', () => { + expect( + getTileVariableWarnings( + [ + makeSqlTile({ + sourceId, + sqlTemplate: + 'SELECT count() FROM $__sourceTable WHERE $__filter(ServiceName, $service)', + }), + ], + [variableFilter], + ), + ).toEqual([]); + }); + + it('warns about an unguarded bare reference in raw SQL', () => { + const warnings = getTileVariableWarnings( + [ + makeSqlTile({ + name: 'Errors', + sourceId, + sqlTemplate: + 'SELECT count() FROM $__sourceTable WHERE ServiceName IN ($service)', + }), + ], + [variableFilter], + ); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('Tile "Errors"'); + expect(warnings[0]).toContain('renders as NULL'); + }); + + it('warns about a reference wrapped in quotes', () => { + const warnings = getTileVariableWarnings( + [ + makeSqlTile({ + sourceId, + sqlTemplate: + "SELECT count() FROM $__sourceTable WHERE ServiceName = '$service'", + }), + ], + [variableFilter], + ); + expect(warnings.join('\n')).toContain('already quotes each value'); + }); + + it('warns when a macro names a variable the dashboard does not declare', () => { + const warnings = getTileVariableWarnings( + [ + makeSqlTile({ + sourceId, + sqlTemplate: + 'SELECT count() FROM $__sourceTable WHERE $__filter(ServiceName, $tenant)', + }), + ], + [variableFilter], + ); + expect(warnings.join('\n')).toContain('$__filter'); + expect(warnings.join('\n')).toContain("unknown variable 'tenant'"); + expect(warnings.join('\n')).toContain('Available variables: service'); + }); + + it('warns about a variable macro in a Lucene builder input', () => { + const warnings = getTileVariableWarnings( + [ + makeSearchTile({ + name: 'Recent Errors', + where: '$__filter(ServiceName, $service)', + whereLanguage: 'lucene', + }), + ], + [variableFilter], + ); + expect(warnings.join('\n')).toContain('Lucene'); + }); + + it('checks a builder tile per-series where', () => { + const warnings = getTileVariableWarnings( + [ + makeSeriesTile({ + name: 'Requests', + where: 'ServiceName IN ($service)', + whereLanguage: 'sql', + }), + ], + [variableFilter], + ); + expect(warnings.join('\n')).toContain('Tile "Requests"'); + expect(warnings.join('\n')).toContain('renders as NULL'); + }); + + it("checks a select item's condition in its own language", () => { + const warnings = getTileVariableWarnings( + [ + makeSeriesTile({ + name: 'Requests', + where: '$__filter(ServiceName, $service)', + whereLanguage: 'lucene', + }), + ], + [variableFilter], + ); + expect(warnings.join('\n')).toContain('Tile "Requests"'); + expect(warnings.join('\n')).toContain('no meaning in a Lucene expression'); + }); + + it('collects issues from every expression a builder tile walks', () => { + const warnings = getTileVariableWarnings( + [ + makeTableTile({ + name: 'Overview', + selectWhere: 'ServiceName IN ($service)', + selectWhereLanguage: 'sql', + having: "count() > 0 AND ServiceName = '$service'", + groupBy: '$service', + }), + ], + [variableFilter], + ); + expect(warnings).toHaveLength(3); + expect(warnings.join('\n')).toContain('already quotes each value'); + expect(warnings.join('\n')).toContain('renders as NULL'); + }); + + it('reports an unknown variable when the dashboard declares none', () => { + const warnings = getTileVariableWarnings( + [ + makeSqlTile({ + sourceId, + sqlTemplate: + 'SELECT count() FROM $__sourceTable WHERE ServiceName IN ($service)', + }), + ], + // A broadcast-only filter declares no variable. + [makeFilter()], + ); + expect(warnings.join('\n')).toContain('unknown variable'); + expect(warnings.join('\n')).toContain('(none)'); + }); + + it('ignores markdown tiles, which have no expressions to check', () => { + const markdownTile: ExternalDashboardTileWithId = { + id: 'md', + x: 0, + y: 0, + w: 12, + h: 3, + name: 'Notes', + config: { displayType: 'markdown', markdown: 'Pick a $service' }, + }; + expect(getTileVariableWarnings([markdownTile], [variableFilter])).toEqual( + [], + ); + }); +}); + +describe('getFilterVariableWarnings', () => { + // The upstream filter of every dependent-filter case below. + const serviceFilter = makeFilter({ + name: 'Service', + expression: 'ServiceName', + isVariableEnabled: true, + variableName: 'service', + }); + + const endpointFilter = ( + overrides: Partial = {}, + ) => + makeFilter({ + name: 'Endpoint', + expression: 'SpanName', + ...overrides, + }); + + it('returns nothing when no filter carries a where', () => { + expect( + getFilterVariableWarnings([serviceFilter, endpointFilter()]), + ).toEqual([]); + }); + + it('accepts a dependent filter guarded by $__filter', () => { + expect( + getFilterVariableWarnings([ + serviceFilter, + endpointFilter({ where: '$__filter(ServiceName, $service)' }), + ]), + ).toEqual([]); + }); + + it('warns about a bare reference that leaves the dropdown empty', () => { + const warnings = getFilterVariableWarnings([ + serviceFilter, + endpointFilter({ where: 'ServiceName IN ($service)' }), + ]); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('Filter "Endpoint"'); + expect(warnings[0]).toContain('renders as NULL'); + }); + + it('names the filter that must publish a referenced variable', () => { + // "Service" collects the value but never exposes it, so $service does not + // exist. The generic unknown-variable warning cannot say why; this one can. + const warnings = getFilterVariableWarnings([ + makeFilter({ name: 'Service', expression: 'ServiceName' }), + endpointFilter({ where: '$__filter(ServiceName, $service)' }), + ]); + const joined = warnings.join('\n'); + expect(joined).toContain('$service is not published as a variable'); + expect(joined).toContain('The filter named "Service"'); + expect(joined).toContain('isVariableEnabled: true'); + }); + + it('warns when a filter references its own variable', () => { + const warnings = getFilterVariableWarnings([ + serviceFilter, + endpointFilter({ + isVariableEnabled: true, + variableName: 'endpoint', + where: '$__filter(SpanName, $endpoint)', + }), + ]); + const joined = warnings.join('\n'); + expect(joined).toContain("references this filter's own variable $endpoint"); + expect(joined).toContain('values already selected'); + }); + + it('warns about a variable macro in a Lucene dropdown query', () => { + const warnings = getFilterVariableWarnings([ + serviceFilter, + endpointFilter({ + where: '$__filter(ServiceName, $service)', + whereLanguage: 'lucene', + }), + ]); + expect(warnings.join('\n')).toContain('Lucene'); + }); + + it('treats a missing whereLanguage as sql, matching the dropdown query', () => { + // resolveFilterValuesWhere defaults to sql, so the macro resolves and + // there is nothing to report. Defaulting to lucene here would produce a + // bogus "no meaning in a Lucene expression" error on a working filter. + const { whereLanguage: _dropped, ...withoutLanguage } = endpointFilter({ + where: '$__filter(ServiceName, $service)', + }); + expect(getFilterVariableWarnings([serviceFilter, withoutLanguage])).toEqual( + [], + ); + }); + + it('reports an unknown variable that no filter would own', () => { + const warnings = getFilterVariableWarnings([ + serviceFilter, + endpointFilter({ where: '$__filter(ServiceName, $tenant)' }), + ]); + const joined = warnings.join('\n'); + expect(joined).toContain('Filter "Endpoint"'); + expect(joined).toContain("unknown variable 'tenant'"); + expect(joined).toContain('Available variables: service'); + }); +}); diff --git a/packages/api/src/mcp/__tests__/dashboards/variables.test.ts b/packages/api/src/mcp/__tests__/dashboards/variables.test.ts new file mode 100644 index 0000000000..9c3f000577 --- /dev/null +++ b/packages/api/src/mcp/__tests__/dashboards/variables.test.ts @@ -0,0 +1,157 @@ +import mongoose from 'mongoose'; + +import { + resolveDashboardVariables, + withResolvedFilterVariableNames, +} from '@/mcp/tools/dashboards/variables'; +import type { ExternalDashboardFilterWithId } from '@/utils/zod'; + +const sourceId = new mongoose.Types.ObjectId().toString(); + +function filter( + overrides: Partial = {}, +): ExternalDashboardFilterWithId { + return { + id: new mongoose.Types.ObjectId().toString(), + type: 'QUERY_EXPRESSION', + name: 'Service', + expression: 'ServiceName', + sourceId, + whereLanguage: 'sql', + ...overrides, + } as ExternalDashboardFilterWithId; +} + +const variableFilter = filter({ + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'service', +}); + +describe('resolveDashboardVariables', () => { + it('returns an empty array for a dashboard with no filters', () => { + // Empty, not undefined: an empty array still registers $__filter and + // $__conditionalAll as macros, so a tile using one against a dashboard + // that declares nothing fails with a message about the variable rather + // than leaking macro text into ClickHouse. + expect(resolveDashboardVariables(undefined, undefined)).toEqual({ + variables: [], + }); + expect(resolveDashboardVariables([], undefined)).toEqual({ + variables: [], + }); + }); + + it('declares nothing for a broadcast-only filter', () => { + expect(resolveDashboardVariables([filter()], undefined)).toEqual({ + variables: [], + }); + }); + + it('defaults every declared variable to an empty selection', () => { + expect(resolveDashboardVariables([variableFilter], undefined)).toEqual({ + variables: [{ name: 'service', expression: 'ServiceName', values: [] }], + }); + }); + + it('applies a supplied selection', () => { + expect( + resolveDashboardVariables( + [variableFilter], + [{ name: 'service', values: ['checkout'] }], + ), + ).toEqual({ + variables: [ + { name: 'service', expression: 'ServiceName', values: ['checkout'] }, + ], + }); + }); + + it('leaves variables the caller did not mention empty', () => { + const env = filter({ + name: 'Environment', + expression: 'Env', + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: 'env', + }); + + const resolved = resolveDashboardVariables( + [variableFilter, env], + [{ name: 'service', values: ['checkout'] }], + ); + + expect(resolved).toEqual({ + variables: [ + { name: 'service', expression: 'ServiceName', values: ['checkout'] }, + { name: 'env', expression: 'Env', values: [] }, + ], + }); + }); + + it('rejects a name the dashboard does not declare', () => { + const resolved = resolveDashboardVariables( + [variableFilter], + [{ name: 'tenant', values: ['acme'] }], + ); + + expect(resolved).toHaveProperty('error'); + if (!('error' in resolved)) throw new Error('expected an error'); + expect(resolved.error).toContain('tenant'); + expect(resolved.error).toContain('Available variables: service'); + }); + + it('lists (none) when the dashboard declares no variables at all', () => { + const resolved = resolveDashboardVariables( + [filter()], + [{ name: 'service', values: ['checkout'] }], + ); + + if (!('error' in resolved)) throw new Error('expected an error'); + expect(resolved.error).toContain('Available variables: (none)'); + }); + + it('lets the last entry win on a repeated name', () => { + const resolved = resolveDashboardVariables( + [variableFilter], + [ + { name: 'service', values: ['a'] }, + { name: 'service', values: ['b'] }, + ], + ); + + expect(resolved).toEqual({ + variables: [ + { name: 'service', expression: 'ServiceName', values: ['b'] }, + ], + }); + }); +}); + +describe('withResolvedFilterVariableNames', () => { + it('leaves a broadcast-only filter untouched', () => { + const broadcastOnly = filter(); + expect(withResolvedFilterVariableNames([broadcastOnly])).toEqual([ + broadcastOnly, + ]); + }); + + it('leaves an explicit variableName untouched', () => { + expect(withResolvedFilterVariableNames([variableFilter])).toEqual([ + variableFilter, + ]); + }); + + it('fills in the name derived from the display name when omitted', () => { + const noExplicitName = filter({ + name: 'Service Name', + isBroadcastEnabled: false, + isVariableEnabled: true, + variableName: undefined, + }); + + expect(withResolvedFilterVariableNames([noExplicitName])).toEqual([ + { ...noExplicitName, variableName: 'Service_Name' }, + ]); + }); +}); diff --git a/packages/api/src/mcp/__tests__/query.test.ts b/packages/api/src/mcp/__tests__/query.test.ts index 21a285c969..a404a831c3 100644 --- a/packages/api/src/mcp/__tests__/query.test.ts +++ b/packages/api/src/mcp/__tests__/query.test.ts @@ -7,6 +7,10 @@ jest.mock('@/utils/trimToolResponse', () => ({ })); import { ClickHouseError } from '@clickhouse/client-common'; +import { + MacroExpansionError, + UnknownVariableError, +} from '@hyperdx/common-utils/dist/macroErrors'; import { annotateIncreaseTopNHint, @@ -232,6 +236,49 @@ describe('errorHint', () => { expect(hint).toBeNull(); }); + describe('unknown dashboard variable', () => { + // Matched on the error's type, not its wording, so rephrasing the + // expansion message cannot silently drop the hint. + it('points at the filter config', () => { + const hint = errorHint( + 'some message the hint does not read', + new UnknownVariableError('filter', 'tenant', ['service', 'env'], 'x'), + ); + + expect(hint).toContain('isVariableEnabled'); + expect(hint).toContain('clickstack_get_dashboard'); + }); + + it('finds the error through a wrapper that keeps it as a cause', () => { + const hint = errorHint( + 'x', + new Error('Query failed', { + cause: new UnknownVariableError('filter', 'tenant', ['service'], 'x'), + }), + ); + + expect(hint).toContain('isVariableEnabled'); + }); + + it('does not fire on a different expansion failure for the same macro', () => { + // This message already says exactly what to do, so the + // declare-a-variable hint would be misleading noise on top of it. The + // old string match could not tell the two apart. + const message = + "Macro '$__filter($service)' requires the variable's filter " + + 'expression, which is not available - pass it explicitly, e.g. ' + + '$__filter(, $service).'; + + expect( + errorHint(message, new MacroExpansionError('filter', message)), + ).toBeNull(); + }); + + it('does not fire on a message that merely mentions an unknown variable', () => { + expect(errorHint("references unknown variable 'tenant'")).toBeNull(); + }); + }); + it('should match V8 string length overflow', () => { const hint = errorHint( 'response length exceeds the maximum allowed size of V8 String', diff --git a/packages/api/src/mcp/__tests__/queryTool.int.test.ts b/packages/api/src/mcp/__tests__/queryTool.int.test.ts index a6eae51702..0efcb681d8 100644 --- a/packages/api/src/mcp/__tests__/queryTool.int.test.ts +++ b/packages/api/src/mcp/__tests__/queryTool.int.test.ts @@ -724,6 +724,36 @@ describe('MCP Query Tools', () => { expect(result.isError).toBe(true); expect(getFirstText(result)).toContain('Invalid'); }); + + it.each([ + '$__filter(ServiceName, $service)', + '$__conditionalAll(1=1, $svc)', + ])( + 'rejects %s, which needs a dashboard to resolve against', + async macro => { + // Rejected up front rather than sent on: an unrecognized macro passes + // through as literal text, so ClickHouse would answer with an opaque + // syntax error pointing at a '$'. + const result = await callTool(client, 'clickstack_sql', { + connectionId: connection._id.toString(), + sql: `SELECT 1 AS value WHERE ${macro}`, + }); + + expect(result.isError).toBe(true); + expect(getFirstText(result)).toContain( + 'only resolve for a tile on a dashboard', + ); + }, + ); + + it('still allows $__filters, which needs no variable context', async () => { + const result = await callTool(client, 'clickstack_sql', { + connectionId: connection._id.toString(), + sql: 'SELECT 1 AS value WHERE $__filters', + }); + + expect(result.isError).toBeFalsy(); + }); }); // ─── ClickHouse query errors ────────────────────────────────────────────────── diff --git a/packages/api/src/mcp/__tests__/sqlToolVariableGuard.test.ts b/packages/api/src/mcp/__tests__/sqlToolVariableGuard.test.ts new file mode 100644 index 0000000000..7a88bcd48b --- /dev/null +++ b/packages/api/src/mcp/__tests__/sqlToolVariableGuard.test.ts @@ -0,0 +1,117 @@ +// Unit test for the clickstack_sql input guard that rejects the variable +// macros. Mocks runConfigTile so the accepted cases prove "the guard let this +// through" without needing ClickHouse: the guard returns before any query, so +// whether runConfigTile was called is exactly the signal under test. + +const mockRunConfigTile = jest.fn(); + +jest.mock('@/mcp/tools/query/helpers', () => { + const actual = jest.requireActual('@/mcp/tools/query/helpers'); + return { + ...actual, + runConfigTile: (...args: unknown[]) => mockRunConfigTile(...args), + }; +}); + +jest.mock('@/models/source', () => ({})); +jest.mock('@/controllers/sources', () => ({})); +jest.mock('@/controllers/connection', () => ({})); + +import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; +import mongoose from 'mongoose'; + +import { registerSql } from '@/mcp/tools/query/sql'; +import type { McpContext, RegisterToolFn, ToolResult } from '@/mcp/tools/types'; + +type Handler = Parameters[2]; + +// The accepted cases run on past the guard into buildTile, which parses this +// through objectIdSchema. +const connectionId = new mongoose.Types.ObjectId().toString(); + +function buildHandler(): Handler { + let captured: Handler | undefined; + const registerTool: RegisterToolFn = (_name, _config, handler) => { + captured = handler; + }; + const context: McpContext = { teamId: 'team-1', userId: 'user-1' }; + const server = new McpServer({ name: 'test', version: '0.0.0' }); + registerSql({ server, context, registerTool }); + if (!captured) throw new Error('handler was not registered'); + return captured; +} + +function textOf(result: ToolResult): string { + const item = result.content[0]; + if (!item || item.type !== 'text') throw new Error('expected text content'); + return item.text; +} + +const okResult = { + content: [{ type: 'text' as const, text: JSON.stringify({ result: [] }) }], +}; + +beforeEach(() => { + jest.clearAllMocks(); + mockRunConfigTile.mockResolvedValue(okResult); +}); + +describe('clickstack_sql variable macro guard', () => { + it.each([ + ['$__filter with an expression', '$__filter(ServiceName, $service)'], + ['$__filter with one argument', '$__filter($service)'], + ['$__conditionalAll', "$__conditionalAll(ServiceName != 'api', $service)"], + ])('rejects %s before issuing a query', async (_label, macro) => { + const handler = buildHandler(); + const result = await handler({ + connectionId, + sql: `SELECT 1 AS value WHERE ${macro}`, + }); + + expect(result.isError).toBe(true); + expect(textOf(result)).toContain('only resolve for a tile on a dashboard'); + // The point of rejecting up front: ClickHouse never sees the literal + // macro text, so the agent gets this instead of a syntax error on '$'. + expect(mockRunConfigTile).not.toHaveBeenCalled(); + }); + + it('does NOT reject $__filters, which takes no variable context', async () => { + // The regression that matters. $__filters (plural) is a different macro + // that every dashboard-scoped raw SQL query is told to use, and the + // singular name is a prefix of it. + const handler = buildHandler(); + const result = await handler({ + connectionId, + sql: 'SELECT 1 AS value WHERE $__filters', + }); + + expect(result.isError).toBeFalsy(); + expect(mockRunConfigTile).toHaveBeenCalledTimes(1); + }); + + it.each([ + ['$__timeFilter', 'SELECT 1 WHERE $__timeFilter(Timestamp)'], + ['a bare $ reference', "SELECT 1 WHERE Body LIKE '%$service%'"], + ['a commented-out macro', 'SELECT 1 -- $__filter(a, $b)'], + ])('does not reject %s', async (_label, sql) => { + const handler = buildHandler(); + const result = await handler({ connectionId, sql }); + + expect(result.isError).toBeFalsy(); + expect(mockRunConfigTile).toHaveBeenCalledTimes(1); + }); + + it('rejects before parsing the time range is even relevant', async () => { + // Ordering check: an invalid time range is reported first, so a caller + // fixing one problem at a time is not bounced between the two. + const handler = buildHandler(); + const result = await handler({ + connectionId, + sql: 'SELECT 1 WHERE $__filter(ServiceName, $service)', + startTime: 'not-a-date', + }); + + expect(result.isError).toBe(true); + expect(textOf(result)).toContain('Invalid'); + }); +}); diff --git a/packages/api/src/mcp/prompts/dashboards/content.ts b/packages/api/src/mcp/prompts/dashboards/content.ts index 9f994c46fb..32f6ed137a 100644 --- a/packages/api/src/mcp/prompts/dashboards/content.ts +++ b/packages/api/src/mcp/prompts/dashboards/content.ts @@ -108,6 +108,10 @@ Apply these before calling clickstack_save_dashboard. Each rule is enforced by t 9. FOR FOCUSED PER-DIMENSION DASHBOARDS, DECLARE A DASHBOARD-LEVEL FILTER. Pass filters: [{ type: "QUERY_EXPRESSION", name, expression, sourceId }] at the top level. The user gets a dropdown in the dashboard header; by default every tile is re-scoped when a value is picked. On mixed-source dashboards add appliesToSourceIds: ["", ...] to restrict the filter to only the tiles whose source carries that column. Omit the field to keep the broadcast-to-all-tiles default. Do NOT hardcode the dimension into each tile's where clause. +9b. USE A VARIABLE FILTER ONLY WHEN BROADCAST CANNOT DO THE JOB. A filter with isVariableEnabled: true publishes its value as $variableName instead of applying it automatically, and only the tiles that reference it are affected. That is the right shape when the predicate is not a plain equality (NOT IN, LIKE, a regex, a range), when the value has to sit somewhere a WHERE condition cannot go (a CTE, a SELECT expression, a JOIN condition), or when different tiles need the same value against different columns. Set isBroadcastEnabled: false alongside it; leaving both on applies the value twice. Reference it as $__filter(, $) rather than a bare $variableName, which renders as NULL before anything is selected. Both macros work in every SQL condition on a builder tile: a select item's where, the tile-level where on search / heatmap / event_patterns tiles, having, groupBy, and orderBy. Set whereLanguage: "sql" on the input that carries one; in a Lucene input they are matched as literal text. + +9c. TO CHAIN ONE DROPDOWN OFF ANOTHER, REFERENCE A VARIABLE IN THE DOWNSTREAM FILTER'S OWN where. A filter's where scopes the values ITS dropdown offers and is never applied to a tile, so a dashboard can pick a service and then offer only that service's endpoints: filters: [{ name: "Service", expression: "ServiceName", isVariableEnabled: true, variableName: "service", ... }, { name: "Endpoint", expression: "SpanName", whereLanguage: "sql", where: "$__filter(ServiceName, $service)", ... }]. Only the upstream filter needs isVariableEnabled, and it can keep broadcasting because its value is read by a dropdown rather than a tile. Use $__filter here too so the downstream dropdown lists everything until a service is picked; a bare $service leaves it empty. + 10. UPDATE IS REPLACE, NOT MERGE. clickstack_save_dashboard with an id overwrites tiles, containers, and filters in their entirety. Call clickstack_get_dashboard first when you only want to add or rename one entry; do not send a partial set or you will silently drop everything you omitted. 11. GROUP RELATED TILES INTO CONTAINERS. REQUIRED at five or more tiles, no exceptions. An ungrouped wall of nine or ten tiles is a readability failure even when each tile is correct in isolation. Containers are the right way to introduce structure; markdown tiles for section labels are not. @@ -159,7 +163,16 @@ Dashboards open with a 15-minute default window. There is no dashboard-level fie - Assuming StatusCode or SeverityText values (always inspect lowCardinalityValues from clickstack_describe_source). - Heatmap on a non-Trace source (heatmap is Trace-only today). - Hardcoding a focus dimension into every tile's where clause (use a dashboard-level filter instead). -- Putting a filter at the tile-config top level on a table / line / stacked_bar / number / pie / bar tile (ignored for these types; put the where on each select item instead, see gotcha 15).`; +- Putting a filter at the tile-config top level on a table / line / stacked_bar / number / pie / bar tile (ignored for these types; put the where on each select item instead, see gotcha 15). +- Enabling both isBroadcastEnabled and isVariableEnabled on one filter (the picked value is then applied twice; set isBroadcastEnabled: false when you add a variable). The exception is rule 9c: a variable read only by another filter's where never reaches a tile, so that filter can keep broadcasting. +- Broadcasting one filter across sources that do not share the column (it is ANDed onto every tile and fails at query time; scope it with appliesToSourceIds or make it a variable). +- Referencing a variable as a bare $var instead of $__filter(, $) or $__conditionalAll(, $). A bare reference renders as NULL with nothing selected, so the tile shows zero rows on a freshly-opened dashboard. +- Dropping the $ from the variable argument of a macro. It is $__filter(ServiceName, $service), not $__filter(ServiceName, service). +- Wrapping a variable reference in quotes. The default SQL format (sqlstring) already quotes each value. +- Adding a redundant format specifier such as \${var:sqlstring}, or omitting a needed one such as \${var:regex} inside match() and \${var:csv} inside a literal. +- Using $__filter or $__conditionalAll in a Lucene where (they are matched as literal text; switch to whereLanguage: "sql", or write ServiceName:$var). +- Chaining a filter's where off a filter that is not variable-enabled. Only isVariableEnabled: true publishes the $variableName token; without it the reference resolves to nothing and the downstream dropdown is unscoped or empty. +- Referencing a filter's OWN variable in its where. That narrows its dropdown to the values already picked, so the rest of the options vanish from a multi-select. Reference a sibling filter's variable instead.`; } export function buildDashboardExamplesPrompt( @@ -922,6 +935,15 @@ For configType: "sql" tiles, write ClickHouse SQL with template macros: $__sourceTable the source's \`database\`.\`table\`. REQUIRES sourceId on the tile. Without it, the query fails to run. + VARIABLE MACROS (only resolve on a dashboard tile, see DASHBOARD VARIABLES): + $__filter(, $) Matches the expression against the variable's selected + values (e.g \` IN ('a', 'b')\`). Expands to + 1=1 when nothing is selected. + $__conditionalAll(, $) Applies the condition only while the variable has a + selection, and expands to 1=1 otherwise. Use it for + operators IN cannot express, such as NOT IN or LIKE. + The variable argument carries the leading $, exactly as a bare reference would. + QUERY PARAMETERS (ClickHouse parameterized syntax): {startDateMilliseconds:Int64} {endDateMilliseconds:Int64} @@ -1101,18 +1123,32 @@ Rules: == DASHBOARD FILTERS == -Optional dashboard-level filter declarations. Each entry adds a dropdown to the dashboard header that scopes tiles against the filter's expression. Use this for focused per-dimension dashboards (per-service, per-tenant, per-endpoint) instead of hardcoding the dimension into every tile's where clause. +Optional dashboard-level filter declarations. Each entry adds a dropdown to the dashboard header. Use this for focused per-dimension dashboards (per-service, per-tenant, per-endpoint) instead of hardcoding the dimension into every tile's where clause. + +A filter does one of two things with the value the user picks, or both: + + BROADCAST (isBroadcastEnabled, ON by default) + The value is ANDed onto every in-scope tile automatically. Tiles need no wiring at all. This is the right choice in most cases. + + VARIABLE (isVariableEnabled, OFF by default) + The value is exposed to tile queries as $variableName, and only the tiles that reference it are affected. See DASHBOARD VARIABLES below. + +Pick ONE. A filter with both modes on applies the value twice, once implicitly to every in-scope tile and once wherever a tile references it, which double-filters and is hard to reason about. Enable both only when you deliberately want the automatic scoping AND a tile that reads the raw value somewhere a plain AND cannot go, or when the variable is read only by another filter's dropdown query rather than by a tile (see DEPENDENT FILTERS). Filter shape: - { type, name, expression, sourceId, where?, whereLanguage?, appliesToSourceIds? } + { type, name, expression, sourceId, where?, whereLanguage?, appliesToSourceIds?, isBroadcastEnabled?, isVariableEnabled?, variableName? } type "QUERY_EXPRESSION" (the only currently supported type). name Human label shown in the filter dropdown (e.g. "Service"). expression Column or attribute path the filter scopes (e.g. "ServiceName" or "SpanAttributes['tenant.id']"). sourceId Which source the dropdown VALUES are queried from. Independent of which tiles get filtered (see appliesToSourceIds below). - where Optional pre-filter that narrows the set of distinct values offered in the dropdown. - whereLanguage "lucene" or "sql". Defaults to "lucene". - appliesToSourceIds Optional list of source IDs the filter applies to. Omit (or pass undefined) to apply the filter to EVERY tile regardless of source (the recommended default). Pass a non-empty array to restrict the filter to tiles whose source is in that list, useful on mixed-source dashboards where the column only exists on some sources. + sourceMetricType REQUIRED when sourceId is a Metric source. Picks which metric table the dropdown values come from. + where Optional pre-filter that narrows the set of distinct values offered in the dropdown. It scopes only this dropdown, never a tile. May reference another filter's variable, which chains one dropdown off another (see DEPENDENT FILTERS below). + whereLanguage "lucene" or "sql". Defaults to "sql", set it explicitly anyway. + appliesToSourceIds Optional list of source IDs the BROADCAST condition applies to. Omit (or pass undefined) to apply it to EVERY tile regardless of source (the recommended default). Pass a non-empty array to restrict it to tiles whose source is in that list, useful on mixed-source dashboards where the column only exists on some sources. Rejected when isBroadcastEnabled is false, and it does NOT restrict which tiles may reference the variable. + isBroadcastEnabled Omit or set to true to enable broadcasting. Set false for a variable-only filter. + isVariableEnabled Set true to expose the value as $variableName. + variableName The token tiles reference. Must match [a-zA-Z][a-zA-Z0-9_]* and be at most 64 characters, and must be unique across the dashboard's variable-enabled filters. Defaults to the display name with whitespace turned into underscores and remaining illegal characters dropped, so "Service Name" becomes $Service_Name and a label with nothing token-safe in it (a non-ASCII name, for instance) must set this field explicitly. Rejected when isVariableEnabled is not true. Example (broadcast to every tile, the common case): filters: [ @@ -1132,6 +1168,88 @@ Example (scoped to the trace source only on a mixed log/trace/metric dashboard): When a value is picked in the dropdown, the renderer combines it with each in-scope tile's existing where clause via AND. Tiles do NOT need to reference the filter name; matching the scope (or no scope set) is enough. +== DASHBOARD VARIABLES == + +A filter with isVariableEnabled: true publishes its selected value to tile queries under variableName. Nothing happens until a tile references it: variables are opt-in per tile, while broadcast is automatic. + +REACH FOR A VARIABLE WHEN BROADCAST CANNOT DO THE JOB. Broadcast plus a builder tile covers the ordinary "scope this dashboard to one service" case with no per-tile wiring and no way to get it wrong. A variable is worth the extra plumbing when: + - the predicate is not a plain equality (NOT IN, LIKE, a regex match, a range) + - the value has to appear somewhere a WHERE condition cannot go (inside a CTE, a SELECT expression, a JOIN condition, an argument to a function) + - different tiles need to apply the same picked value against different columns + - the dashboard mixes sources that do not share the column, so a broadcast filter would inject a column that half the tiles do not have + +THREE WAYS TO REFERENCE A VARIABLE + + $__filter(, $) + The default choice, in SQL. Expands to ( IN ('a', 'b')) when values are selected and to 1=1 when nothing is, so the tile shows everything on a freshly-opened dashboard instead of going blank. + where: "$__filter(ServiceName, $service)" + The one-argument form $__filter($service) reuses the filter's own expression, but it fails when the filter does not carry one, so pass the expression explicitly whenever the tile's column differs from the filter's. + + $__conditionalAll(, $) + Applies the condition verbatim while something is selected, and 1=1 otherwise. Use it for anything IN cannot express. The condition itself should reference the variable. + where: "$__conditionalAll(ServiceName NOT IN ($service), $service)" + + $variableName / \${variableName} / \${variableName:format} + The raw value. In a SQL input, only reach for this when you need the value in a position a predicate cannot occupy: outside a $__filter or $__conditionalAll guard the default sqlstring format renders as NULL when nothing is selected, so "ServiceName IN ($service)" matches zero rows before the user picks anything. In a LUCENE input it is the only form available and it needs no guard: the lucene format renders ("") for an empty selection, which the translator drops to a match-all, so "ServiceName:$service" returns everything until the user picks a service. + +FORMAT SPECIFIERS + +Only add one when the default is wrong. \${service:sqlstring} is redundant in a SQL input. + + Format Example Output Empty state + sqlstring 'a', 'b' NULL the default in SQL inputs + lucene ("a" OR "b") ("") the default in Lucene inputs; ("") is a match-all, so no guard is needed + regex (a|b) .* use with match() + csv a,b use INSIDE a string literal + +BUILDER TILES + +Every expression on a builder tile accepts variable references, in either language. The two MACROS additionally work in every SQL-language expression: a select item's where, the tile-level where on search / heatmap / event_patterns tiles, having, groupBy, and orderBy. There is no tile-level where on table / line / stacked_bar / number / pie / bar tiles, so those carry the macro on each select item. Set whereLanguage: "sql" on whichever input carries one. + select: [{ aggFn: "count", whereLanguage: "sql", where: "$__filter(ServiceName, $service)", alias: "Requests" }] + select: [{ aggFn: "count", whereLanguage: "sql", where: "$__conditionalAll(SpanName NOT IN ($endpoint), $endpoint)", alias: "Other endpoints" }] + having: "$__filter(SpanName, $endpoint)" (post-aggregation, on a groupBy column) + where: "$__filter(ServiceName, $service)", whereLanguage: "sql" (search / heatmap / event_patterns tiles) + +In a LUCENE input the macros have no meaning and are matched as literal text. Reference the variable directly instead, which renders in the lucene format and needs no guard: with nothing selected it becomes ServiceName:("") and the translator drops that to a match-all, so the tile returns everything rather than going empty. + select: [{ aggFn: "count", whereLanguage: "lucene", where: "ServiceName:$service" }] +When in doubt on a variable-driven tile, set whereLanguage: "sql" and use $__filter. + +RAW SQL TILES (for advanced use-cases only) + + SELECT $__timeInterval(Timestamp) AS ts, count() AS errors + FROM $__sourceTable + WHERE $__timeFilter(Timestamp) + AND $__filters + AND $__filter(ServiceName, $service) + AND $__conditionalAll(SpanName NOT IN (\${endpoint}), $endpoint) + GROUP BY ts + ORDER BY ts + +$__filters (plural) and $__filter (singular) are different macros. $__filters applies the BROADCAST filters with no arguments. $__filter takes an expression and a $-prefixed variable name. + +DEPENDENT FILTERS + +A filter's own \`where\` can reference another filter's variable, which chains one dropdown off another: pick a service, and the Endpoint dropdown lists only that service's endpoints. The \`where\` scopes the values THAT dropdown offers and is never applied to a tile, so the upstream filter can keep broadcasting; only the upstream filter needs isVariableEnabled: true. + + filters: [ + { type: "QUERY_EXPRESSION", name: "Service", expression: "ServiceName", + sourceId: "", whereLanguage: "sql", + isVariableEnabled: true, variableName: "service" }, + { type: "QUERY_EXPRESSION", name: "Endpoint", expression: "SpanName", + sourceId: "", whereLanguage: "sql", + where: "$__filter(ServiceName, $service)" } + ] + +Use $__filter here for the same reason a tile does: it expands to 1=1 with nothing selected, so the Endpoint dropdown lists every endpoint until a service is picked. A bare reference (where: "ServiceName IN ($service)") renders as NULL instead, leaving the dropdown empty until the upstream filter is set. That is occasionally what you want ("pick a service first"), and the UI labels such a dropdown as depending on $service, but make it a deliberate choice. + +Reference a SIBLING filter's variable only. A filter whose \`where\` names its own variable narrows its dropdown to the values already picked, so the remaining options disappear from a multi-select. + +VALIDATING + +clickstack_query_tile and clickstack_query_tiles expand variables with an EMPTY selection by default, which is what a freshly-opened dashboard looks like. Optionally, pass variableValues to check that a tile narrows correctly once something is picked: + variableValues: [{ name: "service", values: ["checkout"] }] +Both tools report variable problems under "warnings". Alerts on a tile always evaluate with every variable empty, so a tile carrying an alert must stay correct with nothing selected. + == TABLE TILE LINKING (config.onClick) == Table tiles can wire up row-click navigation via config.onClick. Builder table tiles (displayType: "table") and raw-SQL tiles (configType: "sql", displayType: "table") both accept this; other tile types ignore it. @@ -1439,6 +1557,39 @@ Example: find top patterns for production services over the last 4 hours: AND ROW-CLICK TEMPLATES; builder tiles do not alias map-attribute columns cleanly, so you need a raw SQL tile with explicit AS. +21. Making one filter both broadcast and variable enabled + A filter with isBroadcastEnabled left on AND isVariableEnabled: true + applies the picked value twice, once implicitly to every in-scope tile + and once wherever a tile references it. Pick one mode. Set + isBroadcastEnabled: false when you add isVariableEnabled: true. The + exception is a variable read only by another filter's where (a dependent + dropdown): that value never reaches a tile, so broadcasting stays correct. + +22. Broadcasting one filter across incompatible sources + A broadcast filter with no appliesToSourceIds is ANDed onto EVERY tile, + including tiles whose source has no such column, which fails at query + time. On a mixed log/trace/metric dashboard either scope it with + appliesToSourceIds, or switch it to a variable and reference it only + from the tiles that can use it. + +23. A bare $var instead of $__filter or $__conditionalAll + Wrong: where: "ServiceName IN ($service)" + Correct: where: "$__filter(ServiceName, $service)" + In the default sqlstring format a bare reference renders as NULL when + nothing is selected, so the tile shows zero rows on a freshly-opened + dashboard. The macros expand to 1=1 instead. + +24. A variable macro in a Lucene input + $__filter and $__conditionalAll have no meaning in Lucene and are + matched as literal text. Set whereLanguage: "sql" and use the macro, or + keep Lucene and write ServiceName:$service. + +25. Chaining one filter's dropdown off a filter that is not variable-enabled + A dependent filter's where can only read a variable that some filter + publishes. The UPSTREAM filter needs isVariableEnabled: true (plus its + variableName); the downstream one does not. Without it there is no + $service token and the downstream dropdown is never scoped. + == REFERENCES == External docs for the syntax and macros referenced in this guide: diff --git a/packages/api/src/mcp/tools/dashboards/getDashboard.ts b/packages/api/src/mcp/tools/dashboards/getDashboard.ts index 4eb533bcb5..d03bbb20ee 100644 --- a/packages/api/src/mcp/tools/dashboards/getDashboard.ts +++ b/packages/api/src/mcp/tools/dashboards/getDashboard.ts @@ -8,6 +8,8 @@ import { mcpUserError, validateObjectId } from '@/mcp/utils/errors'; import Dashboard from '@/models/dashboard'; import { convertToExternalDashboard } from '@/routers/external-api/v2/utils/dashboards'; +import { withResolvedFilterVariableNames } from './variables'; + export function registerGetDashboard({ context, registerTool, @@ -57,13 +59,17 @@ export function registerGetDashboard({ if (!dashboard) { return mcpUserError('Dashboard not found'); } + const externalDashboard = convertToExternalDashboard(dashboard); return { content: [ { type: 'text' as const, text: JSON.stringify( { - ...convertToExternalDashboard(dashboard), + ...externalDashboard, + filters: withResolvedFilterVariableNames( + externalDashboard.filters ?? [], + ), ...(frontendUrl ? { url: `${frontendUrl}/dashboards/${dashboard._id}` } : {}), diff --git a/packages/api/src/mcp/tools/dashboards/patchDashboard.ts b/packages/api/src/mcp/tools/dashboards/patchDashboard.ts index 06eeafcc0c..fecd1922fa 100644 --- a/packages/api/src/mcp/tools/dashboards/patchDashboard.ts +++ b/packages/api/src/mcp/tools/dashboards/patchDashboard.ts @@ -18,6 +18,7 @@ import { mcpPatchDashboardSchema } from './schemas'; import { getRawSqlMissingSourceError, getRawSqlTileMacroWarnings, + getTileVariableWarnings, } from './validation'; export function registerPatchDashboard({ @@ -245,7 +246,10 @@ export function registerPatchDashboard({ output.patchedTile = patchedTile; output.hint = 'Use clickstack_query_tile to test the patched tile query.'; - const macroWarnings = getRawSqlTileMacroWarnings([patchedTile]); + const macroWarnings = [ + ...getRawSqlTileMacroWarnings([patchedTile]), + ...getTileVariableWarnings([patchedTile], existingDashboard.filters), + ]; if (macroWarnings.length > 0) { output.warnings = macroWarnings; } diff --git a/packages/api/src/mcp/tools/dashboards/queryTile.ts b/packages/api/src/mcp/tools/dashboards/queryTile.ts index 9e4c3cf104..dcb852094c 100644 --- a/packages/api/src/mcp/tools/dashboards/queryTile.ts +++ b/packages/api/src/mcp/tools/dashboards/queryTile.ts @@ -7,7 +7,11 @@ import Dashboard from '@/models/dashboard'; import { convertToExternalDashboard } from '@/routers/external-api/v2/utils/dashboards'; import { objectIdSchema } from '@/utils/zod'; -import { getRawSqlTileMacroWarnings } from './validation'; +import { + getRawSqlTileMacroWarnings, + getTileVariableWarnings, +} from './validation'; +import { mcpVariableValuesParam, resolveDashboardVariables } from './variables'; export function registerQueryTile({ context, @@ -44,9 +48,10 @@ export function registerQueryTile({ .string() .optional() .describe('End of the query window as ISO 8601. Default: now.'), + variableValues: mcpVariableValuesParam.optional(), }), }, - async ({ dashboardId, tileId, startTime, endTime }) => { + async ({ dashboardId, tileId, startTime, endTime, variableValues }) => { const timeRange = parseTimeRange(startTime, endTime); if ('error' in timeRange) { return mcpUserError(timeRange.error); @@ -69,25 +74,37 @@ export function registerQueryTile({ ); } + const resolvedVariables = resolveDashboardVariables( + externalDashboard.filters, + variableValues, + ); + if ('error' in resolvedVariables) { + return mcpUserError(resolvedVariables.error); + } + const result = await runConfigTile( teamId.toString(), tile, startDate, endDate, + { variables: resolvedVariables.variables }, ); - // Surface non-blocking missing macro warnings alongside + // Surface non-blocking missing macro and variable warnings alongside // the successful result so the agent can spot a tile that runs but // ignores dashboard controls. - const macroWarnings = getRawSqlTileMacroWarnings([tile]); + const warnings = [ + ...getRawSqlTileMacroWarnings([tile]), + ...getTileVariableWarnings([tile], externalDashboard.filters), + ]; if ( - macroWarnings.length > 0 && + warnings.length > 0 && !('isError' in result && result.isError) && result.content?.[0]?.type === 'text' ) { try { const parsed = JSON.parse(result.content[0].text); - parsed.warnings = macroWarnings; + parsed.warnings = warnings; result.content[0].text = JSON.stringify(parsed, null, 2); } catch { // leave result unmodified diff --git a/packages/api/src/mcp/tools/dashboards/queryTiles.ts b/packages/api/src/mcp/tools/dashboards/queryTiles.ts index 9eafdd8f1d..1923a7c67a 100644 --- a/packages/api/src/mcp/tools/dashboards/queryTiles.ts +++ b/packages/api/src/mcp/tools/dashboards/queryTiles.ts @@ -12,7 +12,11 @@ import { import type { ExternalDashboardTileWithId } from '@/utils/zod'; import { objectIdSchema } from '@/utils/zod'; -import { getRawSqlTileMacroWarnings } from './validation'; +import { + getRawSqlTileMacroWarnings, + getTileVariableWarnings, +} from './validation'; +import { mcpVariableValuesParam, resolveDashboardVariables } from './variables'; /** * How many tiles to query against ClickHouse at once. Kept lowish so a batch @@ -214,9 +218,10 @@ export function registerQueryTiles({ .string() .optional() .describe('End of the query window as ISO 8601. Default: now.'), + variableValues: mcpVariableValuesParam.optional(), }), }, - async ({ dashboardId, tileIds, startTime, endTime }) => { + async ({ dashboardId, tileIds, startTime, endTime, variableValues }) => { const timeRange = parseTimeRange(startTime, endTime); if ('error' in timeRange) { return mcpUserError(timeRange.error); @@ -234,6 +239,15 @@ export function registerQueryTiles({ const externalDashboard = convertToExternalDashboard(dashboard); const allTiles = externalDashboard.tiles; + const resolved = resolveDashboardVariables( + externalDashboard.filters, + variableValues, + ); + if ('error' in resolved) { + return mcpUserError(resolved.error); + } + const { variables } = resolved; + // Resolve the target tiles. An explicit `tileIds` array is honored // whenever the field is present — including an empty array, which // deliberately selects nothing rather than falling through to "run @@ -303,6 +317,7 @@ export function registerQueryTiles({ signal => runConfigTile(teamId.toString(), tile, startDate, endDate, { abortSignal: signal, + variables, }), deadlineAt, ); @@ -319,7 +334,10 @@ export function registerQueryTiles({ result.content?.[0]?.type === 'text' ? result.content[0].text : ''; - const warnings = getRawSqlTileMacroWarnings([tile]); + const warnings = [ + ...getRawSqlTileMacroWarnings([tile]), + ...getTileVariableWarnings([tile], externalDashboard.filters), + ]; return { ...base, status: 'ok', diff --git a/packages/api/src/mcp/tools/dashboards/saveDashboard.ts b/packages/api/src/mcp/tools/dashboards/saveDashboard.ts index 2927319002..3425b27ecb 100644 --- a/packages/api/src/mcp/tools/dashboards/saveDashboard.ts +++ b/packages/api/src/mcp/tools/dashboards/saveDashboard.ts @@ -5,7 +5,7 @@ import { z } from 'zod'; import * as config from '@/config'; import type { ToolRegistrar } from '@/mcp/tools/types'; -import { mcpUserError } from '@/mcp/utils/errors'; +import { formatZodIssues, mcpUserError } from '@/mcp/utils/errors'; import Dashboard, { IDashboard } from '@/models/dashboard'; import { cleanupDashboardAlerts, @@ -31,9 +31,12 @@ import { import { mcpContainersParam, mcpFiltersParam, mcpTilesParam } from './schemas'; import { + getFilterVariableWarnings, getRawSqlMissingSourceError, getRawSqlTileMacroWarnings, + getTileVariableWarnings, } from './validation'; +import { withResolvedFilterVariableNames } from './variables'; export function registerSaveDashboard({ context, @@ -168,9 +171,7 @@ async function createDashboard({ filters: stripFilterIds(inputFilters), }); if (!parsed.success) { - return mcpUserError( - `Validation error: ${JSON.stringify(parsed.error.errors)}`, - ); + return mcpUserError(`Validation error:\n${formatZodIssues(parsed.error)}`); } const { tiles, filters, containers: parsedContainers } = parsed.data; @@ -191,7 +192,11 @@ async function createDashboard({ return mcpUserError(validationError); } - const macroWarnings = getRawSqlTileMacroWarnings(tilesWithId); + const macroWarnings = [ + ...getRawSqlTileMacroWarnings(tilesWithId), + ...getTileVariableWarnings(tilesWithId, filters), + ...getFilterVariableWarnings(filters), + ]; const internalTiles = convertExternalTilesToInternal(tilesWithId); const filtersWithIds = convertExternalFiltersToInternal(filters ?? []); @@ -212,13 +217,17 @@ async function createDashboard({ ...(parsedContainers !== undefined ? { containers: parsedContainers } : {}), }).save(); + const externalDashboard = convertToExternalDashboard(newDashboard); return { content: [ { type: 'text' as const, text: JSON.stringify( { - ...convertToExternalDashboard(newDashboard), + ...externalDashboard, + filters: withResolvedFilterVariableNames( + externalDashboard.filters ?? [], + ), ...(frontendUrl ? { url: `${frontendUrl}/dashboards/${newDashboard._id}` } : {}), @@ -264,9 +273,7 @@ async function updateDashboard({ filters: assignFilterIds(inputFilters), }); if (!parsed.success) { - return mcpUserError( - `Validation error: ${JSON.stringify(parsed.error.errors)}`, - ); + return mcpUserError(`Validation error:\n${formatZodIssues(parsed.error)}`); } const { tiles, filters, containers: parsedContainers } = parsed.data; @@ -299,7 +306,17 @@ async function updateDashboard({ return mcpUserError(validationError); } - const macroWarnings = getRawSqlTileMacroWarnings(tilesWithId); + // An omitted `filters` preserves the persisted set rather than clearing it + // (see the `$set` below), so the variable checks have to run against those. + const effectiveFilters = filters ?? existingDashboard.filters; + const macroWarnings = [ + ...getRawSqlTileMacroWarnings(tilesWithId), + ...getTileVariableWarnings(tilesWithId, effectiveFilters), + // Only report on filters this call actually wrote: re-warning about a + // persisted dependent filter the agent did not touch is noise it cannot + // act on from a tiles-only update. + ...getFilterVariableWarnings(filters), + ]; const existingTileIds = new Set( (existingDashboard.tiles ?? []).map((t: { id: string }) => t.id), @@ -363,13 +380,17 @@ async function updateDashboard({ existingTileIds, }); + const externalDashboard = convertToExternalDashboard(updatedDashboard); return { content: [ { type: 'text' as const, text: JSON.stringify( { - ...convertToExternalDashboard(updatedDashboard), + ...externalDashboard, + filters: withResolvedFilterVariableNames( + externalDashboard.filters ?? [], + ), ...(frontendUrl ? { url: `${frontendUrl}/dashboards/${updatedDashboard._id}` } : {}), diff --git a/packages/api/src/mcp/tools/dashboards/schemas.ts b/packages/api/src/mcp/tools/dashboards/schemas.ts index b6d3921b59..fcd003c6c1 100644 --- a/packages/api/src/mcp/tools/dashboards/schemas.ts +++ b/packages/api/src/mcp/tools/dashboards/schemas.ts @@ -8,6 +8,8 @@ import { ChartPaletteTokenSchema, DASHBOARD_CONTAINER_ID_MAX, DASHBOARD_MAX_CONTAINERS, + DASHBOARD_VARIABLE_NAME_MAX_LENGTH, + DASHBOARD_VARIABLE_NAME_PATTERN_ANCHORED, DashboardContainerSchema, DashboardFilterType, MetricsDataType, @@ -74,6 +76,29 @@ const rejectedTileWhereFields = { whereLanguage: rejectedTileWhereField, }; +// A variable reference expands in either language; only the two variable +// MACROS are SQL-only. Every input that can carry one repeats the note rather +// than relying on the agent having fetched the query_guide prompt. +const variableMacroDescription = + 'DASHBOARD VARIABLES: a reference to a variable-enabled dashboard filter ' + + 'expands here in either language ($variableName renders as quoted SQL ' + + 'values, or as a Lucene value list in a Lucene input). With whereLanguage ' + + '"sql" this also accepts $__filter(, $) and ' + + '$__conditionalAll(, $), which are preferred ' + + 'because they expand to 1=1 while nothing is selected, where a bare SQL ' + + 'reference renders as NULL. The two macros have no meaning in a Lucene ' + + 'input and are matched as literal text; write ServiceName:$variableName ' + + 'there. A Lucene reference needs no macro: with nothing selected it renders ' + + 'as ("") and the translator drops it to a match-all.'; + +// Same note for the always-SQL expressions (HAVING), which have no +// whereLanguage to set. +const sqlOnlyVariableMacroDescription = + 'DASHBOARD VARIABLES: accepts $__filter(, $) and ' + + '$__conditionalAll(, $), which expand to 1=1 while ' + + 'the variable has no selection, as well as a bare $variableName reference ' + + '(which renders as NULL when nothing is selected).'; + const timeChartSeriesLimitDescription = 'Maximum number of series to fetch (the "Series Limit" display setting). ' + 'Keeps the top-N groups by aggregated value over the queried range and ' + @@ -198,7 +223,8 @@ const mcpTileSelectItemSchema = z 'whole tile to a subset of rows, put the same `where` on every select ' + 'item; use different filters per item for cohort comparisons (errors ' + 'vs total). There is no tile-level `where` for these tile types. ' + - 'Lucene syntax by default. Example: "level:error"', + 'Lucene syntax by default. Example: "level:error"\n\n' + + variableMacroDescription, ), whereLanguage: SearchConditionTrimmedLanguageSchema.optional().default('lucene'), @@ -676,7 +702,8 @@ const mcpTableTileSchema = mcpTileLayoutSchema.extend({ 'Post-aggregation SQL HAVING expression. Example: "Count > 100" to drop ' + 'groups with few rows, or "StatusMessage != \'\'" to drop empty-message rows ' + 'from a groupBy: "StatusMessage" table. Mirrors the same field on the REST ' + - 'table chart config in `externalDashboardTableChartConfigSchema`.', + 'table chart config in `externalDashboardTableChartConfigSchema`.\n\n' + + sqlOnlyVariableMacroDescription, ), orderBy: z .string() @@ -874,7 +901,8 @@ const mcpHeatmapTileSchema = mcpTileLayoutSchema.extend({ .optional() .default('') .describe( - 'Row-level filter applied before bucketing. Example: "level:error"', + 'Row-level filter applied before bucketing. Example: "level:error"\n\n' + + variableMacroDescription, ), whereLanguage: SearchConditionTrimmedLanguageSchema.optional().default('lucene'), @@ -895,7 +923,10 @@ const mcpSearchTileSchema = mcpTileLayoutSchema.extend({ .string() .optional() .default('') - .describe('Filter in Lucene syntax. Example: "level:error"'), + .describe( + 'Filter in Lucene syntax. Example: "level:error"\n\n' + + variableMacroDescription, + ), whereLanguage: SearchConditionTrimmedLanguageSchema.optional().default('lucene'), select: z @@ -918,7 +949,10 @@ const mcpEventPatternsTileSchema = mcpTileLayoutSchema.extend({ .string() .optional() .default('') - .describe('Filter in Lucene syntax. Example: "level:error"'), + .describe( + 'Filter in Lucene syntax. Example: "level:error"\n\n' + + variableMacroDescription, + ), whereLanguage: SearchConditionTrimmedLanguageSchema.optional().default('lucene'), select: z @@ -1153,7 +1187,17 @@ const mcpDashboardFilterSchema = z .string() .optional() .describe( - 'Optional WHERE clause scoping the dropdown values (e.g. "level:error" in Lucene).', + 'Optional WHERE clause scoping the dropdown values (e.g. "level:error" in Lucene). ' + + "DEPENDENT FILTERS: this clause may reference ANOTHER filter's variable, which " + + "chains one dropdown off another (pick a service, and this filter's dropdown only " + + "offers that service's endpoints). The upstream filter must set isVariableEnabled: true; " + + 'this one does not have to. With whereLanguage "sql", prefer ' + + '$__filter(, $) so the dropdown lists every value until the ' + + 'upstream filter is set; a bare $variableName leaves this dropdown empty until then. ' + + 'With whereLanguage "lucene" a bare reference is already safe: it renders as ("") ' + + 'with nothing selected, which matches all. ' + + "Reference a SIBLING filter's variable only: naming this filter's own variable " + + 'collapses its dropdown to the values already picked.', ), whereLanguage: SearchConditionTrimmedLanguageSchema.describe( 'Filter language for `where` ("lucene" or "sql"). Optional, but set it explicitly.', @@ -1168,12 +1212,52 @@ const mcpDashboardFilterSchema = z 'A non-empty array restricts the filter to only tiles whose source ID is in the list; ' + 'tiles on other sources are not affected by the dropdown value at all. ' + 'Useful on mixed-source dashboards where a column (e.g. SpanName) only exists on ' + - 'a subset of sources.', + 'a subset of sources. ' + + 'Scopes the broadcast condition only, so a non-empty array is rejected when ' + + 'isBroadcastEnabled is false.', + ), + isBroadcastEnabled: z + .boolean() + .optional() + .describe( + 'Whether the selected value is applied as a filter condition on every builder ' + + 'tile this filter applies to (see appliesToSourceIds), and on every raw SQL ' + + 'tile using the $__filters macro. Omitting the field means ENABLED. ' + + 'Set false to build a variable-only filter that tiles opt into by name.', + ), + isVariableEnabled: z + .boolean() + .optional() + .describe( + 'Whether the selected value is exposed to tile queries as a dashboard variable ' + + 'named by variableName. Omitting the field means DISABLED. ' + + 'Tiles reference it as $variableName, or (preferred) via ' + + '$__filter(, $) and ' + + '$__conditionalAll(, $) in any SQL condition ' + + "(a select item's `where`, a tile-level `where`, `having`, or a raw SQL " + + "tile's sqlTemplate). Another filter's `where` can reference it too, which " + + "chains that filter's dropdown off this one.", + ), + variableName: z + .string() + .max(DASHBOARD_VARIABLE_NAME_MAX_LENGTH) + .regex(DASHBOARD_VARIABLE_NAME_PATTERN_ANCHORED) + .optional() + .describe( + "Token tiles reference this filter's selected value by, eg. $variableName. " + + 'Must start with a letter and may contain only letters, numbers, and ' + + `underscores, up to ${DASHBOARD_VARIABLE_NAME_MAX_LENGTH} characters. ` + + 'A default is determined based on the display name. ' + + "Must be unique across the dashboard's variable-enabled filters. " + + 'Rejected when isVariableEnabled is not true.', ), }) .describe( 'A dashboard-level filter the user can adjust in the dashboard filter bar. ' + 'Each filter binds a label/name to a column expression on a source. ' + + 'Every filter either broadcasts its selected value to matching tiles as a ' + + 'condition (isBroadcastEnabled, the default), exposes it to tile queries as a ' + + 'variable (isVariableEnabled + variableName), or both. Typically pick only ONE.' + "Filters are also the contract for row-click navigation: a table tile's " + 'onClick.filters[i].expression must match a filter declared here for the value to land.', ); @@ -1186,11 +1270,17 @@ export const mcpFiltersParam = z 'If another tile\'s onClick targets THIS dashboard with `filters: [{ expression: "X", ... }]`, ' + 'this array MUST declare a filter whose `expression` is "X". Otherwise the value is ' + 'dropped on arrival and the destination opens unfiltered.\n\n' + - 'By default a filter applies to every tile on the dashboard. On mixed-source dashboards, ' + - 'use the optional `appliesToSourceIds` field to restrict a filter to only the tiles whose ' + - 'source carries the referenced column — leave `appliesToSourceIds` omitted to keep the ' + - 'broadcast-to-all-tiles default.\n\n' + - 'Example (broadcast to every tile):\n' + + 'Each filter runs in one of two modes (or both). BROADCAST (the default) applies the ' + + 'selected value as a condition on matching tiles with no per-tile wiring. VARIABLE ' + + '(isVariableEnabled) exposes the value to tile queries as $variableName, which the tile ' + + 'must reference explicitly. Prefer broadcast; reach for a variable in advanced cases. ' + + 'Enabling both on one filter applies the value twice.\n\n' + + 'By default, a broadcast filter applies to every tile on the dashboard. On mixed-source ' + + 'dashboards, use variable mode or the optional `appliesToSourceIds` field to restrict a filter to only the ' + + 'tiles whose source carries the referenced column — leave `appliesToSourceIds` omitted to ' + + 'keep the broadcast-to-all-tiles default. `appliesToSourceIds` does NOT scope variable ' + + 'references; any tile may reference any variable.\n\n' + + 'Example (broadcast to every tile, the common case):\n' + '[\n' + ' { "type": "QUERY_EXPRESSION", "name": "Service", "expression": "ServiceName",\n' + ' "sourceId": "", "whereLanguage": "sql" }\n' + @@ -1200,7 +1290,28 @@ export const mcpFiltersParam = z ' { "type": "QUERY_EXPRESSION", "name": "Service", "expression": "SpanName",\n' + ' "sourceId": "", "whereLanguage": "sql",\n' + ' "appliesToSourceIds": [""] }\n' + - ']', + ']\n\n' + + 'Example (variable only, referenced by tiles as $service):\n' + + '[\n' + + ' { "type": "QUERY_EXPRESSION", "name": "Service", "expression": "ServiceName",\n' + + ' "sourceId": "", "whereLanguage": "sql",\n' + + ' "isBroadcastEnabled": false, "isVariableEnabled": true, "variableName": "service" }\n' + + ']\n' + + 'A tile then uses it as $__filter(ServiceName, $service) in a SQL where clause, which ' + + 'expands to 1=1 while nothing is selected.\n\n' + + "Example (dependent filter: the Endpoint dropdown only lists the selected service's " + + 'endpoints):\n' + + '[\n' + + ' { "type": "QUERY_EXPRESSION", "name": "Service", "expression": "ServiceName",\n' + + ' "sourceId": "", "whereLanguage": "sql",\n' + + ' "isVariableEnabled": true, "variableName": "service" },\n' + + ' { "type": "QUERY_EXPRESSION", "name": "Endpoint", "expression": "SpanName",\n' + + ' "sourceId": "", "whereLanguage": "sql",\n' + + ' "where": "$__filter(ServiceName, $service)" }\n' + + ']\n' + + "A filter's `where` scopes only the values ITS dropdown offers; it is not applied to " + + 'any tile. That is why Service keeps broadcasting here: its variable is read by the ' + + 'Endpoint dropdown, not by a tile, so the value is never applied twice.', ); export const mcpPatchDashboardSchema = z.object({ diff --git a/packages/api/src/mcp/tools/dashboards/validation.ts b/packages/api/src/mcp/tools/dashboards/validation.ts index 5e14d601d6..6d14bd7f04 100644 --- a/packages/api/src/mcp/tools/dashboards/validation.ts +++ b/packages/api/src/mcp/tools/dashboards/validation.ts @@ -1,11 +1,33 @@ +import type { FilterForVariableDeclaration } from '@hyperdx/common-utils/dist/filters'; +import { + getDashboardVariableDeclarations, + getFilterVariableName, + isFilterVariableEnabled, +} from '@hyperdx/common-utils/dist/filters'; +import { + isBuilderSavedChartConfig, + isRawSqlSavedChartConfig, +} from '@hyperdx/common-utils/dist/guards'; import { getSourceDependentMacrosUsed, hasMacro, INTERVAL_MACROS, TIME_RANGE_MACROS, } from '@hyperdx/common-utils/dist/macros'; +import { + DashboardFilter, + DisplayType, + SavedChartConfig, + SearchConditionLanguage, +} from '@hyperdx/common-utils/dist/types'; +import { + getVariableReferences, + mapBuilderVariableTemplates, + validateVariableReferencesInTemplate, +} from '@hyperdx/common-utils/dist/variables'; import { + convertToInternalTileConfig, isConfigTile, isRawSqlExternalTileConfig, } from '@/routers/external-api/v2/utils/dashboards'; @@ -128,3 +150,151 @@ export function getRawSqlTileMacroWarnings( }); return hints; } + +/** Minimal projection needed to check a filter's dropdown-values query. */ +export type FilterForVariableValidation = FilterForVariableDeclaration & + Partial>; + +/** + * Non-blocking checks on the dashboard variables references in a filter's `where`. + * + * A dependent filter is one whose `where` names another filter's variable, so + * that picking a service narrows the endpoint dropdown to that service's + * endpoints. + * + * Three warnings can arise from a dependent filter's `where`: + * - referencing a filter that collects a value but never publishes it + * (`isVariableEnabled` unset), which is the usual cause of an "unknown + * variable" here; + * - referencing the filter's OWN variable, which narrows its dropdown to the + * values already selected so the rest disappear from a multi-select; + * - a `where` that is not `sql`, where the macros are literal text. + */ +export function getFilterVariableWarnings( + filters: FilterForVariableValidation[] | undefined, +): string[] { + if (!filters?.length) return []; + + const variables = getDashboardVariableDeclarations(filters).map( + declaration => ({ ...declaration, values: [] }), + ); + const declaredNames = new Set(variables.map(variable => variable.name)); + + // A map from variable name to to the name of the filter that collects it but is not variable-enabled. + const filtersByUnpublishedVarName = new Map(); + for (const filter of filters) { + if (isFilterVariableEnabled(filter)) continue; + for (const candidate of [ + getFilterVariableName(filter), + filter.variableName?.trim(), + ]) { + if (!candidate) continue; + const key = candidate.toLowerCase(); + if (!filtersByUnpublishedVarName.has(key)) + filtersByUnpublishedVarName.set(key, filter.name); + } + } + + const warnings: string[] = []; + filters.forEach((filter, index) => { + const where = filter.where?.trim(); + if (!where) return; + + const label = filter.name?.trim() || `filter #${index + 1}`; + const language = filter.whereLanguage ?? 'sql'; + + const result = validateVariableReferencesInTemplate(where, variables, { + subject: 'The dropdown values query', + language, + }); + const issues = [...result.errors, ...result.warnings]; + + const referencedNames = new Set( + getVariableReferences(where).map(reference => reference.name), + ); + + const ownName = isFilterVariableEnabled(filter) + ? getFilterVariableName(filter) + : undefined; + if (ownName && referencedNames.has(ownName)) { + issues.push( + `The dropdown values query references this filter's own variable $${ownName}, ` + + 'which narrows the dropdown to the values already selected. Reference ' + + "another filter's variable instead.", + ); + } + + for (const name of referencedNames) { + if (declaredNames.has(name)) continue; + const owner = filtersByUnpublishedVarName.get(name.toLowerCase()); + if (owner == null) continue; + issues.push( + `$${name} is not published as a variable. The filter named "${owner}" collects ` + + `that value but does not expose it: set isVariableEnabled: true (and ` + + `variableName: "${name}") on it so this dropdown can depend on it.`, + ); + } + + warnings.push(...issues.map(issue => `Filter "${label}": ${issue}`)); + }); + + return warnings; +} + +/** + * Non-blocking checks on the dashboard variables a tile's expressions + * reference, run against the variables the dashboard's filters actually + * declare. + */ +export function getTileVariableWarnings( + tiles: ExternalDashboardTileWithId[], + filters: FilterForVariableDeclaration[] | undefined, +): string[] { + const variables = getDashboardVariableDeclarations(filters).map( + declaration => ({ ...declaration, values: [] }), + ); + + const warnings: string[] = []; + tiles.forEach((tile, index) => { + if (!isConfigTile(tile)) return; + const label = tile.name?.trim() || `tile #${index + 1}`; + + // Convert to the internal config format so that variable validators can run + let config: SavedChartConfig; + try { + config = convertToInternalTileConfig(tile).config; + } catch { + // A tile shape the converter refuses is already reported by the schema + // validation that runs alongside this; nothing to add here. + return; + } + + const issues: string[] = []; + const validate = (template: string, language: SearchConditionLanguage) => { + const result = validateVariableReferencesInTemplate(template, variables, { + subject: language === 'lucene' ? 'The Lucene filter' : 'SQL', + language, + }); + issues.push(...result.errors, ...result.warnings); + }; + + if (isRawSqlSavedChartConfig(config)) { + validate(config.sqlTemplate, 'sql'); + } else if ( + isBuilderSavedChartConfig(config) && + config.displayType !== DisplayType.Markdown + ) { + mapBuilderVariableTemplates(config, (template, language) => { + validate(template, language); + return template; + }); + } + + // Not every message from the validators names its subject (the editor + // shows them attached to the input they came from), so identify the tile + // here rather than relying on the `subject` option. + warnings.push(...issues.map(issue => `Tile "${label}": ${issue}`)); + }); + + return warnings; +} diff --git a/packages/api/src/mcp/tools/dashboards/variables.ts b/packages/api/src/mcp/tools/dashboards/variables.ts new file mode 100644 index 0000000000..de90233186 --- /dev/null +++ b/packages/api/src/mcp/tools/dashboards/variables.ts @@ -0,0 +1,98 @@ +import { + getDashboardVariableDeclarations, + getFilterVariableName, + isFilterVariableEnabled, +} from '@hyperdx/common-utils/dist/filters'; +import type { ChartVariable } from '@hyperdx/common-utils/dist/types'; +import { z } from 'zod'; + +import type { + ExternalDashboardFilter, + ExternalDashboardFilterWithId, +} from '@/utils/zod'; + +/** Optional per-variable selection for a tile run. */ +export const mcpVariableValuesParam = z + .array( + z.object({ + name: z + .string() + .describe( + "Variable name, as declared by a filter's variableName. Get the " + + 'list from clickstack_get_dashboard.', + ), + values: z + .array(z.string()) + .describe( + 'Values to treat as selected. An empty array means nothing is selected for this variable.', + ), + }), + ) + .describe( + 'Optional simulated dashboard variable selection for this run. Every ' + + 'variable the dashboard declares defaults to EMPTY, which is the state a ' + + 'freshly-opened dashboard is in. Supply values here to check that a tile narrows ' + + 'as expected once the user picks something. Variables not listed stay empty.', + ); + +export type McpVariableValues = z.infer; + +/** + * Build the variable context a dashboard tile runs with. + * + * The declarations come from the dashboard's variable-enabled filters, with + * every selection empty by default. + * + * An unknown name in `variableValues` is an error rather than a silent no-op: + * it is the agent's own input, and a typo would otherwise run the query with + * the variable still empty and look like a data problem. + */ +export function resolveDashboardVariables( + filters: + | (ExternalDashboardFilter | ExternalDashboardFilterWithId)[] + | undefined, + variableValues: McpVariableValues | undefined, +): { variables: ChartVariable[] } | { error: string } { + // Empty selection for every variable by default + const variables: ChartVariable[] = getDashboardVariableDeclarations( + filters, + ).map(declaration => ({ ...declaration, values: [] })); + + if (!variableValues?.length) return { variables }; + + const byName = new Map(variables.map(variable => [variable.name, variable])); + const unknownVariableValues = variableValues + .map(({ name }) => name) + .filter(name => !byName.has(name)); + if (unknownVariableValues.length > 0) { + const available = + variables.length > 0 + ? variables.map(variable => variable.name).join(', ') + : '(none)'; + return { + error: + `This dashboard declares no variable(s) named ${unknownVariableValues.join(', ')}. ` + + `Available variables: ${available}. A variable exists only when a ` + + 'dashboard filter sets isVariableEnabled; check the filters returned ' + + 'by clickstack_get_dashboard.', + }; + } + + // Override the default empty selection with the given variableValues + for (const { name, values } of variableValues) { + byName.get(name)!.values = values; + } + + return { variables }; +} + +/** Set a derived variableName on filters without explicit variable names, so the agent doesn't guess. */ +export function withResolvedFilterVariableNames( + filters: ExternalDashboardFilterWithId[], +): ExternalDashboardFilterWithId[] { + return filters.map(filter => + isFilterVariableEnabled(filter) + ? { ...filter, variableName: getFilterVariableName(filter) } + : filter, + ); +} diff --git a/packages/api/src/mcp/tools/query/helpers.ts b/packages/api/src/mcp/tools/query/helpers.ts index 97b0156da9..8743c88fb0 100644 --- a/packages/api/src/mcp/tools/query/helpers.ts +++ b/packages/api/src/mcp/tools/query/helpers.ts @@ -6,8 +6,10 @@ import { splitAndTrimWithBracket, } from '@hyperdx/common-utils/dist/core/utils'; import { isBuilderSavedChartConfig } from '@hyperdx/common-utils/dist/guards'; +import { UnknownVariableError } from '@hyperdx/common-utils/dist/macroErrors'; import type { ChartConfigWithDateRange, + ChartVariable, MetricTable, } from '@hyperdx/common-utils/dist/types'; import { @@ -400,6 +402,8 @@ export async function runConfigTile( maxResults?: number; granularity?: string; abortSignal?: AbortSignal; + /** Dashboard variables and their selected values. Omitted when not in a dashboard context. */ + variables?: ChartVariable[]; }, ) { if (!isConfigTile(tile)) { @@ -442,6 +446,7 @@ export async function runConfigTile( whereLanguage: (builderConfig.whereLanguage as 'lucene' | 'sql') ?? 'lucene', bodyExpression: selectStr || undefined, + variables: options?.variables, // Forward the batch deadline's abort signal so an event-patterns // tile that overruns is cancelled server-side alongside the generic // chart-config path, rather than escaping cancellation and letting @@ -570,6 +575,7 @@ export async function runConfigTile( implicitColumnExpression: implicitColumn, useTextIndexForImplicitColumn, dateRange: [startDate, endDate] as [Date, Date], + variables: options?.variables, } satisfies ChartConfigWithDateRange; // Apply seriesLimit as LIMIT to categorical charts (pie/bar) @@ -647,6 +653,7 @@ export async function runConfigTile( ...savedConfig, ...sourceFields, dateRange: [startDate, endDate] as [Date, Date], + variables: options?.variables, } satisfies ChartConfigWithDateRange; const metadata = getMetadata(clickhouseClient); @@ -832,7 +839,7 @@ export function clickHouseErrorResult( (e.cause instanceof Error ? e.cause.message : '') || String(e) : String(e); - const hint = errorHint(raw); + const hint = errorHint(raw, e); const base = hint ? `${raw}\n\nHINT: ${hint}` : raw; const text = `${prefix ? `${prefix}: ` : ''}${base}${suffix ? ` ${suffix}` : ''}`; @@ -842,8 +849,34 @@ export function clickHouseErrorResult( return isServerError(e) ? mcpServerError(text) : mcpUserError(text); } +/** Walk the cause chain looking for an error of the given class. */ +function findCause( + error: unknown, + is: (e: unknown) => e is T, + depth = 5, +): T | undefined { + let current = error; + for (let i = 0; i <= depth; i++) { + if (is(current)) return current; + if (!(current instanceof Error)) return undefined; + current = current.cause; + } + return undefined; +} + /** @internal Exported for testing only. */ -export function errorHint(msg: string): string | null { +export function errorHint(msg: string, error?: unknown): string | null { + const unknownVariableError = findCause( + error, + (e): e is UnknownVariableError => e instanceof UnknownVariableError, + ); + if (unknownVariableError) { + return ( + "A variable exists only when one of the dashboard's filters sets " + + 'isVariableEnabled. Call clickstack_get_dashboard to see the declared ' + + 'filters and the names they can be referenced by.' + ); + } if ( /Cannot (convert|parse) string .* (to|as) (type )?DateTime64/i.test(msg) ) { diff --git a/packages/api/src/mcp/tools/query/runEventPatterns.ts b/packages/api/src/mcp/tools/query/runEventPatterns.ts index 1da06eb6d0..ae4cec39b8 100644 --- a/packages/api/src/mcp/tools/query/runEventPatterns.ts +++ b/packages/api/src/mcp/tools/query/runEventPatterns.ts @@ -4,7 +4,10 @@ import { splitAndTrimWithBracket, } from '@hyperdx/common-utils/dist/core/utils'; import { minePatterns } from '@hyperdx/common-utils/dist/drain'; -import type { ChartConfigWithDateRange } from '@hyperdx/common-utils/dist/types'; +import type { + ChartConfigWithDateRange, + ChartVariable, +} from '@hyperdx/common-utils/dist/types'; import { DisplayType } from '@hyperdx/common-utils/dist/types'; import { ClickhouseClient } from '@/clickhouse'; @@ -44,6 +47,8 @@ export async function mineWindowPatterns( sampleSize?: number; trendBuckets?: number; abortSignal?: AbortSignal; + /** Dashboard variables and their selected values. Omitted outside a dashboard context. */ + variables?: ChartVariable[]; }, ): Promise< | { error: McpErrorResult } @@ -126,6 +131,9 @@ export async function mineWindowPatterns( ? source.useTextIndexForImplicitColumn : undefined; + // `variables` rides on both configs so the renderer expands variable + // references and macros in `where` the same way it does for every other + // dashboard tile. Undefined (the standalone-tool case) is a no-op there. const sampleConfig = { displayType: DisplayType.Search, source: source._id.toString(), @@ -143,6 +151,7 @@ export async function mineWindowPatterns( orderBy: [{ ordering: 'DESC' as const, valueExpression: 'rand()' }], limit: { limit: sampleSize, offset: 0 }, dateRange: [startDate, endDate] as [Date, Date], + variables: options?.variables, } satisfies ChartConfigWithDateRange; const countConfig = { @@ -161,6 +170,7 @@ export async function mineWindowPatterns( useTextIndexForImplicitColumn, limit: { limit: 1, offset: 0 }, dateRange: [startDate, endDate] as [Date, Date], + variables: options?.variables, } satisfies ChartConfigWithDateRange; // The client is constructed fresh per call (not pooled) and is only used for @@ -279,6 +289,8 @@ export async function runEventPatterns( topN?: number; trendBuckets?: number; abortSignal?: AbortSignal; + /** Dashboard variables and their selected values. Omitted outside a dashboard context. */ + variables?: ChartVariable[]; }, ) { const topN = options?.topN ?? 20; @@ -300,6 +312,7 @@ export async function runEventPatterns( sampleSize: options?.sampleSize, trendBuckets, abortSignal: options?.abortSignal, + variables: options?.variables, }); if ('error' in mined) return mined.error; diff --git a/packages/api/src/mcp/tools/query/sql.ts b/packages/api/src/mcp/tools/query/sql.ts index 94eb52f79c..8c36fd4c72 100644 --- a/packages/api/src/mcp/tools/query/sql.ts +++ b/packages/api/src/mcp/tools/query/sql.ts @@ -1,3 +1,4 @@ +import { hasVariableMacro } from '@hyperdx/common-utils/dist/variables'; import { z } from 'zod'; import type { ToolRegistrar } from '@/mcp/tools/types'; @@ -86,6 +87,19 @@ export function registerSql({ context, registerTool }: ToolRegistrar) { } const { startDate, endDate } = timeRange; + // $__filter / $__conditionalAll read the variables a dashboard's filters + // declare, and this tool has no dashboard behind it. + if (hasVariableMacro(input.sql)) { + return mcpUserError( + 'The $__filter and $__conditionalAll macros only resolve for a tile ' + + 'on a dashboard, because they read the dashboard variables its ' + + 'filters declare. This tool has no dashboard, so they would be ' + + 'sent to ClickHouse as literal text. Inline the condition here, ' + + 'or save the query as a dashboard tile and validate it with ' + + 'clickstack_query_tile.', + ); + } + const tile = buildTile('MCP SQL', 24, 6, { configType: 'sql' as const, displayType: 'table' as const, diff --git a/packages/api/src/mcp/utils/errors.ts b/packages/api/src/mcp/utils/errors.ts index 31c85cf996..89c9d8b333 100644 --- a/packages/api/src/mcp/utils/errors.ts +++ b/packages/api/src/mcp/utils/errors.ts @@ -1,4 +1,5 @@ import mongoose from 'mongoose'; +import { z } from 'zod'; /** * Error category for MCP tool failures. @@ -73,3 +74,21 @@ export function validateObjectId( } return null; } + +/** Render a Zod error as one `path: message` line per issue. */ +export function formatZodIssues(error: z.ZodError): string { + return error.errors + .map(issue => { + const path = issue.path + .map((segment, index) => + typeof segment === 'number' + ? `[${segment}]` + : index === 0 + ? segment + : `.${segment}`, + ) + .join(''); + return path ? `${path}: ${issue.message}` : issue.message; + }) + .join('\n'); +} diff --git a/packages/common-utils/src/__tests__/filters.test.ts b/packages/common-utils/src/__tests__/filters.test.ts index 4482afcc0a..3e30f8e6af 100644 --- a/packages/common-utils/src/__tests__/filters.test.ts +++ b/packages/common-utils/src/__tests__/filters.test.ts @@ -1593,6 +1593,22 @@ describe('filters', () => { expect(getDashboardVariableDeclarations([])).toEqual([]); }); + it('accepts the external filter shape, which has sourceId not source', () => { + // The external API and MCP hold filters with `sourceId`; only the name, + // expression and the two variable fields decide what a filter declares, + // so the signature is structural rather than tied to DashboardFilter. + expect( + getDashboardVariableDeclarations([ + { + name: 'Service', + expression: 'ServiceName', + isVariableEnabled: true, + variableName: 'service', + }, + ]), + ).toEqual([{ name: 'service', expression: 'ServiceName' }]); + }); + it('skips filters that do not expose a variable', () => { expect( getDashboardVariableDeclarations([ diff --git a/packages/common-utils/src/__tests__/variables.test.ts b/packages/common-utils/src/__tests__/variables.test.ts index 1a465f5b78..9b7618a3b1 100644 --- a/packages/common-utils/src/__tests__/variables.test.ts +++ b/packages/common-utils/src/__tests__/variables.test.ts @@ -1,4 +1,8 @@ -import { MalformedMacroArgsError } from '@/macroErrors'; +import { + MacroExpansionError, + MalformedMacroArgsError, + UnknownVariableError, +} from '@/macroErrors'; import type { BuilderChartConfig, ChartVariable } from '@/types'; import { filterReferencedVariables, @@ -309,6 +313,39 @@ describe('substituteVariables', () => { ).toThrow("Macro '$__filter' references unknown variable 'nope'"); }); + it('throws a typed UnknownVariableError carrying the name and the declared set', () => { + // Callers outside this package react to this specific failure (the MCP + // tools attach a "declare the filter as a variable" hint to it), and + // must not have to match on the message to recognize it. + expect.assertions(5); + try { + substituteVariables('WHERE $__filter(ServiceName, $nope)', { + variables: [SERVICE, variable('env', ['prod'])], + inputLanguage: 'sql', + }); + } catch (e) { + expect(e).toBeInstanceOf(UnknownVariableError); + // Still a MacroExpansionError, so existing handling keeps working. + expect(e).toBeInstanceOf(MacroExpansionError); + const error = e as UnknownVariableError; + expect(error.macro).toBe('filter'); + expect(error.variableName).toBe('nope'); + expect(error.availableVariables).toEqual(['service', 'env']); + } + }); + + it('reports an empty declared set rather than omitting the field', () => { + expect.assertions(1); + try { + substituteVariables('WHERE $__conditionalAll(1=1, $nope)', { + variables: [], + inputLanguage: 'sql', + }); + } catch (e) { + expect((e as UnknownVariableError).availableVariables).toEqual([]); + } + }); + it('throws on the one-argument form when the variable has no expression', () => { expect(() => substituteVariablesSql('WHERE $__filter($service)', [ @@ -1868,3 +1905,40 @@ describe('validateVariableReferencesInTemplate', () => { }); }); }); + +describe('macros naming an unknown variable', () => { + it('reports the message expansion gives', () => { + const { errors, warnings } = validateVariableReferencesInTemplate( + '$__filter(ServiceName, $tenant)', + [SERVICE], + ); + + expect(warnings).toEqual([]); + expect(errors).toEqual([ + "Macro '$__filter' references unknown variable 'tenant'. Available variables: service.", + ]); + }); + + it('says nothing when the macro names a declared variable', () => { + expect( + validateVariableReferencesInTemplate( + "$__conditionalAll(ServiceName != 'api', $service)", + [SERVICE], + ), + ).toEqual({ errors: [], warnings: [] }); + }); + + it('leaves the Lucene message alone rather than piling on', () => { + // In a Lucene input the macro is literal text, so complaining about the + // name it happens to carry is noise on top of the real problem. + const { errors, warnings } = validateVariableReferencesInTemplate( + '$__filter(ServiceName, tenant)', + [SERVICE], + { language: 'lucene' }, + ); + + expect(errors).toEqual([]); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('no meaning in a Lucene expression'); + }); +}); diff --git a/packages/common-utils/src/filters.ts b/packages/common-utils/src/filters.ts index e4bdad6ebd..aff93e099b 100644 --- a/packages/common-utils/src/filters.ts +++ b/packages/common-utils/src/filters.ts @@ -898,14 +898,32 @@ export function getDashboardVariableFilters( return results; } +/** Minimal projection of fields necessary to extract the variables a dashboard declares. */ +export type FilterForVariableDeclaration = Pick< + DashboardFilter, + 'name' | 'expression' +> & + Partial>; + /** The variables a dashboard declares, in filter order. */ export function getDashboardVariableDeclarations( - filters: DashboardFilter[] | undefined, + filters: FilterForVariableDeclaration[] | undefined, ): DashboardVariableDeclaration[] { - return getDashboardVariableFilters(filters).map(({ filter, name }) => ({ - name, - expression: filter.expression, - })); + const declarations: DashboardVariableDeclaration[] = []; + const takenNames = new Set(); + + for (const filter of filters ?? []) { + if (!isFilterVariableEnabled(filter)) continue; + + // There shouldn't be any duplicate names, but if there are then the first one wins. + const name = getFilterVariableName(filter); + if (!name || takenNames.has(name)) continue; + takenNames.add(name); + + declarations.push({ name, expression: filter.expression }); + } + + return declarations; } export type ResolvedFilterValuesQuery = { diff --git a/packages/common-utils/src/macroErrors.ts b/packages/common-utils/src/macroErrors.ts index 21c3bf2fcb..fcaf19f9ad 100644 --- a/packages/common-utils/src/macroErrors.ts +++ b/packages/common-utils/src/macroErrors.ts @@ -18,3 +18,19 @@ export class MacroExpansionError extends Error { this.name = 'MacroExpansionError'; } } + +/** + * Thrown during expansion when `$__filter` / `$__conditionalAll` names a + * variable that the context does not declare. + */ +export class UnknownVariableError extends MacroExpansionError { + constructor( + macro: MacroName, + public readonly variableName: string, + public readonly availableVariables: string[], + message: string, + ) { + super(macro, message); + this.name = 'UnknownVariableError'; + } +} diff --git a/packages/common-utils/src/variables.ts b/packages/common-utils/src/variables.ts index b9eca06eb1..4f320a0496 100644 --- a/packages/common-utils/src/variables.ts +++ b/packages/common-utils/src/variables.ts @@ -5,7 +5,11 @@ import { isQuoteEscapedByBackslash, splitAndTrimWithBracket, } from './core/utils'; -import { MacroExpansionError, MalformedMacroArgsError } from './macroErrors'; +import { + MacroExpansionError, + MalformedMacroArgsError, + UnknownVariableError, +} from './macroErrors'; import { decodeSpecialTokensToSource, encodeSpecialTokens, @@ -503,13 +507,14 @@ function requireVariable( ): ChartVariable { const variable = ctx.variables.find(v => v.name === variableName); if (!variable) { - throw new MacroExpansionError( + const available = ctx.variables.map(v => v.name); + throw new UnknownVariableError( macroName, + variableName, + available, `Macro '$__${macroName}' references unknown variable '${variableName}'. ` + `Available variables: ${ - ctx.variables.length > 0 - ? ctx.variables.map(v => v.name).join(', ') - : '(none)' + available.length > 0 ? available.join(', ') : '(none)' }.`, ); } @@ -874,7 +879,7 @@ export function substituteVariables( * structural rather than tied to one config type so both runtime configs (which * carry `variables`) and saved configs (which don't) can be walked. */ -type BuilderVariableFields = { +export type BuilderVariableFields = { select: SelectList; where?: string; whereLanguage?: SearchConditionLanguage; @@ -930,7 +935,7 @@ const mapSortList = ( * Calls the given map function to rewrite every chart builder expression * that may contain variable references, leaving the rest of the config untouched. */ -function mapBuilderVariableTemplates( +export function mapBuilderVariableTemplates( config: T, map: TemplateMapper, ): T {