Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions packages/cli/src/commands/logs-filter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});
});
5 changes: 5 additions & 0 deletions packages/cli/src/commands/logs-filter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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}`);
Expand Down
6 changes: 6 additions & 0 deletions packages/cli/src/commands/logs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Expand Down Expand Up @@ -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"][],
Expand Down
7 changes: 7 additions & 0 deletions packages/cli/src/commands/metrics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
54 changes: 54 additions & 0 deletions packages/control-plane/src/modules/logs/query.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
3 changes: 3 additions & 0 deletions packages/control-plane/src/modules/logs/query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"),
Expand All @@ -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;
Expand Down
6 changes: 6 additions & 0 deletions packages/control-plane/src/modules/logs/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ export interface LogQuery {
untilMs: number;
/** Filter by tail outcome (any of). Empty = all. */
outcomes: Set<LogEntry["outcome"]>;
/**
* 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<LogEntry["scriptType"]>;
/** Filter by deployment short id (8 hex). Implies scriptType=deployment. */
Expand Down
53 changes: 53 additions & 0 deletions packages/sdk/src/client/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});
});
1 change: 1 addition & 0 deletions packages/sdk/src/client/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions packages/sdk/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
60 changes: 60 additions & 0 deletions packages/sdk/src/logs/is-error.test.ts
Original file line number Diff line number Diff line change
@@ -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> = {}): 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);
});
});
49 changes: 49 additions & 0 deletions packages/sdk/src/logs/is-error.ts
Original file line number Diff line number Diff line change
@@ -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;
}
6 changes: 6 additions & 0 deletions packages/sdk/src/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading
Loading