observability/metrics: an operator-usable schema, cardinality bounds, and closing #401's metrics testing gap
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 9
- Forks
- 0
- Avg merge
- 3h 3m
- Merged PRs (30d)
- 509
Description
Workstream of #522, and the metrics half of the gap #401 recorded ("metrics not at all"). This is a research/design pass, no code changes.
What is measured today
All twelve-ish instruments turn out to be six, and all six live in one package. Every custom metric instrument in the tree is declared in pkg/flowstate/v1/plugin/telemetry.go:22-27 and created at pkg/flowstate/v1/plugin/telemetry.go:40-45:
flowstate.plugin.operation.duration(Float64Histogram, units)flowstate.plugin.calls(Int64Counter)flowstate.plugin.health.checks(Int64Counter)flowstate.plugin.restarts(Int64Counter)flowstate.plugin.launch.failures(Int64Counter)flowstate.plugin.protocol.errors(Int64Counter)
Recorded at plugin/telemetry.go:64-65 (duration + calls, on every plugin RPC), plugin/plugin.go:346 and :378 (health, on health-check transitions), plugin/plugin.go:709 (restarts, on a supervised restart), and plugin/launch.go:86 and :89 (launch failures, protocol errors). That is the entire custom metrics surface. grep -rl otel/metric across the tree turns up exactly five non-test files, all under pkg/flowstate/v1/plugin/ or plugin/sdk/.
Beyond that, cmd/flow/telemetry.go:224 wires the Temporal SDK's own opentelemetry.NewMetricsHandler, which emits Temporal's own upstream-named instruments (workflow task latency, activity metrics, etc.). #422 leaves those upstream-named deliberately ("renaming other people's metrics is its own sin") and this issue doesn't revisit that call.
So today: plugin process health and plugin RPC latency/error rate are covered. Nothing else is.
What an operator would ask during an incident, and what answers today
None of these have an instrument:
- Are runs failing, and where?
pkg/flowstate/v1/engine/activities.go:308-338(startTaskSpan) opens a span per task/step withflowstate.task.name,flowstate.step.id(when non-empty), andflowstate.attempt, and records outcome as span status. There is no counter alongside it. A step failure is visible in a trace someone already has open, not in a rate. - Which step, how long? Same function is also the only place step duration exists, and it exists only as span duration, not a histogram. No
flowstate.step.durationhistogram exists anywhere. - Is the worker keeping up? No queue-depth, no in-flight-activity gauge/updowncounter, no backlog signal outside whatever Temporal's own SDK metrics expose.
- Are retries climbing?
flowstate.attemptis a span attribute (activities.go:331), read once per span, never aggregated. No retry counter. - Are webhook deliveries being refused, and why?
pkg/flowstate/v1/server/webhook.gohas a documented, deliberate refusal model — size bound, concurrency bound (DefaultWebhookConcurrency,webhook.go:87-97), HMAC failure, CEL failure — each refusal is a distinct HTTP status and a log line (WithWebhookLogger,webhook.go:233-239). None of it is a counter. An operator cannot tell "delivery rate dropped because the sender stopped" from "delivery rate dropped because we're refusing everything with 503" without reading logs. - Are schedules firing?
pkg/flowstate/v1/schedule.goandscheduler.gohave no metrics import at all.
Ranked by incident usefulness, the missing set is: step/run outcome counter (are things failing) > step duration histogram (which step is slow) > retry/attempt counter (is backoff working or hiding a broken dependency) > webhook refusal counter by reason (is the receiver rejecting good traffic) > worker saturation gauge (is capacity the problem) > schedule fire/miss counter (lowest — schedules are the least urgent path in an incident, most useful for a slow leak, not a page).
Attribute-key consistency (invariant 1)
Within the existing metrics, the keys are internally consistent and match the sibling span attributes exactly: flowstate.plugin.name and flowstate.task.name appear identically as metric attributes (plugin/telemetry.go:50-52, plugin/plugin.go:346,378) and as span attributes in the same file. flowstate.plugin.outcome (metric-only, telemetry.go:63) and flowstate.plugin.health.status (plugin.go:346,378) don't collide with anything else in the tree. So invariant 1 holds for what exists, but it holds on a small surface — there is nothing to compare because run id, workflow name, and trigger identity have never been spelled as a metric attribute anywhere.
They are spelled two different ways elsewhere, though, which the new schema needs to pick between rather than invent a third:
- Span attributes use a dotted, prefixed form:
flowstate.step.id(engine/activities.go:322, the only run/step-shaped span attribute that exists today). - The data-plane (webhook JSON, CEL, run filters) uses flat snake_case with no prefix:
workflow_id/run_id(server/webhook.go:615,618,runfilter.go:74-75,run_identity.go:82-83), anddelivery_id(webhook.go:622).
These are different boundaries — one is telemetry, one is the wire/CEL surface — so this isn't yet a violation of invariant 1. But it means whoever defines flowstate.run.id / flowstate.workflow.name for metrics has two existing conventions pulling in different directions, and should follow the telemetry-side one (flowstate.step.id, flowstate.task.name, flowstate.plugin.name) for consistency with the spans and logs the metric will sit next to, not the wire-format one.
One explicit boundary already drawn, worth keeping: plugin/sdk/telemetry.go:43-44 states outright that "Plugin and task names are permitted attributes. Workflow, run, and step identifiers must be used only on spans and logs, never metrics." That line is why the existing plugin metrics never carry a run or step id. Any new engine-side metric schema has to either honor that same rule (aggregate by task/workflow name and step id-class, never by run instance) or explicitly override it with a stated cardinality bound — see below.
Cardinality — the important part
Everything currently emitted as a metric attribute is bounded by construction:
flowstate.plugin.name/flowstate.task.name: bounded by the number of plugins and tasks a deployment has installed, which is operator-controlled, not attacker-controlled.flowstate.plugin.outcome,flowstate.plugin.health.status: fixed small enumerations.
Everything proposed above is not automatically bounded, and each has a different owner for the bound:
| Candidate label | Who chooses the value | Bound |
|---|---|---|
flowstate.workflow.name |
Workflow author | Bounded in practice by how many distinct workflow files a deployment loads — but a workflow name is a free string in the Flowfile schema, so nothing stops an author (or a generator, or a fuzzed input) from minting one name per run. Do not derive this label from anything not drawn from the loaded workflow registry at metric-emission time. If the run's workflow name isn't found in the registry, label it flowstate.workflow.name="unknown" rather than passing the raw string through. |
flowstate.step.id |
Workflow author, but scoped to one workflow's YAML | Bounded per workflow (an author can still write thousands of steps, but the DSL already imposes some practical ceiling via validation elsewhere) — safer than workflow name because it's not attacker-facing, but still needs a documented cap or a "top-N + other" view rather than an open string label. |
flowstate.run.id / any run identity |
Generated per execution | Unbounded by definition — a run id must never be a metric attribute. One run id = one time series in most backends' worst case; a busy deployment mints thousands per hour. This is exactly what a trace/log field is for, not a metric label. |
flowstate.trigger.name |
Workflow author (schedule/webhook trigger names are user-chosen) | Same shape as workflow name — bound to the trigger registry, fall back to unknown for anything not found there. |
flowstate.delivery.id |
The external caller (a webhook sender) | Unbounded and attacker-controlled — a hostile sender can mint an arbitrary number of delivery ids just by POSTing. Never a metric label, full stop; it belongs on the span and the log line the refusal already produces (webhook.go's logger), never on a counter. |
flowstate.webhook.refusal_reason |
The receiver's own classification | Bounded — it's the receiver's own enum (body too large, concurrency exceeded, bad HMAC, CEL failure), not caller input. Safe as a label. |
| tenant / namespace | Operator-provisioned | Bounded in practice by how many tenants a deployment has provisioned, which is an operator decision, not caller input — safe, same shape as plugin name. |
Proposed rule, matching the house pattern used for CEL cost and paged listing: bound the resource the attacker or author controls, not the one that's convenient. Concretely:
- Never put a generated identifier (run id, delivery id, execution id) on a metric. It goes on spans and logs, which is exactly what invariant 2/#401 and
plugin/sdk/telemetry.go:43-44already say for the plugin surface — extend the same rule to the engine's metrics rather than re-deciding it per subsystem. - Any author-chosen string used as a label (workflow name, step id, trigger name) is resolved against the deployment's own loaded registry before being used as a label value; a value not found in the registry is emitted as a fixed sentinel (
unknown), never passed through raw. This is what keeps a workflow named by an attacker-controlled webhook payload (if that's ever a code path) from becoming a label. - Where a bound is hit — say a cap on distinct step ids or trigger names actually recorded — the overflow needs a stated behavior: drop the label to the sentinel and keep counting under it (an OTel
Viewwith an attribute filter, per the Go metrics guidance, is the mechanism), not silently drop the whole data point and not let cardinality grow unbounded. State this explicitly in the metric's doc comment, the same wayDefaultCostLimitandmaxListScanare stated rather than implied.
Testing (closing #401)
#401 already lays out the shape: sdkmetric.NewManualReader() plus a sdkmetric.MeterProvider built with it, run the code under test, reader.Collect(ctx, &rm), assert with metricdatatest.AssertEqual (name, aggregation, value, attributes). In priority order:
- The no-secrets property on metric attributes, matching the log and span versions #401 already has (
cmd/flow/telemetry_test.go:694-855for logs,engine/tracing_test.go'srequireNoSecretInSpansfor spans). This is the one #422's house gate explicitly ties its own closure to. - Each new instrument moves and carries the attributes this issue proposes — one test per instrument is enough; these are narrow.
- The Temporal metrics handler wiring: run a workflow through the local driver with the manual reader attached, assert the SDK's own instruments appear with expected resource attributes.
- The cardinality-bound behavior: an author-chosen name not present in the workflow/trigger registry resolves to the sentinel label, not the raw string — this is the test that would have caught the tenancy-style ambiguity bug class before it happens.
For this to run in CI without flaking, the same conditions isolateTelemetry already establishes for logs need to hold for meters: a fresh no-op (or manual-reader-backed) provider per test, no dependence on wall-clock export intervals (the manual reader sidesteps PeriodicReader entirely — this is exactly why #401 recommends it over a real exporter), and no shared global MeterProvider state leaking between parallel tests. isolateTelemetry's existing "fresh-noop-provider" approach extends to meters unchanged per #401's closing note, so the mechanism doesn't need to be invented, just applied to metrics test files that don't exist yet.
Local vs. durable driver
The existing plugin metrics apply identically to both drivers today, because plugin RPCs happen the same way regardless of which engine drives the workflow — there's no driver-conditional code in plugin/telemetry.go. That symmetry should hold for whatever engine-level metrics land here (step outcome, step duration, retry count): both drivers execute the same step model per CLAUDE.md's "both execution drivers must agree" section, and a metric that only fires on one driver would make a local run stop predicting what the metric dashboards say about production, which is the exact failure mode that section exists to prevent.
Worker saturation (queue depth, in-flight activity count) is the one candidate that's legitimately driver-specific: it only means something for the durable driver's worker process, since local execution has no queue to be behind on. That one should be scoped explicitly to the durable driver rather than stubbed out or faked for local, and documented as such in its own doc comment so nobody goes looking for it in a local run's output.
Summary of what this workstream should land
- A stable
flowstate.*metric and attribute schema for step/run outcome, step duration, retry count, webhook refusal-by-reason, and (durable-only) worker saturation, spelled to match the existing span attribute convention (flowstate.step.id,flowstate.task.name) rather than the wire-format one (run_id,workflow_id). - Explicit cardinality bounds per label as tabled above, with registry-lookup-or-sentinel as the mechanism and an
unknownfallback documented per label. - A hard rule, stated once and applied everywhere: generated identifiers (run id, delivery id) never become metric attributes.
- Tests per #401's shape: manual reader,
metricdatatest, one no-secrets test, one per-instrument test, one registry-fallback (cardinality) test. - Verification that both drivers emit the same engine-level metrics, with worker-saturation metrics explicitly scoped to the durable driver only.
Related: #522 (umbrella and invariants), #401 (metrics testing gap this closes), #422 (attribute schema and containment, which this workstream's schema needs to register into once that registry exists).
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with pkg/flowstate/v1/plugin/telemetry.go, engine/activities.go, and server/webhook.go to inventory the existing instruments and proposed signals. Review #401's manual-reader pattern and cmd/flow/telemetry_test.go:694-855 for the testing gates. Done means a documented operator-usable metric schema, explicit cardinality rules, and a focused test plan covering attributes, secrets, wiring, and sentinel behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- backend, observability, testing
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100