platform-lite: serve the ClickHouse logs endpoint (/analytics/endpoints/logs) - #99
platform-lite: serve the ClickHouse logs endpoint (/analytics/endpoints/logs)#99barryroodt wants to merge 19 commits into
Conversation
…ts/logs)
mcp >= the #326 migration queries /v1/projects/{ref}/analytics/endpoints/logs
with ClickHouse-dialect SQL over a unified 'logs' stream; platform-lite only
served the legacy BigQuery-era logs.all, so any logs eval against a current
mcp build 404s at the fixture.
- unified 'logs' VIEW over the seeded tables (source discriminator +
log_attributes jsonb built from columns, metadata fallback)
- minimal dialect translation: log_attributes['k'] -> jsonb access (numeric
cast for status/exec-time keys), countIf -> count(*) FILTER
- read-only enforced by a postgres read-only transaction (not regex): mutating
SQL incl. data-modifying CTEs is rejected before touching fixture state
- iso_timestamp_start/end accepted but ignored (scenario seeds carry fixed
dates; the legacy route makes the same choice)
- contract test: mcp edge-function preset, countIf aggregation, runtime source
Live A/B of mcp PR#333 showed claude-sonnet-5 emitting genuine ClickHouse (countIf(toInt32OrZero(log_attributes['status']) >= 400)); the fixture rejected it and the model adapted with postgres-only SQL that the hosted ClickHouse endpoint would refuse — greening the eval by fixture-adaptation. Provide toInt32OrZero/toInt64OrZero/toUInt32OrZero (text + numeric overloads, CH 0-on-garbage semantics) so the fixture accepts the model's natural dialect. Contract test uses the verbatim model-emitted query.
Second fixture gap from the live PR#333 treatment rerun: the model nests toString() inside toInt32OrZero(). One anyelement cast function covers it; verbatim-model-SQL contract test added.
…me-semantics limitation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The PR claims data-modifying CTEs are rejected, but the existing tests called
the translator/db directly. Exercise the actual HTTP route: normal ClickHouse
query returns the {result} shape; WITH x AS (DELETE ... RETURNING *) SELECT is
rejected by the read-only transaction with fixture rows provably unchanged;
plain non-SELECT hits the 400 prefix gate.
…face Review question on #99: why are these SQL statements defined here instead of imported? Answer, now in-code: the logs relation shape is the hosted platform's Logflare/ClickHouse contract (supabase/platform#35096, platform-internal, no npm artifact); source names track mcp's logsServiceSchema under the pinned MCP_SERVER_VERSION; the OrZero/toString family reimplements ClickHouse builtins; and the verbatim test SQL is frozen observed output on purpose (importing live definitions would make the contract tests tautological).
…t tripwire Review follow-up on #99: the earlier provenance note (and my reply) claimed nothing was importable - wrong on one count. The pinned mcp package DOES export logsServiceSchema from its /platform entrypoint; what it enumerates is the service-preset namespace, not the unified-stream source names the view discriminates on (those exist only in preset SQL strings and the query_logs description). Comment corrected, and the importable artifact is now used for what it's genuinely good for: a drift tripwire that fails loudly when a version bump changes the service enum, pointing at what to resync.
…ovenance Second thought on the tripwire: it asserted the service-PRESET enum, which is a different namespace from the view's source names, so its failure could not demonstrate view staleness. Worse, it imported the resolved devDependency (^0.8.1 -> 0.8.2 today) while the harness runs the MCP_SERVER_VERSION pin (0.8.1), so it guarded a version the fixture never exercises. The verbatim frozen preset SQL in these tests remains the honest alignment contract. The provenance comment now records the exported-schema nuance and why importing it would track the wrong artifact.
Verified against supabase/platform directly: #35096 (getLogs -> logs.all.otel unified stream, ClickHouse dialect) and #35970 (query_logs passthrough, timestamps normalized platform-side) are both OPEN, so the contract this fixture models is what mcp main is written against, not what hosted serves today. The 35970 e2e spec uses the same source vocabulary as this view (postgres_logs), which is a good consistency signal.
#35096 backs the getLogs PRESET path (logs.all.otel + CH dialect) that mcp main emits post-#326; #35970 backs the custom-SQL passthrough that the still open mcp#333 targets - main does not depend on it. Hosted serves the /analytics/endpoints/logs route today; it is the per-PR capabilities that are pending, not the route.
The header bundled query_logs into 'current mcp' while the provenance bullets below correctly note #333 is still open; both now say: current main emits the get_logs presets, the #333 branch emits query_logs, and the fixture models both.
…ate test
- 400 prefix-reject body now carries a message key: mcp's assertSuccess
parses non-2xx bodies as {message}, so the informative read-only text
was collapsing to the generic 'Failed to fetch logs' fallback (error
kept for shape consistency with the 200 SQL-error path)
- CTE-reject test pins status 200: the prefix gate's 400 message also
matches /read-only/i, so unifying the rejection paths would otherwise
leave the read-only transaction guard silently untested
- move src/management-api/debugging.test.ts -> test/clickhouse-logs.test.ts:
platform-lite tests live under test/, and the src placement collided on
basename with the existing test/debugging.test.ts
1970f54 to
b759ca2
Compare
Proposal 4 (review): drop the implicit numeric cast on response.status_code/status_code/execution_time_ms map access. Hosted ClickHouse map values are String, so a bare comparison like log_attributes['response.status_code'] >= 500 errors there — the fixture now errors identically instead of silently accepting SQL that would fail hosted (eval-greens-locally hazard). Models adapt by wrapping in toInt32OrZero, exactly as the frozen fixtures show; a new test pins the error friction, and the constructed query_logs-style test now wraps its comparison like a hosted-correct query must. Minors: type the read-only transaction result (cast gone); parametrize the two verbatim PR-333 fixtures with it.each; assert function_id/level values in the runtime-preset test instead of bare row count; typed Pick<> partial for the fake store; document the two unmodeled preset sources (workflow_run_logs, realtime_logs) in the view header.
ba53d15 to
9d164ad
Compare
- openapi.json: advertise /analytics/endpoints/logs — spliced the single generated path entry (upstream does advertise it; AnalyticsResponse ref already present) instead of taking the full regen's unrelated drift; pinned alongside logs.all in openapi.test.ts - unmodeled sources now error loudly: compileClickHouseLogsSql rejects queries naming workflow_run_logs/realtime_logs (no backing table) so a branch-action/realtime eval fails visibly instead of reading a silent 0-row result as 'no logs'; tested at translator and HTTP level - route test store: real init-free ProjectInstance in a real Map — the exact ProjectStore shape, both casts gone
9d164ad to
89b814d
Compare
|
Surfacing these threads for awareness: (1) (2) I'm wondering how this approach of translating CH dialect for our PGLite logs backend compares with running actual ClickHouse separate from the PGLite logs backend, using their in-memory chdb and chdb-node bindings? That might get us closer feature parity w/o manual translation for each new syntax the agent tries using, though I'm not sure about the integration lift. |
storage_logs was half-modeled: the table exists and the logs VIEW serves a 'storage_logs' source (which mcp's storage preset filters on), but seedLogRow silently dropped 'storage' seeds — a storage eval would read the resulting empty result as 'no logs', the exact false-green the unmodeled-source guard exists to prevent. Add seedStorageLog (base columns; the preset selects only id/timestamp/event_message) and a verbatim storage-preset test. seedLogRow's fall-through was the same bug at the seed layer: any unknown source (typo or unsupported service) silently seeded nothing. It now throws at seed time, naming the supported sources; tested.
…o shims Two more hosted-parity closures: - Restrict /analytics/endpoints/logs to the 'logs' relation (mattrossman's review question). ENFORCEMENT is DB-level: the route transaction runs SET LOCAL ROLE logs_reader, granted SELECT only on the logs view, so postgres name resolution denies backing-table access under any spelling (edge_logs, public.edge_logs, "edge_logs"). The FROM/JOIN regex remains as best-effort message shaping pointing the model at the source-filter idiom. The legacy logs.all route sets no role and keeps table access for its BigQuery-era dialect. The CTE read-only test now uses an INSERT CTE (passes prefix gate and regex) so the transaction stays the tested last line of defense; qualified/quoted bypass spellings are pinned in tests. - Drop the numeric *OrZero overloads: ClickHouse's toInt32OrZero family takes String only, so toInt32OrZero(42) must error here as it does hosted. They existed for the translator's implicit numeric casts, which are already gone. Negative parity test added.
e546500 to
c3dfccf
Compare
|
chdb could be a good fit eventually, yeah, thanks for the threads. Today it'd be a second engine (legacy |
| * deliberately FROZEN observed output (regression fixtures) - importing live | ||
| * definitions would make those contract tests follow the thing they test. | ||
| * | ||
| * KNOWN LIMITATION (time semantics): iso_timestamp_start/end are ignored |
There was a problem hiding this comment.
The KNOWN LIMITATION note covers the query params, but the same time-semantics hole is reachable through the SQL itself, where it fails silently instead of loudly.
For example, seeds carry fixed past dates while now() is real wall-clock, so select count(*) from logs where source = 'edge_logs' and timestamp > now() - interval '24 hours' returns 200 {result: []} with no error, and the agent concludes "no errors occurred".
Until relative-time seeding lands, a guard that rejects now()/current_timestamp in logs SQL (loud, like the other guards) would keep evals honest.
| @@ -125,7 +214,20 @@ export async function seedLogRow(logsDb: PGlite, row: LogRow): Promise<void> { | |||
|
|
|||
| if (normalizedSource === 'auth') { | |||
| await seedAuthLog(logsDb, log); | |||
| return; | |||
| } | |||
|
|
|||
| if (normalizedSource === 'storage' || normalizedSource === 'storage_logs') { | |||
| await seedStorageLog(logsDb, log); | |||
| return; | |||
| } | |||
|
|
|||
| // Loud failure over a silent no-op: a dropped seed surfaces later as a false | |||
| // "no logs" query result, which reads as a passing scenario. Same doctrine as | |||
| // the unmodeled-source guard in debugging.ts. | |||
| throw new Error( | |||
| `unknown log seed source '${row.source}' — expected edge-function, edge, postgres/database, auth, or storage` | |||
| ); | |||
| } | |||
|
|
|||
| type NormalizedLogSeed = { | |||
| @@ -259,6 +361,26 @@ async function seedAuthLog( | |||
| ); | |||
| } | |||
|
|
|||
| async function seedStorageLog( | |||
| logsDb: PGlite, | |||
| log: NormalizedLogSeed | |||
| ): Promise<void> { | |||
| await logsDb.query( | |||
| `INSERT INTO storage_logs | |||
| (id, identifier, timestamp, ts, event_message, message, source, level, metadata) | |||
| VALUES ($1, $2, $3, $3, $4, $4, $5, $6, $7::jsonb)`, | |||
| [ | |||
| log.id, | |||
| metadataText(log.metadata, ['identifier']), | |||
| log.ts, | |||
| log.message, | |||
| log.source, | |||
| log.level, | |||
| log.metadataJson, | |||
| ] | |||
| ); | |||
| } | |||
|
|
|||
| function metadataText( | |||
| metadata: Record<string, unknown>, | |||
| keys: string[] | |||
There was a problem hiding this comment.
This branch relabels function_edge_logs rows as function_logs, so one seeded edge-function row appears 3× in the unified view (unfiltered counts and group by source overcount 3× and show a phantom console-log stream).
I suggest treating function_logs as unmodeled (loud error, like workflow_run_logs) until it can be seeded independently, and pinning group by source output in a test.
There was a problem hiding this comment.
You're right, and it's 3× exactly. I started with your suggestion, then found mcp 0.9.0 has an edge-function-runtime preset reading source = 'function_logs', so rejecting it would break on the next pin bump. It gets its own table and seed source instead, so an edge-function seed no longer writes there. Fixed in 7d84ccf.
| } | ||
| return sql | ||
| .replace( | ||
| /\blog_attributes\['([^']+)'\]/gi, |
There was a problem hiding this comment.
Missing-key semantics diverge from hosted: ClickHouse Map access returns '' for absent keys, ->> returns NULL, so log_attributes['error'] = '' silently matches nothing locally.
Compile to coalesce(log_attributes->>'${key}', ''). Also make the regex whitespace-tolerant (\[\s*'([^']+)'\s*\]): log_attributes[ 'k' ] currently falls through to jsonb subscripting with quote-wrapped values.
There was a problem hiding this comment.
Both correct, fixed in 7d84ccf. Map access compiles to coalesce(log_attributes->>'k', '') and the subscript is whitespace-tolerant. One thing worth knowing: the spaced form was throwing invalid input syntax for type json, not just returning quote-wrapped values.
| // generic "Failed to fetch logs" fallback; `error` kept for shape | ||
| // consistency with the 200 SQL-error path. | ||
| const stmt = sql.trim().replace(/;+\s*$/, ''); | ||
| if (stmt.includes(';') || !/^\s*(select|with)\b/i.test(stmt)) { |
There was a problem hiding this comment.
Suggestion: This gate 400s SQL hosted accepts: leading comments (-- …\nselect), semicolons inside string literals (like '%;%'), and (select …).
The role + read-only transaction do the real enforcement. Please strip leading comments before the prefix test and ignore ; in quoted literals, so agents aren't failed for a fixture artifact.
There was a problem hiding this comment.
Agreed, all four 400'd. The gate now tests a copy with literals and comments blanked, so those cases pass while a genuine second statement still doesn't (7d84ccf). Did you get an answer on whether hosted hard-rejects non-SELECT? Our 400 path should match whatever it does.
| // ProjectInstance (constructor is init-free; the route only touches logsDb) in | ||
| // a real Map — the exact ProjectStore shape, no casts. Dedicated instance so a | ||
| // failing guard can't poison the other tests' fixture rows. | ||
| describe('/v1/projects/:ref/analytics/endpoints/logs route', () => { |
There was a problem hiding this comment.
The ignored-timestamp behavior is documented but not test-pinned. A future "fix" that starts filtering would pass this suite while emptying every seeded scenario.
Please add a route test passing a now-ish iso_timestamp_start/end and asserting seed rows still return.
Also worth mentioning that a modeled-but-unseeded source returning 200 {result: []} with no error, and any execution of toInt64OrZero/toUInt32OrZero (note v::numeric gives '10.5' → 10.5 where ClickHouse returns 0) is missing.
There was a problem hiding this comment.
All three added in 7d84ccf. Your toInt64OrZero note applied to the whole family, they parse a whole integer or return 0 now. One correction worth having: out-of-range strings return 0 rather than wrapping, so toInt32OrZero('2147483648') is 0. The wrapping in the docs is the numeric overload. Checked on play.clickhouse.com.
|
Thanks again for tackling this @barryroodt. The PR is in a great shape. Included some comments but overall is good |
Five review findings from @Rodriguespn, each reproduced against a live PGlite instance before changing anything. Silent empty results on wall-clock SQL. The KNOWN LIMITATION note covered iso_timestamp_start/end but the same hole was reachable through the SQL: seeds carry fixed past dates while now() is real wall-clock, so `timestamp > now() - interval '24 hours'` returned 200 {result: []} and a model read that as "no errors occurred". Wall-clock functions now reject loudly, matching the unmodeled-source doctrine. function_logs fan-out. seedLogRow writes an edge-function row into both edge_logs and function_edge_logs, and the unified view then unioned function_edge_logs a second time as function_logs, so ONE seed counted 3 and `group by source` showed a console stream carrying request rows. function_logs now has its own runtime-shaped table (event_type, execution_id, function_id, deployment_id, version), is unioned once, and is seeded from a new 'edge-function-runtime' source. Rejecting the source was the wrong fix: mcp 0.9.0 adds an 'edge-function-runtime' preset that reads exactly this source, so it must stay queryable once the pin moves. Missing-key semantics. A ClickHouse Map yields '' for an absent key while ->> yields NULL, so `log_attributes['error'] = ''` matched every row hosted and none here. Map access compiles to coalesce(..., '') now. The subscript pattern is also whitespace-tolerant: `log_attributes[ 'k' ]` fell through to pg jsonb subscripting, stayed jsonb, and threw "invalid input syntax for type json" when compared to a text literal. Prefix gate false rejections. The gate is only message shaping (the role and the read-only transaction enforce), so 400ing SQL hosted accepts just fails a model for a fixture artifact. Both the multi-statement scan and the statement-kind test read a blanked copy of the SQL where literals and comments become whitespace, which covers leading comments and `like '%;%'` together; a leading paren is tolerated. postgres still sees the original. A genuine second statement is still rejected. OrZero semantics. v::numeric accepted '10.5' as 10.5 and passed negatives through unsigned. These now parse a whole integer or yield 0, with range checks per type. Verified against play.clickhouse.com rather than inferred from the docs: the overflow-wrapping note there is about the numeric overload (toInt32(2147483648::Int64) is -2147483648), while a too-large string is a parse error, so toInt32OrZero('2147483648') is 0. Tests: 54 passing across clickhouse-logs and openapi, including the iso_timestamp_start/end pin that a future window-honouring change would otherwise break silently, the 0.9.0 edge-function-runtime preset verbatim, per-source row counts, and 17 OrZero cases. Every fix was mutation-checked by reverting it and confirming a test fails.
The guard rejected any wall-clock function anywhere in the statement, including a bare `select now()`. That form is an orientation probe: models open with it before building a window (observed in the mcp#333 A/B, where the only windowless calls were a now() check and a data sample). It reads no seeded row, so it cannot produce the false "no logs" the guard exists to prevent, and rejecting it costs a turn while teaching nothing. The check now requires the statement to reference the 'logs' relation, so the wall-clock FILTER over fixed-date seeds still rejects loudly while the probe passes. Pinned both directions: three probe forms are accepted, and a CTE that hides now() behind a join onto logs is still rejected.
c5eaad4 to
d0c6b81
Compare
Rodriguespn
left a comment
Rodriguespn
left a comment
There was a problem hiding this comment.
LGTM. Nice work!
Approving this to unblock you, but I would suggest porting the logs_reader/read-only wrapper to the legacy logs.all route as a fast follow-up.
It still runs agent SQL with write privileges, so an agent on the older path can mutate log state that the new hardened route then faithfully serves. The guard machinery already exists a few lines below, so it should be cheap to reuse.
|
From what I can piece together, we're using PGlite to simulate the DB behind the CH endpoint? Since we want our evals to be as accurate as possible, I'm worried that seeding/simulating logs will be doing us a disservice. Have we considered chdb-wasm or chdb-node instead? Beyond ClickHouse, how are application logs surfaced right now (or is it all seeded/fake data)? |
4 participants
What
Teaches platform-lite the ClickHouse logs endpoint that current mcp actually calls:
GET /v1/projects/{ref}/analytics/endpoints/logs, taking ClickHouse-dialect SQL over the unifiedlogsstream.Why
Since supabase/mcp#326,
get_logs(and the proposedquery_logsin supabase/mcp#333) query/analytics/endpoints/logswith ClickHouse SQL. platform-lite only served the legacy BigQuery-eralogs.all, so any logs eval against a locally built mcp 404s at the fixture. The gap is masked today because evals pin a publishedMCP_SERVER_VERSION; it bites the moment anyone points the harness at an mcp checkout (which is how we validated mcp#333).How
logsVIEW over the existing seeded tables: asourcediscriminator plus a jsonblog_attributesmap built from the flat columns, with seededmetadataas fallback. ClickHouse-shaped SQL runs against it with minimal translation.compileClickHouseLogsSql):log_attributes['k']to jsonb access (numeric cast for status/exec-time keys so>= 500comparisons work) andcountIf(...)tocount(*) FILTER (WHERE ...). A small shim family (toInt32OrZero/toInt64OrZero/toUInt32OrZero/toString) covers casts models genuinely emitted during live runs. Anything else surfaces the raw SQL error to the model, which is deliberate: the supported surface is documented and only grows from observed model output.WITH x AS (DELETE ...) SELECT), is rejected before it can touch shared fixture state.iso_timestamp_start/endare accepted but ignored, matching the legacy route: scenario seeds carry fixed dates while mcp defaults windows from the current clock, so a faithful filter would empty every scenario. Documented in-code as a known limitation; window-correctness needs relative-time seeding and a discriminating eval (follow-up).edge-functionpreset, acountIfaggregation, and the exacttoInt32OrZero(toString(...))query the model emitted), plus the runtime source and the legacy route untouched.Verification
pnpm typecheckclean,pnpm vitest run src/management-api/debugging.test.ts8/8: five translator/view contract tests plus three route-level tests at the HTTP boundary (normal ClickHouse query returns the{result}shape;WITH x AS (DELETE ... RETURNING *) SELECTis rejected by the read-only transaction with fixture rows asserted unchanged; plain non-SELECT hits the 400 prefix gate).investigate-logs-001-top-error-functionpasses against a locally built mcpmain(3/3 checks) where it previously 404'd, and against an mcp checkout of feat: add query_logs tool for custom log queries mcp#333 the model's first genuine ClickHouse aggregation succeeds end to end.Found while running an A/B validation of supabase/mcp#333 through the eval workspace; the run details are in that PR's thread.