diff --git a/docs/architecture.md b/docs/architecture.md index fa906396b..47a7ad6ec 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -284,7 +284,7 @@ See [otel.md](./otel.md) for the full setup. Every layer emits telemetry to Dash | Signal source | What's emitted | |---------------|----------------| -| Edge Function (Deno) | `lorekit.memory.*` spans, `lorekit.webhook.github` spans, DB child spans named by SQL statement | +| Edge Function (Deno) | `lorekit.memory.*` spans, `lorekit.webhook.github` spans, DB child spans named by `db.query.summary` (`SELECT memories`) with a parameterised `db.query.text` | | Next.js server | HTTP server spans via `@vercel/otel` | | Browser (RUM) | Page loads, navigation, fetch traces, errors via `@dash0/sdk-web` | diff --git a/docs/key-files.md b/docs/key-files.md index ff5eee72a..3fa16cc05 100644 --- a/docs/key-files.md +++ b/docs/key-files.md @@ -28,6 +28,7 @@ else. Grouped loosely by subsystem in migration/PR order. | `supabase/functions/mcp/index.ts` | Self-contained Deno MCP server (production) | | `supabase/functions/_shared/telemetry/otel.ts` | Reusable OTel for Edge Functions: `traceRequest()`, `createTracedClient()`, and the SINGLE source of the OTLP resource attributes (`buildResourceAttributes`), endpoint/dataset resolution (`getOtlpConfig`) and attribute encoding (`toOtlpValue`) that BOTH the span and metric exporters use — a second copy of any of them puts a metric on a different resource than the spans beside it, and the two silently stop correlating. Also stamps the self-time attribution (`lorekit.io.wait_ms` / `lorekit.io.calls` / `lorekit.self_time_ms`) on every root request span | | `supabase/functions/_shared/telemetry/otlp-metrics.ts` | The metric half of the edge exporter — `buildOtlpMetricsPayload` + `exportMetrics`, POSTing `resourceMetrics` to `/v1/metrics`. Cumulative monotonic sums ONLY, because that is the shape `pg_stat_statements` counters already have: handing them over cumulative (with `stats_reset` as `startTimeUnixNano`) lets Dash0 own both `rate()` and reset detection, where computing deltas here would mean persisting a snapshot and re-implementing reset detection. Unlike `ExportBatch.flush()` it AWAITS and REPORTS the outcome — a swallowed failure here would be a cron job answering `200` while nothing reaches Dash0 | +| `supabase/functions/_shared/telemetry/span-semconv.ts` | Pure, import-free semconv rendering for edge spans: `querySummary` (the DB span name), `renderStatement` / `conditionSql` / `postgrestLogicToSql` (a parameterised `db.query.text` — no filter value reaches the span name or the statement; an unparseable `.or()` string collapses to `(?)`), `redactUrls` (strips request URLs, and the filter values in their query strings, from a recorded `error.message`; Postgres's own wording can still echo a value), `resolveServerAddress` (never names a BYOD host), `errorTypeFrom` / `errorTypeOf` (the bounded `error.type` — catch blocks pass `errorTypeOf(e)`, which falls back to a PostgREST error object's `code`), `httpRouteFor`. Executed by `packages/mcp-core/src/telemetry/span-semconv.spec.ts` | | `packages/mcp-core/src/telemetry/io-ledger.ts` | Pure `mergeBusyMs` / `attributeIoTime` — the self-time split behind `lorekit.self_time_ms`. The closest thing to a CPU profile a managed Deno isolate can produce (no host for Dash0's eBPF profiler, no userland V8 profiler, and OTLP profiles have no JS exporter). Intervals are MERGED, never summed: handlers issue concurrent queries, so adding two overlapping 40 ms calls claims 80 ms of wait in a request that waited 40 and drives self time negative on exactly the requests worth profiling. Mirrored into `supabase/functions/_shared/telemetry/io-ledger.ts`, guarded by `edge-parity.spec.ts` | | `packages/mcp-core/src/telemetry/db-query-metrics.ts` | Pure `buildDbQueryMetrics` — `pg_stat_statements` rows → the three cumulative sums (`lorekit.db.query.time` in **seconds**, `.calls`, `.rows`). Holds the decisions that are silent when wrong: the ms→s conversion, the EPOCH (not `now`) fallback for a never-reset counter — `startTimeMs === timeMs` is a zero-length series a backend divides by zero over — `queryid` kept as a STRING (int64), and the bounded attribute set with deliberately NO tenant dimension. Mirrored into `supabase/functions/_shared/telemetry/db-query-metrics.ts`, guarded by `edge-parity.spec.ts` | | `supabase/functions/profiling/index.ts` | Query-level profiling export: read `lorekit_db_query_stats()` → map → POST as OTLP metrics. SERVICE-ROLE only (the counters are the whole cluster's, so an `lk_ro_*` tenant token is refused too) and deliberately NOT behind `createRouter` — a cron poke is not a tenant request and must not land in `usage_events`. Distinguishes `503` (nothing to send / nowhere to send it) from `502` (Dash0 rejected it), because a `200` on either is the silent failure the endpoint exists to prevent | diff --git a/docs/otel.md b/docs/otel.md index f3fd17dd0..e79141532 100644 --- a/docs/otel.md +++ b/docs/otel.md @@ -6,7 +6,7 @@ LoreKit emits traces, metrics, and logs to Dash0 from every layer of the stack. | Layer | SDK | Signals | |-------|-----|---------| -| Edge Function (Deno) | Lightweight OTLP/JSON via `fetch()` | Traces per tool call + webhook; DB child spans named by SQL statement; self-time attribution on every root span; Postgres query-cost metrics (opt-in) | +| Edge Function (Deno) | Lightweight OTLP/JSON via `fetch()` | Traces per tool call + webhook; DB child spans named by `db.query.summary` (`SELECT memories`) with a parameterised `db.query.text`; `http.route` + `error.type` on root spans; self-time attribution on every root span; Postgres query-cost metrics (opt-in) | | Next.js server | `@vercel/otel` | HTTP server spans, Supabase query spans, custom INTERNAL spans for every mutating server action | | Browser (RUM) | `@dash0/sdk-web` | Page loads, navigation, Web Vitals, fetch tracing, errors, sessions | | CLI (`@lorekit/cli`) | Lightweight OTLP/JSON via `fetch()` (zero-dep, no SDK) | One span + one counter point per human-facing command (`install` / `uninstall` / `doctor` / `list` / `search` / `show` / `stats` / `scopes` / `diff` / `tree` / `lint` / `dedupe` / `obligations` / `link` / `migrate`) | @@ -33,7 +33,7 @@ Every `tools/call` invocation produces a trace tree: ``` lorekit.memory.write (INTERNAL — tool dispatch) - └── UPSERT INTO memories WHERE ... (CLIENT — Postgres, db.query.text set) + └── CALL memory_write (CLIENT — Postgres; parameterised db.query.text) ``` Attributes on `lorekit.memory.*` spans: @@ -585,15 +585,78 @@ Users can read their own rows (self-service "my usage" view via RLS). --- -DB child spans carry OTel database semconv: +DB child spans carry OTel database semconv. The span is NAMED by +`db.query.summary`; no filter value reaches the name or the statement, and a +failure's `error.message` has URLs redacted (see "Error messages" below): | Attribute | Example | |-----------|---------| +| span name = `db.query.summary` | `SELECT memories` / `CALL memory_write` — bounded: one per operation × table/function | | `db.system` | `postgresql` | -| `db.operation.name` | `SELECT` / `INSERT` | -| `db.collection.name` | `memories` | -| `db.query.text` | `SELECT key,value FROM memories WHERE scope = '...' LIMIT 50` | +| `db.operation.name` | `SELECT` / `INSERT` / `RPC` | +| `db.collection.name` | `memories` (the function name for an RPC) | +| `db.query.text` | `SELECT key,value FROM memories WHERE scope = $1 AND archived_at IS NULL LIMIT $2` — values are `$n` placeholders, `LIMIT` included | +| `server.address` | `pqokxlhvnosogizsjztg.supabase.co` — omitted for a BYOD client, whose host is the user's own project | | `db.response.rows` | `7` | +| `db.response.status_code` | `23505` — the SQLSTATE / PostgREST code whenever PostgREST returns an error object, including `PGRST116` (a `.single()` with no row, which is not marked as an error); the span's `error.type` when it is one | + +**Why the values are gone.** The span used to be named after the statement with +every filter value interpolated (`… WHERE key = 'reviewer-lessons::…'`, +`… to_tsquery('zsh glob no matches …')`, `… user_id = '6e05…'`). In the week +before the change that produced **511 distinct span names** on `api` — 287 +carrying full-text search terms (which the CLI hook distils from an agent's +tool-failure text and keeps out of its own telemetry), 133 memory keys, 4 user +ids. A span name is a grouping key; and `db.query.text` is defined as the +sanitised statement. The rendering lives in the pure +`_shared/telemetry/span-semconv.ts`: identifiers from our code are kept, every +value is a placeholder, and a `.or()` logic string it cannot parse collapses to +`(?)` rather than being guessed at. `span-semconv.spec.ts` runs a real +`createTracedClient` chain with sentinel values and asserts none reaches the +exported span. + +**Error messages.** Every span's `error()` / `clientError()` runs its message +through `redactUrls`, so a handler or root span that records a failed request's +message cannot carry its URL either. A failed DB span records PostgREST's error +text as `error.message`. A failed *request* is the dangerous case: postgrest-js does not +reject on a network failure, it **resolves** with `status: 0` and the request URL +— every filter value in its query string — in the message. That case records +`PostgrestError: fetch failed`; every other message (including a thrown error on +the rejection arm) is kept with URLs replaced by `` (`redactUrls`). What is +left is Postgres's own wording, which can still echo a value (e.g. `invalid input +syntax for type uuid: "…"`, or the slug in `unknown_org: `). That residue +is why the token lookups in `mcp/auth.ts` / `_shared/api/auth.ts` stay off +`createTracedClient` as defence in depth (`mcp-auth-tracing.spec.ts`, +`rest-auth-tracing.spec.ts`). + +### Root request spans: `http.route` and `error.type` + +| Attribute | Example | Set by | +|-----------|---------|--------| +| `http.route` | `/memories/:id`, `/mcp`, `/health` | `createRouter` once a route MATCHES (so a 403 carries it; a 404/405 does not), and each single-route function at the top of its `traceRequest` callback. Same shape as `url.path`, template not value. The router's handler child span keeps its function-relative `http.route` (`/:id`) | +| `error.type` | `UserInputError`, `MethodNotFound`, `missing_token`, `TypeError`, `23505`, `503` | Every `span.error()` / `span.clientError()` — an explicit type, else the `Name:` prefix of the message (`errorTypeFrom`), else `_OTHER`. Catch blocks pass `errorTypeOf(e)`: an `Error`'s name, else a PostgREST error object's `code` (those objects have no `name`, so `` `${e.name}: …` `` would read `undefined`). A 5xx response with nothing more specific records the status code. Never the message itself — that stays on `error.message`, with URLs redacted on every span | + +**`http.route` renames the Dash0 operation.** With it present, Dash0 names a +root span's operation `{method} {route}` — `POST /mcp`, `GET /memories/:id` — +where it used to be the function (`mcp`, `memories`, from `faas.name`). That was +observed on this change's own CI smoke trace, against what the operation-rule +docs' table order suggests. A request that matches no route (a 404, a discovery +probe) keeps the function-named operation. + +So: per-endpoint RED now comes for free in Dash0's operation views, and anything +that must address a whole **function** filters on `faas_name`, which is the same +under both namings — never on `dash0_operation_name="mcp"`: + +```promql +sum by (faas_name, http_route) (increase({otel_metric_name="dash0.spans", service_name="api", service_namespace="lorekit", otel_span_kind="SERVER"}[1h])) +``` + +The two check rules that filtered `dash0_operation_name="mcp"` ("API — mcp +operation has stopped sending spans", "API — mcp operation elevated error rate") +must move to `faas_name="mcp", otel_span_kind="SERVER"` **before** this deploys, +or the absence rule fires two hours after the deploy and the error-rate rule +silently stops matching. The `api-health-red` and `error-hotspot-triage` +dashboards group by `dash0_operation_name` and will show routes instead of +functions. --- diff --git a/docs/telemetry-quality-review.md b/docs/telemetry-quality-review.md index dbbcdae75..7efdea965 100644 --- a/docs/telemetry-quality-review.md +++ b/docs/telemetry-quality-review.md @@ -141,9 +141,12 @@ resource attributes only. --- -## Recommendations (not applied — design/judgment calls) +## Recommendations (design/judgment calls — status noted per item) -1. **`db.query.text` and DB span names inline literal filter values** +1. **`db.query.text` and DB span names inline literal filter values** — + **applied** (2026-10): spans are named by `db.query.summary`, the statement + is parameterised, see [otel.md](./otel.md) → DB child spans. Original + finding kept below for the record. (`_shared/telemetry/otel.ts` `buildSql`). Span names like `SELECT ... FROM memories WHERE scope = 'repo::acme/x' AND key = '...'` are **high-cardinality** (an anti-pattern for span names, which should group) and @@ -155,7 +158,9 @@ resource attributes only. the span. This is a deliberate design change to all edge DB spans, so it is left for maintainer sign-off rather than auto-applied. -2. **`error.message` is a non-registry attribute.** `Span.error()` / +2. **`error.message` is a non-registry attribute.** — **partly applied** + (2026-10): every `error()` / `clientError()` now also records a bounded + `error.type`; `error.message` is kept so existing queries keep working. `Span.error()` / `clientError()` set `error.message`; the conventions use `exception.message` (with `exception.type`) or `error.type`. Also, PostgREST error strings placed there can carry incidental detail. Recommend migrating to `error.type` + diff --git a/packages/evals/README.md b/packages/evals/README.md index c9bbbe251..368703e60 100644 --- a/packages/evals/README.md +++ b/packages/evals/README.md @@ -526,7 +526,12 @@ Two mechanisms now stand against that, and only the second is trusted: 1. **Ask.** `--disable-slash-commands` drops skills and commands, `--strict-mcp-config` admits only the harness's own server, and a - session-scoped `--settings` file sets `enabledPlugins: {}`. + session-scoped `--settings` file sets `"@builtin": false` in + `enabledPlugins` for each of Claude Code's built-in plugins + (`BUILTIN_PLUGINS` in `src/harness/arm.mjs`). An empty `enabledPlugins: {}` + is not enough: it sets nothing, so a default-on built-in still loads — + on a clean CI runner that was `cc-plugin-agents-md` and + `cc-plugin-plugin-authoring`. 2. **Verify.** `src/grading/environment.mjs` reads the run's own `{"type":"system","subtype":"init"}` event and its `hook_started` events, and reports what _actually_ loaded. A rep whose environment is dirty is diff --git a/packages/evals/src/harness/arm.mjs b/packages/evals/src/harness/arm.mjs index 9486d66c5..45acb73fb 100644 --- a/packages/evals/src/harness/arm.mjs +++ b/packages/evals/src/harness/arm.mjs @@ -221,11 +221,44 @@ export async function prepareArm( }; } +/** + * Claude Code's BUILT-IN plugins, by the name their `@builtin` id uses. + * + * WHY AN EMPTY `enabledPlugins` STOPPED BEING ENOUGH. + * A built-in ships inside the `claude` binary rather than being installed, and + * some are on by default: `{}` sets nothing, so a default-on built-in still + * loads. The unpinned `npm install -g` in evals.yml picked up a release that + * does exactly that (reproduced on 2.1.287; 2.1.280 ships no built-ins), and + * on a GitHub runner with no `~/.claude` at all the session loaded + * `cc-plugin-agents-md` and `cc-plugin-plugin-authoring`, so preflight rightly + * reported the clean room CONTAMINATED. Only an explicit + * `"@builtin": false` switches one off. + * + * The list is every built-in 2.1.287 ships except `sec-default`, which is + * enabled from managed policy only, so a session setting cannot switch it + * either way. It is a request, like every flag here, so a built-in a later + * release adds is not silently trusted: preflight names it in a + * `plugins-loaded` finding, and the fix is to add it below. + */ +export const BUILTIN_PLUGINS = Object.freeze([ + "agents-md", + "claude-test", + "diff", + "mermaid", + "mods-guide", + "plugin-authoring", + "responsive-mode", + "telemetry", + "tips", + "you-should-know", +]); + /** * A session-scoped settings override that switches OFF the developer's enabled * plugins. Plugins are auto-discovered from user settings and carry skills, * agents and hooks of their own, so they are a second route by which the - * machine's configuration leaks into a measurement. + * machine's configuration leaks into a measurement. Claude Code's own + * built-ins (`BUILTIN_PLUGINS` above) are a third, and are switched off by name. * * `--disable-slash-commands` covers skills and commands; this covers plugins. * Neither is trusted — `environment.mjs` verifies the result from the run's own @@ -243,10 +276,10 @@ export async function prepareArm( */ export async function writeSettingsOverride(sandbox) { const file = path.join(sandbox.root, "claude-settings.json"); - await fsp.writeFile( - file, - JSON.stringify({ enabledPlugins: {} }, null, 2) + "\n", + const enabledPlugins = Object.fromEntries( + BUILTIN_PLUGINS.map((name) => [`${name}@builtin`, false]), ); + await fsp.writeFile(file, JSON.stringify({ enabledPlugins }, null, 2) + "\n"); return file; } diff --git a/packages/evals/test/grading/environment.test.mjs b/packages/evals/test/grading/environment.test.mjs index 52a350f6b..e664399b6 100644 --- a/packages/evals/test/grading/environment.test.mjs +++ b/packages/evals/test/grading/environment.test.mjs @@ -282,7 +282,15 @@ test("a prepared arm carries its own launch options", async () => { assert.equal(arm.settingsPath.startsWith(sandbox.cwd), false); const settings = JSON.parse(await fsp.readFile(arm.settingsPath, "utf8")); - assert.deepEqual(settings.enabledPlugins, {}); + // Every entry switches a plugin OFF; nothing is switched on. + assert.ok(Object.keys(settings.enabledPlugins).length > 0); + assert.ok( + Object.values(settings.enabledPlugins).every((on) => on === false), + ); + // The two built-ins that loaded on a clean CI runner when the override was + // `{}`, which sets nothing and so left every default-on built-in running. + assert.equal(settings.enabledPlugins["agents-md@builtin"], false); + assert.equal(settings.enabledPlugins["plugin-authoring@builtin"], false); // Crucially NOT `hooks: {}` — `--settings` overrides every scope, so that // would switch off the harness's own SessionStart hook and silently turn // arm B into arm A. diff --git a/packages/mcp-core/src/auth/rest-auth-tracing.spec.ts b/packages/mcp-core/src/auth/rest-auth-tracing.spec.ts index ec6f059ba..143671d3f 100644 --- a/packages/mcp-core/src/auth/rest-auth-tracing.spec.ts +++ b/packages/mcp-core/src/auth/rest-auth-tracing.spec.ts @@ -65,9 +65,10 @@ describe('REST auth resolution telemetry (supabase/functions/_shared/api/auth.ts }); it('never routes the token lookup through createTracedClient', () => { - // createTracedClient interpolates `eq()` values into the span name and - // db.query.text. The value here is the token hash, so tracing this query - // that way would publish a credential into telemetry. + // createTracedClient keeps `eq()` values out of the span name and + // db.query.text, and redacts URLs from a failed request's message — but it + // records DB error messages, which can echo a value in another form. The + // value here is the token hash, so this guard stays as defence in depth. expect(executable).not.toContain('createTracedClient'); }); diff --git a/packages/mcp-core/src/mcp-guards/mcp-auth-tracing.spec.ts b/packages/mcp-core/src/mcp-guards/mcp-auth-tracing.spec.ts index d5a62029f..ce0480e79 100644 --- a/packages/mcp-core/src/mcp-guards/mcp-auth-tracing.spec.ts +++ b/packages/mcp-core/src/mcp-guards/mcp-auth-tracing.spec.ts @@ -25,10 +25,13 @@ import path from 'node:path'; * path that matters, so the guard requires the `finally` form. * * 3. THE TOKEN LOOKUP IS NOT TRACED THROUGH `createTracedClient`. That wrapper - * interpolates filter VALUES into the span name and `db.query.text` - * (`buildSql` over `eq()` arguments), and the filter on this query is the - * token hash — the stored credential. This is the security half of the - * guard: it must stay a hand-rolled span over the raw client. + * no longer puts filter VALUES in the span name or `db.query.text` (they + * are `$n` placeholders — see `_shared/telemetry/span-semconv.ts`) and it + * redacts URLs from a failed request's message, but it still records DB + * error messages, which can echo a value in another form. The filter on + * this query is the token hash, the stored credential, so this stays the + * security half of the guard, as defence in depth: a hand-rolled span over + * the raw client. * * 4. THE GOTRUE CALL'S SPAN CARRIES NO CLAIM OR TOKEN DATA. Only a boolean * `db.success` — never the JWT, the resolved user id, or the raw error @@ -105,9 +108,9 @@ describe('MCP auth resolution telemetry (supabase/functions/mcp/auth.ts)', () => }); it('never routes the token lookup through createTracedClient', () => { - // createTracedClient interpolates `eq()` values into the span name and - // db.query.text. The value here is the token hash, so tracing this query - // that way would publish a credential into telemetry. + // createTracedClient keeps `eq()` values out of the span name and + // db.query.text and redacts URLs from error messages, but still records DB + // error text. The value here is the token hash — defence in depth. expect(executable).not.toContain('createTracedClient'); }); diff --git a/packages/mcp-core/src/telemetry/otel-conventions.spec.ts b/packages/mcp-core/src/telemetry/otel-conventions.spec.ts index 82b3573ce..e1ed6e630 100644 --- a/packages/mcp-core/src/telemetry/otel-conventions.spec.ts +++ b/packages/mcp-core/src/telemetry/otel-conventions.spec.ts @@ -115,7 +115,16 @@ describe('edge span-kind assignment (SERVER root / CLIENT db / INTERNAL child)', it('makes every DB query a CLIENT span', () => { // The CLIENT span is the outbound edge to Postgres. expect(otel).toMatch(/SPAN_KIND_CLIENT\)/); - expect(otel).toMatch(/this\.parent\.child\(sql,[\s\S]*?SPAN_KIND_CLIENT\)/); + expect(otel).toMatch(/this\.parent\.child\(summary,[\s\S]*?SPAN_KIND_CLIENT\)/); + }); + + it('names DB spans by the bounded summary, never by the statement', () => { + // The statement used to be the span name, with filter values interpolated: + // 511 distinct api span names in a week. The name must be the + // `querySummary` result; the statement only ever reaches `db.query.text`. + expect(otel).toMatch(/const summary = querySummary\(this\.state\.op, this\.state\.table\)/); + expect(otel).not.toMatch(/this\.parent\.child\(sql,/); + expect(otel).not.toMatch(/this\.parent\.child\(buildSql\(/); }); it('defaults child spans to INTERNAL', () => { diff --git a/packages/mcp-core/src/telemetry/span-semconv.spec.ts b/packages/mcp-core/src/telemetry/span-semconv.spec.ts new file mode 100644 index 000000000..05bb8d580 --- /dev/null +++ b/packages/mcp-core/src/telemetry/span-semconv.spec.ts @@ -0,0 +1,493 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { fileURLToPath, pathToFileURL } from 'node:url'; +import path from 'node:path'; + +/** + * EXECUTING cover for the edge's semantic-convention span fields: + * `_shared/telemetry/span-semconv.ts` (pure) and the `otel.ts` code that uses + * it — DB span naming, parameterised `db.query.text`, `server.address`, + * `error.type`, and the 5xx `error.type` fallback on the root span. + * + * The property that matters is a NEGATIVE one — no filter value reaches the + * span name or an attribute on success, and no request URL (with the values in + * its query string) reaches `error.message` on failure — so the DB tests run a real + * `createTracedClient` chain with recognisable sentinel values and scan the + * whole exported OTLP payload for them. A source-scan could not prove that. + * + * Both edge modules are reached by a DYNAMIC import over a computed path for + * the same NX-boundary reason `otel-detached-child.spec.ts` gives: `supabase/` + * is a separate project, and this is a test reaching across the repo to + * execute files it does not own. + */ + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../../../..'); +const telemetryDir = path.join(repoRoot, 'supabase', 'functions', '_shared', 'telemetry'); +const load = (file: string) => import(pathToFileURL(path.join(telemetryDir, file)).href); + +const semconv = (await load('span-semconv.ts')) as { + querySummary(op: string, target: string): string; + conditionSql(column: string, operator: string, value: unknown, negate?: boolean): string | null; + postgrestLogicToSql(expression: string): string; + renderStatement(s: { + op: string; table: string; columns: string; filters: string[]; orderBy?: string; hasLimit: boolean; returning?: string; + }): string; + textSearchSql(column: string, type?: string): string; + resolveServerAddress(clientUrl: unknown, hostedUrl: unknown): string | undefined; + errorTypeFrom(message: unknown): string; + errorTypeOf(err: unknown): string; + httpRouteFor(functionName: string, routePath: string): string; + redactUrls(message: string): string; +}; + +type Attrs = Record; +interface RecordedSpan { name: string; kind: number; attributes: Attrs; status: 'ok' | 'error' } + +const otel = (await load('otel.ts')) as { + Span: new ( + name: string, + ctx: { traceId: string; spanId: string; sampled: boolean }, + batch: unknown, + kind?: number, + ) => { + error(m: string, t?: string): unknown; + clientError(m: string, t?: string): unknown; + end(): void; + }; + ExportBatch: new () => { drain(): RecordedSpan[] }; + createTracedClient(db: unknown, span: unknown): { + // deno-style fluent surface; typed loosely, the assertions are on output + // eslint-disable-next-line @typescript-eslint/no-explicit-any + from(table: string): any; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + rpc(fn: string, args?: Record): any; + }; + traceRequest(req: Request, op: string, fn: (span: unknown) => Promise): Promise; +}; + +const HOSTED_URL = 'https://pqokxlhvnosogizsjztg.supabase.co'; +const ENDPOINT = 'https://otlp.test.invalid'; + +// ── pure module ───────────────────────────────────────────────────────────── + +describe('querySummary (db.query.summary and the DB span name)', () => { + it('is ` `, and `CALL ` for an RPC', () => { + expect(semconv.querySummary('SELECT', 'memories')).toBe('SELECT memories'); + expect(semconv.querySummary('UPSERT', 'memories')).toBe('UPSERT memories'); + expect(semconv.querySummary('RPC', 'memory_write')).toBe('CALL memory_write'); + }); +}); + +describe('conditionSql', () => { + it('renders every value as a placeholder', () => { + expect(semconv.conditionSql('key', 'eq', 'secret-key')).toBe('key = ?'); + expect(semconv.conditionSql('scope', 'neq', 'x')).toBe('scope <> ?'); + expect(semconv.conditionSql('id', 'in', ['a', 'b'])).toBe('id = ANY(?)'); + expect(semconv.conditionSql('tags', 'cs', '{"a"}')).toBe('tags @> ?'); + expect(semconv.conditionSql('tags', 'ov', ['a'])).toBe('tags && ?'); + }); + + it('renders IS keywords literally — they are bounded, not data', () => { + expect(semconv.conditionSql('archived_at', 'is', null)).toBe('archived_at IS NULL'); + expect(semconv.conditionSql('archived_at', 'is', null, true)).toBe('archived_at IS NOT NULL'); + expect(semconv.conditionSql('flag', 'is', true)).toBe('flag IS TRUE'); + expect(semconv.conditionSql('x', 'is', 'not-a-keyword')).toBe('x IS ?'); + }); + + it('negates non-IS operators, maps full-text operators, and strips modifiers', () => { + expect(semconv.conditionSql('scope', 'eq', 'x', true)).toBe('NOT (scope = ?)'); + expect(semconv.conditionSql('fts', 'wfts(english)', 'q')).toBe('fts @@ websearch_to_tsquery(?)'); + expect(semconv.conditionSql('key', 'like(any)', '{a,b}')).toBe('key LIKE ?'); + }); + + it('returns null for an operator it does not know, so the caller redacts', () => { + expect(semconv.conditionSql('x', 'bogus', 1)).toBeNull(); + }); +}); + +describe('postgrestLogicToSql (the `.or()` argument)', () => { + it('handles the expiry clause every read path uses', () => { + expect(semconv.postgrestLogicToSql('expires_at.is.null,expires_at.gt.now()')) + .toBe('(expires_at IS NULL OR expires_at > ?)'); + }); + + it('drops the keyset-cursor values, nested and() included', () => { + const ts = '2026-10-01T12:34:56.789Z'; + const id = '744e9990-2713-44ac-9e6f-e523ae8bb201'; + const out = semconv.postgrestLogicToSql(`updated_at.lt.${ts},and(updated_at.eq.${ts},id.lt.${id})`); + expect(out).toBe('(updated_at < ? OR (updated_at = ? AND id < ?))'); + expect(out).not.toContain(ts); + expect(out).not.toContain(id); + }); + + it('copes with quoted values, commas and braces inside values', () => { + const out = semconv.postgrestLogicToSql('tags.cs.{"a,b","c}d"},key.in.("x,1","y\\"z"),not.and(scope.eq.repo::o/r,key.is.null)'); + expect(out).toBe('(tags @> ? OR key = ANY(?) OR NOT (scope = ? AND key IS NULL))'); + }); + + it('redacts the whole fragment when it cannot vouch for it', () => { + for (const bad of [ + 'key.eq."unterminated', // unbalanced quote + 'and(key.eq.x', // unclosed group + 'key.bogus.secret', // unknown operator + 'ke y.eq.secret', // not an identifier + 'justavalue', // no operator at all + '', // empty + ]) { + const out = semconv.postgrestLogicToSql(bad); + expect(out, bad).toBe('(?)'); + } + }); +}); + +describe('renderStatement (db.query.text)', () => { + it('numbers placeholders in reading order, LIMIT included', () => { + const sql = semconv.renderStatement({ + op: 'SELECT', + table: 'memories', + columns: 'id,scope,value', + filters: ['key = ?', 'archived_at IS NULL', '(expires_at IS NULL OR expires_at > ?)'], + orderBy: 'updated_at DESC', + hasLimit: true, + }); + expect(sql).toBe( + 'SELECT id,scope,value FROM memories WHERE key = $1 AND archived_at IS NULL AND (expires_at IS NULL OR expires_at > $2) ORDER BY updated_at DESC LIMIT $3', + ); + }); + + it('renders RETURNING for mutations only', () => { + expect(semconv.renderStatement({ op: 'INSERT', table: 'audit_log', columns: 'action, user_id', filters: [], hasLimit: false, returning: 'id' })) + .toBe('INSERT INTO audit_log (action, user_id) RETURNING id'); + expect(semconv.renderStatement({ op: 'RPC', table: 'f', columns: '', filters: [], hasLimit: false, returning: 'id' })) + .toBe('CALL f(...)'); + }); + + it('lists UPDATE columns as assignments and keeps the CALL shape for RPCs', () => { + expect(semconv.renderStatement({ op: 'UPDATE', table: 'memories', columns: 'archived_at, value', filters: ['id = ?'], hasLimit: false })) + .toBe('UPDATE memories SET archived_at = $1, value = $2 WHERE id = $3'); + expect(semconv.renderStatement({ op: 'RPC', table: 'memory_write', columns: '', filters: [], hasLimit: true })) + .toBe('CALL memory_write(...) LIMIT $1'); + }); +}); + +describe('resolveServerAddress', () => { + it('names the hosted project host', () => { + expect(semconv.resolveServerAddress(HOSTED_URL, HOSTED_URL)).toBe('pqokxlhvnosogizsjztg.supabase.co'); + }); + + it('falls back to the hosted URL when the client URL is unreadable', () => { + expect(semconv.resolveServerAddress(undefined, HOSTED_URL)).toBe('pqokxlhvnosogizsjztg.supabase.co'); + }); + + it('never names a BYOD project — a different host is the user’s infrastructure', () => { + expect(semconv.resolveServerAddress('https://someone-else.supabase.co', HOSTED_URL)).toBeUndefined(); + }); + + it('is undefined when nothing is configured', () => { + expect(semconv.resolveServerAddress(undefined, undefined)).toBeUndefined(); + expect(semconv.resolveServerAddress('not a url', 'also not')).toBeUndefined(); + }); +}); + +describe('errorTypeFrom', () => { + it('takes the class / code prefix the edge call sites already write', () => { + expect(semconv.errorTypeFrom('UserInputError: q is required')).toBe('UserInputError'); + expect(semconv.errorTypeFrom('MethodNotFound: resources/list')).toBe('MethodNotFound'); + expect(semconv.errorTypeFrom('missing_token')).toBe('missing_token'); + }); + + it('is _OTHER for an `undefined: …` placeholder (a plain error object has no name)', () => { + expect(semconv.errorTypeFrom('undefined: connection refused')).toBe('_OTHER'); + expect(semconv.errorTypeFrom(undefined)).toBe('_OTHER'); + }); + + it('is _OTHER — never the message — for prose', () => { + expect(semconv.errorTypeFrom('spec generation failed: boom')).toBe('_OTHER'); + expect(semconv.errorTypeFrom('')).toBe('_OTHER'); + expect(semconv.errorTypeFrom('a'.repeat(80) + ': x')).toBe('_OTHER'); + }); +}); + +describe('redactUrls', () => { + it('replaces every URL, query string included, and keeps the rest', () => { + const msg = 'TypeError: error sending request for url (https://x.supabase.co/rest/v1/memories?key=eq.secret-key&scope=eq.repo::a/b): connection refused'; + expect(semconv.redactUrls(msg)).toBe('TypeError: error sending request for url (): connection refused'); + }); + + it('leaves a message without a URL alone', () => { + expect(semconv.redactUrls('duplicate key value violates unique constraint')).toBe('duplicate key value violates unique constraint'); + }); +}); + +describe('errorTypeOf', () => { + it('uses an Error\'s class name', () => { + expect(semconv.errorTypeOf(new TypeError('x'))).toBe('TypeError'); + }); + + it('uses a PostgREST error object\'s code when there is no name (pr-reviewer on #682)', () => { + expect(semconv.errorTypeOf({ message: 'duplicate key', code: '23505' })).toBe('23505'); + }); + + it('is _OTHER for anything else', () => { + expect(semconv.errorTypeOf('boom')).toBe('_OTHER'); + expect(semconv.errorTypeOf(null)).toBe('_OTHER'); + expect(semconv.errorTypeOf({ message: 'no code' })).toBe('_OTHER'); + }); +}); + +describe('httpRouteFor', () => { + it('joins the mount point and the template in url.path shape', () => { + expect(semconv.httpRouteFor('memories', '/:id')).toBe('/memories/:id'); + expect(semconv.httpRouteFor('memories', '/usage/runs')).toBe('/memories/usage/runs'); + expect(semconv.httpRouteFor('memories', '/')).toBe('/memories'); + }); +}); + +// ── otel.ts, executed ─────────────────────────────────────────────────────── + +/** A postgrest-js stand-in: every builder method chains, awaiting resolves. */ +function fakeClient(result: { data: unknown; error: unknown; status?: number }, supabaseUrl?: string) { + const builder: Record = new Proxy({}, { + get(_t, prop) { + if (prop === 'then') { + return (onF: (v: unknown) => unknown, onR: (e: unknown) => unknown) => Promise.resolve(result).then(onF, onR); + } + return () => builder; + }, + }); + return { + ...(supabaseUrl ? { supabaseUrl } : {}), + from: () => builder, + rpc: () => builder, + }; +} + +function newRoot() { + const batch = new otel.ExportBatch(); + const root = new otel.Span('lorekit.test', { traceId: 'a'.repeat(32), spanId: 'b'.repeat(16), sampled: true }, batch, 2); + return { root, batch }; +} + +let hadDeno: boolean; +let env: Record; + +beforeEach(() => { + hadDeno = 'Deno' in globalThis; + env = { SUPABASE_URL: HOSTED_URL }; + (globalThis as Record)['Deno'] = { env: { get: (k: string) => env[k] } }; +}); + +afterEach(() => { + if (!hadDeno) delete (globalThis as Record)['Deno']; +}); + +describe('createTracedClient — no filter value reaches telemetry', () => { + const SECRETS = { + key: 'reviewer-lessons::sentinel-key-7f3a', + userId: '6e05fc90-b04b-4e30-b996-7a6248f13a4e', + search: 'zsh glob sentinel-search-terms', + tag: 'sentinel-tag', + cursorTs: '2026-10-01T12:34:56.789Z', + }; + + it('names the span by summary and parameterises the statement', async () => { + const { root, batch } = newRoot(); + const db = otel.createTracedClient(fakeClient({ data: [{ id: 1 }], error: null }, HOSTED_URL), root); + + await db.from('memories') + .select('id,key,value,scope,tags,updated_at') + .eq('user_id', SECRETS.userId) + .eq('key', SECRETS.key) + .is('archived_at', null) + .or('expires_at.is.null,expires_at.gt.now()') + .or(`updated_at.lt.${SECRETS.cursorTs},and(updated_at.eq.${SECRETS.cursorTs},id.lt.${SECRETS.userId})`) + .textSearch('fts', SECRETS.search, { type: 'websearch', config: 'english' }) + .contains('tags', [SECRETS.tag]) + .in('scope', ['repo::secret/one', 'repo::secret/two']) + .order('updated_at', { ascending: false }) + .limit(37); + + const [span] = batch.drain(); + expect(span.name).toBe('SELECT memories'); + expect(span.kind).toBe(3); // CLIENT + expect(span.attributes['db.query.summary']).toBe('SELECT memories'); + expect(span.attributes['db.collection.name']).toBe('memories'); + expect(span.attributes['db.operation.name']).toBe('SELECT'); + expect(span.attributes['server.address']).toBe('pqokxlhvnosogizsjztg.supabase.co'); + expect(span.attributes['db.query.text']).toBe( + 'SELECT id,key,value,scope,tags,updated_at FROM memories WHERE user_id = $1 AND key = $2 AND archived_at IS NULL' + + ' AND (expires_at IS NULL OR expires_at > $3) AND (updated_at < $4 OR (updated_at = $5 AND id < $6))' + + ' AND fts @@ websearch_to_tsquery($7) AND tags @> $8 AND scope = ANY($9) ORDER BY updated_at DESC LIMIT $10', + ); + + // Name + attributes are everything this span exports that a value could + // ride on (ids and timestamps are excluded: random hex could match). + const exported = JSON.stringify({ name: span.name, attributes: span.attributes }); + for (const [what, value] of Object.entries({ ...SECRETS, scope: 'repo::secret' })) { + expect(exported, `${what} leaked into the DB span`).not.toContain(value); + } + }); + + it('keeps a mutation\'s .select() as RETURNING, not as its SET list', async () => { + // Regression from the PR's own CI smoke trace: `update({ archived_at })` + // followed by `.select('id,scope,key')` rendered `SET id = $1, scope = $2, key = $3`. + const { root, batch } = newRoot(); + const db = otel.createTracedClient(fakeClient({ data: [], error: null }), root); + await db.from('memories').update({ archived_at: 'now' }).eq('id', SECRETS.userId).is('archived_at', null).select('id,scope,key'); + const [span] = batch.drain(); + expect(span.name).toBe('UPDATE memories'); + expect(span.attributes['db.query.text']).toBe( + 'UPDATE memories SET archived_at = $1 WHERE id = $2 AND archived_at IS NULL RETURNING id,scope,key', + ); + }); + + it('names an RPC `CALL `', async () => { + const { root, batch } = newRoot(); + const db = otel.createTracedClient(fakeClient({ data: null, error: null }), root); + await db.rpc('memory_write', { p_key: SECRETS.key }); + const [span] = batch.drain(); + expect(span.name).toBe('CALL memory_write'); + expect(span.attributes['db.query.text']).toBe('CALL memory_write(...)'); + expect(JSON.stringify({ name: span.name, attributes: span.attributes })).not.toContain(SECRETS.key); + }); + + it('omits server.address for a BYOD client', async () => { + const { root, batch } = newRoot(); + const db = otel.createTracedClient(fakeClient({ data: [], error: null }, 'https://byod-project.supabase.co'), root); + await db.from('memories').select('id'); + const [span] = batch.drain(); + // Array form: a dotted string would be read as a nested path and pass vacuously. + expect(span.attributes).not.toHaveProperty(['server.address']); + expect(JSON.stringify(span.attributes)).not.toContain('byod-project'); + }); + + it('names a resolved fetch failure (status 0) instead of copying its URL-bearing message', async () => { + // postgrest-js RESOLVES a failed fetch — it does not reject — with the + // request URL, filter values included, in the message (pr-reviewer on #682). + const { root, batch } = newRoot(); + const url = `https://pqokxlhvnosogizsjztg.supabase.co/rest/v1/memories?key=eq.${encodeURIComponent(SECRETS.key)}`; + const db = otel.createTracedClient( + fakeClient({ data: null, error: { message: `TypeError: error sending request for url (${url})`, code: '' }, status: 0 }), + root, + ); + await db.from('memories').select('id').eq('key', SECRETS.key); + const [span] = batch.drain(); + expect(span.status).toBe('error'); + expect(span.attributes['error.message']).toBe('PostgrestError: fetch failed'); + expect(JSON.stringify(span.attributes)).not.toContain('sentinel-key'); + }); + + it('redacts URLs from a thrown error on the rejection arm', async () => { + const { root, batch } = newRoot(); + const rejecting = { + from: () => new Proxy({}, { + get(_t, prop) { + if (prop === 'then') { + return (_f: unknown, onR: (e: unknown) => unknown) => + Promise.reject(new TypeError(`error sending request for url (https://h.test/rest/v1/m?key=eq.${SECRETS.key})`)).catch(onR); + } + return () => rejecting.from(); + }, + }), + }; + const db = otel.createTracedClient(rejecting, root); + await db.from('memories').select('id').eq('key', SECRETS.key).then(undefined, () => undefined); + const [span] = batch.drain(); + expect(span.attributes['error.message']).toBe('TypeError: error sending request for url ()'); + }); + + it('records the SQLSTATE as db.response.status_code and error.type', async () => { + const { root, batch } = newRoot(); + const db = otel.createTracedClient( + fakeClient({ data: null, error: { message: 'connection terminated unexpectedly', code: '08006' } }), + root, + ); + await db.rpc('memory_delete', {}); + const [span] = batch.drain(); + expect(span.status).toBe('error'); + expect(span.attributes['db.response.status_code']).toBe('08006'); + expect(span.attributes['error.type']).toBe('08006'); + }); +}); + +describe('Span.error / clientError — error.type', () => { + it('derives the class from the message prefix, or takes an explicit type', () => { + const { root, batch } = newRoot(); + root.clientError('UserInputError: q is required'); + root.end(); + const derived = new otel.Span('x', { traceId: 'a'.repeat(32), spanId: 'c'.repeat(16), sampled: true }, batch); + derived.error('Unhandled: boom', 'TypeError'); + derived.end(); + + const [client, server] = batch.drain(); + expect(client.attributes['error.type']).toBe('UserInputError'); + expect(client.status).toBe('ok'); // a client error does not flip the status + expect(server.attributes['error.type']).toBe('TypeError'); + expect(server.status).toBe('error'); + }); +}); + +describe('Span.error — every span redacts URLs and never throws', () => { + it('redacts a request URL from a handler span\'s message (pr-reviewer on #682)', () => { + const { root, batch } = newRoot(); + root.error('Unhandled: TypeError: error sending request for url (https://h.test/rest/v1/memories?fts=wfts.secret%20search)', '_OTHER'); + root.end(); + const [span] = batch.drain(); + expect(span.attributes['error.message']).toBe('Unhandled: TypeError: error sending request for url ()'); + expect(JSON.stringify(span)).not.toContain('secret'); + }); + + it('tolerates an undefined message — callers pass `e.name`, which can be undefined', () => { + const { root, batch } = newRoot(); + expect(() => root.error(undefined as unknown as string)).not.toThrow(); + expect(() => root.clientError(undefined as unknown as string)).not.toThrow(); + root.end(); + expect(batch.drain()[0].attributes['error.type']).toBe('_OTHER'); + }); +}); + +describe('traceRequest — 5xx error.type fallback', () => { + let realFetch: typeof globalThis.fetch; + let posted: string[]; + + beforeEach(() => { + posted = []; + realFetch = globalThis.fetch; + env['OTEL_EXPORTER_OTLP_ENDPOINT'] = ENDPOINT; + globalThis.fetch = ((_url: string, init?: { body?: string }) => { + posted.push(String(init?.body ?? '')); + return Promise.resolve({ ok: true, status: 200 }); + }) as unknown as typeof globalThis.fetch; + }); + + afterEach(() => { + globalThis.fetch = realFetch; + }); + + async function rootAttrs(status: number, before?: (span: { error(m: string, t?: string): unknown }) => void) { + await otel.traceRequest(new Request('https://edge.test/memories'), 'lorekit.memories', async (span) => { + before?.(span as { error(m: string, t?: string): unknown }); + return new Response(null, { status }); + }); + await Promise.resolve(); + const body = JSON.parse(posted.at(-1) as string); + const span = body.resourceSpans[0].scopeSpans[0].spans[0]; + return Object.fromEntries( + (span.attributes as Array<{ key: string; value: Record }>) + .map((a) => [a.key, Object.values(a.value)[0]]), + ); + } + + it('records the status code as error.type on an unexplained 5xx', async () => { + expect((await rootAttrs(503))['error.type']).toBe('503'); + }); + + it('keeps the class a handler already named', async () => { + const attrs = await rootAttrs(500, (span) => span.error('Unhandled: boom', 'TypeError')); + expect(attrs['error.type']).toBe('TypeError'); + }); + + it('sets nothing on a success or a 4xx', async () => { + expect(await rootAttrs(200)).not.toHaveProperty(['error.type']); + expect(await rootAttrs(404)).not.toHaveProperty(['error.type']); + }); +}); diff --git a/scripts/telemetry/emit-correlated-trace.mts b/scripts/telemetry/emit-correlated-trace.mts index 2d889a214..a7d0b0ed1 100644 --- a/scripts/telemetry/emit-correlated-trace.mts +++ b/scripts/telemetry/emit-correlated-trace.mts @@ -140,15 +140,21 @@ export function buildCorrelatedTrace( apiServer1.setAttributes({ 'http.request.method': 'POST', 'url.path': '/memories', + 'http.route': '/memories', 'faas.name': 'memories', 'lorekit.tool.name': 'memory.write', 'http.response.status_code': 200, }); apiServer1 - .child('INSERT INTO memories (scope, key, value)', { + // Named by db.query.summary, values as placeholders — the shape + // `createTracedClient` emits (see _shared/telemetry/span-semconv.ts). + .child('INSERT memories', { 'db.system': 'postgresql', 'db.operation.name': 'INSERT', 'db.collection.name': 'memories', + 'db.query.summary': 'INSERT memories', + 'db.query.text': 'INSERT INTO memories (scope, key, value)', + 'server.address': 'pqokxlhvnosogizsjztg.supabase.co', }, SPAN_KIND_CLIENT) .end(); apiServer1.end(); @@ -167,15 +173,19 @@ export function buildCorrelatedTrace( apiServer2.setAttributes({ 'http.request.method': 'GET', 'url.path': '/memories', + 'http.route': '/memories', 'faas.name': 'memories', 'lorekit.tool.name': 'memory.read', 'http.response.status_code': 200, }); apiServer2 - .child('SELECT scope, key, value FROM memories', { + .child('SELECT memories', { 'db.system': 'postgresql', 'db.operation.name': 'SELECT', 'db.collection.name': 'memories', + 'db.query.summary': 'SELECT memories', + 'db.query.text': 'SELECT scope, key, value FROM memories WHERE scope = $1 LIMIT $2', + 'server.address': 'pqokxlhvnosogizsjztg.supabase.co', }, SPAN_KIND_CLIENT) .end(); apiServer2.end(); diff --git a/supabase/functions/CLAUDE.md b/supabase/functions/CLAUDE.md index a2d0b31f7..105a9ed82 100644 --- a/supabase/functions/CLAUDE.md +++ b/supabase/functions/CLAUDE.md @@ -33,7 +33,20 @@ All Zod schemas live in `packages/schemas/` (@lorekit/schemas). Import via the i Every function calls `traceRequest(req, 'lorekit.{resource}', ...)` as the root span. Auth resolution creates a `lorekit.rest.auth` child span automatically. Router creates a child span per handler call named `lorekit.{function}.{method}.{path}`. -DB operations get child spans automatically via `createTracedClient`. +DB operations get child spans automatically via `createTracedClient`, named by +`db.query.summary` (`SELECT memories`, `CALL memory_write`) with a +parameterised `db.query.text` — filter values never reach the span name or the +statement, and URLs are redacted from a failure's `error.message` +(`_shared/telemetry/span-semconv.ts`). Postgres's own error wording can still +echo a value, so keep credential lookups off the traced client (see +docs/otel.md → "Error messages"). +Set `http.route` on the root span: `createRouter` does it for routed functions; +a single-route function does it at the top of its `traceRequest` callback. +`span.error()` / `span.clientError()` record a bounded `error.type` from the +message's `Name:` prefix — keep that prefix, or pass the type explicitly. In a +catch block pass `errorTypeOf(e)` (`_shared/telemetry/span-semconv.ts`), never +`(e as Error).name`: a PostgREST error is a plain object with no `name`, and +`errorTypeOf` falls back to its `code`. Add `span.child('lorekit.{resource}.{operation}')` for any significant sub-operation. **Self-time attribution is automatic.** `traceRequest` stamps diff --git a/supabase/functions/_shared/api/CLAUDE.md b/supabase/functions/_shared/api/CLAUDE.md index c962915cf..17cbe8a0b 100644 --- a/supabase/functions/_shared/api/CLAUDE.md +++ b/supabase/functions/_shared/api/CLAUDE.md @@ -292,7 +292,7 @@ Deno.serve(async (req) => { if (e instanceof RestError) return e.toResponse(cors); const mapped = translateDbError(e); if (mapped) return mapped.toResponse(cors); - span.error(`Unhandled: ${(e as Error).message}`); + span.error(`Unhandled: ${(e as Error).message}`, errorTypeOf(e)); return internalError(cors); } }); diff --git a/supabase/functions/_shared/api/auth.ts b/supabase/functions/_shared/api/auth.ts index 68af77728..823e033e3 100644 --- a/supabase/functions/_shared/api/auth.ts +++ b/supabase/functions/_shared/api/auth.ts @@ -72,11 +72,13 @@ export async function resolveRestAuth(req: Request, parentSpan: Span): Promise = { data: T | null; error: PostgrestError | null; count?: number | null; + /** HTTP status; `0` when the request never got a response (postgrest-js resolves a failed fetch, it does not reject). */ + status?: number; }; // ── Supabase Edge Runtime global ────────────────────────────────────────────── @@ -487,10 +504,26 @@ export class Span { return this; } - error(message: string): this { + /** + * Mark the span ERROR (a server-side fault). + * + * `type` becomes `error.type` — the bounded grouping key the OTel + * conventions pair with a failure, so "how many requests failed, by class" + * is a `sum by (error_type)` rather than a regex over the free-text + * `error.message`. Omit it and it is derived from the message's + * `ClassName: …` prefix (`errorTypeFrom`), which every call site already + * writes; a message without one records `_OTHER`, never the message. + */ + error(message: string, type?: string): this { + // Every span, not only DB spans: a handler or root span that records a + // failed request's message would otherwise carry its URL — and the filter + // values in its query string. Coerced, because callers pass `e.name` and + // the like, which can be undefined; a throw here would break the request. + const text = redactUrls(String(message ?? '')); this.status = 'error'; - this.statusMessage = message; - this.attributes['error.message'] = message; + this.statusMessage = text; + this.attributes['error.message'] = text; + this.attributes['error.type'] = type || errorTypeFrom(text); return this; } @@ -505,8 +538,23 @@ export class Span { * conventions, server spans should only carry status=ERROR for server-side * faults, not for client errors. */ - clientError(message: string): this { - this.attributes['error.message'] = message; + clientError(message: string, type?: string): this { + const text = redactUrls(String(message ?? '')); + this.attributes['error.message'] = text; + // Same bounded key as `error()`, so client errors are countable by class + // (`UserInputError`, `MethodNotFound`, `missing_token`) without the span + // status changing — the request was still handled correctly. + this.attributes['error.type'] = type || errorTypeFrom(text); + return this; + } + + /** + * Set `error.type` only when nothing more specific has been recorded. + * Used for the HTTP-status fallback on a 5xx response, which must never + * overwrite the class a handler already named. + */ + setErrorTypeIfAbsent(type: string): this { + if (!('error.type' in this.attributes)) this.attributes['error.type'] = type; return this; } @@ -631,6 +679,18 @@ function withTraceparent(response: T, ctx: TraceContext): T * }); * }); * ``` + * + * `http.route` is NOT set here: only the caller knows which route matched. + * `createRouter` stamps it on this span once a route matches, and each + * single-route function sets it at the top of its callback. + * + * NOTE: it renames the Dash0 operation. With `http.route` present Dash0 names + * the operation `{method} {route}` (`POST /mcp`, `GET /memories/:id`) instead + * of by `faas.name` (`mcp`, `memories`) — observed on this change's own CI + * smoke trace. Alerts and panels that must address a whole function filter on + * `faas_name`, which is stable across both namings; `dash0_operation_name` is + * now per endpoint. A request that matches no route keeps the `faas.name` + * operation. */ export async function traceRequest( req: Request, @@ -658,9 +718,13 @@ export async function traceRequest( try { response = await fn(span); span.setAttributes({ 'http.response.status_code': response.status }); + // OTel HTTP server conventions: a 5xx with no more specific cause records + // the status code as `error.type`. A handler that already named the class + // (`span.error('Unhandled: …', errorTypeOf(e))`) keeps it. + if (response.status >= 500) span.setErrorTypeIfAbsent(String(response.status)); return withTraceparent(response, ctx); } catch (err) { - span.error(`${(err as Error).name}: ${(err as Error).message}`); + span.error(`${(err as Error).name}: ${(err as Error).message}`, errorTypeOf(err)); throw err; } finally { // Stamped BEFORE end() — attributes set after a span is added to the batch @@ -724,15 +788,30 @@ function isExpectedDbError(error: { code?: string; message?: string }): boolean return typeof error.message === 'string' && error.message.includes('unknown_org'); } -type Op = 'SELECT' | 'INSERT' | 'UPDATE' | 'DELETE' | 'UPSERT' | 'RPC'; +type Op = DbOperation; interface QueryState { table: string; op: Op; columns: string; + /** + * WHERE fragments with `?` placeholders — NEVER the filter values. Built by + * `span-semconv.ts` and numbered `$1…$n` by `renderStatement`. See that + * module's header for why values must stay out of span names and + * `db.query.text`. + */ filters: string[]; orderBy?: string; lim?: number; + /** `server.address` for the DB span, resolved once per traced client. */ + serverAddress?: string; + /** + * A mutation's `.select(...)` — the RETURNING list. Kept apart from + * `columns` so `UPDATE … .select('id,scope,key')` renders as + * `SET archived_at = $1 … RETURNING id,scope,key`, not as a SET list of the + * returned columns (which the CI smoke trace showed it doing). + */ + returning?: string; /** * The postgrest builder being accumulated. Deliberately untyped, because the * real type CHANGES ALONG THE CHAIN and no single annotation describes it: @@ -756,25 +835,33 @@ interface QueryState { qb: any; } +/** The parameterised `db.query.text` for a query — placeholders, never values. */ function buildSql(s: QueryState): string { - const parts: string[] = []; - switch (s.op) { - case 'SELECT': parts.push(`SELECT ${s.columns || '*'} FROM ${s.table}`); break; - case 'INSERT': parts.push(`INSERT INTO ${s.table}${s.columns ? ` (${s.columns})` : ''}`); break; - case 'UPDATE': parts.push(`UPDATE ${s.table} SET ...`); break; - case 'DELETE': parts.push(`DELETE FROM ${s.table}`); break; - case 'UPSERT': parts.push(`UPSERT INTO ${s.table}${s.columns ? ` (${s.columns})` : ''}`); break; - case 'RPC': parts.push(`CALL ${s.table}(...)`); break; + return renderStatement({ + op: s.op, + table: s.table, + columns: s.columns, + filters: s.filters, + orderBy: s.orderBy, + hasLimit: s.lim !== undefined, + returning: s.returning, + }); +} + +/** Read an env var without throwing where `Deno` (or env permission) is absent. */ +function readEnv(name: string): string | undefined { + try { + return typeof Deno === 'undefined' ? undefined : Deno.env.get(name); + } catch { + return undefined; } - if (s.filters.length) parts.push(`WHERE ${s.filters.join(' AND ')}`); - if (s.orderBy) parts.push(`ORDER BY ${s.orderBy}`); - if (s.lim !== undefined) parts.push(`LIMIT ${s.lim}`); - return parts.join(' '); } /** * Fluent traced query builder — mirrors the Supabase query builder API - * but wraps execution in a child span named after the SQL-like statement. + * but wraps execution in a CLIENT child span named by `db.query.summary` + * (`SELECT memories`, `CALL memory_write`) with the parameterised statement in + * `db.query.text`. */ /** * A query narrowed to at most one row by `.single()` / `.maybeSingle()`. @@ -819,20 +906,30 @@ export class TracedQuery> { // ── column selection ────────────────────────────────────────────────────── select(cols = '*', opts?: { head?: boolean; count?: 'exact' | 'planned' | 'estimated' }): this { - this.state.columns = cols; + // On a mutation, `.select()` is the RETURNING list, not the column list. + if (this.state.op === 'SELECT') this.state.columns = cols; + else this.state.returning = cols; this.state.qb = this.state.qb.select(cols, opts); return this; } // ── filters ─────────────────────────────────────────────────────────────── - eq(col: string, val: unknown): this { this.state.filters.push(`${col} = '${val}'`); this.state.qb = this.state.qb.eq(col, val); return this; } - neq(col: string, val: unknown): this { this.state.filters.push(`${col} != '${val}'`); this.state.qb = this.state.qb.neq(col, val); return this; } - gt(col: string, val: unknown): this { this.state.filters.push(`${col} > '${val}'`); this.state.qb = this.state.qb.gt(col, val); return this; } - gte(col: string, val: unknown): this { this.state.filters.push(`${col} >= '${val}'`); this.state.qb = this.state.qb.gte(col, val); return this; } - lt(col: string, val: unknown): this { this.state.filters.push(`${col} < '${val}'`); this.state.qb = this.state.qb.lt(col, val); return this; } - lte(col: string, val: unknown): this { this.state.filters.push(`${col} <= '${val}'`); this.state.qb = this.state.qb.lte(col, val); return this; } - is(col: string, val: unknown): this { this.state.filters.push(`${col} IS ${val}`); this.state.qb = this.state.qb.is(col, val); return this; } - in(col: string, vals: V[]): this { this.state.filters.push(`${col} IN (${vals.map((v) => `'${v}'`).join(', ')})`); this.state.qb = this.state.qb.in(col, vals); return this; } + // + // Each filter records a fragment with a PLACEHOLDER for its value — the + // value itself goes to postgrest only. `conditionSql` returns null solely + // for an operator it does not know; every operator below is known, so the + // `?? '?'` is a redaction backstop, not a path that runs. + private where(col: string, op: string, val: unknown, negate = false): void { + this.state.filters.push(conditionSql(col, op, val, negate) ?? '?'); + } + eq(col: string, val: unknown): this { this.where(col, 'eq', val); this.state.qb = this.state.qb.eq(col, val); return this; } + neq(col: string, val: unknown): this { this.where(col, 'neq', val); this.state.qb = this.state.qb.neq(col, val); return this; } + gt(col: string, val: unknown): this { this.where(col, 'gt', val); this.state.qb = this.state.qb.gt(col, val); return this; } + gte(col: string, val: unknown): this { this.where(col, 'gte', val); this.state.qb = this.state.qb.gte(col, val); return this; } + lt(col: string, val: unknown): this { this.where(col, 'lt', val); this.state.qb = this.state.qb.lt(col, val); return this; } + lte(col: string, val: unknown): this { this.where(col, 'lte', val); this.state.qb = this.state.qb.lte(col, val); return this; } + is(col: string, val: unknown): this { this.where(col, 'is', val); this.state.qb = this.state.qb.is(col, val); return this; } + in(col: string, vals: V[]): this { this.where(col, 'in', vals); this.state.qb = this.state.qb.in(col, vals); return this; } // `val` may be a STRING array literal (`{"a","b,c"}`) as well as an array. // postgrest-js forwards a string verbatim as `ov.` / `cs.` // (PostgrestFilterBuilder 2.110.8) — that overload is what lets @@ -841,33 +938,34 @@ export class TracedQuery> { // comma/brace/quote into several labels, so do NOT narrow these back to // `V[]`: `memories.tags` is free text with no CHECK constraint. overlaps(col: string, val: V[] | string): this { - this.state.filters.push(`${col} && '${typeof val === 'string' ? val : `{${val.join(',')}}`}'`); + this.where(col, 'ov', val); // deno-lint-ignore no-explicit-any -- the string|array overload isn't in the narrowed public type this.state.qb = (this.state.qb as any).overlaps(col, val); return this; } contains(col: string, val: V[] | string | Record): this { - const rendered = typeof val === 'string' - ? val - : Array.isArray(val) ? `{${val.join(',')}}` : JSON.stringify(val); - this.state.filters.push(`${col} @> '${rendered}'`); + this.where(col, 'cs', val); // deno-lint-ignore no-explicit-any -- PostgREST builder's .contains() overloads vary by version this.state.qb = (this.state.qb as any).contains(col, val); return this; } textSearch(col: string, query: string, opts?: { type?: string; config?: string }): this { - this.state.filters.push(`${col} @@ to_tsquery('${query}')`); + // The search text is the caller's — often an agent's own error output via + // the CLI hook — so it is the one value most worth keeping out. + this.state.filters.push(textSearchSql(col, opts?.type)); // deno-lint-ignore no-explicit-any -- PostgREST builder lacks textSearch overload in its public type this.state.qb = (this.state.qb as any).textSearch(col, query, opts); return this; } or(filters: string, opts?: { referencedTable?: string }): this { - this.state.filters.push(`(${filters})`); + // A PostgREST logic string carries values inline (keyset cursors, label + // filters); parsed down to columns + operators, or `(?)` if unparseable. + this.state.filters.push(postgrestLogicToSql(filters)); this.state.qb = this.state.qb.or(filters, opts); return this; } not(col: string, operator: string, val: unknown): this { - this.state.filters.push(`${col} NOT ${operator} '${val}'`); + this.where(col, operator, val, true); // deno-lint-ignore no-explicit-any -- PostgREST builder's .not() types vary by version this.state.qb = (this.state.qb as any).not(col, operator, val); return this; @@ -952,12 +1050,20 @@ export class TracedQuery> { resolve?: ((v: PostgrestResponse) => R1 | PromiseLike) | null, reject?: ((r: unknown) => R2 | PromiseLike) | null, ): Promise { - const sql = buildSql(this.state); - const dbSpan = this.parent.child(sql, { + // Named by the bounded summary, NOT the statement: a span name is a + // grouping key, and the statement used to carry filter values (511 + // distinct api span names in a week). `db.query.text` keeps the full + // shape, parameterised. + const summary = querySummary(this.state.op, this.state.table); + const dbSpan = this.parent.child(summary, { 'db.system': 'postgresql', 'db.operation.name': this.state.op, 'db.collection.name': this.state.table, - 'db.query.text': sql, + 'db.query.summary': summary, + 'db.query.text': buildSql(this.state), + // The peer, so the service map draws api → Supabase. Omitted for a BYOD + // client — see `resolveServerAddress`. + ...(this.state.serverAddress ? { 'server.address': this.state.serverAddress } : {}), }, SPAN_KIND_CLIENT); // deno-lint-ignore no-explicit-any -- PostgREST builder is awaited as opaque; result is cast below @@ -969,6 +1075,16 @@ export class TracedQuery> { dbSpan.setAttributes({ 'db.response.rows': rows, 'db.success': !result.error }); if (result.error) { + // The SQLSTATE / PostgREST code is OTel's `db.response.status_code` + // and the most specific bounded `error.type` a DB failure has — + // `23505`, `LK001`, `P0001` say more than `PostgrestError` does. + const code = result.error.code || undefined; + if (code) dbSpan.setAttributes({ 'db.response.status_code': code }); + // postgrest-js RESOLVES a failed fetch as `status: 0`, with the + // request URL — every filter value in its query string — in the + // message. Name the failure instead of copying it; any other message + // keeps its text with URLs redacted. + const detail = result.status === 0 ? 'fetch failed' : redactUrls(result.error.message); if (result.error.code === 'PGRST116') { // .single() no rows — expected, not an error dbSpan.setAttributes({ 'db.no_rows': true }); @@ -982,9 +1098,9 @@ export class TracedQuery> { // input. Marking these ERROR inflated error-rate alerts with // requests the API was handling correctly (e.g. DELETE on a // nonexistent org resolving to a clean 404). - dbSpan.clientError(`PostgrestError: ${result.error.message}`); + dbSpan.clientError(`PostgrestError: ${detail}`, code); } else { - dbSpan.error(`PostgrestError: ${result.error.message}`); + dbSpan.error(`PostgrestError: ${detail}`, code); } } @@ -992,7 +1108,7 @@ export class TracedQuery> { return resolve ? resolve(result) : result as unknown as R1; }, (err: unknown) => { - dbSpan.error(`${(err as Error).name}: ${(err as Error).message}`); + dbSpan.error(`${(err as Error).name}: ${redactUrls(String((err as Error)?.message))}`); dbSpan.end(); return reject ? reject(err) : Promise.reject(err) as Promise; }, @@ -1001,8 +1117,9 @@ export class TracedQuery> { } /** - * Wrap a Supabase client so every `.from()` call returns a TracedQuery - * that auto-spans with the SQL-like statement name. + * Wrap a Supabase client so every `.from()` / `.rpc()` call returns a + * TracedQuery that auto-spans as a CLIENT span named by `db.query.summary` + * (`SELECT memories`), with the parameterised statement in `db.query.text`. * * @example * ```ts @@ -1012,7 +1129,8 @@ export class TracedQuery> { * .select('key,value') * .eq('scope', scope) * .limit(50); - * // → child span: "SELECT key,value FROM memories WHERE scope = '...' LIMIT 50" + * // → CLIENT span "SELECT memories" + * // db.query.text = "SELECT key,value FROM memories WHERE scope = $1 LIMIT $2" * ``` */ /** @@ -1042,17 +1160,23 @@ type PublicTables = Database['public']['Tables']; type RowOf = PublicTables[K]['Row']; export function createTracedClient(supabase: DbClient, parentSpan: Span) { + // `supabaseUrl` is a protected field on SupabaseClient — read defensively, + // since a test double will not carry it (then the hosted URL is used). + const serverAddress = resolveServerAddress( + (supabase as unknown as { supabaseUrl?: unknown }).supabaseUrl, + readEnv('SUPABASE_URL'), + ); return { from>(table: K): TracedQuery { return new TracedQuery( - { table: table as string, op: 'SELECT', columns: '*', filters: [], qb: supabase.from(table) }, + { table: table as string, op: 'SELECT', columns: '*', filters: [], serverAddress, qb: supabase.from(table) }, parentSpan, ); }, rpc[]>(fn: string, args?: Record, opts?: Record): TracedRpcQuery { return new TracedQuery( // deno-lint-ignore no-explicit-any -- SupabaseClient.rpc() generic overload isn't publicly typed; cast is safe - { table: fn, op: 'RPC', columns: '', filters: [], qb: (supabase as any).rpc(fn, args, opts) }, + { table: fn, op: 'RPC', columns: '', filters: [], serverAddress, qb: (supabase as any).rpc(fn, args, opts) }, parentSpan, // Same object, narrower promise: `TracedQuery.then` yields `T[]`, and an // RPC yields `T`. Only the result shape differs, which the class's own diff --git a/supabase/functions/_shared/telemetry/span-semconv.ts b/supabase/functions/_shared/telemetry/span-semconv.ts new file mode 100644 index 000000000..bb8ded364 --- /dev/null +++ b/supabase/functions/_shared/telemetry/span-semconv.ts @@ -0,0 +1,403 @@ +// Pure, import-free helpers that turn what an edge request did into +// semantic-convention span fields WITHOUT copying caller data into them. +// +// WHY THIS EXISTS +// `TracedQuery` used to interpolate every filter VALUE into both the DB span's +// name and its `db.query.text`: +// +// SELECT id,scope,value FROM memories WHERE key = 'reviewer-lessons::…' … +// SELECT … FROM memories WHERE fts @@ to_tsquery('zsh glob no matches …') +// SELECT plan_name FROM user_plans WHERE user_id = '6e05fc90-…' LIMIT 1 +// +// That made the `api` service emit 511 distinct span names in one week (287 of +// them carrying full-text search terms, 133 memory keys, 4 user UUIDs). A span +// name is a GROUPING key, so every one of those is a separate operation in the +// trace explorer; and `db.query.text` is defined by the OTel database +// conventions as the parameterised / sanitised statement, not the literal one. +// The search terms are the sharpest case: the CLI hook distils them from an +// agent's tool-failure text and deliberately keeps that text out of its OWN +// telemetry, and the server then wrote it into span names anyway. +// +// So the rule here is: identifiers that come from OUR code (table, columns, +// operators, function names) are rendered; every value is a `$n` placeholder. +// When a filter string cannot be parsed, the whole fragment collapses to a +// placeholder — the failure mode is "less detail", never "leaked a value". +// +// Import-free on purpose, so the mcp-core vitest suite can load and execute it +// directly (see `packages/mcp-core/src/telemetry/span-semconv.spec.ts`), the +// same cross-tree pattern `otel-detached-child.spec.ts` uses for `otel.ts`. + +export type DbOperation = 'SELECT' | 'INSERT' | 'UPDATE' | 'DELETE' | 'UPSERT' | 'RPC'; + +/** + * The placeholder a filter fragment carries for a value. Numbered to `$1…$n` + * only when the whole statement is rendered, so fragments can be built in any + * order. Safe as a sentinel because no fragment ever contains caller text — + * only identifiers and SQL operators, none of which use `?`. + */ +const PARAM = '?'; + +/** A plain SQL identifier — the only shape a column name may take in output. */ +const IDENTIFIER = /^[A-Za-z_][A-Za-z0-9_]*$/; + +/** + * Bounded, low-cardinality summary of a DB call: `{operation} {target}`. + * + * This is OTel's `db.query.summary` and, per the conventions, the DB span's + * NAME. A stored-procedure call reads `CALL ` (the vocabulary the + * previous `CALL fn(...)` names already used), every other operation reads + * `
`. + */ +export function querySummary(op: DbOperation, target: string): string { + return op === 'RPC' ? `CALL ${target}` : `${op} ${target}`; +} + +const COMPARISON_SQL: Record = { + eq: '=', + neq: '<>', + gt: '>', + gte: '>=', + lt: '<', + lte: '<=', + like: 'LIKE', + ilike: 'ILIKE', + match: '~', + imatch: '~*', + isdistinct: 'IS DISTINCT FROM', + cs: '@>', + cd: '<@', + ov: '&&', + sl: '<<', + sr: '>>', + nxl: '&<', + nxr: '&>', + adj: '-|-', +}; + +const TEXT_SEARCH_FN: Record = { + fts: 'to_tsquery', + plfts: 'plainto_tsquery', + phfts: 'phraseto_tsquery', + wfts: 'websearch_to_tsquery', +}; + +/** The keyword-like values `IS` accepts. Bounded, so rendering them leaks nothing. */ +const IS_LITERALS = new Set(['null', 'true', 'false', 'unknown']); + +/** + * The tsquery constructor a postgrest-js `textSearch(…, { type })` call maps + * to. Mirrors postgrest-js: no type is plain `to_tsquery`. + */ +export function textSearchFunction(type?: string): string { + if (type === 'plain') return 'plainto_tsquery'; + if (type === 'phrase') return 'phraseto_tsquery'; + if (type === 'websearch') return 'websearch_to_tsquery'; + return 'to_tsquery'; +} + +/** `col @@ (?)` for a full-text filter. */ +export function textSearchSql(column: string, type?: string): string { + return `${column} @@ ${textSearchFunction(type)}(${PARAM})`; +} + +/** + * Render ONE filter as SQL with a placeholder for its value. + * + * `operator` is a PostgREST operator (`eq`, `in`, `is`, `cs`, `fts(english)`, + * `like(any)`, …). `value` is inspected for exactly one purpose — whether an + * `is` filter compares against a keyword (`null`/`true`/`false`/`unknown`), + * which is rendered literally because it is a bounded keyword, not data. + * + * Returns `null` for an operator it does not know, so the caller can redact + * rather than guess. + */ +export function conditionSql(column: string, operator: string, value: unknown, negate = false): string | null { + // `fts(english)` / `like(any)` carry a modifier in parentheses; it changes + // HOW the value is matched, not what the operator is. + const op = operator.replace(/\(.*\)$/, '').toLowerCase(); + + if (op === 'is') { + const keyword = String(value).toLowerCase(); + const rhs = IS_LITERALS.has(keyword) ? keyword.toUpperCase() : PARAM; + return `${column} IS ${negate ? 'NOT ' : ''}${rhs}`; + } + + let body: string | null = null; + if (op === 'in') body = `${column} = ANY(${PARAM})`; + else if (op in TEXT_SEARCH_FN) body = `${column} @@ ${TEXT_SEARCH_FN[op]}(${PARAM})`; + else if (op in COMPARISON_SQL) body = `${column} ${COMPARISON_SQL[op]} ${PARAM}`; + if (body === null) return null; + + return negate ? `NOT (${body})` : body; +} + +// ── PostgREST logic-tree strings (`.or()`) ───────────────────────────────── + +class LogicParseError extends Error {} + +/** + * Sanitise a PostgREST logic-tree filter string — the argument postgrest-js + * `.or()` takes, e.g. + * + * expires_at.is.null,expires_at.gt.now() + * updated_at.lt.2026-10-01T…,and(updated_at.eq.2026-10-01T…,id.lt.4f1c…) + * tags.cs.{"a,b"},key.in.("x","y") + * + * into a parenthesised SQL fragment with every value replaced by a placeholder: + * + * (expires_at IS NULL OR expires_at > ?) + * + * The top level is OR-joined, because that is what `.or()` means. TOTAL: any + * input the parser does not understand — an unknown operator, a column that is + * not a plain identifier, unbalanced quoting — returns `(?)`. The point of the + * parse is to keep values OUT; a fragment it cannot vouch for is dropped whole. + */ +export function postgrestLogicToSql(expression: string): string { + try { + const parser = new LogicParser(expression); + const parts = parser.conditions(); + if (!parser.done()) throw new LogicParseError('trailing input'); + return `(${parts.join(' OR ')})`; + } catch { + return `(${PARAM})`; + } +} + +class LogicParser { + private i = 0; + constructor(private readonly s: string) {} + + done(): boolean { + return this.i >= this.s.length; + } + + /** A comma-separated list of conditions, up to the end or a closing `)`. */ + conditions(): string[] { + const out = [this.condition()]; + while (this.s[this.i] === ',') { + this.i++; + out.push(this.condition()); + } + return out; + } + + private condition(): string { + const group = /^(not\.)?(and|or)\(/.exec(this.s.slice(this.i)); + if (group) { + this.i += group[0].length; + const inner = this.conditions(); + if (this.s[this.i] !== ')') throw new LogicParseError('unclosed group'); + this.i++; + const joined = `(${inner.join(group[2] === 'and' ? ' AND ' : ' OR ')})`; + return group[1] ? `NOT ${joined}` : joined; + } + + const column = this.segment(); + if (!IDENTIFIER.test(column)) throw new LogicParseError('column is not an identifier'); + let operator = this.segment(); + let negate = false; + if (operator === 'not') { + negate = true; + operator = this.segment(); + } + const value = this.value(); + const sql = conditionSql(column, operator, value, negate); + if (sql === null) throw new LogicParseError('unknown operator'); + return sql; + } + + /** Text up to the next `.` (consumed). An operator may carry a `(modifier)`. */ + private segment(): string { + const start = this.i; + let depth = 0; + while (this.i < this.s.length) { + const c = this.s[this.i]; + if (c === '(') depth++; + else if (c === ')') { + if (depth === 0) throw new LogicParseError('segment ended early'); + depth--; + } else if (depth === 0 && c === '.') break; + else if (depth === 0 && c === ',') throw new LogicParseError('segment ended early'); + this.i++; + } + if (this.s[this.i] !== '.') throw new LogicParseError('expected "."'); + const seg = this.s.slice(start, this.i); + this.i++; // the '.' + return seg; + } + + /** + * A value, up to the next top-level `,` or `)`. Values may be double-quoted + * (with `\"` escapes), or bracketed — `(a,b)` for `in`, `{a,b}` for array + * operators — and may contain dots (timestamps, decimals). Returned raw; the + * caller only ever inspects it for an `IS` keyword. + */ + private value(): string { + const start = this.i; + let depth = 0; + let quoted = false; + while (this.i < this.s.length) { + const c = this.s[this.i]; + if (quoted) { + if (c === '\\') this.i++; + else if (c === '"') quoted = false; + } else if (c === '"') quoted = true; + else if (c === '(' || c === '{') depth++; + else if (c === ')' || c === '}') { + if (depth === 0) break; + depth--; + } else if (c === ',' && depth === 0) break; + this.i++; + } + if (quoted || depth !== 0) throw new LogicParseError('unbalanced value'); + return this.s.slice(start, this.i); + } +} + +// ── statement rendering ──────────────────────────────────────────────────── + +export interface StatementShape { + op: DbOperation; + table: string; + /** Column list as written by our code (`'id,key,value'`, `Object.keys(row)`). */ + columns: string; + /** Fragments built by `conditionSql` & co. — placeholders only, never values. */ + filters: readonly string[]; + orderBy?: string; + hasLimit: boolean; + /** A mutation's `.select(...)` columns, rendered as `RETURNING …`. */ + returning?: string; +} + +/** + * The parameterised statement for `db.query.text`. + * + * Placeholders are numbered `$1…$n` in reading order, which is how Postgres + * itself writes a prepared statement. The `LIMIT` is a placeholder too: it is + * often the caller's own `limit` argument, and a literal would split one call + * site into a statement per page size. + */ +export function renderStatement(s: StatementShape): string { + const parts: string[] = []; + const cols = s.columns.trim(); + switch (s.op) { + case 'SELECT': parts.push(`SELECT ${cols || '*'} FROM ${s.table}`); break; + case 'INSERT': parts.push(`INSERT INTO ${s.table}${cols ? ` (${cols})` : ''}`); break; + case 'UPDATE': parts.push(`UPDATE ${s.table} SET ${setList(cols)}`); break; + case 'DELETE': parts.push(`DELETE FROM ${s.table}`); break; + case 'UPSERT': parts.push(`UPSERT INTO ${s.table}${cols ? ` (${cols})` : ''}`); break; + case 'RPC': parts.push(`CALL ${s.table}(...)`); break; + } + if (s.filters.length) parts.push(`WHERE ${s.filters.join(' AND ')}`); + if (s.orderBy) parts.push(`ORDER BY ${s.orderBy}`); + if (s.hasLimit) parts.push(`LIMIT ${PARAM}`); + const mutates = s.op === 'INSERT' || s.op === 'UPDATE' || s.op === 'UPSERT' || s.op === 'DELETE'; + if (mutates && s.returning?.trim()) parts.push(`RETURNING ${s.returning.trim()}`); + + let n = 0; + return parts.join(' ').replace(/\?/g, () => `$${++n}`); +} + +/** `a = ?, b = ?` from an UPDATE's column list; `...` when it is unknown. */ +function setList(columns: string): string { + const names = columns.split(',').map((c) => c.trim()).filter((c) => IDENTIFIER.test(c)); + return names.length ? names.map((c) => `${c} = ${PARAM}`).join(', ') : '...'; +} + +// ── server.address ───────────────────────────────────────────────────────── + +/** Hostname of a URL-ish value, or `undefined` when it is not one. */ +function hostOf(url: unknown): string | undefined { + if (url === undefined || url === null || url === '') return undefined; + try { + return new URL(String(url)).hostname || undefined; + } catch { + return undefined; + } +} + +/** + * `server.address` for a traced DB call: the PostgREST host the client talks + * to — but ONLY when that is LoreKit's own hosted project. + * + * `clientUrl` is the URL the supabase-js client was built with; `hostedUrl` is + * the function's `SUPABASE_URL`. A client pointed somewhere else is a BYOD + * storage adapter talking to a USER'S OWN Supabase project, and that project's + * host is the user's infrastructure, not ours to put in telemetry — so it is + * omitted rather than named. When the client's URL cannot be read (a test + * double), the hosted URL is the only project the function knows, and it is + * used. + */ +export function resolveServerAddress(clientUrl: unknown, hostedUrl: unknown): string | undefined { + const hosted = hostOf(hostedUrl); + const client = hostOf(clientUrl); + if (client === undefined) return hosted; + return client === hosted ? client : undefined; +} + +// ── error.message ────────────────────────────────────────────────────────── + +/** + * Replace every URL in an error message with ``. + * + * A transport failure's message carries the request URL, and a PostgREST URL + * carries every filter value in its query string (`?key=eq.`). The + * DB span records the message as `error.message`, so without this the values + * the statement rendering keeps out of `db.query.text` arrive through the + * error instead. The rest of the message — the part that says what failed — is + * kept. + */ +export function redactUrls(message: string): string { + return message.replace(/\bhttps?:\/\/[^\s"'<>)]+/gi, ''); +} + +// ── error.type ───────────────────────────────────────────────────────────── + +/** The `error.type` fallback when nothing more specific is known (OTel's own). */ +export const ERROR_TYPE_OTHER = '_OTHER'; + +/** + * A bounded `error.type` from an error message of the shape every edge call + * site already uses — `ClassName: detail` (`UserInputError: q is required`, + * `MethodNotFound: resources/list`) or a bare code (`missing_token`). + * + * Anything else — prose with spaces before the colon, an empty message — is + * `_OTHER`, never the message itself: `error.type` is a grouping key, and the + * free text already rides on `error.message`. + */ +export function errorTypeFrom(message: unknown): string { + const m = /^([A-Za-z_][A-Za-z0-9_.]{0,63})(?::|$)/.exec(String(message ?? '').trim()); + // `${e.name}: …` over a value with no `name` (a PostgREST error object is a + // plain object) renders `undefined: …` — a placeholder, not a class. + if (!m || m[1] === 'undefined' || m[1] === 'null') return ERROR_TYPE_OTHER; + return m[1]; +} + +/** + * A bounded `error.type` for a caught value: an `Error`'s class name, else the + * `code` a PostgREST / Postgres error object carries (`23505`, `PGRST301`), + * else `_OTHER`. Use it wherever a catch block names the type, instead of + * `(e as Error).name`, which is `undefined` for those plain error objects. + */ +export function errorTypeOf(err: unknown): string { + if (err && typeof err === 'object') { + const { name, code } = err as { name?: unknown; code?: unknown }; + if (typeof name === 'string' && name) return errorTypeFrom(name); + // SQLSTATEs start with a digit (`23505`), so not `errorTypeFrom`'s grammar. + if (typeof code === 'string' && /^[A-Za-z0-9_.]{1,64}$/.test(code)) return code; + } + return ERROR_TYPE_OTHER; +} + +// ── http.route ───────────────────────────────────────────────────────────── + +/** + * `http.route` for a router-matched edge request: the function's mount point + * plus the route TEMPLATE, in the same shape `url.path` reports + * (`/memories/:id`, never `/memories/744e…`). The index route is the bare + * mount point (`/memories`), matching what a caller actually requests. + */ +export function httpRouteFor(functionName: string, routePath: string): string { + const tail = routePath === '/' ? '' : routePath; + return `/${functionName}${tail}`; +} diff --git a/supabase/functions/blog/index.ts b/supabase/functions/blog/index.ts index 644720e43..9ed70c795 100644 --- a/supabase/functions/blog/index.ts +++ b/supabase/functions/blog/index.ts @@ -1,5 +1,6 @@ import { createClient } from 'npm:@supabase/supabase-js@2'; import { traceRequest } from '../_shared/telemetry/otel.ts'; +import { errorTypeOf } from '../_shared/telemetry/span-semconv.ts'; import { corsHeaders, handlePreflight } from '../_shared/api/cors.ts'; import { notFound, methodNotAllowed, internalError } from '../_shared/api/respond.ts'; import { relativePath } from '../_shared/api/router.ts'; @@ -37,6 +38,7 @@ Deno.serve(async (req) => { const rel = relativePath(new URL(req.url).pathname, 'blog'); if (rel !== '/likes') return notFound('Route', cors); + span.setAttributes({ 'http.route': '/blog/likes' }); const db = createClient(SUPABASE_URL, SERVICE_ROLE_KEY, { auth: { persistSession: false, autoRefreshToken: false }, @@ -47,7 +49,7 @@ Deno.serve(async (req) => { if (req.method === 'POST') return await handleAddLike(req, db, span, cors); return methodNotAllowed(cors); } catch (e) { - span.error(`Unhandled: ${(e as Error).message}`); + span.error(`Unhandled: ${(e as Error).message}`, errorTypeOf(e)); return internalError(cors); } }); diff --git a/supabase/functions/groom-sweep/index.ts b/supabase/functions/groom-sweep/index.ts index 0b45f1844..53a9146cd 100644 --- a/supabase/functions/groom-sweep/index.ts +++ b/supabase/functions/groom-sweep/index.ts @@ -44,6 +44,7 @@ function json(body: unknown, status: number): Response { Deno.serve((req: Request) => traceRequest(req, 'lorekit.groom_sweep', async (span) => { + span.setAttributes({ 'http.route': '/groom-sweep' }); if (req.method !== 'POST') { return json({ error: 'method_not_allowed', allow: 'POST' }, 405); } diff --git a/supabase/functions/health/index.ts b/supabase/functions/health/index.ts index 5ceec2527..e0f73f54c 100644 --- a/supabase/functions/health/index.ts +++ b/supabase/functions/health/index.ts @@ -19,6 +19,7 @@ Deno.serve(async (req: Request) => { const ts = new Date().toISOString(); return traceRequest(req, 'lorekit.health', async (span) => { + span.setAttributes({ 'http.route': '/health' }); let dbStatus: 'ok' | 'error' = 'ok'; let dbError: string | undefined; diff --git a/supabase/functions/mcp/auth.ts b/supabase/functions/mcp/auth.ts index 37bf9acfa..42ac6b7c9 100644 --- a/supabase/functions/mcp/auth.ts +++ b/supabase/functions/mcp/auth.ts @@ -169,10 +169,12 @@ async function resolveAuthTiers( // so a slow `api_tokens` read is distinguishable from a slow hash or a slow // GoTrue call rather than being one undifferentiated auth cost. // - // Deliberately NOT `createTracedClient`: it renders filter VALUES into the - // span name and `db.query.text` (`buildSql` interpolates `eq()` arguments), - // and the filter here is the token hash — the stored credential. The query - // therefore runs on the raw client and only the timing is spanned. + // Deliberately NOT `createTracedClient`. Its span name and `db.query.text` + // carry no filter values (`$n` placeholders), and a failed request's + // message is recorded with URLs redacted (span-semconv.ts) — but a DB + // error message can still echo a value in some other form, and the filter + // here is the token hash, the stored credential. Defence in depth: the + // query runs on the raw client and only the timing is spanned. const lookupSpan = authSpan?.child('SELECT user_id,permissions,scopes,org_access,org_ids FROM api_tokens', { 'db.system': 'postgresql', 'db.operation.name': 'SELECT', diff --git a/supabase/functions/mcp/index.ts b/supabase/functions/mcp/index.ts index c86cb7647..4ac4f6edb 100644 --- a/supabase/functions/mcp/index.ts +++ b/supabase/functions/mcp/index.ts @@ -72,6 +72,11 @@ Deno.serve(async (req: Request) => { // request produces at least one span. resolveAuth is intentionally inside // traceRequest so unauthenticated calls are still visible in telemetry. return traceRequest(req, 'lorekit.mcp', async (span) => { + // The MCP endpoint is the function root. Discovery probes for other paths + // (`/mcp/.well-known/openid-configuration`) land here too but are not + // that route, so they are left without one rather than mislabelled. + if (/\/mcp\/?$/.test(url.pathname)) span.setAttributes({ 'http.route': '/mcp' }); + // POST-only, checked BEFORE authentication. // // A request's method is knowable from the request line alone — nothing diff --git a/supabase/functions/mcp/installation-sync.ts b/supabase/functions/mcp/installation-sync.ts index ef39c887e..d242be329 100644 --- a/supabase/functions/mcp/installation-sync.ts +++ b/supabase/functions/mcp/installation-sync.ts @@ -144,5 +144,8 @@ async function processSync(req: Request, span: Span): Promise { } export function handleInstallationSync(req: Request): Promise { - return traceRequest(req, 'lorekit.installation.sync', (span) => processSync(req, span)); + return traceRequest(req, 'lorekit.installation.sync', (span) => { + span.setAttributes({ 'http.route': '/mcp/installations/sync' }); + return processSync(req, span); + }); } diff --git a/supabase/functions/mcp/webhook.ts b/supabase/functions/mcp/webhook.ts index 2620a0a62..148b4a6ad 100644 --- a/supabase/functions/mcp/webhook.ts +++ b/supabase/functions/mcp/webhook.ts @@ -860,5 +860,8 @@ async function processWebhook(req: Request, span: Span): Promise { } export function handleWebhook(req: Request): Promise { - return traceRequest(req, 'lorekit.webhook.github', (span) => processWebhook(req, span)); + return traceRequest(req, 'lorekit.webhook.github', (span) => { + span.setAttributes({ 'http.route': '/mcp/webhooks/github' }); + return processWebhook(req, span); + }); } diff --git a/supabase/functions/memories/index.ts b/supabase/functions/memories/index.ts index fde037299..47580c56b 100644 --- a/supabase/functions/memories/index.ts +++ b/supabase/functions/memories/index.ts @@ -1,4 +1,5 @@ import { traceRequest } from '../_shared/telemetry/otel.ts'; +import { errorTypeOf } from '../_shared/telemetry/span-semconv.ts'; import { resolveRestAuth } from '../_shared/api/auth.ts'; import { createRouter } from '../_shared/api/router.ts'; import { corsHeaders, handlePreflight } from '../_shared/api/cors.ts'; @@ -115,7 +116,7 @@ Deno.serve(async (req) => { if (e instanceof RestError) return e.toResponse(cors); const mapped = translateDbError(e); if (mapped) return mapped.toResponse(cors); - span.error(`Unhandled: ${(e as Error).message}`); + span.error(`Unhandled: ${(e as Error).message}`, errorTypeOf(e)); return internalError(cors); } }); diff --git a/supabase/functions/openapi/index.ts b/supabase/functions/openapi/index.ts index 0d2fd3c74..908a2a69e 100644 --- a/supabase/functions/openapi/index.ts +++ b/supabase/functions/openapi/index.ts @@ -1,4 +1,5 @@ import { traceRequest } from '../_shared/telemetry/otel.ts'; +import { errorTypeOf } from '../_shared/telemetry/span-semconv.ts'; import { corsHeaders, handlePreflight } from '../_shared/api/cors.ts'; import { internalError } from '../_shared/api/respond.ts'; import { generateSpec } from '../_shared/schemas/openapi/spec.ts'; @@ -32,14 +33,16 @@ Deno.serve(async (req) => { // never render (see DOCS_URL note). Redirect to the real docs so old // bookmarks keep working. A 302 has no HTML body, so the sandbox is moot. if (url.pathname.endsWith('/ui')) { + span.setAttributes({ 'http.route': '/openapi/ui' }); return new Response(null, { status: 302, headers: { Location: DOCS_URL, ...cors } }); } + span.setAttributes({ 'http.route': '/openapi' }); try { const spec = getSpec(); return new Response(JSON.stringify(spec), { headers: { 'Content-Type': 'application/json', ...cors } }); } catch (e) { - span.error(`spec generation failed: ${(e as Error).message}`); + span.error(`spec generation failed: ${(e as Error).message}`, errorTypeOf(e)); return internalError(cors); } }); diff --git a/supabase/functions/orgs/index.ts b/supabase/functions/orgs/index.ts index 15f540644..a61e0a6a2 100644 --- a/supabase/functions/orgs/index.ts +++ b/supabase/functions/orgs/index.ts @@ -1,4 +1,5 @@ import { traceRequest } from '../_shared/telemetry/otel.ts'; +import { errorTypeOf } from '../_shared/telemetry/span-semconv.ts'; import { resolveRestAuth } from '../_shared/api/auth.ts'; import { createRouter } from '../_shared/api/router.ts'; import { corsHeaders, handlePreflight } from '../_shared/api/cors.ts'; @@ -65,7 +66,7 @@ Deno.serve(async (req) => { if (e instanceof RestError) return e.toResponse(cors); const mapped = translateDbError(e); if (mapped) return mapped.toResponse(cors); - span.error(`Unhandled: ${(e as Error).message}`); + span.error(`Unhandled: ${(e as Error).message}`, errorTypeOf(e)); return internalError(cors); } }); diff --git a/supabase/functions/profiling/index.ts b/supabase/functions/profiling/index.ts index 19292335e..3c0d986c7 100644 --- a/supabase/functions/profiling/index.ts +++ b/supabase/functions/profiling/index.ts @@ -65,6 +65,7 @@ function parseLimit(url: URL): number { Deno.serve((req: Request) => traceRequest(req, 'lorekit.profiling', async (span) => { + span.setAttributes({ 'http.route': '/profiling' }); if (req.method !== 'POST') { return json({ error: 'method_not_allowed', allow: 'POST' }, 405); }