diff --git a/docs/designs/repo/compass-agent-effect-otel/design.md b/docs/designs/repo/compass-agent-effect-otel/design.md index 6fed8a248..c41b00877 100644 --- a/docs/designs/repo/compass-agent-effect-otel/design.md +++ b/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**