From 880e568b39db01be2ab908357053dfe5ccb63964 Mon Sep 17 00:00:00 2001 From: "mads.thines" Date: Fri, 2 Oct 2026 12:17:53 +0000 Subject: [PATCH 01/11] fix(edge-otel): name DB spans by summary, parameterise db.query.text, add server.address DB CLIENT spans were named after the full statement with filter values interpolated, so the api service emitted 511 distinct span names in a week (full-text search terms, memory keys, user ids). Spans are now named by db.query.summary (SELECT memories / CALL memory_write), db.query.text uses $n placeholders, and server.address names the hosted Supabase host. Root SERVER spans gain http.route (router-matched and single-route functions) and every error()/clientError() call records a bounded error.type. Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com> --- supabase/functions/_shared/api/CLAUDE.md | 2 +- supabase/functions/_shared/api/router.ts | 9 + supabase/functions/_shared/telemetry/otel.ts | 177 +++++++-- .../_shared/telemetry/span-semconv.ts | 364 ++++++++++++++++++ supabase/functions/blog/index.ts | 3 +- supabase/functions/groom-sweep/index.ts | 1 + supabase/functions/health/index.ts | 1 + supabase/functions/mcp/index.ts | 5 + supabase/functions/mcp/installation-sync.ts | 5 +- supabase/functions/mcp/webhook.ts | 5 +- supabase/functions/memories/index.ts | 2 +- supabase/functions/openapi/index.ts | 4 +- supabase/functions/orgs/index.ts | 2 +- supabase/functions/profiling/index.ts | 1 + 14 files changed, 532 insertions(+), 49 deletions(-) create mode 100644 supabase/functions/_shared/telemetry/span-semconv.ts diff --git a/supabase/functions/_shared/api/CLAUDE.md b/supabase/functions/_shared/api/CLAUDE.md index c962915cf..a12f6325f 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}`, (e as Error).name); return internalError(cors); } }); diff --git a/supabase/functions/_shared/api/router.ts b/supabase/functions/_shared/api/router.ts index cd3f2ad93..653a81ae5 100644 --- a/supabase/functions/_shared/api/router.ts +++ b/supabase/functions/_shared/api/router.ts @@ -14,6 +14,7 @@ import { parseScopeTypeAttribute, } from '../scope/scope-type-attribute.ts'; import type { Span } from '../telemetry/otel.ts'; +import { httpRouteFor } from '../telemetry/span-semconv.ts'; /** * Request header carrying a client-supplied grouping key (a PR ref, session id, @@ -212,6 +213,14 @@ export function createRouter(routes: Route[], functionName: string) { if (!m) return methodNotAllowed(cors); const { route, params } = m; + // `http.route` belongs on the SERVER span — that is where the OTel HTTP + // conventions put it and where a per-endpoint `sum by (http_route)` over + // the root spans reads it. The handler child span below keeps its + // function-relative value for existing queries; the root gets the full template in + // `url.path`'s shape (`/memories/:id`). Stamped as soon as a route MATCHES, + // so a 403 from the permission gate below still carries it; a 404/405 + // matched no route, and inventing one would be a lie. + span.setAttributes({ 'http.route': httpRouteFor(functionName, route.path) }); if (route.requires === 'jwt' && !isJwtAuth(resolved.auth)) return forbidden('This endpoint requires a Supabase JWT (not an API token)', cors); if (route.requires === 'read' && !hasPermission(resolved.auth, 'read')) return forbidden('Read permission required', cors); if (route.requires === 'write' && !hasPermission(resolved.auth, 'write')) return forbidden('Write permission required', cors); diff --git a/supabase/functions/_shared/telemetry/otel.ts b/supabase/functions/_shared/telemetry/otel.ts index d27e1d1f9..115e519a2 100644 --- a/supabase/functions/_shared/telemetry/otel.ts +++ b/supabase/functions/_shared/telemetry/otel.ts @@ -6,9 +6,11 @@ * - ExportBatch: collects spans request-scoped, flushes fire-and-forget * after the response via EdgeRuntime.waitUntil (guarantees export before * the Deno isolate shuts down) - * - Span: child spans, db.statement naming for Postgres, W3C traceparent + * - Span: child spans, W3C traceparent, bounded `error.type` * - createTracedClient(): wraps @supabase/supabase-js so every .from() - * chain gets an automatic child span with the SQL-like statement + * chain gets an automatic CLIENT span named `SELECT memories` (the + * `db.query.summary`) with the parameterised statement in `db.query.text` + * — filter values never reach telemetry (see span-semconv.ts) * - traceRequest(): root entry point — extracts incoming traceparent, * builds the root span, flushes the batch * @@ -27,6 +29,16 @@ import type { Database } from '../db/database.types.ts'; import type { DbClient } from '../db/db-client.ts'; import { formatTraceparent, parseTraceparent } from './trace-context.ts'; import { attributeIoTime, type IoInterval } from './io-ledger.ts'; +import { + conditionSql, + errorTypeFrom, + postgrestLogicToSql, + querySummary, + renderStatement, + resolveServerAddress, + textSearchSql, + type DbOperation, +} from './span-semconv.ts'; import { translateDbError } from '../api/errors.ts'; /** PostgREST error shape returned by @supabase/supabase-js. */ @@ -487,10 +499,21 @@ 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 { this.status = 'error'; this.statusMessage = message; this.attributes['error.message'] = message; + this.attributes['error.type'] = type ?? errorTypeFrom(message); return this; } @@ -505,8 +528,22 @@ export class Span { * conventions, server spans should only carry status=ERROR for server-side * faults, not for client errors. */ - clientError(message: string): this { + clientError(message: string, type?: string): this { this.attributes['error.message'] = message; + // 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(message); + 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 +668,13 @@ 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. Dash0 still names + * the operation by `faas.name` (its FaaS rule outranks its HTTP-route rule), so + * the attribute adds a per-endpoint grouping key without renaming the + * `memories` / `mcp` operations existing check rules filter on. */ export async function traceRequest( req: Request, @@ -658,8 +702,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: …', e.name)`) keeps it. + if (response.status >= 500) span.setErrorTypeIfAbsent(String(response.status)); return withTraceparent(response, ctx); } catch (err) { + // `error.type` is derived from the `Name:` prefix — the thrown class. span.error(`${(err as Error).name}: ${(err as Error).message}`); throw err; } finally { @@ -724,15 +773,23 @@ 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; /** * The postgrest builder being accumulated. Deliberately untyped, because the * real type CHANGES ALONG THE CHAIN and no single annotation describes it: @@ -756,25 +813,32 @@ 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, + }); +} + +/** 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()`. @@ -825,14 +889,22 @@ export class TracedQuery> { } // ── 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 +913,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 +1025,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 +1050,11 @@ 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 }); if (result.error.code === 'PGRST116') { // .single() no rows — expected, not an error dbSpan.setAttributes({ 'db.no_rows': true }); @@ -982,9 +1068,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: ${result.error.message}`, code); } else { - dbSpan.error(`PostgrestError: ${result.error.message}`); + dbSpan.error(`PostgrestError: ${result.error.message}`, code); } } @@ -1012,7 +1098,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 +1129,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..bd097270d --- /dev/null +++ b/supabase/functions/_shared/telemetry/span-semconv.ts @@ -0,0 +1,364 @@ +// 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; +} + +/** + * 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}`); + + 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.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: string): string { + const m = /^([A-Za-z_][A-Za-z0-9_.]{0,63})(?::|$)/.exec(message.trim()); + return m ? m[1] : 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..a4847844d 100644 --- a/supabase/functions/blog/index.ts +++ b/supabase/functions/blog/index.ts @@ -37,6 +37,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 +48,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}`, (e as Error).name); 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/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..0c825d8be 100644 --- a/supabase/functions/memories/index.ts +++ b/supabase/functions/memories/index.ts @@ -115,7 +115,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}`, (e as Error).name); return internalError(cors); } }); diff --git a/supabase/functions/openapi/index.ts b/supabase/functions/openapi/index.ts index 0d2fd3c74..655c62f01 100644 --- a/supabase/functions/openapi/index.ts +++ b/supabase/functions/openapi/index.ts @@ -32,14 +32,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}`, (e as Error).name); return internalError(cors); } }); diff --git a/supabase/functions/orgs/index.ts b/supabase/functions/orgs/index.ts index 15f540644..8cafea5a6 100644 --- a/supabase/functions/orgs/index.ts +++ b/supabase/functions/orgs/index.ts @@ -65,7 +65,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}`, (e as Error).name); 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); } From 83b2f4d9c249fafce26768fbf7ed7fd75a5ee03f Mon Sep 17 00:00:00 2001 From: "mads.thines" Date: Fri, 2 Oct 2026 12:20:32 +0000 Subject: [PATCH 02/11] test(edge-otel): execute span-semconv and DB span rendering; update docs Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com> --- docs/architecture.md | 2 +- docs/key-files.md | 1 + docs/otel.md | 52 ++- docs/telemetry-quality-review.md | 9 +- .../src/auth/rest-auth-tracing.spec.ts | 8 +- .../src/mcp-guards/mcp-auth-tracing.spec.ts | 11 +- .../src/telemetry/otel-conventions.spec.ts | 11 +- .../src/telemetry/span-semconv.spec.ts | 384 ++++++++++++++++++ scripts/telemetry/emit-correlated-trace.mts | 14 +- supabase/functions/CLAUDE.md | 9 +- 10 files changed, 481 insertions(+), 20 deletions(-) create mode 100644 packages/mcp-core/src/telemetry/span-semconv.spec.ts 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..9332b4663 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 ever reaches telemetry; an unparseable `.or()` string collapses to `(?)`), `resolveServerAddress` (never names a BYOD host), `errorTypeFrom`, `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..2337f98d1 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,55 @@ 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`, and no filter value reaches the name or any attribute: | 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, on failure only; also the span's `error.type` | + +**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. + +**One residual path.** The rejection arm (a thrown fetch, not a PostgREST error) +still records the thrown message in `error.message`, and a Deno fetch failure +renders the request URL into it. That is why the token lookups in +`mcp/auth.ts` / `_shared/api/auth.ts` stay off `createTracedClient` +(`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`, `503` | Every `span.error()` / `span.clientError()` — an explicit type, else the `Name:` prefix of the message (`errorTypeFrom`), else `_OTHER`. A 5xx response with nothing more specific records the status code. Never the message itself — that stays on `error.message` | + +Dash0 keeps naming the edge operations by `faas.name` (`memories`, `mcp`): its +FaaS rule is evaluated before its HTTP-route rule, so adding `http.route` does +not rename the operations existing check rules filter on. Per-endpoint RED is a +grouping over the root spans instead: + +```promql +sum by (http_route, http_response_status_code) (increase({otel_metric_name="dash0.spans", service_name="api", otel_span_kind="SERVER"}[1h])) +``` --- diff --git a/docs/telemetry-quality-review.md b/docs/telemetry-quality-review.md index dbbcdae75..a97ad9f11 100644 --- a/docs/telemetry-quality-review.md +++ b/docs/telemetry-quality-review.md @@ -143,7 +143,10 @@ resource attributes only. ## Recommendations (not applied — design/judgment calls) -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/mcp-core/src/auth/rest-auth-tracing.spec.ts b/packages/mcp-core/src/auth/rest-auth-tracing.spec.ts index ec6f059ba..b7aefdc6f 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,11 @@ 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, but its rejection arm records the thrown message, which a + // Deno fetch failure fills with the request URL — filter values included. + // The value here is the token hash, so tracing this query that way could + // still publish a credential into telemetry. 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..c0f8810ca 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`), but its + * rejection arm still records the thrown error's message, and a Deno fetch + * failure renders the request URL — filter values included — into it. The + * filter on this query is the token hash, the stored credential, so this is + * still the security half of the guard: it must stay 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 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..dbcf13d88 --- /dev/null +++ b/packages/mcp-core/src/telemetry/span-semconv.spec.ts @@ -0,0 +1,384 @@ +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 any + * exported attribute or span name — so the DB tests below 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; + }): string; + textSearchSql(column: string, type?: string): string; + resolveServerAddress(clientUrl: unknown, hostedUrl: unknown): string | undefined; + errorTypeFrom(message: string): string; + httpRouteFor(functionName: string, routePath: 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('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 — 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('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 }, 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('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('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('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..8829dd3e8 100644 --- a/supabase/functions/CLAUDE.md +++ b/supabase/functions/CLAUDE.md @@ -33,7 +33,14 @@ 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 telemetry +(`_shared/telemetry/span-semconv.ts`). +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. Add `span.child('lorekit.{resource}.{operation}')` for any significant sub-operation. **Self-time attribution is automatic.** `traceRequest` stamps From 8a479c6b661aa9299b1acd9b43a1d635489f42cd Mon Sep 17 00:00:00 2001 From: "mads.thines" Date: Fri, 2 Oct 2026 12:29:46 +0000 Subject: [PATCH 03/11] test(edge-otel): index-signature access in span-semconv spec Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com> --- packages/mcp-core/src/telemetry/span-semconv.spec.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/mcp-core/src/telemetry/span-semconv.spec.ts b/packages/mcp-core/src/telemetry/span-semconv.spec.ts index dbcf13d88..d29bc640e 100644 --- a/packages/mcp-core/src/telemetry/span-semconv.spec.ts +++ b/packages/mcp-core/src/telemetry/span-semconv.spec.ts @@ -231,11 +231,11 @@ let env: Record; beforeEach(() => { hadDeno = 'Deno' in globalThis; env = { SUPABASE_URL: HOSTED_URL }; - (globalThis as Record).Deno = { env: { get: (k: string) => env[k] } }; + (globalThis as Record)['Deno'] = { env: { get: (k: string) => env[k] } }; }); afterEach(() => { - if (!hadDeno) delete (globalThis as Record).Deno; + if (!hadDeno) delete (globalThis as Record)['Deno']; }); describe('createTracedClient — no filter value reaches telemetry', () => { @@ -343,7 +343,7 @@ describe('traceRequest — 5xx error.type fallback', () => { beforeEach(() => { posted = []; realFetch = globalThis.fetch; - env.OTEL_EXPORTER_OTLP_ENDPOINT = ENDPOINT; + 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 }); From 5a50f2bffa4ede6331f83a5cc45b887283474715 Mon Sep 17 00:00:00 2001 From: "mads.thines" Date: Fri, 2 Oct 2026 12:44:28 +0000 Subject: [PATCH 04/11] fix(edge-otel): render a mutation's select() as RETURNING; document the operation rename The PR's own CI smoke trace showed two things the unit tests could not: UPDATE ... .select('id,scope,key') rendered the returned columns as the SET list, and Dash0 names root-span operations by http.route once it is present (POST /mcp), not by faas.name. Fix the first; document the second and the check rules that must move to faas_name before deploy. Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com> --- docs/otel.md | 24 ++++++++++++++---- .../src/telemetry/span-semconv.spec.ts | 22 +++++++++++++++- supabase/functions/_shared/telemetry/otel.ts | 25 +++++++++++++++---- .../_shared/telemetry/span-semconv.ts | 4 +++ 4 files changed, 64 insertions(+), 11 deletions(-) diff --git a/docs/otel.md b/docs/otel.md index 2337f98d1..feeb2e0e8 100644 --- a/docs/otel.md +++ b/docs/otel.md @@ -626,15 +626,29 @@ renders the request URL into it. That is why the token lookups in | `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`, `503` | Every `span.error()` / `span.clientError()` — an explicit type, else the `Name:` prefix of the message (`errorTypeFrom`), else `_OTHER`. A 5xx response with nothing more specific records the status code. Never the message itself — that stays on `error.message` | -Dash0 keeps naming the edge operations by `faas.name` (`memories`, `mcp`): its -FaaS rule is evaluated before its HTTP-route rule, so adding `http.route` does -not rename the operations existing check rules filter on. Per-endpoint RED is a -grouping over the root spans instead: +**`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 (http_route, http_response_status_code) (increase({otel_metric_name="dash0.spans", service_name="api", otel_span_kind="SERVER"}[1h])) +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. + --- ## CLI usage telemetry (`@lorekit/cli`) diff --git a/packages/mcp-core/src/telemetry/span-semconv.spec.ts b/packages/mcp-core/src/telemetry/span-semconv.spec.ts index d29bc640e..2e43d65b9 100644 --- a/packages/mcp-core/src/telemetry/span-semconv.spec.ts +++ b/packages/mcp-core/src/telemetry/span-semconv.spec.ts @@ -28,7 +28,7 @@ const semconv = (await load('span-semconv.ts')) as { 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; + 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; @@ -151,6 +151,13 @@ describe('renderStatement (db.query.text)', () => { ); }); + 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'); @@ -285,6 +292,19 @@ describe('createTracedClient — no filter value reaches telemetry', () => { } }); + 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); diff --git a/supabase/functions/_shared/telemetry/otel.ts b/supabase/functions/_shared/telemetry/otel.ts index 115e519a2..e3ab0293f 100644 --- a/supabase/functions/_shared/telemetry/otel.ts +++ b/supabase/functions/_shared/telemetry/otel.ts @@ -671,10 +671,15 @@ 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. Dash0 still names - * the operation by `faas.name` (its FaaS rule outranks its HTTP-route rule), so - * the attribute adds a per-endpoint grouping key without renaming the - * `memories` / `mcp` operations existing check rules filter on. + * 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, @@ -790,6 +795,13 @@ interface QueryState { 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: @@ -822,6 +834,7 @@ function buildSql(s: QueryState): string { filters: s.filters, orderBy: s.orderBy, hasLimit: s.lim !== undefined, + returning: s.returning, }); } @@ -883,7 +896,9 @@ 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; } diff --git a/supabase/functions/_shared/telemetry/span-semconv.ts b/supabase/functions/_shared/telemetry/span-semconv.ts index bd097270d..179285894 100644 --- a/supabase/functions/_shared/telemetry/span-semconv.ts +++ b/supabase/functions/_shared/telemetry/span-semconv.ts @@ -265,6 +265,8 @@ export interface StatementShape { filters: readonly string[]; orderBy?: string; hasLimit: boolean; + /** A mutation's `.select(...)` columns, rendered as `RETURNING …`. */ + returning?: string; } /** @@ -289,6 +291,8 @@ export function renderStatement(s: StatementShape): string { 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}`); From 226a62472ca7015e984b2dc2a0369050814cdb5e Mon Sep 17 00:00:00 2001 From: "mads.thines" Date: Fri, 2 Oct 2026 13:13:29 +0000 Subject: [PATCH 05/11] docs(edge-otel): correct comments that still said createTracedClient interpolates filter values Addresses the pr-reviewer advisory on #682: the token-lookup guards stay, but their reason is now the rejection arm's error message, not the span name. Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com> --- supabase/functions/_shared/api/auth.ts | 12 +++++++----- supabase/functions/_shared/telemetry/otel.ts | 5 +++-- supabase/functions/mcp/auth.ts | 10 ++++++---- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/supabase/functions/_shared/api/auth.ts b/supabase/functions/_shared/api/auth.ts index 68af77728..b0b1a05f9 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> { } /** - * 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 diff --git a/supabase/functions/mcp/auth.ts b/supabase/functions/mcp/auth.ts index 37bf9acfa..ffada4bae 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` + // no longer carry filter values (`$n` placeholders — span-semconv.ts), but + // its rejection arm records the thrown message, and a Deno fetch failure + // renders the request URL — this filter's value included — into it. The + // filter here is the token hash, the stored credential, so 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', From 940c8478574a92f157938270fed9218f8fb3f992 Mon Sep 17 00:00:00 2001 From: "mads.thines" Date: Fri, 2 Oct 2026 13:30:44 +0000 Subject: [PATCH 06/11] fix(evals): switch off Claude Code built-in plugins in the preflight clean room Evals preflight failed on three heads of this PR with "CONTAMINATED - plugins-loaded: cc-plugin-agents-md, cc-plugin-plugin-authoring" on a runner that has no ~/.claude. evals.yml installs @anthropic-ai/claude-code unpinned; 2.1.287 ships built-in plugins, some on by default (2.1.280, the latest at the last green preflight, ships none), and the harness's `enabledPlugins: {}` override sets nothing, so they load. Set `"@builtin": false` for each built-in instead. The environment check is unchanged: any plugin that still loads fails preflight. Reproduced locally with claude 2.1.287: the old override reports the same two plugins, and the new one reports clean with the harness's SessionStart hook still firing. Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com> --- packages/evals/README.md | 7 +++- packages/evals/src/harness/arm.mjs | 41 +++++++++++++++++-- .../evals/test/grading/environment.test.mjs | 10 ++++- 3 files changed, 52 insertions(+), 6 deletions(-) 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. From 025e98b0ece89e972a1eca5b77420d8bf2c8ba3e Mon Sep 17 00:00:00 2001 From: "mads.thines" Date: Fri, 2 Oct 2026 13:55:04 +0000 Subject: [PATCH 07/11] fix(edge-otel): keep a failed request's URL out of DB span error.message postgrest-js resolves a failed fetch (status 0) with the request URL, filter values included, in result.error.message, and the DB span copied it into error.message on every traced query. Record 'fetch failed' for status 0 and redact URLs from every other recorded message; correct the docs and comments that blamed only the rejection arm. Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com> --- docs/otel.md | 21 +++++--- docs/telemetry-quality-review.md | 2 +- .../src/auth/rest-auth-tracing.spec.ts | 7 ++- .../src/mcp-guards/mcp-auth-tracing.spec.ts | 18 +++---- .../src/telemetry/span-semconv.spec.ts | 49 ++++++++++++++++++- supabase/functions/_shared/api/auth.ts | 12 ++--- supabase/functions/_shared/telemetry/otel.ts | 14 ++++-- .../_shared/telemetry/span-semconv.ts | 16 ++++++ supabase/functions/mcp/auth.ts | 10 ++-- 9 files changed, 113 insertions(+), 36 deletions(-) diff --git a/docs/otel.md b/docs/otel.md index feeb2e0e8..ecb75604e 100644 --- a/docs/otel.md +++ b/docs/otel.md @@ -586,7 +586,8 @@ Users can read their own rows (self-service "my usage" view via RLS). --- DB child spans carry OTel database semconv. The span is NAMED by -`db.query.summary`, and no filter value reaches the name or any attribute: +`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 | |-----------|---------| @@ -597,7 +598,7 @@ DB child spans carry OTel database semconv. The span is NAMED by | `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, on failure only; also the span's `error.type` | +| `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::…'`, @@ -613,11 +614,17 @@ value is a placeholder, and a `.or()` logic string it cannot parse collapses to `createTracedClient` chain with sentinel values and asserts none reaches the exported span. -**One residual path.** The rejection arm (a thrown fetch, not a PostgREST error) -still records the thrown message in `error.message`, and a Deno fetch failure -renders the request URL into it. That is why the token lookups in -`mcp/auth.ts` / `_shared/api/auth.ts` stay off `createTracedClient` -(`mcp-auth-tracing.spec.ts`, `rest-auth-tracing.spec.ts`). +**Error messages.** 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` diff --git a/docs/telemetry-quality-review.md b/docs/telemetry-quality-review.md index a97ad9f11..7efdea965 100644 --- a/docs/telemetry-quality-review.md +++ b/docs/telemetry-quality-review.md @@ -141,7 +141,7 @@ 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** — **applied** (2026-10): spans are named by `db.query.summary`, the statement 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 b7aefdc6f..143671d3f 100644 --- a/packages/mcp-core/src/auth/rest-auth-tracing.spec.ts +++ b/packages/mcp-core/src/auth/rest-auth-tracing.spec.ts @@ -66,10 +66,9 @@ describe('REST auth resolution telemetry (supabase/functions/_shared/api/auth.ts it('never routes the token lookup through createTracedClient', () => { // createTracedClient keeps `eq()` values out of the span name and - // db.query.text, but its rejection arm records the thrown message, which a - // Deno fetch failure fills with the request URL — filter values included. - // The value here is the token hash, so tracing this query that way could - // still publish a credential into telemetry. + // 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 c0f8810ca..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 @@ -26,12 +26,12 @@ import path from 'node:path'; * * 3. THE TOKEN LOOKUP IS NOT TRACED THROUGH `createTracedClient`. That wrapper * no longer puts filter VALUES in the span name or `db.query.text` (they - * are `$n` placeholders — see `_shared/telemetry/span-semconv.ts`), but its - * rejection arm still records the thrown error's message, and a Deno fetch - * failure renders the request URL — filter values included — into it. The - * filter on this query is the token hash, the stored credential, so this is - * still the security half of the guard: it must stay a hand-rolled span - * over the raw client. + * 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 @@ -108,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/span-semconv.spec.ts b/packages/mcp-core/src/telemetry/span-semconv.spec.ts index 2e43d65b9..86ab7677d 100644 --- a/packages/mcp-core/src/telemetry/span-semconv.spec.ts +++ b/packages/mcp-core/src/telemetry/span-semconv.spec.ts @@ -34,6 +34,7 @@ const semconv = (await load('span-semconv.ts')) as { resolveServerAddress(clientUrl: unknown, hostedUrl: unknown): string | undefined; errorTypeFrom(message: string): string; httpRouteFor(functionName: string, routePath: string): string; + redactUrls(message: string): string; }; type Attrs = Record; @@ -199,6 +200,17 @@ describe('errorTypeFrom', () => { }); }); +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('httpRouteFor', () => { it('joins the mount point and the template in url.path shape', () => { expect(semconv.httpRouteFor('memories', '/:id')).toBe('/memories/:id'); @@ -210,7 +222,7 @@ describe('httpRouteFor', () => { // ── otel.ts, executed ─────────────────────────────────────────────────────── /** A postgrest-js stand-in: every builder method chains, awaiting resolves. */ -function fakeClient(result: { data: unknown; error: unknown }, supabaseUrl?: string) { +function fakeClient(result: { data: unknown; error: unknown; status?: number }, supabaseUrl?: string) { const builder: Record = new Proxy({}, { get(_t, prop) { if (prop === 'then') { @@ -325,6 +337,41 @@ describe('createTracedClient — no filter value reaches telemetry', () => { 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( diff --git a/supabase/functions/_shared/api/auth.ts b/supabase/functions/_shared/api/auth.ts index b0b1a05f9..823e033e3 100644 --- a/supabase/functions/_shared/api/auth.ts +++ b/supabase/functions/_shared/api/auth.ts @@ -73,12 +73,12 @@ 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 ────────────────────────────────────────────── @@ -1070,6 +1073,11 @@ export class TracedQuery> { // `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 }); @@ -1083,9 +1091,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}`, code); + dbSpan.clientError(`PostgrestError: ${detail}`, code); } else { - dbSpan.error(`PostgrestError: ${result.error.message}`, code); + dbSpan.error(`PostgrestError: ${detail}`, code); } } @@ -1093,7 +1101,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; }, diff --git a/supabase/functions/_shared/telemetry/span-semconv.ts b/supabase/functions/_shared/telemetry/span-semconv.ts index 179285894..abbcc8272 100644 --- a/supabase/functions/_shared/telemetry/span-semconv.ts +++ b/supabase/functions/_shared/telemetry/span-semconv.ts @@ -335,6 +335,22 @@ export function resolveServerAddress(clientUrl: unknown, hostedUrl: unknown): st 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). */ diff --git a/supabase/functions/mcp/auth.ts b/supabase/functions/mcp/auth.ts index ffada4bae..42ac6b7c9 100644 --- a/supabase/functions/mcp/auth.ts +++ b/supabase/functions/mcp/auth.ts @@ -170,11 +170,11 @@ async function resolveAuthTiers( // GoTrue call rather than being one undifferentiated auth cost. // // Deliberately NOT `createTracedClient`. Its span name and `db.query.text` - // no longer carry filter values (`$n` placeholders — span-semconv.ts), but - // its rejection arm records the thrown message, and a Deno fetch failure - // renders the request URL — this filter's value included — into it. The - // filter here is the token hash, the stored credential, so the query runs - // on the raw client and only the timing is spanned. + // 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', From d4eedf9c629b15f9f475e384cf2bc2491950e685 Mon Sep 17 00:00:00 2001 From: "mads.thines" Date: Fri, 2 Oct 2026 14:07:29 +0000 Subject: [PATCH 08/11] docs(edge-otel): stop claiming filter values never reach telemetry Error messages are URL-redacted, not value-free: Postgres's own wording can still echo a value. Say exactly that, and list redactUrls in key-files. Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com> --- docs/key-files.md | 2 +- supabase/functions/CLAUDE.md | 7 +++++-- supabase/functions/_shared/telemetry/otel.ts | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/key-files.md b/docs/key-files.md index 9332b4663..091eb5543 100644 --- a/docs/key-files.md +++ b/docs/key-files.md @@ -28,7 +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 ever reaches telemetry; an unparseable `.or()` string collapses to `(?)`), `resolveServerAddress` (never names a BYOD host), `errorTypeFrom`, `httpRouteFor`. Executed by `packages/mcp-core/src/telemetry/span-semconv.spec.ts` | +| `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`, `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/supabase/functions/CLAUDE.md b/supabase/functions/CLAUDE.md index 8829dd3e8..e538dd480 100644 --- a/supabase/functions/CLAUDE.md +++ b/supabase/functions/CLAUDE.md @@ -35,8 +35,11 @@ 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`, named by `db.query.summary` (`SELECT memories`, `CALL memory_write`) with a -parameterised `db.query.text` — filter values never reach telemetry -(`_shared/telemetry/span-semconv.ts`). +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 diff --git a/supabase/functions/_shared/telemetry/otel.ts b/supabase/functions/_shared/telemetry/otel.ts index 185fc96a1..0cded3de5 100644 --- a/supabase/functions/_shared/telemetry/otel.ts +++ b/supabase/functions/_shared/telemetry/otel.ts @@ -10,7 +10,8 @@ * - createTracedClient(): wraps @supabase/supabase-js so every .from() * chain gets an automatic CLIENT span named `SELECT memories` (the * `db.query.summary`) with the parameterised statement in `db.query.text` - * — filter values never reach telemetry (see span-semconv.ts) + * — filter values never reach the name or the statement, and URLs are + * redacted from a failure's error.message (see span-semconv.ts) * - traceRequest(): root entry point — extracts incoming traceparent, * builds the root span, flushes the batch * From eaa6b91926f9b01f38d55efceeab557cb773289a Mon Sep 17 00:00:00 2001 From: "mads.thines" Date: Fri, 2 Oct 2026 14:07:50 +0000 Subject: [PATCH 09/11] test(edge-otel): state the span-semconv spec's negative property precisely Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com> --- packages/mcp-core/src/telemetry/span-semconv.spec.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/mcp-core/src/telemetry/span-semconv.spec.ts b/packages/mcp-core/src/telemetry/span-semconv.spec.ts index 86ab7677d..e5de92187 100644 --- a/packages/mcp-core/src/telemetry/span-semconv.spec.ts +++ b/packages/mcp-core/src/telemetry/span-semconv.spec.ts @@ -8,8 +8,9 @@ import path from 'node:path'; * 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 any - * exported attribute or span name — so the DB tests below run a real + * 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. * From 1540895cfffe085244c9ba2e219d3b818316848a Mon Sep 17 00:00:00 2001 From: "mads.thines" Date: Fri, 2 Oct 2026 14:25:09 +0000 Subject: [PATCH 10/11] fix(edge-otel): redact URLs on every span's error message; name plain error objects by code Two pr-reviewer findings on #682: handler and root spans still recorded a failed request's raw message (URL and search terms included), and a rethrown PostgREST error object (no name) produced error.type 'undefined'. Span.error and clientError now redact URLs and tolerate a non-string message; catch blocks pass errorTypeOf(e), which falls back to the object's code. Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com> --- docs/otel.md | 8 ++-- .../src/telemetry/span-semconv.spec.ts | 43 ++++++++++++++++++- supabase/functions/_shared/api/CLAUDE.md | 2 +- supabase/functions/_shared/api/router.ts | 4 +- supabase/functions/_shared/telemetry/otel.ts | 20 ++++++--- .../_shared/telemetry/span-semconv.ts | 25 +++++++++-- supabase/functions/blog/index.ts | 3 +- supabase/functions/memories/index.ts | 3 +- supabase/functions/openapi/index.ts | 3 +- supabase/functions/orgs/index.ts | 3 +- 10 files changed, 93 insertions(+), 21 deletions(-) diff --git a/docs/otel.md b/docs/otel.md index ecb75604e..e79141532 100644 --- a/docs/otel.md +++ b/docs/otel.md @@ -614,8 +614,10 @@ value is a placeholder, and a `.or()` logic string it cannot parse collapses to `createTracedClient` chain with sentinel values and asserts none reaches the exported span. -**Error messages.** A failed DB span records PostgREST's error text as -`error.message`. A failed *request* is the dangerous case: postgrest-js does not +**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 @@ -631,7 +633,7 @@ is why the token lookups in `mcp/auth.ts` / `_shared/api/auth.ts` stay off | 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`, `503` | Every `span.error()` / `span.clientError()` — an explicit type, else the `Name:` prefix of the message (`errorTypeFrom`), else `_OTHER`. A 5xx response with nothing more specific records the status code. Never the message itself — that stays on `error.message` | +| `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` — diff --git a/packages/mcp-core/src/telemetry/span-semconv.spec.ts b/packages/mcp-core/src/telemetry/span-semconv.spec.ts index e5de92187..05bb8d580 100644 --- a/packages/mcp-core/src/telemetry/span-semconv.spec.ts +++ b/packages/mcp-core/src/telemetry/span-semconv.spec.ts @@ -33,7 +33,8 @@ const semconv = (await load('span-semconv.ts')) as { }): string; textSearchSql(column: string, type?: string): string; resolveServerAddress(clientUrl: unknown, hostedUrl: unknown): string | undefined; - errorTypeFrom(message: string): string; + errorTypeFrom(message: unknown): string; + errorTypeOf(err: unknown): string; httpRouteFor(functionName: string, routePath: string): string; redactUrls(message: string): string; }; @@ -194,6 +195,11 @@ describe('errorTypeFrom', () => { 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'); @@ -212,6 +218,22 @@ describe('redactUrls', () => { }); }); +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'); @@ -404,6 +426,25 @@ describe('Span.error / clientError — error.type', () => { }); }); +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[]; diff --git a/supabase/functions/_shared/api/CLAUDE.md b/supabase/functions/_shared/api/CLAUDE.md index a12f6325f..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}`, (e as Error).name); + span.error(`Unhandled: ${(e as Error).message}`, errorTypeOf(e)); return internalError(cors); } }); diff --git a/supabase/functions/_shared/api/router.ts b/supabase/functions/_shared/api/router.ts index 653a81ae5..1e743d712 100644 --- a/supabase/functions/_shared/api/router.ts +++ b/supabase/functions/_shared/api/router.ts @@ -14,7 +14,7 @@ import { parseScopeTypeAttribute, } from '../scope/scope-type-attribute.ts'; import type { Span } from '../telemetry/otel.ts'; -import { httpRouteFor } from '../telemetry/span-semconv.ts'; +import { errorTypeOf, httpRouteFor } from '../telemetry/span-semconv.ts'; /** * Request header carrying a client-supplied grouping key (a PR ref, session id, @@ -402,7 +402,7 @@ export function createRouter(routes: Route[], functionName: string) { return res; } catch (e) { const durationMs = Date.now() - startedMs; - hs.error(`${(e as Error).name}: ${(e as Error).message}`).end(); + hs.error(`${(e as Error).name}: ${(e as Error).message}`, errorTypeOf(e)).end(); if (usageUserId !== null) { recordUsageEvent(resolved.db, { userId: usageUserId, diff --git a/supabase/functions/_shared/telemetry/otel.ts b/supabase/functions/_shared/telemetry/otel.ts index 0cded3de5..ea1b81f7c 100644 --- a/supabase/functions/_shared/telemetry/otel.ts +++ b/supabase/functions/_shared/telemetry/otel.ts @@ -33,6 +33,7 @@ import { attributeIoTime, type IoInterval } from './io-ledger.ts'; import { conditionSql, errorTypeFrom, + errorTypeOf, postgrestLogicToSql, querySummary, redactUrls, @@ -514,10 +515,15 @@ export class Span { * 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.attributes['error.type'] = type ?? errorTypeFrom(message); + this.statusMessage = text; + this.attributes['error.message'] = text; + this.attributes['error.type'] = type || errorTypeFrom(text); return this; } @@ -533,11 +539,12 @@ export class Span { * faults, not for client errors. */ clientError(message: string, type?: string): this { - this.attributes['error.message'] = message; + 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(message); + this.attributes['error.type'] = type || errorTypeFrom(text); return this; } @@ -717,8 +724,7 @@ export async function traceRequest( if (response.status >= 500) span.setErrorTypeIfAbsent(String(response.status)); return withTraceparent(response, ctx); } catch (err) { - // `error.type` is derived from the `Name:` prefix — the thrown class. - 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 diff --git a/supabase/functions/_shared/telemetry/span-semconv.ts b/supabase/functions/_shared/telemetry/span-semconv.ts index abbcc8272..bb8ded364 100644 --- a/supabase/functions/_shared/telemetry/span-semconv.ts +++ b/supabase/functions/_shared/telemetry/span-semconv.ts @@ -365,9 +365,28 @@ export const ERROR_TYPE_OTHER = '_OTHER'; * `_OTHER`, never the message itself: `error.type` is a grouping key, and the * free text already rides on `error.message`. */ -export function errorTypeFrom(message: string): string { - const m = /^([A-Za-z_][A-Za-z0-9_.]{0,63})(?::|$)/.exec(message.trim()); - return m ? m[1] : ERROR_TYPE_OTHER; +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 ───────────────────────────────────────────────────────────── diff --git a/supabase/functions/blog/index.ts b/supabase/functions/blog/index.ts index a4847844d..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'; @@ -48,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}`, (e as Error).name); + span.error(`Unhandled: ${(e as Error).message}`, errorTypeOf(e)); return internalError(cors); } }); diff --git a/supabase/functions/memories/index.ts b/supabase/functions/memories/index.ts index 0c825d8be..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}`, (e as Error).name); + 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 655c62f01..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'; @@ -41,7 +42,7 @@ Deno.serve(async (req) => { 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}`, (e as Error).name); + 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 8cafea5a6..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}`, (e as Error).name); + span.error(`Unhandled: ${(e as Error).message}`, errorTypeOf(e)); return internalError(cors); } }); From a54351c05fed92cd7890844778514cded99f04a7 Mon Sep 17 00:00:00 2001 From: "mads.thines" Date: Fri, 2 Oct 2026 14:39:28 +0000 Subject: [PATCH 11/11] docs(edge-otel): point catch-block guidance at errorTypeOf Co-authored-by: dash0-dev[bot] <257284812+dash0-dev[bot]@users.noreply.github.com> --- docs/key-files.md | 2 +- supabase/functions/CLAUDE.md | 5 ++++- supabase/functions/_shared/telemetry/otel.ts | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/key-files.md b/docs/key-files.md index 091eb5543..3fa16cc05 100644 --- a/docs/key-files.md +++ b/docs/key-files.md @@ -28,7 +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`, `httpRouteFor`. Executed by `packages/mcp-core/src/telemetry/span-semconv.spec.ts` | +| `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/supabase/functions/CLAUDE.md b/supabase/functions/CLAUDE.md index e538dd480..105a9ed82 100644 --- a/supabase/functions/CLAUDE.md +++ b/supabase/functions/CLAUDE.md @@ -43,7 +43,10 @@ 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. +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/telemetry/otel.ts b/supabase/functions/_shared/telemetry/otel.ts index ea1b81f7c..42993e804 100644 --- a/supabase/functions/_shared/telemetry/otel.ts +++ b/supabase/functions/_shared/telemetry/otel.ts @@ -720,7 +720,7 @@ export async function traceRequest( 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: …', e.name)`) keeps it. + // (`span.error('Unhandled: …', errorTypeOf(e))`) keeps it. if (response.status >= 500) span.setErrorTypeIfAbsent(String(response.status)); return withTraceparent(response, ctx); } catch (err) {