diff --git a/packages/cli/src/commands/logs-filter.test.ts b/packages/cli/src/commands/logs-filter.test.ts index 8a94233..d236008 100644 --- a/packages/cli/src/commands/logs-filter.test.ts +++ b/packages/cli/src/commands/logs-filter.test.ts @@ -198,3 +198,43 @@ describe("describeFilters", () => { ); }); }); + +/** + * `--errors` parity between --follow and --since. + * + * Both sides now call the same `isError` from @solcreek/sdk, so these + * rows mirror control-plane/src/modules/logs/query.test.ts > errors + * filter. A live tail that disagreed with the historical query would be + * worse than no filter — someone debugging an incident would draw the + * wrong conclusion. + */ +describe("errors filter (live tail)", () => { + const TENANT_FAILURE = entry({ + outcome: "ok", + request: { url: "/dashboard", method: "GET" }, + exceptions: [{ name: "Error", message: "Network connection lost.", timestamp: 0 }], + }); + + test("an ok-with-exception entry matches", () => { + expect(matchesClientSide(TENANT_FAILURE, { errors: true })).toBe(true); + }); + + test("a healthy entry does not", () => { + expect( + matchesClientSide( + entry({ outcome: "ok", request: { url: "/", method: "GET", status: 200 } }), + { + errors: true, + }, + ), + ).toBe(false); + }); + + test("it is still invisible to --outcome exception", () => { + expect(matchesClientSide(TENANT_FAILURE, { outcomes: ["exception"] })).toBe(false); + }); + + test("describeFilters names it", () => { + expect(describeFilters({ errors: true })).toContain("errors"); + }); +}); diff --git a/packages/cli/src/commands/logs-filter.ts b/packages/cli/src/commands/logs-filter.ts index d74b488..e5f745f 100644 --- a/packages/cli/src/commands/logs-filter.ts +++ b/packages/cli/src/commands/logs-filter.ts @@ -9,10 +9,14 @@ * for the same flags. */ +import { isError } from "@solcreek/sdk"; import type { LogEntry, LogQueryFilters } from "@solcreek/sdk"; export function matchesClientSide(entry: LogEntry, filters: LogQueryFilters): boolean { if (filters.outcomes?.length && !filters.outcomes.includes(entry.outcome)) return false; + // Same function the server filters with, so `--errors --follow` and + // `--errors --since` cannot disagree. + if (filters.errors && !isError(entry)) return false; if (filters.scriptTypes?.length && !filters.scriptTypes.includes(entry.scriptType)) return false; if (filters.deployment && entry.deployId !== filters.deployment) return false; if (filters.branch && entry.branch !== filters.branch) return false; @@ -42,6 +46,7 @@ function searchMatches(entry: LogEntry, needle: string): boolean { export function describeFilters(filters: LogQueryFilters): string { const bits: string[] = []; if (filters.outcomes?.length) bits.push(`outcome=${filters.outcomes.join(",")}`); + if (filters.errors) bits.push("errors"); if (filters.scriptTypes?.length) bits.push(`scriptType=${filters.scriptTypes.join(",")}`); if (filters.deployment) bits.push(`deployment=${filters.deployment}`); if (filters.branch) bits.push(`branch=${filters.branch}`); diff --git a/packages/cli/src/commands/logs.ts b/packages/cli/src/commands/logs.ts index e2e562b..498fea9 100644 --- a/packages/cli/src/commands/logs.ts +++ b/packages/cli/src/commands/logs.ts @@ -61,6 +61,11 @@ export const logsCommand = defineCommand({ type: "string", description: "Filter by tail outcome. Repeatable via comma (ok,exception).", }, + errors: { + type: "boolean", + description: + "Only failed requests — matches the error count in `creek metrics`. Broader than --outcome exception: also catches an exception thrown after the response started, and 5xx responses.", + }, "script-type": { type: "string", description: "Filter by production/branch/deployment. Repeatable via comma.", @@ -133,6 +138,7 @@ export const logsCommand = defineCommand({ ...(args.outcome ? { outcomes: parseList(args.outcome as string) as LogEntry["outcome"][] } : {}), + ...(args.errors ? { errors: true } : {}), ...(args["script-type"] ? { scriptTypes: parseList(args["script-type"] as string) as LogEntry["scriptType"][], diff --git a/packages/cli/src/commands/metrics.ts b/packages/cli/src/commands/metrics.ts index 03701e2..2e3df99 100644 --- a/packages/cli/src/commands/metrics.ts +++ b/packages/cli/src/commands/metrics.ts @@ -170,6 +170,13 @@ function printHuman(slug: string, r: MetricsResponse): void { consola.log( ` Errors: ${c(fmtNumber(totals.errs), errColor)} ${c(`(${errPct} of invocations)`, "dim")}`, ); + // The number alone used to be a dead end: a tenant saw 40 errors here + // and `creek logs --outcome exception` returned nothing, because most + // of them were `outcome: "ok"` with an exception attached. `--errors` + // is the filter that matches this count. + if (totals.errs > 0) { + consola.log(c(` └─ creek logs --errors --since ${r.period}`, "dim")); + } consola.log(""); printBreakdown("Method", r.breakdowns.method); diff --git a/packages/control-plane/src/modules/logs/query.test.ts b/packages/control-plane/src/modules/logs/query.test.ts index 5b444e6..068cbfb 100644 --- a/packages/control-plane/src/modules/logs/query.test.ts +++ b/packages/control-plane/src/modules/logs/query.test.ts @@ -215,3 +215,57 @@ describe("matchesQuery", () => { expect(matchesQuery(entry(), q)).toBe(false); }); }); + +/** + * The `--errors` filter, and the gap it exists to close. + * + * Reported 2026-07-30: `creek metrics` showed 40 errors while + * `creek logs --outcome exception` returned nothing, because the failing + * entries were `outcome: "ok"` with an exception attached. `--outcome` is + * modelled on Cloudflare's TailOutcome enum and cannot express "this + * request went wrong"; `errors=1` can. + */ +describe("errors filter", () => { + const TENANT_FAILURE = entry({ + outcome: "ok", + request: { url: "/dashboard", method: "GET" }, + exceptions: [{ name: "Error", message: "Network connection lost.", timestamp: 0 }], + }); + + test("the reported entry is invisible to --outcome exception", () => { + const q = parseQuery(new URLSearchParams({ outcome: "exception" }), NOW); + expect(matchesQuery(TENANT_FAILURE, q)).toBe(false); + }); + + test("...and visible to errors=1", () => { + const q = parseQuery(new URLSearchParams({ errors: "1" }), NOW); + expect(matchesQuery(TENANT_FAILURE, q)).toBe(true); + }); + + test("errors=1 excludes healthy requests", () => { + const q = parseQuery(new URLSearchParams({ errors: "1" }), NOW); + expect( + matchesQuery(entry({ outcome: "ok", request: { url: "/", method: "GET", status: 200 } }), q), + ).toBe(false); + }); + + test("errors=1 catches a 5xx that reported outcome ok", () => { + const q = parseQuery(new URLSearchParams({ errors: "1" }), NOW); + expect( + matchesQuery(entry({ outcome: "ok", request: { url: "/", method: "GET", status: 500 } }), q), + ).toBe(true); + }); + + test("absent errors param leaves everything matching", () => { + const q = parseQuery(new URLSearchParams(), NOW); + expect(q.errorsOnly).toBe(false); + expect(matchesQuery(entry({ outcome: "ok" }), q)).toBe(true); + }); + + test("errors combines with outcome as an AND, not a widening", () => { + // Both filters apply; `--errors` must not quietly relax `--outcome`. + const q = parseQuery(new URLSearchParams({ errors: "1", outcome: "canceled" }), NOW); + expect(matchesQuery(TENANT_FAILURE, q)).toBe(false); + expect(matchesQuery(entry({ outcome: "canceled" }), q)).toBe(true); + }); +}); diff --git a/packages/control-plane/src/modules/logs/query.ts b/packages/control-plane/src/modules/logs/query.ts index 616aea3..f129377 100644 --- a/packages/control-plane/src/modules/logs/query.ts +++ b/packages/control-plane/src/modules/logs/query.ts @@ -13,6 +13,7 @@ * (caller passes Date.now() so tests can pin time). */ +import { isError } from "@solcreek/sdk"; import type { LogEntry, LogQuery } from "./types.js"; const DEFAULT_LIMIT = 100; @@ -52,6 +53,7 @@ export function parseQuery(params: URLSearchParams, now: number): LogQuery { sinceMs, untilMs, outcomes: pickSet(params.getAll("outcome"), VALID_OUTCOMES), + errorsOnly: params.get("errors") === "1", scriptTypes: pickSet(params.getAll("scriptType"), VALID_SCRIPT_TYPES), deployId: params.get("deployment"), branch: params.get("branch"), @@ -64,6 +66,7 @@ export function parseQuery(params: URLSearchParams, now: number): LogQuery { export function matchesQuery(entry: LogEntry, q: LogQuery): boolean { if (entry.timestamp < q.sinceMs || entry.timestamp > q.untilMs) return false; if (q.outcomes.size > 0 && !q.outcomes.has(entry.outcome)) return false; + if (q.errorsOnly && !isError(entry)) return false; if (q.scriptTypes.size > 0 && !q.scriptTypes.has(entry.scriptType)) return false; if (q.deployId !== null && entry.deployId !== q.deployId) return false; if (q.branch !== null && entry.branch !== q.branch) return false; diff --git a/packages/control-plane/src/modules/logs/types.ts b/packages/control-plane/src/modules/logs/types.ts index f4043ac..1b73d92 100644 --- a/packages/control-plane/src/modules/logs/types.ts +++ b/packages/control-plane/src/modules/logs/types.ts @@ -38,6 +38,12 @@ export interface LogQuery { untilMs: number; /** Filter by tail outcome (any of). Empty = all. */ outcomes: Set; + /** + * Only entries the shared `isError` predicate classifies as failures. + * Orthogonal to `outcomes` — see @solcreek/sdk's is-error.ts for why + * this is a separate filter rather than a widening of that one. + */ + errorsOnly: boolean; /** Filter by script variant. Empty = all. */ scriptTypes: Set; /** Filter by deployment short id (8 hex). Implies scriptType=deployment. */ diff --git a/packages/sdk/src/client/client.test.ts b/packages/sdk/src/client/client.test.ts index 2c1fed3..659a905 100644 --- a/packages/sdk/src/client/client.test.ts +++ b/packages/sdk/src/client/client.test.ts @@ -111,3 +111,56 @@ describe("CreekClient", () => { expect(String(url)).toContain("name=chunks%2Fssr%20a.js"); }); }); + +/** + * getLogs query-string serialization. + * + * Raised in Copilot review of the `--errors` PR, and a genuine hole: the + * predicate, the server-side filter and the `--follow` client filter were + * all covered, but nothing asserted that the client actually PUTS the + * filter on the wire. If it silently dropped `errors`, `--errors --since` + * would return unfiltered results while `--errors --follow` filtered + * correctly — exactly the metrics-vs-logs inconsistency that PR exists to + * remove, reintroduced one layer down. + */ +describe("CreekClient.getLogs — filter serialization", () => { + const client = new CreekClient("http://localhost:8787", "test-token"); + + function urlOf(): URL { + return new URL(mockFetch.mock.calls[0][0] as string, "http://localhost:8787"); + } + + test("errors: true is sent as errors=1", async () => { + mockFetch.mockResolvedValue(jsonResponse(200, { entries: [], truncated: false })); + + await client.getLogs("blog", { errors: true }); + + expect(urlOf().searchParams.get("errors")).toBe("1"); + }); + + test("the param is absent when errors is unset or false", async () => { + // Sending errors=0 would be worse than sending nothing — the server + // reads `=== "1"`, so a stray value would be silently ignored rather + // than rejected, and the difference would never surface. + mockFetch.mockResolvedValue(jsonResponse(200, { entries: [], truncated: false })); + await client.getLogs("blog", { since: "1h" }); + expect(urlOf().searchParams.has("errors")).toBe(false); + + mockFetch.mockReset(); + mockFetch.mockResolvedValue(jsonResponse(200, { entries: [], truncated: false })); + await client.getLogs("blog", { errors: false }); + expect(urlOf().searchParams.has("errors")).toBe(false); + }); + + test("errors rides alongside outcome rather than replacing it", async () => { + // The server ANDs the two; both must reach it for that to hold. + mockFetch.mockResolvedValue(jsonResponse(200, { entries: [], truncated: false })); + + await client.getLogs("blog", { errors: true, outcomes: ["canceled"], since: "6h" }); + + const p = urlOf().searchParams; + expect(p.get("errors")).toBe("1"); + expect(p.getAll("outcome")).toEqual(["canceled"]); + expect(p.get("since")).toBe("6h"); + }); +}); diff --git a/packages/sdk/src/client/index.ts b/packages/sdk/src/client/index.ts index 7af5e98..07eb0a2 100644 --- a/packages/sdk/src/client/index.ts +++ b/packages/sdk/src/client/index.ts @@ -230,6 +230,7 @@ export class CreekClient { if (filters?.branch) url.searchParams.set("branch", filters.branch); if (filters?.search) url.searchParams.set("search", filters.search); if (filters?.limit !== undefined) url.searchParams.set("limit", String(filters.limit)); + if (filters?.errors) url.searchParams.set("errors", "1"); for (const o of filters?.outcomes ?? []) url.searchParams.append("outcome", o); for (const s of filters?.scriptTypes ?? []) url.searchParams.append("scriptType", s); for (const l of filters?.levels ?? []) url.searchParams.append("level", l); diff --git a/packages/sdk/src/index.ts b/packages/sdk/src/index.ts index db57d7f..cfb2099 100644 --- a/packages/sdk/src/index.ts +++ b/packages/sdk/src/index.ts @@ -4,3 +4,4 @@ export * from "./framework/index.js"; export * from "./client/index.js"; export * from "./bindings/index.js"; export * from "./doctor/index.js"; +export * from "./logs/is-error.js"; diff --git a/packages/sdk/src/logs/is-error.test.ts b/packages/sdk/src/logs/is-error.test.ts new file mode 100644 index 0000000..2c84e86 --- /dev/null +++ b/packages/sdk/src/logs/is-error.test.ts @@ -0,0 +1,60 @@ +/** + * The predicate `creek metrics` counts with and `creek logs --errors` + * filters by. These MUST agree — a tenant reported seeing "40 errors" in + * metrics and getting an empty list from logs (2026-07-30). + * + * The table below is the contract. tail-worker/src/analytics.ts holds a + * mirror of this function (it has no dependencies and cannot import the + * SDK); its own test drives the same cases, so a divergence shows up as + * a failure on whichever side was edited. + */ + +import { describe, it, expect } from "vitest"; +import { isError, type ErrorClassifiable } from "./is-error"; + +function entry(over: Partial = {}): ErrorClassifiable { + return { outcome: "ok", exceptions: [], ...over }; +} + +const EXCEPTION = { name: "Error", message: "Network connection lost.", timestamp: 0 }; + +describe("isError", () => { + it("a clean ok request is not an error", () => { + expect(isError(entry({ request: { status: 200 } }))).toBe(false); + }); + + it("any non-ok outcome is an error", () => { + for (const outcome of [ + "exception", + "exceededCpu", + "exceededMemory", + "canceled", + "responseStreamDisconnected", + "scriptNotFound", + "unknown", + ] as const) { + expect(isError(entry({ outcome }))).toBe(true); + } + }); + + it('an "ok" invocation that recorded an exception IS an error', () => { + // The exact shape the tenant's failing requests had, and the one + // `--outcome exception` could never match. An exception thrown after + // the response started streaming leaves outcome "ok". + expect(isError(entry({ outcome: "ok", exceptions: [EXCEPTION] }))).toBe(true); + }); + + it('an "ok" invocation that returned 5xx IS an error', () => { + expect(isError(entry({ outcome: "ok", request: { status: 503 } }))).toBe(true); + }); + + it("4xx is not an error — that is the client's fault, not the worker's", () => { + expect(isError(entry({ request: { status: 404 } }))).toBe(false); + }); + + it("a missing status is not treated as 5xx", () => { + // The tenant's `/dashboard` entry had no status code at all. Reading + // `undefined >= 500` as true would be wrong for a different reason. + expect(isError(entry({ outcome: "ok", request: {} }))).toBe(false); + }); +}); diff --git a/packages/sdk/src/logs/is-error.ts b/packages/sdk/src/logs/is-error.ts new file mode 100644 index 0000000..c72848e --- /dev/null +++ b/packages/sdk/src/logs/is-error.ts @@ -0,0 +1,49 @@ +/** + * "Did this request go wrong?" — the canonical predicate. + * + * This is a DERIVED notion, deliberately broader than `outcome`. A + * request can complete with `outcome: "ok"` and still have failed the + * visitor: an exception thrown after the response started streaming, or + * a 5xx the worker returned on purpose. `outcome` alone sees neither. + * + * Why it exists as shared code (reported 2026-07-30): + * + * `creek metrics` counts errors with this rule — tail-worker stamps it + * into the Analytics Engine `double2` column, which the metrics SQL sums + * as `errs`. But `creek logs --outcome exception` filtered on the raw + * `outcome` field alone. A tenant saw "40 errors" in metrics and got an + * empty list from logs, because their failing entries were + * `outcome: "ok"` with a `Network connection lost.` exception. They had + * to dump everything and grep it themselves. + * + * The gap was not a broken filter — it was a MISSING one. `--outcome` is + * modelled on Cloudflare's TailOutcome enum and answers a different, + * narrower question. So `--errors` exists alongside it rather than + * changing what `--outcome` means, and both are backed by this function. + * + * `outcome` itself is never rewritten to paper over the difference: it is + * Cloudflare's fact, passed through verbatim, and "responded fine then + * threw" is genuinely distinct from "the invocation failed". + * + * ⚠️ tail-worker keeps its own copy (`src/analytics.ts`) because it has + * no dependencies and cannot import this package. That copy is the WRITE + * side — it decides the AE column this predicate is meant to agree with. + * Change one, change the other, or `creek metrics` and `creek logs + * --errors` will disagree again. + */ + +import type { LogEntry } from "../types/index.js"; + +/** Minimal shape the predicate needs — accepts any LogEntry mirror. */ +export interface ErrorClassifiable { + outcome: LogEntry["outcome"]; + request?: { status?: number }; + exceptions: unknown[]; +} + +export function isError(entry: ErrorClassifiable): boolean { + if (entry.outcome !== "ok") return true; + if (entry.exceptions.length > 0) return true; + if (entry.request?.status !== undefined && entry.request.status >= 500) return true; + return false; +} diff --git a/packages/sdk/src/types/index.ts b/packages/sdk/src/types/index.ts index 3e6fa66..eecfe36 100644 --- a/packages/sdk/src/types/index.ts +++ b/packages/sdk/src/types/index.ts @@ -224,6 +224,12 @@ export interface LogQueryFilters { /** "now" or ISO timestamp. */ until?: string; outcomes?: LogEntry["outcome"][]; + /** + * Only entries `isError()` classifies as failures. Broader than + * `outcomes` and intentionally orthogonal to it — this is the filter + * that lines up with the error count in `creek metrics`. + */ + errors?: boolean; scriptTypes?: LogEntry["scriptType"][]; /** 8-hex deployId — implies scriptType=deployment. */ deployment?: string; diff --git a/packages/tail-worker/src/analytics.test.ts b/packages/tail-worker/src/analytics.test.ts index 660127a..06707bf 100644 --- a/packages/tail-worker/src/analytics.test.ts +++ b/packages/tail-worker/src/analytics.test.ts @@ -141,3 +141,78 @@ describe("writeBatchToAnalytics", () => { expect(points.map((p) => p.blobs?.[2])).toEqual(["production", "production", "branch"]); }); }); + +/** + * Mirror check for the write-side `isError`. + * + * Same cases as @solcreek/sdk's is-error.test.ts. This worker cannot + * import the SDK, so the two copies are kept honest by driving both + * through the same table — if either side is edited alone, its own suite + * fails and the other file is named right here. + * + * The stakes: this function decides the AE column `creek metrics` sums, + * and the SDK copy decides what `creek logs --errors` returns. They + * disagreed once already — metrics reported 40 errors that logs could + * not find (reported 2026-07-30). + */ +describe("isError parity with @solcreek/sdk (via the AE double2 column)", () => { + const base = { + v: 1 as const, + timestamp: 0, + team: "acme", + project: "site", + scriptType: "production" as const, + logs: [], + }; + const errFlag = (e: Record): number => { + const points: Array<{ doubles?: number[] }> = []; + writeBatchToAnalytics( + { ANALYTICS: { writeDataPoint: (dp: { doubles?: number[] }) => points.push(dp) } as never }, + [{ ...base, ...e } as never], + ); + return points[0].doubles![1]; + }; + const EXC = { name: "Error", message: "Network connection lost.", timestamp: 0 }; + + test("a clean ok request is not an error", () => { + expect( + errFlag({ outcome: "ok", exceptions: [], request: { url: "u", method: "GET", status: 200 } }), + ).toBe(0); + }); + + test("any non-ok outcome is an error", () => { + for (const outcome of [ + "exception", + "exceededCpu", + "exceededMemory", + "canceled", + "responseStreamDisconnected", + "scriptNotFound", + "unknown", + ]) { + expect(errFlag({ outcome, exceptions: [] })).toBe(1); + } + }); + + test('an "ok" invocation that recorded an exception IS an error', () => { + expect(errFlag({ outcome: "ok", exceptions: [EXC] })).toBe(1); + }); + + test('an "ok" invocation that returned 5xx IS an error', () => { + expect( + errFlag({ outcome: "ok", exceptions: [], request: { url: "u", method: "GET", status: 503 } }), + ).toBe(1); + }); + + test("4xx is not an error", () => { + expect( + errFlag({ outcome: "ok", exceptions: [], request: { url: "u", method: "GET", status: 404 } }), + ).toBe(0); + }); + + test("a missing status is not treated as 5xx", () => { + expect(errFlag({ outcome: "ok", exceptions: [], request: { url: "u", method: "GET" } })).toBe( + 0, + ); + }); +}); diff --git a/packages/tail-worker/src/analytics.ts b/packages/tail-worker/src/analytics.ts index 0f7b57c..8346bc9 100644 --- a/packages/tail-worker/src/analytics.ts +++ b/packages/tail-worker/src/analytics.ts @@ -60,6 +60,20 @@ function statusBucket(status: number | undefined): string { return "5xx"; } +/** + * ⚠️ MIRROR of `isError` in @solcreek/sdk (src/logs/is-error.ts). + * + * This copy is the WRITE side: its result becomes the AE `double2` + * column that `creek metrics` sums as the error count. The SDK copy is + * the READ side, shared by the control-plane log filter and the CLI's + * `--follow` filter, and `creek logs --errors` is meant to return + * exactly the requests counted here. + * + * Duplicated because this worker has no dependencies and cannot import + * the SDK — same reason LogEntry is re-declared in types.ts. Change one, + * change the other, or the two surfaces disagree again (they already did + * once: metrics reported 40 errors that `creek logs` could not find). + */ function isError(entry: LogEntry): boolean { if (entry.outcome !== "ok") return true; if (entry.exceptions.length > 0) return true;