-
Notifications
You must be signed in to change notification settings - Fork 79
feat: KEEP-1042 per-workflow execution rate gauge #2415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -594,6 +594,90 @@ export async function getWorkflowErrorsByWorkflowFromDb(): Promise<WorkflowError | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Runs started in the last hour, per workflow, and how many of them errored. | ||
| * | ||
| * Feeds the per-workflow execution rate alert. A block or event trigger can | ||
| * start a run for every block the chain produces, and nothing bounded it: one | ||
| * such trigger once wrote over a million runs before anyone noticed the table. | ||
| * A one-hour window catches that within hours. | ||
| * | ||
| * The window is a range scan on idx_workflow_executions_started_at. Only the | ||
| * busiest workflows are kept (see pickWorkflowExecutionRates), so the gauge | ||
| * stays small however many workflows run. | ||
| */ | ||
| export type WorkflowExecutionRate = { | ||
| workflowId: string; | ||
| orgSlug: string; | ||
| runs: number; | ||
| errored: number; | ||
| }; | ||
|
|
||
| export const WORKFLOW_EXECUTION_RATE_TOP_N = 20; | ||
|
|
||
| /** | ||
| * The top workflows by runs, plus the top workflows by errored runs. A workflow | ||
| * that errors on every run can sit below the busiest ones by volume, and error | ||
| * accumulation is half of what the alert is for. | ||
| */ | ||
| export function pickWorkflowExecutionRates( | ||
| rows: WorkflowExecutionRate[], | ||
| topN: number = WORKFLOW_EXECUTION_RATE_TOP_N | ||
| ): WorkflowExecutionRate[] { | ||
| const byRuns = [...rows].sort((a, b) => b.runs - a.runs).slice(0, topN); | ||
| const byErrored = rows | ||
| .filter((row) => row.errored > 0) | ||
| .sort((a, b) => b.errored - a.errored) | ||
| .slice(0, topN); | ||
| const picked = new Map<string, WorkflowExecutionRate>(); | ||
| for (const row of [...byRuns, ...byErrored]) { | ||
| picked.set(row.workflowId, row); | ||
| } | ||
| return [...picked.values()]; | ||
| } | ||
|
|
||
| /** | ||
| * The query behind getWorkflowExecutionRatesFromDb. Exported so a test can | ||
| * EXPLAIN the SQL the collector really sends, not a hand-written copy of it. | ||
| */ | ||
| export function workflowExecutionRatesQuery() { | ||
| return db | ||
| .select({ | ||
| workflowId: workflowExecutions.workflowId, | ||
| orgSlug: sql<string>`COALESCE(${organization.slug}, 'none')`, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| runs: count(), | ||
| errored: sql<string>`COUNT(*) FILTER (WHERE ${inArray(workflowExecutions.status, [...ERROR_STATUSES])})`, | ||
| }) | ||
| .from(workflowExecutions) | ||
| .innerJoin(workflows, eq(workflowExecutions.workflowId, workflows.id)) | ||
| .leftJoin(organization, eq(workflows.organizationId, organization.id)) | ||
| .where(sql`${workflowExecutions.startedAt} >= now() - interval '1 hour'`) | ||
| .groupBy(workflowExecutions.workflowId, organization.slug); | ||
| } | ||
|
|
||
| export async function getWorkflowExecutionRatesFromDb(): Promise< | ||
| WorkflowExecutionRate[] | ||
| > { | ||
| try { | ||
| const rows = await workflowExecutionRatesQuery(); | ||
| return pickWorkflowExecutionRates( | ||
| rows.map((row) => ({ | ||
| workflowId: row.workflowId, | ||
| orgSlug: row.orgSlug, | ||
| runs: Number(row.runs) || 0, | ||
| errored: Number(row.errored) || 0, | ||
| })) | ||
| ); | ||
| } catch (error) { | ||
| logSystemWarn( | ||
| ErrorCategory.DATABASE, | ||
| "[Metrics] Failed to query per-workflow execution rates from DB", | ||
| error | ||
| ); | ||
| return []; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Returning The failure is correlated with what the gauge is watching: a workflow starting runs at high rate is itself a cause of timeouts on the metrics pool, so the series is most likely to disappear exactly when it matters.
|
||
| } | ||
| } | ||
|
|
||
| // TECH-6544: per-(error_category, error_type) error counts over a ROLLING | ||
| // 1-HOUR window, PLATFORM-WIDE (all orgs). System errors are platform faults | ||
| // (DB, RPC, infra, workflow engine), not a managed-client concern, so this | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,147 @@ | ||
| /** | ||
| * The per-workflow execution rate collector against a real Postgres. | ||
| * | ||
| * It runs on every metrics scrape, on the metrics pool and under its statement | ||
| * timeout, so two things matter beyond the numbers: the one-hour window must be | ||
| * answerable from idx_workflow_executions_started_at rather than a scan of the | ||
| * whole run table, and the FILTER on the errored statuses must count what the | ||
| * alert thinks it counts. | ||
| */ | ||
|
|
||
| import "dotenv/config"; | ||
| import { drizzle } from "drizzle-orm/postgres-js"; | ||
| import postgres from "postgres"; | ||
| import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; | ||
| import { | ||
| organization, | ||
| users, | ||
| workflowExecutions, | ||
| workflows, | ||
| } from "../../lib/db/schema"; | ||
|
|
||
| // vitest runs in Node, not an SSR context; the metrics module is server-only. | ||
| vi.mock("server-only", () => ({})); | ||
| // tests/setup.ts globally stubs @/lib/db. The whole point here is the SQL. | ||
| vi.unmock("@/lib/db"); | ||
|
|
||
| const DATABASE_URL = process.env.DATABASE_URL ?? ""; | ||
|
|
||
| const PREFIX = "test_rates_"; | ||
| const USER = `${PREFIX}user`; | ||
| const ORG = `${PREFIX}org`; | ||
| const WF_BUSY = `${PREFIX}wf_busy`; | ||
| const WF_BAD = `${PREFIX}wf_bad`; | ||
| const WF_OLD = `${PREFIX}wf_old`; | ||
| const MINUTE_MS = 60 * 1000; | ||
|
|
||
| type Metrics = typeof import("@/lib/metrics/db-metrics"); | ||
|
|
||
| describe("per-workflow execution rates (real database)", () => { | ||
| let queryClient: ReturnType<typeof postgres>; | ||
| let db: ReturnType<typeof drizzle>; | ||
| let metrics: Metrics; | ||
|
|
||
| async function cleanup(): Promise<void> { | ||
| const like = `${PREFIX}%`; | ||
| await queryClient`DELETE FROM workflow_executions WHERE id LIKE ${like}`; | ||
| await queryClient`DELETE FROM workflows WHERE id LIKE ${like}`; | ||
| await queryClient`DELETE FROM organization WHERE id LIKE ${like}`; | ||
| await queryClient`DELETE FROM users WHERE id LIKE ${like}`; | ||
| } | ||
|
|
||
| beforeAll(async () => { | ||
| // Seeds organizations into a database the retention suite counts in full. | ||
| const host = new URL(DATABASE_URL).hostname; | ||
| if (!["localhost", "127.0.0.1", "::1", "postgres", "db"].includes(host)) { | ||
| throw new Error(`refusing to run against a non-local database: ${host}`); | ||
| } | ||
| queryClient = postgres(DATABASE_URL); | ||
| db = drizzle(queryClient); | ||
| metrics = await import("@/lib/metrics/db-metrics"); | ||
|
|
||
| await cleanup(); | ||
| // The window is now() - 1 hour inside the query, so the seed is relative to | ||
| // the wall clock rather than to a fixed instant. | ||
| const now = Date.now(); | ||
| const minutesAgo = (minutes: number): Date => | ||
| new Date(now - minutes * MINUTE_MS); | ||
|
|
||
| await db.insert(users).values({ | ||
| id: USER, | ||
| name: "rates probe", | ||
| email: `${PREFIX}probe@keeperhub.test`, | ||
| emailVerified: true, | ||
| createdAt: new Date(now), | ||
| updatedAt: new Date(now), | ||
| }); | ||
| await db | ||
| .insert(organization) | ||
| .values({ id: ORG, name: ORG, slug: ORG, createdAt: new Date(now) }); | ||
| await db.insert(workflows).values( | ||
| [WF_BUSY, WF_BAD, WF_OLD].map((id) => ({ | ||
| id, | ||
| name: id, | ||
| userId: USER, | ||
| organizationId: ORG, | ||
| nodes: [], | ||
| edges: [], | ||
| })) | ||
| ); | ||
|
|
||
| const run = ( | ||
| workflowId: string, | ||
| n: number, | ||
| status: "success" | "error" | "system_error", | ||
| startedAt: Date | ||
| ) => ({ | ||
| id: `${workflowId}_run_${status}_${n}`, | ||
| workflowId, | ||
| userId: USER, | ||
| status, | ||
| startedAt, | ||
| }); | ||
| await db.insert(workflowExecutions).values([ | ||
| ...[1, 2, 3, 4, 5].map((n) => run(WF_BUSY, n, "success", minutesAgo(10))), | ||
| run(WF_BAD, 1, "error", minutesAgo(20)), | ||
| run(WF_BAD, 2, "system_error", minutesAgo(30)), | ||
| run(WF_BAD, 3, "success", minutesAgo(40)), | ||
| // Outside the window: two hours old, however many there are. | ||
| ...[1, 2, 3, 4, 5, 6, 7].map((n) => | ||
| run(WF_OLD, n, "error", minutesAgo(120)) | ||
| ), | ||
| ]); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await cleanup(); | ||
| await queryClient.end(); | ||
| }); | ||
|
|
||
| it("counts runs and errored runs per workflow inside the last hour only", async () => { | ||
| const rates = await metrics.getWorkflowExecutionRatesFromDb(); | ||
| const mine = rates | ||
| .filter((rate) => rate.workflowId.startsWith(PREFIX)) | ||
| .sort((a, b) => a.workflowId.localeCompare(b.workflowId)); | ||
|
|
||
| expect(mine).toEqual([ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This asserts through The subject stated in the file header is the window and the |
||
| { workflowId: WF_BAD, orgSlug: ORG, runs: 3, errored: 2 }, | ||
| { workflowId: WF_BUSY, orgSlug: ORG, runs: 5, errored: 0 }, | ||
| ]); | ||
| }); | ||
|
|
||
| it("answers the window from idx_workflow_executions_started_at", async () => { | ||
| const query = metrics.workflowExecutionRatesQuery().toSQL(); | ||
| const plan = await queryClient.begin(async (tx) => { | ||
| // A handful of seeded rows makes a sequential scan the cheapest plan, | ||
| // so it is turned off to ask whether the index can answer at all. | ||
| await tx`SET LOCAL enable_seqscan = off`; | ||
| return await tx.unsafe( | ||
| `EXPLAIN (FORMAT JSON) ${query.sql}`, | ||
| query.params as never[] | ||
| ); | ||
| }); | ||
| expect(JSON.stringify(plan)).toContain( | ||
| '"Index Name":"idx_workflow_executions_started_at"' | ||
| ); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outcome="all"is a superset ofoutcome="errored"on the same metric rather than a partition of it, so errored runs are counted in both series. Any query that does not pinoutcomeadds them together. With the fixture added indb-metrics-cache.test.ts(4200 runs, 3900 errored),sum by (workflow_id)returns 8100, and atopkwithout an outcome filter ranks a workflow that errors on nearly everything at close to double its real volume.keeperhub_workflow_errors_by_workflowdoes not have this problem becauseerror_typeis disjoint (user/system/unknown/na), so summing across it is correct. Emittingoutcome="succeeded"instead of"all", or splitting into two metric names, makes this label safe to aggregate. The consuming rules do not live in this repo, so nothing here would catch a missing filter.