docs(effect-otel): design the flush-shape counters and batch-size histogram (RIG-3694)

docs/designs/repo/compass-agent-effect-otel/design.md

@@ -192,6 +192,166 @@ 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 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 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). 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.
+
+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"}` | 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 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
+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.
+
+**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)`
+(`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 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`/`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
+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**