feat: add query_logs tool for custom log queries - #333
Conversation
barryroodt
left a comment
barryroodt
left a comment
There was a problem hiding this comment.
Really clean addition, Jordi. It mirrors the merged get_logs sibling almost exactly (same endpoint, same untrusted-data wrapping, same 24h defaulting), and I reproduced the checks against the PR head: tsc --noEmit, biome ci ., and the server.test.ts + logs.test.ts suites (111 tests) all pass. Nothing here blocks merge. A couple of small things worth a look:
The sql description is missing function_edge_logs. The source list guides the model on what to filter by, and right now it lists function_logs (edge-function runtime console output) but not function_edge_logs (the invocation/request logs the get_logs edge-function preset queries, see logs.ts:37). Since that description is effectively the contract the model writes SQL against, a model following it can't reach invocation logs and will filter on the wrong source. Everything else in the list looks right.
query_logs has no execution test yet. The test change adds it to the tool-listing assertion, which is exactly right for registration, but the new timestamp-defaulting / passthrough / wrapping logic in execute is the bug-prone part and it's currently untested (whereas get_logs has three). server.test.ts:1567 is a ready-made template and the existing /endpoints/logs mock already covers it, so no new mock needed. Happy to leave this as a fast-follow if you'd rather not expand scope here.
Tiny nit: sql: z.string() could take .min(1) to match executeSqlOptionsSchema.query, so an empty query gives a clear validation message instead of an opaque backend error. Very much a nice-to-have.
None of this is load-bearing for shipping. The function_edge_logs line is the one I'd genuinely want fixed before merge; the rest are optional.
|
Thanks for the thorough review! Addressed all three in f37cfc5:
🤖 Addressed by Claude Code |
barryroodt
left a comment
barryroodt
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround @jordienr
function_edge_logsadded to the source list.min(1)on the schema- behavioral tests for
query_logs
LGTM
@mattrossman, @Rodriguespn - since I'm still the new guy, perhaps a quick scan and thumbs-up from either of you would be advisable 😁
mattrossman
added
the
publish-preview
commit: |
|
on further thought, let's leave both tools and potentially deprecate and remove getLogs in favour of queryLogs. |
|
Addressed the deprecation + platform-availability guidance in a4d7228:
🤖 Addressed by Claude Code |
|
Following up with fresh numbers against the current head (ecc456d) Rerun of our eval A/B against the new head: passed all 3 eval checks on the first attempt, and the a4d7228 deprecation note fully flipped tool selection, 8 One finding: the 24 hour default still silently widens narrow questions. The eval asks about the last 15 minutes; all 8 Suggestion: same mechanism as a4d7228 - make the line directive, something like "when the user asks about a specific time range, always pass iso_timestamp_start/iso_timestamp_end to match it". Unrelated side effect: this exercise surfaced drift in our own evals fixture, fixed in supabase/evals#99. |
|
Thanks for the fresh eval run — great to see the deprecation note flip selection cleanly, and good catch on the time-range default. Fixed in ea6d163: the timestamp guidance is now a directive rather than a permissive mention, using the same mechanism as the deprecation line. Both
Applied to 🤖 Addressed by Claude Code |
|
Nice! Reran the A/B against ea6d163 and the directive line does exactly what we hoped: the model now passes an explicit 15 minute window matching the question on every analytical |
…s split this committ assume we will have merged or should be merged to gether with cahnges on skills and MCP - supabase/agent-skills#112 - supabase/mcp#333 query_logs (hosted only, ClickHouse SQL) is now the preferred tool for production projects; get_logs remains the only option for local and self-hosted. Updates the MCP tools table, log-querying section, and skill step 3 in ai-agents.mdx, and the tip admonition in debugging.mdx. logs.mdx already had this right at line 321.
…s split this committ assume we will have merged or should be merged to gether with cahnges on skills and MCP - supabase/agent-skills#112 - supabase/mcp#333 query_logs (hosted only, ClickHouse SQL) is now the preferred tool for production projects; get_logs remains the only option for local and self-hosted. Updates the MCP tools table, log-querying section, and skill step 3 in ai-agents.mdx, and the tip admonition in debugging.mdx. logs.mdx already had this right at line 321.
…s split this committ assume we will have merged or should be merged to gether with cahnges on skills and MCP - supabase/agent-skills#112 - supabase/mcp#333 query_logs (hosted only, ClickHouse SQL) is now the preferred tool for production projects; get_logs remains the only option for local and self-hosted. Updates the MCP tools table, log-querying section, and skill step 3 in ai-agents.mdx, and the tip admonition in debugging.mdx. logs.mdx already had this right at line 321.
Adds a query_logs debugging tool that runs a custom read-only ClickHouse SQL query against a project's unified logs stream, for cases where the get_logs service presets are too coarse. Reuses the existing analytics logs endpoint and validates that queries are SELECT/WITH only.
- add function_edge_logs to the sql source-hint list so models can reach edge function invocation logs - require a non-empty sql query (.min(1)), matching execute_sql - add execution tests for query_logs: sql passthrough + timestamp defaulting, custom window forwarding, and empty-query rejection
- mark get_logs as deprecated on hosted projects in favour of query_logs, while keeping it as the path for CLI/self-hosted - document that query_logs (ClickHouse) is hosted-only and will not work on CLI/self-hosted yet
The permissive mention of iso_timestamp_start/iso_timestamp_end wasn't steering model behaviour, so narrow time-range questions silently inherited the 24h default and over-counted. Make it a directive instruction in both get_logs and query_logs, matching the mechanism that flipped tool selection.
The description promises iso_timestamp_start defaults to 24h before the end, but the handler always computed start from now(), so supplying only iso_timestamp_end produced an inverted/empty window. Derive the end first (supplied or now), then default start to end - 24h, shared by get_logs and query_logs.
resolveLogWindow now rejects a malformed iso_timestamp_start/end with a clear error instead of throwing a raw "Invalid time value", and rejects a start at or after the end. Also rebases onto main to pick up the regenerated management API types.
c1bdc7f to
09e521e
Compare
Both tools currently ship an identical description to every client regardless of platform (hosted vs local/self-hosted), so labeling get_logs "Deprecated" risked a client universally hiding or deprioritizing it, which would break self-hosted users since get_logs is their only working logs tool. Reframe as environment-scoped preference (prefer query_logs on hosted, use get_logs on local/self-hosted) instead of an unqualified deprecation.
|
|
||
| export type DebuggingOperations = { | ||
| getLogs(projectId: string, options: GetLogsOptions): Promise<unknown>; | ||
| queryLogs(projectId: string, options: QueryLogsOptions): Promise<unknown>; |
There was a problem hiding this comment.
Making queryLogs a required in DebuggingOperations breaks every external SupabasePlatform implementer on upgrade (this type ships via the ./platform subpath export and the CLI/studio and mcp.supabase.com controller implement it out-of-repo), and a stale JS implementer would still pass the if (debugging) group check, so query_logs gets listed in tools/list and then crashes with TypeError: debugging.queryLogs is not a function at call time.
We agreed here that this tool should be platform-API-only, and Matt proposed exactly this mechanism: make it queryLogs?(…) and register the tool only when the platform provides it. Then self-hosted/CLI need no changes and the tool is simply absent instead of erroring.
This is also a breaking change to a published type, so can you please change the title of this PR to include feat!: and also make sure that the merged commit that lands on main starts with feat!: please?
Check CONTRIBUTING's expand/contract rule for more details.
| return { result: wrapWithUntrustedDataBoundary(result) }; | ||
| }, | ||
| }), | ||
| query_logs: injectableTool({ |
There was a problem hiding this comment.
Related to the types.ts comment: this registration is unconditional, so every platform with a debugging group advertises query_logs, including BigQuery-dialect (CLI/self-hosted) backends where the description itself says the query will fail.
Guarding with if (debugging.queryLogs) (once the member is optional) hides the tool cleanly on unsupported platforms.
| throw new Error('iso_timestamp_start must be before iso_timestamp_end.'); | ||
| } | ||
|
|
||
| return { iso_timestamp_start: start, iso_timestamp_end: end }; |
There was a problem hiding this comment.
resolveLogWindow validates with Date.parse but forwards the original strings verbatim.
Two consequences: (a) Date.parse accepts non-ISO strings like "Feb 1 2024" even though the error message promises ISO 8601 rejection and (b) offset-less timestamps are parsed as server-local time while the management API treats them as UTC.
I propose normalizing both with new Date(ms).toISOString() before returning fixes both, or move format enforcement into the schema with z.iso.datetime() so the constraint also shows up in the JSON Schema the model sees.
…registration DebuggingOperations.queryLogs is used by external SupabasePlatform implementers (CLI, studio, mcp.supabase.com controller) outside this repo. Making it required would break them on upgrade: a stale implementer still passes the existing `if (debugging)` group check, so query_logs gets listed in tools/list and then crashes with "debugging.queryLogs is not a function" at call time. Make queryLogs optional and only register the query_logs tool when the platform actually implements it, so an implementer without ClickHouse support (self-hosted/CLI today) simply doesn't get the tool listed instead of erroring. This makes the DebuggingOperations change purely additive.
| endTimestamp.getTime() - 24 * 60 * 60 * 1000 | ||
| ); // Last 24 hours | ||
|
|
||
| const result = await debugging.getLogs(project_id, { |
There was a problem hiding this comment.
Just a heads-up that routing get_logs through resolveLogWindow silently changes existing behavior: inputs that previously flowed through to the API are now rejected client-side, and a supplied iso_timestamp_end now anchors the default start instead of using now-24h.
That's arguably all improvement, but none of the six new validation tests cover get_logs, mirroring the inverted/malformed/anchoring cases for get_logs (or exporting resolveLogWindow and unit-testing it once) would pin the shared behavior.
| ); | ||
| } | ||
|
|
||
| if (startMs >= endMs) { |
There was a problem hiding this comment.
nit: Both tool descriptions say "The API caps the requested range at 24 hours", but nothing pre-validates.
Cheap win: if (endMs - startMs > DAY_MS) throw new Error('The log window can be at most 24 hours.') plus the matching unit test.
|
|
||
| return response.data; | ||
| }, | ||
| async queryLogs(projectId: string, options: QueryLogsOptions) { |
There was a problem hiding this comment.
q: Does /v1/.../analytics/endpoints/logs hard-rejects non-SELECT statements?
| ).rejects.toThrow(); | ||
| }); | ||
|
|
||
| test('query logs rejects a start at or after the end', async () => { |
There was a problem hiding this comment.
nit: Adding the equal-timestamps case, and giving these negative tests a message matcher (rejects.toThrow(/must be before/)) instead of bare rejects.toThrow(), would make them mean what they say.
Same theme at line 2075: the default-window assertions only check the params are truthy, an exact assertion would catch a wrong default or swapped bounds.
resolveLogWindow now:
- enforces the 24h API cap client-side with a clear error, instead of
relying on an unvalidated description promise
- normalizes accepted timestamps to canonical UTC ISO strings before
forwarding them, instead of passing the original strings through verbatim
- is exported and unit-tested directly (default anchoring, offset
normalization, malformed/inverted/oversized-window rejection), covering
get_logs and query_logs' shared behavior in one place
Also enforces ISO 8601 with an explicit UTC "Z" suffix or offset at the
schema level via z.iso.datetime({ offset: true }), so offset-less
timestamps (ambiguous local-time interpretation) are rejected before
reaching resolveLogWindow, and the constraint shows up in the tool's JSON
schema.
|
Since this adds a tool and rewords the
Also linking the eval evidence here: the pkg.pr.new preview build of this branch passed the full supabase/evals regression suite (8/8, identical to the published-version baseline - CI run) and locally against the three debugging evals that exercise the analytics logs endpoint (evals#79, with transcripts showing agents successfully using Conditions of the run: model Thanks for taking care of this @jordienr! |
- add an equal-timestamps case to the start-at-or-after-end rejection test - assert on the actual rejection message (Invalid ISO datetime, must be before, min-length) instead of a bare rejects.toThrow() - assert the exact default window (end near now, start = end - 24h) instead of just checking the params are truthy
Rodriguespn
left a comment
Rodriguespn
left a comment
There was a problem hiding this comment.
LGTM, thanks for taking another look @jordienr. Could you please address the nit comments I left, whether implementing them or closing them. Up to you 🙁
Important
One last thing before merging, this is also a breaking change as we changed get_logs description and inputSchema.
Can you change the title of this PR to include feat!: and also make sure that the merged commit that lands on main starts with feat!: please?
Check CONTRIBUTING's expand/contract rule for more details.
5 participants
What
Adds a new
query_logstool to thedebuggingfeature group. It runs a custom ClickHouse SQL query against a project's unified logs stream, for cases where theget_logsservice presets are too coarse (filtering, aggregating, or joining across log fields).Tracks AI-701. Builds on the ClickHouse logs endpoint work in O11Y-1813 and the
get_logsClickHouse migration (#326).How it addresses the observability team's concerns
execute_sql), so the result is wrapped inwrapWithUntrustedDataBoundary, the same best-effort guardrailexecute_sqluses. Not foolproof, but consistent with the existing arbitrary-query tool.GET /v1/projects/{ref}/analytics/endpoints/logs(ClickHouse / logs.all.otel) and takes raw ClickHouse-dialect SQL. Does not touch the deprecated BigQuery-backedlogs.all.GETwithsqlas a query param. No new POST handler.get_logs(feat: support edge function runtime logs in get_logs #326) under the samedebugginggroup, so it inherits identical platform availability and introduces no new exposure on CLI/self-hosted.Details
query_logsparams:project_id,sql, optionaliso_timestamp_start/iso_timestamp_end. Defaults to the last 24h window (matchingget_logs).debugginggroup.Verification
tsc --noEmitclean,biome ciclean.logs.test.ts,server.test.ts). The 5 failing checks are|e2e|tests requiring liveSUPABASE_ACCESS_TOKEN/ANTHROPIC_API_KEY, which fork PRs don't receive — they pass when run in the upstream context with secrets.Note
Per CONTRIBUTING, feature PRs should track an accepted issue — this tracks AI-701.