docs(effect-otel): design the flush-shape counters and batch-size histogram (RIG-3694) - #1224
Open
rigel-mintaka wants to merge 2 commits into
Open
rigel-mintaka wants to merge 2 commits into
rigel-mintaka wants to merge 2 commits into
Conversation
…togram (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 <matt@rigel.build>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Compass engineering docs preview: https://compass-obs-rig-3694-flush-s.compass-eng-docs.pages.dev Deployed from Changed pages: |
…g (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 <matt@rigel.build>
rigel-mintaka
marked this pull request as ready for review
September 16, 2026 00:43
Contributor
Author
|
Parked for Matt — overnight fork, assumption stated (no action needed on this PR). I stacked the RIG-3694 implementation on top of this PR rather than waiting for it to merge. Assumption: "design PR first, then implement" constrains merge order, not authoring order. The stack is linear, so this record still lands first and the implementation still lands against a frozen contract — only the writing happens while this PR waits at the gate. If that reading is wrong, drop the implementation PR and I'll re-submit it after this one merges. The cost is a rebase and re-review of one narrow two-file commit, and it falls on me. This PR is unaffected either way — it is review-clean and ready for your approval as it stands. |
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.
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 andthe 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 matt@rigel.build