feat(metrics): record stream, queue and per-shard counters (#44) - #64
Merged
Conversation
Several metric families were registered and exported but never written, so
they scraped as 0 forever. That is worse than being absent: a dashboard built
on flo_stream_append_records_total{topic="orders"} reads "no traffic" on a
stream actively taking writes.
Wired the write paths to the record* methods that already existed:
- stream append — recordAppend with the batch's logical record count, so a
three-record batch counts three rather than one entry
- stream read — recordRead, likewise summing batch contents, and
recordEmptyRead when a read returns nothing
- queue enqueue/dequeue — recordEnqueue, recordDequeue, recordEmptyDequeue
- per-shard — shardMetrics() had no callers at all, so every flo_shard_* row
exported 0 while its global flo_* equivalent moved. The shard now resolves
its metrics once at wire-up and records alongside the global counters.
registerStream and registerQueue already return the per-entity metrics and the
handlers already called them, discarding the result — so most of this is
capturing a pointer that was being thrown away.
## Tests
Four e2e tests asserting values, not presence: append three records and assert
the counter says 3, read them back and assert the read counter says 3, enqueue
two and dequeue and assert both, and assert per-shard commands are non-zero.
A parser pulls the numeric value out of the exposition line rather than
substring-matching, so a wrong number fails instead of passing on the metric
name appearing.
All four fail with the instrumentation stashed — verified rather than assumed,
since a metrics test that passes uninstrumented is exactly what let this hide.
## Not in this change
- processing and workflow: neither handler has a metrics_registry field and the
shard does not wire one, so those need plumbing first rather than a call at
an existing site. Both keep separate module-local metrics that are unrelated
to the registry families.
- KVMetrics and TieredLogMetrics: exportPrometheus does not emit them and
registerTieredLog has no callers. Whether to export or delete them is a
product decision, not a mechanical fix.
The note in docs/deployment/clustering.mdx covers all of these families, so it
stays until the rest land.
oreofeolurin
added a commit
that referenced
this pull request
Aug 30, 2026
#67) Completes the instrumentation started in #64. Both families exported 0 regardless of traffic because neither handler ever touched the registry. Contrary to the note in #64, no plumbing was needed: every dispatch function in both handlers already receives `shard: *Shard`, which carries the registry. The counters go in at the points every transition already passes through: - workflow: `completeRun` is the single terminal transition, so completed / failed / cancelled / timed_out all record there; started in `handleStart`, signal delivery in `handleSignal`, and step execution at the four `step_completed` history sites (all four have `shard` in scope). - processing: submitted in `handleSubmit`, and `persistStatusChange` covers cancelled and stopped. ## A crash this surfaced Eight workflow unit tests began aborting. Their test shard is built as `var shard: Shard = undefined` with fields assigned one at a time, so `metrics_registry` held garbage and the new read dereferenced it. The tests passed before only because nothing in the workflow path read that field. Both fields are now assigned in the helper. This is the third instance of the same hazard today — `allocator.create` and `= undefined` both leave field defaults inapplicable, and the compiler cannot see it. ## Tests Two more value-asserting e2e tests: start two workflows and assert `flo_workflow_started_total` is 2; submit a job and assert `flo_processing_jobs_submitted_total` is 1. Both fail with the instrumentation stashed. test-unit, test-integration, and the metrics / workflow / processing e2e filters all pass. ## Still open on #44 KVMetrics and TieredLogMetrics remain unexported and unregistered. Whether to emit or delete them is a product decision, so #44 stays open for that alone.
This was referenced Aug 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Partial fix for #44 — the families with the clearest user impact. Scope is stated explicitly at the bottom rather than implied.
The problem
Families were registered and exported but never written, so they scraped as
0forever:...on a stream actively taking writes. That's worse than the family being absent — a dashboard or alert reads "no traffic" instead of "not instrumented".
What's wired
recordAppendwith the batch's logical record count — a 3-record batch counts 3, not 1 entryrecordRead(same batch expansion),recordEmptyReadon nothingrecordEnqueue,recordDequeue,recordEmptyDequeueshardMetrics()had zero callers, so everyflo_shard_*row exported 0 while its global equivalent movedregisterStreamandregisterQueuealready return the per-entity metrics, and the handlers already called them and discarded the result with_ =. Most of this is capturing a pointer that was being thrown away.Tests assert values, not presence
Four e2e tests, with a parser that pulls the number out of the exposition line rather than substring-matching:
All four fail with the instrumentation stashed — I checked rather than assumed. A metrics test that passes uninstrumented is precisely what let this hide, and the issue calls that out specifically.
Verified:
test-unit,test-integration, and themetrics,e2e/stream,e2e/queuefilters all pass.Deliberately not in this change
processing and workflow — neither handler has a
metrics_registryfield and the shard doesn't wire one, so these need plumbing first rather than a call next to an existing site. Both also keep separate module-local metrics (src/processing/metrics.zig,context.zig) unrelated to the registry families, which is worth untangling in its own change.KVMetricsandTieredLogMetrics—exportPrometheusdoesn't emit them andregisterTieredLoghas no callers. Whether to export or delete is a product decision; I'm not making it unilaterally.The note in
docs/deployment/clustering.mdxcovers all these families, so it stays until the rest land. #44 should stay open.