feat: KEEP-1042 per-workflow execution rate gauge - #2415
Conversation
A block or event trigger can start a run for every block, and nothing noticed when one ran away. keeperhub_workflow_executions_last_hour reports, per workflow, the runs started in the last hour and how many of them errored, for the top 20 workflows by runs plus the top 20 by errored runs, so one runaway workflow stands out and the series count stays bounded. The alert rules that read it live in the infra repo. The query is a range scan on idx_workflow_executions_started_at: 8 ms on staging for 1,529 runs in the hour, and the same plan on prod.
joelorzet
left a comment
There was a problem hiding this comment.
Four things below. Two of them are label schema rather than logic, and those get expensive to change once alert rules and dashboards are written against the metric, so they are worth settling before this lands.
| return db | ||
| .select({ | ||
| workflowId: workflowExecutions.workflowId, | ||
| orgSlug: sql<string>`COALESCE(${organization.slug}, 'none')`, |
There was a problem hiding this comment.
COALESCE(slug, 'none') differs from every other DB gauge in this module, which use COALESCE(slug, ANONYMOUS_ORG_SLUG) (_anonymous). Org-less workflows land on a label value nothing else emits, so anything filtering org_slug != "_anonymous" or joining against the sibling gauges drops them, and personal workflows are the ones most likely to carry an unbounded trigger.
none is also already in use in this file as a billing_status value, so the same string ends up meaning two different things. ANONYMOUS_ORG_SLUG is already imported at the top.
| "[Metrics] Failed to query per-workflow execution rates from DB", | ||
| error | ||
| ); | ||
| return []; |
There was a problem hiding this comment.
Returning [] on failure is indistinguishable from "no workflows ran in the last hour". The caller calls workflowExecutionsLastHour.reset() before the loop, so a statement timeout clears the gauge and writes nothing back, and every series drops out of the scrape body instead of holding the previous reading. A series that was present in the previous scrape and is absent from this one goes stale immediately, and a pending alert on it loses its for progress.
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.
getStuckPendingTransactionCountsFromDb returns null in this position and the caller guards with if (stuckPendingTxCounts !== null) before resetting. getLastFinishedExecutionAgeSecondsFromDb spells out the reasoning: leave the gauge unset so staleness governs rather than a misleading value. The same shape fits here.
| workflowExecutionsLastHour.reset(); | ||
| for (const row of executionRates) { | ||
| const labels = { workflow_id: row.workflowId, org_slug: row.orgSlug }; | ||
| workflowExecutionsLastHour.set({ ...labels, outcome: "all" }, row.runs); |
There was a problem hiding this comment.
outcome="all" is a superset of outcome="errored" on the same metric rather than a partition of it, so errored runs are counted in both series. Any query that does not pin outcome adds them together. With the fixture added in db-metrics-cache.test.ts (4200 runs, 3900 errored), sum by (workflow_id) returns 8100, and a topk without an outcome filter ranks a workflow that errors on nearly everything at close to double its real volume.
keeperhub_workflow_errors_by_workflow does not have this problem because error_type is disjoint (user/system/unknown/na), so summing across it is correct. Emitting outcome="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.
| .filter((rate) => rate.workflowId.startsWith(PREFIX)) | ||
| .sort((a, b) => a.workflowId.localeCompare(b.workflowId)); | ||
|
|
||
| expect(mine).toEqual([ |
There was a problem hiding this comment.
This asserts through getWorkflowExecutionRatesFromDb, which applies the top 20 by runs plus top 20 by errored pick, but the seed is 5 runs and 3 runs. On a database that has 20 workflows busier than that inside the window, the seeded rows are cut by the pick and toEqual fails, so the test passes only on a near empty database.
The subject stated in the file header is the window and the FILTER, and workflowExecutionRatesQuery() is already exported for exactly this. Asserting against the query removes the dependency on whatever else is in the table, and pickWorkflowExecutionRates is pure so it covers cleanly on its own.
What and why
A block or event trigger can start a run for every block the chain produces, and nothing noticed when one ran away. This adds the signal a per-workflow execution rate alert needs:
keeperhub_workflow_executions_last_hour, a DB gauge with, per workflow, the runs started in the last hour (outcome="all") and how many of them errored (outcome="errored").Only the busiest workflows are kept: the top 20 by runs, plus the top 20 by errored runs, so a workflow that fails on every run still shows up when it is not among the busiest. That bounds the gauge at 40 workflows and 80 series. The series are reset on every refresh, so a workflow that drops out is cleared instead of keeping its last value. The alert rules that read the gauge are a separate change in the infrastructure repo.
Cost
The collector runs on every metrics scrape, on the metrics pool, under its statement timeout. The one-hour window is a range scan on
idx_workflow_executions_started_at, the index the retention change added:EXPLAIN (ANALYZE, BUFFERS)of the SQL the collector sends: 8.3 ms for 1,529 runs, an index scan, 1,228 buffers.EXPLAINof the same statement: the same plan, an estimated 2,301 runs in the hour.It adds one short query to the scrape's
Promise.all. The pool size is unchanged.Testing
tests/db/workflow-execution-rates.db.test.tsruns the collector against a real Postgres. It checks the runs and errored runs per workflow inside the window only, and runsEXPLAINon the generated SQL to prove the index can answer the window.tests/unit/workflow-execution-rates.test.tscovers which workflows are kept.tests/unit/db-metrics-cache.test.tscovers the gauge series, and that a workflow which drops out is cleared.pnpm check,pnpm type-check,pnpm test:unit(23,282 tests),pnpm test:dbandpnpm buildpass.pnpm test:integrationpasses exceptprotocol-superfluid-onchain, which needs the RPC config only CI has; it fails the same way locally without this change and passes in CI.