From 8b9f190b6f122e79e559e6286cc670683897c7a9 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 13 Sep 2026 22:03:26 -0400 Subject: [PATCH 1/2] docs(effect-otel): design the flush-shape counters and batch-size histogram (RIG-3694) Design only; the implementation follows against this frozen record. RIG-3694 asks whether turn coalescing produces many tiny batches or saturates the cap. No existing instrument answers it: the twelve metrics in Decision 2 count losses, retries, and depths, and a raw batch rate is uninterpretable without knowing WHY each batch flushed. The filed size/timer/shutdown taxonomy does not survive contact with the code. The spine has no timed flush: pumpLoop's only idle wait is the wake latch, the sole Effect.sleep is the priority-retry backoff, and takeBatch never waits for a fuller batch. A reason="timer" label could never be incremented, and an inert label is forbidden, so it is excluded. The code-true taxonomy is four reasons, not three: full (the cap closed the batch), drain (teardown residue), short (1..255 queued, the coalescing signal), and empty. The empty case is reachable and was missed in drafting: a stale coalesced wake exits the idle loop with both lanes empty, and the terminal guard returns only when `ended`, so takeBatch produces a zero-frame batch and the spine opens a stream carrying nothing. takeBatch has no empty guard. Counting it separately keeps the other three honest -- folded into short it would inflate the tiny-batch rate that is precisely the signal this issue wants, making a wasted round trip look like aggressive coalescing. Adds a batch-size histogram, the first in this module. Effect requires an explicit boundary spec; MetricBoundaries.exponential({start:1,factor:2, count:10}) gives [1,2,4,8,16,32,64,128,256] plus +Inf, verified by executing it rather than reading the types. Power-of-two buckets hold constant relative resolution where the variation is, the top boundary lands exactly on PUBLISH_BATCH_MAX so saturation is one bucket delta, and +Inf stays structurally empty as an invariant check. Purely additive: Decision 2's rows and prose are untouched, following the Decision 3a precedent in compass-agent-loop-otel. The freeze protects decision content, and this rewrites none. Ledger-impact: none Co-authored-by: Matt Wilkinson --- .../repo/compass-agent-effect-otel/design.md | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/docs/designs/repo/compass-agent-effect-otel/design.md b/docs/designs/repo/compass-agent-effect-otel/design.md index 6fed8a248..0aae7b108 100644 --- a/docs/designs/repo/compass-agent-effect-otel/design.md +++ b/docs/designs/repo/compass-agent-effect-otel/design.md @@ -192,6 +192,118 @@ The existing exported methods `droppedTraceCount()` / `failedPriorityCount()` (`frame-sink.test.ts:315,389`) and they are the frozen `PublishSpine` shape. Metrics are additive, driven from the same increment sites. +### Decision 2a — flush-shape instrumentation (RIG-3694 amendment) + +*Added by RIG-3694 after the record froze; Decision 2's rows above are +untouched. Decision 2 rejected histograms "in this cut" and explicitly +reserved "a later record once a concrete dashboard needs one" — this +amendment is that reserved rider, with RIG-3694 as the concrete need.* + +**Problem.** RIG-3694 asks whether turn coalescing produces +pathological flush shapes — many tiny cycled batches versus batches that +saturate the cap. No existing instrument answers it: the 12 Decision-2 +metrics count losses, retries, and depths, never flush shape, and a raw +batch *rate* is uninterpretable without knowing WHY each batch flushed. +The spine has **no timed flush**, so the filed size/timer/shutdown +taxonomy is wrong. Verified in `pumpLoop` +(`packages/compass-agent/src/transport/publish-spine.ts`): the only idle +wait is the wake latch — `while (priority.length === 0 && traceSize() +=== 0 && !ended) { yield* Queue.take(wake); }` — and the sole +`Effect.sleep(Duration.millis(delay))` is the priority-retry backoff. +`takeBatch` never waits for a fuller batch: it drains priority first +(`while (batch.length < PUBLISH_BATCH_MAX && priority.length > 0)`), +then takes only what is already queued (`const traceFrames = yield* +Queue.takeUpTo(traceQ, room)`). Every batch therefore flushes +immediately, for exactly one of four code-true reasons: + +- **`full`** — the take hit `PUBLISH_BATCH_MAX` (256): the cap, not the + queue's emptiness, closed the batch. +- **`drain`** — teardown flush: `ended` was set by `drain()` and the + batch carries the residue (the loop exits only at `if (ended && + priority.length === 0 && traceSize() === 0) return;`, so a non-empty + residue still flushes through the normal send). +- **`short`** — the lanes held between 1 and 255 frames at the take: the + immediate-flush steady state, and the coalescing signal RIG-3694 is + after. +- **`empty`** — `batch.length === 0`. A stale coalesced wake exits the + idle loop, and the terminal guard returns only when `ended`, so + `takeBatch` runs against two empty lanes and the spine opens a stream + carrying no frames. `takeBatch` has no empty guard and `pumpLoop` does + not skip the send, so this is a real, reachable path — its own comment + names it ("a stale coalesced wake causes at most one immediate take + before re-blocking"). Counting it separately keeps the other three + honest: folded into `short` it would inflate the tiny-batch rate that + is precisely RIG-3694's signal, making a wasted round trip look like + aggressive coalescing. + +Precedence `full` > `drain` > `empty` > `short`: a cap-filled batch +during drain flushed because of the cap; an empty batch is a stale-wake +artifact whether or not `ended` is set, and `drain` otherwise explains +the short residue. A `timer` reason is deliberately EXCLUDED — nothing +in the code can ever increment it, and an inert label is forbidden +(`rule://no-inert-gating`). + +**New rows** (same table shape as Decision 2; sites cited by symbol + +file, per the citation rule for post-freeze amendments): + +| Source (quoted) | Metric | Kind | +| --- | --- | --- | +| `pumpLoop` in `packages/compass-agent/src/transport/publish-spine.ts` — each cycled batch send, `const result = yield* Effect.either(Effect.tryPromise(() => publish(oneBatch())))`, classified from `batch.length` / `ended` at the take | `compass_agent.transport.publish.batches_flushed` `{reason="full"\|"drain"\|"short"\|"empty"}` | counter | +| same site — `batch.length`, bounded 0..`PUBLISH_BATCH_MAX`. Zero is reachable: a stale coalesced wake exits the idle loop with both lanes empty, and the terminal guard returns only when `ended`, so `takeBatch` can produce an empty batch (`pumpLoop`'s own comment: "a stale coalesced wake causes at most one immediate take before re-blocking") | `compass_agent.transport.publish.batch_size` | histogram | + +**Counter shape.** One base `Metric.counter(name, { incremental: true })` +plus four `Metric.tagged(base, "reason", …)` pre-tagged constants — +exactly the `trace_frames_lost` pattern in +`packages/compass-agent/src/transport/otel-metrics.ts` (static reason +set ⇒ pre-tagged constants; the dynamic-label BASE-counter pattern is +reserved for `control.unmapped`'s open `event_type` set). No deviation. + +**What is counted.** Batch send *attempts*, aligned with the Decision-1 +`…publish.batch` span (which also fires per attempt, carrying +`batch_size` / `priority_count` / `retry_index` as span attributes): a +failed priority batch re-enqueued at the front is re-taken and counted +again on retry, visible against `priority_batch_retries`. Both +instruments update at one site — in `pumpLoop`, immediately after `const +{ batch, priorityCount } = yield* takeBatch;`, before the send — so the +classification reads `batch.length` and `ended` in the same tick as the +take. + +**Histogram buckets.** Effect 3.22.1 requires an explicit boundary spec: +`Metric.histogram` is typed `(name: string, boundaries: +MetricBoundaries.MetricBoundaries, description?: string)` +(`effect@3.22.1/dist/dts/Metric.d.ts`), and +`MetricBoundaries.exponential({ start, factor, count })` +(`dist/dts/MetricBoundaries.d.ts`) builds `count - 1` finite boundaries +(internal: `Arr.makeBy(options.count - 1, i => options.start * +Math.pow(options.factor, i))`) with `fromIterable` appending the +terminal `+Inf` bucket (`Arr.appendAll(Chunk.of( +Number.POSITIVE_INFINITY))`, both in +`dist/cjs/internal/metric/boundaries.js`). Choose +`MetricBoundaries.exponential({ start: 1, factor: 2, count: 10 })` ⇒ +finite boundaries **[1, 2, 4, 8, 16, 32, 64, 128, 256]** (+`+Inf`). +Rationale: the value is bounded 0..256 and the question is +tiny-versus-saturated, so power-of-two buckets give constant *relative* +resolution across the whole range — the `le=1` bucket holds both the +empty stale-wake batch and the single-frame batch (the `empty` counter +reason separates them), `le=2` isolates the coalesced-turn tiny-batch +signature, the top boundary lands exactly on `PUBLISH_BATCH_MAX` so +saturation is the `(128, 256]` bucket delta, and the `+Inf` bucket is +structurally empty — a cheap invariant check, since `takeBatch` caps at +`PUBLISH_BATCH_MAX`. +Linear buckets would waste resolution: at width 26 the entire 1..26 +tiny-batch region — where the interesting variation lives — collapses +into one bucket. Nine finite buckets is one time series per bucket per +agent, negligible cardinality. + +**Registry namespace.** Neither new metric takes the gauge factory's +test-namespace prefix: histogram bucket counts and counter counts are +monotone and delta-readable under bun's concurrent test files, which is +the module's stated reason counters skip the factory +(`otel-metrics.ts`: "Counters read as a delta"; the same rule stated at +the factory's consumer, `createPublishSpine` in `publish-spine.ts`: +"Counters take no namespace — read as a delta"; only last-writer-wins +gauges race). + ### Decision 3 — exporter wiring against the deployed stack **There is no existing OTLP/Grafana endpoint config on the agent today** From b5d810a1064122e6c808f9e2d7a5c515a949d643 Mon Sep 17 00:00:00 2001 From: mintaka Date: Tue, 15 Sep 2026 20:20:45 -0400 Subject: [PATCH 2/2] docs(effect-otel): drop the unreachable empty reason, add the lane tag (RIG-3694) Review + design-critique on PR #1224. Both agents independently returned the same HIGH, and it refutes a correction I had made myself. I had added a fourth `empty` flush reason, arguing a stale coalesced wake could leave takeBatch with two empty lanes. That is wrong. The idle wait is a `while` whose condition is RE-EVALUATED after every wake take, so a stale wake re-enters and re-blocks rather than falling through; the loop is left only with a non-empty lane, or with `ended`, which returns at the terminal guard before takeBatch. Nothing removes frames in between: the pump is the sole consumer, the sliding queue evicts on offer rather than on read, producers only append, and drain() joins the fiber instead of interrupting it. I had read the inner while as an if, and read the code's "at most one immediate take before re-blocking" comment as evidence FOR reachability when it documents the opposite. Confirmed by execution: a faithful reduction takes zero batches on that path, and the reviewer drove 14,600 batches through two adversarial harnesses for a minimum batch size of 1. So `empty` was exactly the inert label this section cites no-inert-gating to exclude `timer` for. Dropped, with the unreachability proof recorded beside the timer exclusion so the next reader does not re-derive it. Adds `lane={priority|trace|mixed}` to the histogram, per Matt. The critic found a real ambiguity sitting on this issue's exact question: takeBatch drains priority first and control acks arrive one at a time, so a healthy ack stream produces the same size-1 signature as trace coalescing genuinely failing. Derived from priorityCount and batch.length, both already destructured at the classification site, and priorityCount is fixed before trace frames are appended, so all three cases are reachable. Also records why span aggregation is not enough (the span carries batch_size at this same site, but it is sampled and short-retained while this is a fleet rate over weeks), clarifies that `drain` means taken-after-teardown-began rather than one final batch, and drops an imprecise metric count -- the module exports 12 constants over 11 distinct names, and the number was not load-bearing. Ledger-impact: none Co-authored-by: Matt Wilkinson --- .../repo/compass-agent-effect-otel/design.md | 110 +++++++++++++----- 1 file changed, 79 insertions(+), 31 deletions(-) diff --git a/docs/designs/repo/compass-agent-effect-otel/design.md b/docs/designs/repo/compass-agent-effect-otel/design.md index 0aae7b108..c41b00877 100644 --- a/docs/designs/repo/compass-agent-effect-otel/design.md +++ b/docs/designs/repo/compass-agent-effect-otel/design.md @@ -201,7 +201,7 @@ amendment is that reserved rider, with RIG-3694 as the concrete need.* **Problem.** RIG-3694 asks whether turn coalescing produces pathological flush shapes — many tiny cycled batches versus batches that -saturate the cap. No existing instrument answers it: the 12 Decision-2 +saturate the cap. No existing instrument answers it: the Decision-2 metrics count losses, retries, and depths, never flush shape, and a raw batch *rate* is uninterpretable without knowing WHY each batch flushed. The spine has **no timed flush**, so the filed size/timer/shutdown @@ -214,50 +214,82 @@ wait is the wake latch — `while (priority.length === 0 && traceSize() (`while (batch.length < PUBLISH_BATCH_MAX && priority.length > 0)`), then takes only what is already queued (`const traceFrames = yield* Queue.takeUpTo(traceQ, room)`). Every batch therefore flushes -immediately, for exactly one of four code-true reasons: +immediately, for exactly one of three code-true reasons: - **`full`** — the take hit `PUBLISH_BATCH_MAX` (256): the cap, not the queue's emptiness, closed the batch. - **`drain`** — teardown flush: `ended` was set by `drain()` and the batch carries the residue (the loop exits only at `if (ended && priority.length === 0 && traceSize() === 0) return;`, so a non-empty - residue still flushes through the normal send). + residue still flushes through the normal send). Read it as "taken + after teardown began", not "the one final batch": classification reads + `ended` at the take, so a batch queued before teardown but taken after + it also counts here. Bounded — teardown happens once per session — but + a dashboard should not assume exactly one. - **`short`** — the lanes held between 1 and 255 frames at the take: the immediate-flush steady state, and the coalescing signal RIG-3694 is after. -- **`empty`** — `batch.length === 0`. A stale coalesced wake exits the - idle loop, and the terminal guard returns only when `ended`, so - `takeBatch` runs against two empty lanes and the spine opens a stream - carrying no frames. `takeBatch` has no empty guard and `pumpLoop` does - not skip the send, so this is a real, reachable path — its own comment - names it ("a stale coalesced wake causes at most one immediate take - before re-blocking"). Counting it separately keeps the other three - honest: folded into `short` it would inflate the tiny-batch rate that - is precisely RIG-3694's signal, making a wasted round trip look like - aggressive coalescing. - -Precedence `full` > `drain` > `empty` > `short`: a cap-filled batch -during drain flushed because of the cap; an empty batch is a stale-wake -artifact whether or not `ended` is set, and `drain` otherwise explains -the short residue. A `timer` reason is deliberately EXCLUDED — nothing -in the code can ever increment it, and an inert label is forbidden -(`rule://no-inert-gating`). + +Precedence `full` > `drain` > `short`: a cap-filled batch during drain +flushed because of the cap; `drain` explains only the short residue. + +**No `timer` and no `empty` reason**, because neither can be +incremented, and an inert label is forbidden (`rule://no-inert-gating`). +`timer`: there is no timed flush, per the trace above. `empty`: a batch +is never zero-length. The idle wait is a `while` whose condition is +**re-evaluated after every wake take** — `while (priority.length === 0 +&& traceSize() === 0 && !ended) { yield* Queue.take(wake); }` — so a +stale coalesced wake re-enters and re-blocks rather than falling +through (which is what the code's "at most one immediate take before +re-blocking" comment describes: one `Queue.take(wake)` iteration, not a +batch take). Exiting with `!ended` therefore requires a non-empty lane, +and nothing removes frames between that check and `takeBatch`: the pump +is the sole consumer (`Queue.takeUpTo(traceQ, room)` is the only take), +the sliding queue evicts on **offer** rather than on read, producers +only append to `priority`, and `drain()` joins the pump fiber rather +than interrupting it. Exiting with `ended` and empty lanes returns at +the terminal guard, before `takeBatch`. So `batch.length` is bounded +1..`PUBLISH_BATCH_MAX`, and a skip-guard for a zero-length batch would +itself be dead code. **New rows** (same table shape as Decision 2; sites cited by symbol + file, per the citation rule for post-freeze amendments): | Source (quoted) | Metric | Kind | | --- | --- | --- | -| `pumpLoop` in `packages/compass-agent/src/transport/publish-spine.ts` — each cycled batch send, `const result = yield* Effect.either(Effect.tryPromise(() => publish(oneBatch())))`, classified from `batch.length` / `ended` at the take | `compass_agent.transport.publish.batches_flushed` `{reason="full"\|"drain"\|"short"\|"empty"}` | counter | -| same site — `batch.length`, bounded 0..`PUBLISH_BATCH_MAX`. Zero is reachable: a stale coalesced wake exits the idle loop with both lanes empty, and the terminal guard returns only when `ended`, so `takeBatch` can produce an empty batch (`pumpLoop`'s own comment: "a stale coalesced wake causes at most one immediate take before re-blocking") | `compass_agent.transport.publish.batch_size` | histogram | +| `pumpLoop` in `packages/compass-agent/src/transport/publish-spine.ts` — each cycled batch send, `const result = yield* Effect.either(Effect.tryPromise(() => publish(oneBatch())))`, classified from `batch.length` / `ended` at the take | `compass_agent.transport.publish.batches_flushed` `{reason="full"\|"drain"\|"short"}` | counter | +| same site — `batch.length` and `priorityCount`, both destructured at the classification site (`const { batch, priorityCount } = yield* takeBatch;`). Bounded 1..`PUBLISH_BATCH_MAX` (the idle wait re-evaluates its condition after every wake take, so the loop is left only with a non-empty lane or with `ended` — and the latter returns at the terminal guard before `takeBatch`) | `compass_agent.transport.publish.batch_size` `{lane="priority"\|"trace"\|"mixed"}` | histogram | **Counter shape.** One base `Metric.counter(name, { incremental: true })` -plus four `Metric.tagged(base, "reason", …)` pre-tagged constants — +plus three `Metric.tagged(base, "reason", …)` pre-tagged constants — exactly the `trace_frames_lost` pattern in `packages/compass-agent/src/transport/otel-metrics.ts` (static reason set ⇒ pre-tagged constants; the dynamic-label BASE-counter pattern is reserved for `control.unmapped`'s open `event_type` set). No deviation. +**Why the histogram carries a `lane` tag.** Without it a tiny-batch +reading is ambiguous in exactly the place RIG-3694 asks about. `takeBatch` +drains the priority lane first (`while (batch.length < +PUBLISH_BATCH_MAX && priority.length > 0)`), and priority frames — +control acks and lifecycle — arrive one at a time +(`enqueuePriority` does `priority.push(frame)` per frame), so a healthy +ack stream produces the same size-1/size-2 signature as trace coalescing +genuinely failing. A fleet chart could not tell them apart. + +`lane` is derived at the classification site from the two values already +in hand: `priorityCount === batch.length` ⇒ `priority`, +`priorityCount === 0` ⇒ `trace`, otherwise `mixed`. A static 3-value set, +so it is pre-taggable exactly like `reason` and needs no new plumbing. +Cost is ×3 on the histogram's series; against the existing instrument set +that is negligible, and it buys the one distinction the issue turns on: +`batch_size{lane="trace"}` is the coalescing signal, and +`{lane="priority"}` is the ack stream that would otherwise masquerade +as it. + +The tag goes on the histogram rather than the counter because the shape +question lives in the buckets: knowing a batch was priority-only without +its size distribution does not answer the question. + **What is counted.** Batch send *attempts*, aligned with the Decision-1 `…publish.batch` span (which also fires per attempt, carrying `batch_size` / `priority_count` / `retry_index` as span attributes): a @@ -268,6 +300,24 @@ instruments update at one site — in `pumpLoop`, immediately after `const classification reads `batch.length` and `ended` in the same tick as the take. +**Why not aggregate the existing span instead.** Decision 2 rejected a +`batch_size` histogram for this cut — "the span set already carries +per-attempt durations queryable in Tempo" — and left the door open: "a +histogram cut can ride a later record once a concrete dashboard needs +one." RIG-3694 is that concrete need, so the rider is being taken, not +overturned. The zero-code alternative is real and must be named: the +Decision-1 span already carries `batch_size: batch.length` at this exact +site, so span-attribute aggregation would yield the distribution with no +new instrument. It is rejected because a span is *sampled* and retained +for days, while this question is a fleet-wide rate over weeks: a sampled +span set cannot give an exact count, and the shape of a rare tiny-batch +regime is precisely what sampling erases. A `Metric` is pre-aggregated +at the agent, costs one time series per bucket, and survives the span's +retention window. The two are complementary rather than redundant, and +the counter earns its place on the same argument: `full` means +`batch.length` is exactly `PUBLISH_BATCH_MAX`, which is not recoverable +from the histogram's `(128, 256]` bucket. + **Histogram buckets.** Effect 3.22.1 requires an explicit boundary spec: `Metric.histogram` is typed `(name: string, boundaries: MetricBoundaries.MetricBoundaries, description?: string)` @@ -281,15 +331,13 @@ Number.POSITIVE_INFINITY))`, both in `dist/cjs/internal/metric/boundaries.js`). Choose `MetricBoundaries.exponential({ start: 1, factor: 2, count: 10 })` ⇒ finite boundaries **[1, 2, 4, 8, 16, 32, 64, 128, 256]** (+`+Inf`). -Rationale: the value is bounded 0..256 and the question is +Rationale: the value is bounded 1..256 and the question is tiny-versus-saturated, so power-of-two buckets give constant *relative* -resolution across the whole range — the `le=1` bucket holds both the -empty stale-wake batch and the single-frame batch (the `empty` counter -reason separates them), `le=2` isolates the coalesced-turn tiny-batch -signature, the top boundary lands exactly on `PUBLISH_BATCH_MAX` so -saturation is the `(128, 256]` bucket delta, and the `+Inf` bucket is -structurally empty — a cheap invariant check, since `takeBatch` caps at -`PUBLISH_BATCH_MAX`. +resolution across the whole range — the `le=1`/`le=2` buckets isolate +the coalesced-turn tiny-batch signature, the top boundary lands exactly +on `PUBLISH_BATCH_MAX` so saturation is the `(128, 256]` bucket delta, +and the `+Inf` bucket is structurally empty — a cheap invariant check, +since `takeBatch` caps at `PUBLISH_BATCH_MAX`. Linear buckets would waste resolution: at width 26 the entire 1..26 tiny-batch region — where the interesting variation lives — collapses into one bucket. Nine finite buckets is one time series per bucket per