picatz / picatz/flowstate

observability/metrics: an operator-usable schema, cardinality bounds, and closing #401's metrics testing gap

Open
#526 2 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

design kind/design-record observability
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, unit s)
  • 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:

  1. Are runs failing, and where? pkg/flowstate/v1/engine/activities.go:308-338 (startTaskSpan) opens a span per task/step with flowstate.task.name, flowstate.step.id (when non-empty), and flowstate.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.
  2. 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.duration histogram exists anywhere.
  3. 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.
  4. Are retries climbing? flowstate.attempt is a span attribute (activities.go:331), read once per span, never aggregated. No retry counter.
  5. Are webhook deliveries being refused, and why? pkg/flowstate/v1/server/webhook.go has 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.
  6. Are schedules firing? pkg/flowstate/v1/schedule.go and scheduler.go have 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), and delivery_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-44 already 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 View with 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 way DefaultCostLimit and maxListScan are 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:

  1. The no-secrets property on metric attributes, matching the log and span versions #401 already has (cmd/flow/telemetry_test.go:694-855 for logs, engine/tracing_test.go's requireNoSecretInSpans for spans). This is the one #422's house gate explicitly ties its own closure to.
  2. Each new instrument moves and carries the attributes this issue proposes — one test per instrument is enough; these are narrow.
  3. 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.
  4. 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 unknown fallback 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.