feat(ingest): cut per-record work on the OTLP logs ingest path by pmoust · Pull Request #3509 · NOFireAI/ravel

pmoust · 2026-10-30T14:28:45Z

Implement the ADR-3492 decisions for the logs ingest path.

D1: the server's logs ingest flush builds its RLOG writer with
PageChoice::Estimated, so each integer and string page's encoding is
chosen from statistics instead of sealing every candidate. The choice is
IngestConfig::rlog_page_choice (default Estimated); the bulk loader, which
writes through the same flush, sets Exhaustive, and every other RLOG writer
keeps Exhaustive. The
clustering-key flush tests' reference writer moves to Estimated with it,
since they compare the flush's object bytes.

D2: the flush checks the buffered rows' stream attributes the way the row
writer's finish() does, builds one ColumnarLogBatch from them and hands it
to push_columnar. A test pins that the object is byte-identical to pushing
the rows one at a time, for both page choices, with and without a
clustering sort. The measurement D2 required is the logseg_encode bench in
#3500 (#3494): on the benchmark shape the columnar arm, including
try_from_records per 2,000-record batch, cost 4.5 to 8.6% less CPU per
record than the row writer in all four comparisons.

D3a: NormalizedLogRecord.stream_attrs is an Arc<[u8]> built once per OTLP
scope and shared by its records, so normalization allocates 1 time per
record instead of 2. The flush still copies the blob once per record into
the columnar batch, so the end-to-end saving is buffered memory (one shared
blob per scope instead of one owned copy per buffered record), not the
allocation count of a record that reaches a flush. The struct-literal sites in ravel-cli, ravel-bench,
ravel-failure-tests and ravel-server tests follow the type; the CLI load
path copies the blob once per record. The wide-stream label-charge fixture
now gives each record its own resource, because a shared blob no longer
allocates in proportion to the projected charge the test separates from
it.

D5: log stream admission decides each distinct stream id once per request
and replays that outcome for its later records; per-record outcomes, the
admitted and rejected counters and the epoch set are unchanged, which a
property test checks against per-record admission over interleaved and
recurring streams and caps around the active count.

The router computes each record's byte estimate once and carries it to the
shard in a ShardRecords buffer, which merge_rows_unstamped charges instead
of recomputing.

D6: the age-check tick is IngestConfig::effective_flush_tick(),
min(flush_tick, max(max_flush_delay / 4, 25 ms)). It is 200 ms at the
default 2 s delay and 25 ms at a 100 ms delay, where the idle flush hold
grows by 175 ms. The idle hold, the flush trigger age bound, the shard tick
loops and the server's deferral-cap startup validation all use it, and the
startup refusal states the bound. docs/consistency-model.md, docs/ingest.md
and the cost-model guide now name the effective tick where they named
flush_tick.

Known and unchanged: the ingest byte budget and the resolved-label
projection still charge stream_attrs per record, so a shared blob is
over-charged, never under-charged.

Fixes: #3498
Refs: #3492

Implement the ADR-3492 decisions for the logs ingest path. D1: the flush builds its RLOG writer with PageChoice::Estimated, so each integer and string page's encoding is chosen from statistics instead of sealing every candidate. Every other RLOG writer keeps Exhaustive. The clustering-key flush tests' reference writer moves to Estimated with it, since they compare the flush's object bytes. D2: the flush checks the buffered rows' stream attributes the way the row writer's finish() does, builds one ColumnarLogBatch from them and hands it to push_columnar. A test pins that the object is byte-identical to pushing the rows one at a time, for both page choices, with and without a clustering sort. The measurement D2 required is the logseg_encode bench in #3500 (#3494): on the benchmark shape the columnar arm, including try_from_records per 2,000-record batch, cost 4.5 to 8.6% less CPU per record than the row writer in all four comparisons. D3a: NormalizedLogRecord.stream_attrs is an Arc<[u8]> built once per OTLP scope and shared by its records, so normalization allocates 1 time per record instead of 2. The struct-literal sites in ravel-cli, ravel-bench, ravel-failure-tests and ravel-server tests follow the type; the CLI load path copies the blob once per record. The wide-stream label-charge fixture now gives each record its own resource, because a shared blob no longer allocates in proportion to the projected charge the test separates from it. D5: log stream admission decides each distinct stream id once per request and replays that outcome for its later records; per-record outcomes, the admitted and rejected counters and the epoch set are unchanged, which a property test checks against per-record admission over interleaved and recurring streams and caps around the active count. The router computes each record's byte estimate once and carries it to the shard in a ShardRecords buffer, which merge_rows_unstamped charges instead of recomputing. D6: the age-check tick is IngestConfig::effective_flush_tick(), min(flush_tick, max(max_flush_delay / 4, 25 ms)). It is 200 ms at the default 2 s delay and 25 ms at a 100 ms delay, where the idle flush hold grows by 175 ms. The idle hold, the flush trigger age bound, the shard tick loops and the server's deferral-cap startup validation all use it, and the startup refusal states the bound. docs/consistency-model.md, docs/ingest.md and the cost-model guide now name the effective tick where they named flush_tick. Known and unchanged: the ingest byte budget and the resolved-label projection still charge stream_attrs per record, so a shared blob is over-charged, never under-charged. Fixes: #3498 Refs: #3492 Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>

pmoust · 2026-10-30T14:28:47Z

@claude-fleet review

claude-fleet · 2026-10-30T14:28:52Z

Review task queued.

Task: 518a25f9-b493-40d7-b9e2-f7f94a910bb0
Commit: 8038ebc
Model: claude-opus-5 (effort medium)
Executor: ci-16gb-fsn1-1
Status: done. Review posted.

claude-fleet

Review ready.

PR #3509 does what its description says: the OTLP logs ingest path now shares one stream_attrs blob per scope, admits once per stream id instead of once per record, carries each record's byte estimate from the router to its shard, flushes through push_columnar with PageChoice::Estimated, and derives the flush tick from max_flush_delay. I read every changed file and the unchanged code the changes depend on (ravel-logseg writer and ColumnarLogBatch::try_from_records, EpochIdSet::admit, the server's deferral-cap validation). One doc paragraph is now wrong, and it is normative.

Findings:

docs/log-segment-format.md:725 still states the pre-ADR-3492 wiring. It says the logs ingest flush "selects Estimated once ADR-3492 decision 1 is wired into it, and writes Exhaustive until then", and that ravel-cli load writes Exhaustive. After this PR both halves are false: flush_rlog_config sets PageChoice::Estimated, and services/ravel-cli/src/load/logs.rs:493 builds a LogIngestRouter, so ravel-cli load writes through the same flush and is now Estimated too. This is the format document's own "which writers use which" table-in-prose, and the repo rule is that the doc moves in the same commit as the behavior. docs/ingest.md, docs/consistency-model.md and docs/guides/cost-model.md were updated; this one was missed.
crates/ravel-ingest/src/log_shard.rs:429, to_logseg_record does stream_attrs: rec.stream_attrs.to_vec(). D3a removes one blob copy per record at normalization, and this adds one back per record at flush. The saving is real for buffered memory (a 10k-record buffer holds one Arc<[u8]> per record instead of 10k copies) and real for records dropped by admission or by a validation error, but for a record that reaches a flush the end-to-end allocation count per record is unchanged. Worth either taking &Arc<[u8]> through to the batch builder if ColumnarLogBatch can hold a borrowed blob, or stating in the commit message that the win is buffer residency rather than total allocations, so the #3500 figures are read correctly.

Verified:

Admission equivalence. admit_batch_per_id caches one decision per stream id and replays it, so the per-id and per-record forms can only differ if a repeat occurrence of an id could get a different answer. EpochIdSet::admit (admission.rs:333-349) only grows active_count and inserts an admitted id into current, so a second occurrence of an admitted id hits the membership fast path and a second occurrence of a rejected id sees a count that has not shrunk. The new proptest compares full post-state tuples, not just the returned set, so series_admitted_total and series_rejected_cap_total are pinned too.
The estimate carried from the router is the one the budget charged: log_router.rs computes record_bytes once, folds it into estimate, and zips the same vector into ShardRecords::push. Nothing mutates the records between those two points.
check_stream_attrs matches the row writer. It builds the first blob per stream id in push order, same as build_object's BTreeMap directory pass (ravel-logseg/src/writer.rs:480-525), and raises the identical LogSegError::InconsistentStreamAttrs message; the new test asserts the flush message equals what finish() produces, so a message change on either side breaks it.
Byte identity between the row and columnar paths at ingest shape is already covered independently by ravel-logseg/tests/differential.rs:983-1017 (estimated_mode_is_deterministic), and the_columnar_flush_writes_the_row_writers_bytes adds the ingest-side case under both page choices with and without the clustering sort.
effective_flush_tick has no remaining bypass: no use of .flush_tick survives outside config.rs. The server refusal text's hardcoded "200 ms" is accurate because flush_tick has no CLI flag.
The allocation test's tightening is the real guard here: FLOOR_PER_RECORD drops 2 to 1 with a strict assert_eq!(many - few, FLOOR_PER_RECORD * (MANY - FEW)), which fails if the blob is ever copied per record again.

Checks run:

cargo test -p ravel-ingest --lib -- admission::tests config::tests log_shard::tests: ok, 98 passed, 0 failed.
Fix-proof for the page-choice change: with flush_rlog_config reverted to PageChoice::Exhaustive, the_flush_writer_estimates_page_encodings FAILED at log_shard.rs:3383 and the_columnar_flush_writes_the_row_writers_bytes still passed (it asserts row/columnar parity under whichever choice is configured, so it is not the guard for D1). Source restored; working tree is clean.

Smaller things:

Flush peak memory grows: flush_rows_batch materializes a full Vec<LogRecord> and then a ColumnarLogBatch while the shard's own buffer is still live. For a buffer at the byte budget that is roughly three copies in flight where the row path streamed one record at a time. Probably fine at current budgets, but it is the opposite direction from the rest of the PR.
services/ravel-server/src/lib.rs hardcodes "200 ms" and "25 ms" in the refusal text and repeats them in flush_tick_bound_tests, so a change to FLUSH_TICK_FLOOR leaves a message that compiles, passes and lies. Formatting the floor from the constant would make the test pin the constant instead of a transcription of it.
services/ravel-cli/src/load/cli.rs:656 describes the flush tick in terms that no longer hold now that it is derived.
FLUSH_TICK_FLOOR is pub but has no use outside config.rs and its own tests.

Not checked:

The workspace suite and the feature lanes (sql, flight-sql, esql, agents, mcp). I ran only the three ravel-ingest lib test modules plus the targeted revert run.
ravel-otlp, ravel-server, ravel-cli and ravel-failure-tests suites, including logs_normalize_allocations.rs and label_charge_allocations.rs, whose assertions I read but did not execute.
The logseg_encode benchmark figures the description cites from #3500. I did not reproduce them and cannot say the quoted per-line improvement holds on this tree.
Behavior under a real object store, and any effect of the shorter tick on flush rate at production record sizes.

Not checked:

Workspace suite and the sql, flight-sql, esql, agents and mcp feature lanes; I ran only the ravel-ingest lib tests for admission, config and log_shard plus one targeted revert run.
ravel-otlp, ravel-server, ravel-cli and ravel-failure-tests suites, including logs_normalize_allocations.rs and label_charge_allocations.rs, whose assertions I read but did not execute.
The logseg_encode benchmark figures from #3500 quoted in the description; not reproduced on this tree.
Behavior against a real object store, and the effect of the shorter effective flush tick on flush rate and object count at production record sizes.

Findings outside the diff:

docs/log-segment-format.md:725 (should_fix): This paragraph still describes the pre-ADR-3492 wiring and is now wrong in both halves. flush_rlog_config in crates/ravel-ingest/src/log_shard.rs sets PageChoice::Estimated in this PR, so the ingest flush no longer "writes Exhaustive until then"; and services/ravel-cli/src/load/logs.rs:493 builds a LogIngestRouter, so ravel-cli load goes through that same flush and is no longer an Exhaustive writer either. This is the format document's own statement of which writers use which page choice, so a reader checking what produced an object gets the wrong answer. docs/ingest.md, docs/consistency-model.md and docs/guides/cost-model.md were updated in this PR; this one needs the same treatment.

Task 518a25f9-b493-40d7-b9e2-f7f94a910bb0 on ci-16gb-fsn1-1, model claude-opus-5, effort medium.

claude-fleet · 2026-10-30T14:45:55Z

    LogRecord {
        stream_id: rec.stream_id,
-        stream_attrs: rec.stream_attrs,
+        stream_attrs: rec.stream_attrs.to_vec(),


nit: stream_attrs.to_vec() copies the shared blob once per record at flush, which is the allocation D3a just removed from normalization. The change still wins on buffered memory (one Arc<[u8]> per buffered record instead of one owned Vec each) and on records dropped by admission or a validation error before any flush, but for a record that reaches a flush the end-to-end allocation count per record is unchanged. If ColumnarLogBatch::try_from_records can be given a borrowed or Arc-held blob, threading &Arc<[u8]> through would make the saving end-to-end. Otherwise it is worth saying in the commit message that the win is buffer residency, so the #3500 per-line figures are not read as an allocation-count claim.

The ingest flush always built its RLOG writer with PageChoice::Estimated, and ravel-cli load writes through the same log ingest router and shard flush, so the bulk loader lost the exhaustive page choice ADR-3492 decision 1 and the ADR-2135 amendment keep for it. load_zstd_level saw 7 zstd pages where the exhaustive writer stores 13. The page choice is now IngestConfig::rlog_page_choice. It defaults to Estimated, which the server's logs ingest flush picks up through IngestConfig::default(), and the log loader's router config sets Exhaustive. A ravel-ingest test pins that the flush writer follows the setting, and a ravel-cli test pins that the loader's config asks for Exhaustive while the default it builds on estimates. Refs: #3498 Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>

pmoust · 2026-10-30T15:06:10Z

@claude-fleet review

claude-fleet · 2026-10-30T15:06:15Z

Review task queued.

Task: a8cd10d7-f2f3-4f07-bd32-92453c78a049
Commit: e6f2184
Model: claude-opus-5 (effort medium)
Executor: rp2
Status: done. Review posted.

claude-fleet

Previous round at 8038ebc: 2 findings.

Previous finding	Classification	Cited line
docs/log-segment-format.md:725 (should_fix): This paragraph still describes the pre-ADR-3492 wiring and is now wrong in both halves. `flush_rlog_config` in `crates/ravel-ingest/src/log_shard.rs` sets `PageChoice::Estimated` in this PR, so the ingest flush no longer "writes `Exhaustive` until then"; and `services/ravel-cli/src/load/logs.rs:493` builds a `LogIngestRouter`, so `ravel-cli load` goes through that same flush and is no longer an `Exhaustive` writer either. This is the format document's own statement of which writers use which page choice, so a reader checking what produced an object gets the wrong answer. `docs/ingest.md`, `docs/consistency-model.md` and `docs/guides/cost-model.md` were updated in this PR; this one needs the same treatment.	open, not reposted: docs/log-segment-format.md:725 is not in this diff	docs/log-segment-format.md:725
crates/ravel-ingest/src/log_shard.rs:429 (nit): `stream_attrs.to_vec()` copies the shared blob once per record at flush, which is the allocation D3a just removed from normalization. The change still wins on buffered memory (one `Arc<[u8]>` per buffered record instead of one owned `Vec` each) and on records dropped by admission or a validation error before any flush, but for a record that reaches a flush the end-to-end allocation count per record is unchanged. If `ColumnarLogBatch::try_from_records` can be given a borrowed or `Arc`-held blob, threading `&Arc<[u8]>` through would make the saving end-to-end. Otherwise it is worth saying in the commit message that the win is buffer residency, so the #3500 per-line figures are not read as an allocation-count claim.	open	crates/ravel-ingest/src/log_shard.rs:429

Request changes: the previous should_fix doc finding is half fixed and still stands.

The new commit resolves the ravel-cli load half of it by giving IngestConfig an rlog_page_choice lever that the loader sets to Exhaustive (services/ravel-cli/src/load/logs.rs:2087-2100, crates/ravel-ingest/src/config.rs:821-826), with a unit test on each side. docs/log-segment-format.md:725-727 is untouched, so the format document still says the logs ingest flush "writes Exhaustive until then", which this PR makes false.

Findings:

should_fix, docs/log-segment-format.md:725 — still says the ingest flush writes Exhaustive until decision 1 is wired in (previous finding 1, now only the first half)
nit, crates/ravel-ingest/src/log_shard.rs:429 — the flush still copies the shared blob per record (previous finding 2, unchanged)

Verified:

D1: flush_rlog_config reads the config lever, and the loader is the only non-default caller — crates/ravel-ingest/src/log_shard.rs:409-419, services/ravel-cli/src/load/logs.rs:493-500; build_ingest_config has one log router call site.
D2 equivalence: check_stream_attrs reproduces the row writer's check exactly — same first-blob-per-id-in-push-order rule and the same message as crates/ravel-logseg/src/writer.rs:499-515. The byte-identity claim rests on the crate's own row/columnar invariant (crates/ravel-logseg/src/writer.rs:413-415) and its property test at writer.rs:7032, not only on the new 48-record fixture.
D2 error mapping: a collision still reaches LogWriteError::StreamIdCollision and record_stream_id_collision, because flush_rows_batch wraps it in RlogEncodeError::Finish — crates/ravel-ingest/src/log_shard.rs:463 and 1073-1077.
D5 equivalence: within one batch active_count only grows and an admitted id is in current, so a replayed outcome cannot differ — crates/ravel-ingest/src/admission.rs:333-349 against 1083-1104.
D5 estimate carrying: the per-record estimates are zipped onto the same unfiltered records vec, nothing between the two reorders or drops a record — crates/ravel-ingest/src/log_router.rs:587-617; merge_rows_unstamped is all-or-nothing, so a carried sum is never charged for records that did not land (log_shard.rs:628-656).
D6: no config.flush_tick read remains outside effective_flush_tick and tests; the server's refusal text is accurate because validate_flush_deferral_cap always builds on the default 200 ms tick — services/ravel-server/src/lib.rs:2274-2290.
D6 durability: hold + tick still equals max_flush_lifetime at any delay, so FLUSH_BOUND_SLACK_HOURS keeps its derivation — crates/ravel-ingest/src/config.rs:505-512.
D3a: one Arc<[u8]> per ScopeLogs, asserted by Arc::ptr_eq — crates/ravel-otlp/src/logs_normalize.rs:443-444, 1435-1439.

Smaller things:

docs/ingest.md:1023 now states the flush configures PageChoice::Estimated without the loader exception the changelog does mention. Same drift class as the finding above; worth one clause.
crates/ravel-catalog/src/provisioning.rs:441-476 still writes "one flush_tick" in the slack derivation. The derivation stays correct under the effective tick, so this is only a stale term, but it is the same wording the PR updated everywhere in ravel-ingest.
rows_to_batch (the hand-back conversion, log_shard.rs:445-452) keeps the first blob per stream with no check_stream_attrs, so the new flush path and the hand-back path now disagree on a diverging blob. Pre-existing and I did not find a reachable input, so no finding.
wide_stream_log_request moving to one resource per record (services/ravel-server/tests/label_charge_allocations.rs:123-141) means the common OTLP shape, many records under one scope, is no longer the shape that test measures. The direction is safe (over-charge), and the PR says so.

Not checked:

Nothing was compiled or run here: no cargo check, clippy or test run, so the IngestConfig PartialEq comparison added in services/ravel-cli/src/load/logs/tests.rs:5183-5190 and the new tests are unverified beyond reading.
The #3500 / #3494 CPU figures quoted for D2 are outside this repo state.
Mechanical .into() / .to_vec() churn across the 20-odd tests/ files was skimmed, not read line by line.

Not checked:

No build, clippy or test run was performed in this workspace; the new tests and the IngestConfig PartialEq comparison in services/ravel-cli/src/load/logs/tests.rs were read, not executed.
The #3500/#3494 CPU measurements quoted for D2.
Line-by-line reading of the mechanical .into()/.to_vec() churn in the ~20 ravel-ingest and ravel-cli test files.

Task a8cd10d7-f2f3-4f07-bd32-92453c78a049 on rp2, model claude-opus-5, effort medium.

claude-fleet · 2026-10-30T15:11:49Z

    LogRecord {
        stream_id: rec.stream_id,
-        stream_attrs: rec.stream_attrs,
+        stream_attrs: rec.stream_attrs.to_vec(),


nit: Still open since 8038ebc. stream_attrs.to_vec() copies the shared blob once per record at flush, which is the allocation D3a just removed from normalization. The change still wins on buffered memory (one Arc<[u8]> per buffered record instead of one owned Vec each) and on records dropped by admission or a validation error before any flush, but for a record that reaches a flush the end-to-end allocation count per record is unchanged. If ColumnarLogBatch::try_from_records can be given a borrowed or Arc-held blob, threading &Arc<[u8]> through would make the saving end-to-end. Otherwise it is worth saying in the commit message that the win is buffer residency, so the #3500 per-line figures are not read as an allocation-count claim.

claude-fleet · 2026-10-30T15:11:49Z

 - Objects are RLOG, built with `ravel_logseg::RlogWriter`, not RSEG built
-  with `SegmentWriter`. They land under the `l` keyspace
+  with `SegmentWriter`. The flush configures the writer with
+  `PageChoice::Estimated` (ADR-3492 decision 1), and hands a row-major buffer


nit: This sentence says the logs flush configures the writer with PageChoice::Estimated without the exception the same PR adds: ravel-cli load drives this same flush with PageChoice::Exhaustive.
Failure: a reader checking what wrote a loader-produced object concludes its pages were estimated.
Evidence: services/ravel-cli/src/load/logs.rs:2087-2100 sets rlog_page_choice: PageChoice::Exhaustive for the loader's router, and crates/ravel-ingest/src/log_shard.rs:415 passes it straight to the writer; changelog.d/3498.changed.md:3 does carry the exception.

The format doc still said the logs ingest flush writes Exhaustive until ADR-3492 decision 1 is wired in. It now is: the server's logs ingest flush writes Estimated through IngestConfig::rlog_page_choice, ravel-cli load writes through the same flush with Exhaustive, and compaction and the erasure rewrite stay Exhaustive. Refs: #3498 Signed-off-by: Panagiotis Moustafellos <pmoust@nofire.ai>

pmoust · 2026-10-30T15:13:07Z

@claude-fleet review

claude-fleet · 2026-10-30T15:13:12Z

Review task queued.

Task: ffa373c0-1e20-4c70-a269-6440e9af2034
Commit: 3062625
Model: claude-opus-5 (effort medium)
Executor: ravel-clickbench-ubuntu-2
Status: done. Review posted.

claude-fleet

Previous round at e6f2184: 3 findings.

Previous finding	Classification	Cited line
docs/log-segment-format.md:725 (should_fix): This paragraph still describes the pre-ADR-3492 wiring and is now wrong in both halves. `flush_rlog_config` in `crates/ravel-ingest/src/log_shard.rs` sets `PageChoice::Estimated` in this PR, so the ingest flush no longer "writes `Exhaustive` until then"; and `services/ravel-cli/src/load/logs.rs:493` builds a `LogIngestRouter`, so `ravel-cli load` goes through that same flush and is no longer an `Exhaustive` writer either. This is the format document's own statement of which writers use which page choice, so a reader checking what produced an object gets the wrong answer. `docs/ingest.md`, `docs/consistency-model.md` and `docs/guides/cost-model.md` were updated in this PR; this one needs the same treatment.	not classified	none
crates/ravel-ingest/src/log_shard.rs:429 (nit): `stream_attrs.to_vec()` copies the shared blob once per record at flush, which is the allocation D3a just removed from normalization. The change still wins on buffered memory (one `Arc<[u8]>` per buffered record instead of one owned `Vec` each) and on records dropped by admission or a validation error before any flush, but for a record that reaches a flush the end-to-end allocation count per record is unchanged. If `ColumnarLogBatch::try_from_records` can be given a borrowed or `Arc`-held blob, threading `&Arc<[u8]>` through would make the saving end-to-end. Otherwise it is worth saying in the commit message that the win is buffer residency, so the #3500 per-line figures are not read as an allocation-count claim.	not classified	none
docs/ingest.md:1023 (nit): This sentence says the logs flush configures the writer with `PageChoice::Estimated` without the exception the same PR adds: `ravel-cli load` drives this same flush with `PageChoice::Exhaustive`. Failure: a reader checking what wrote a loader-produced object concludes its pages were estimated. Evidence: services/ravel-cli/src/load/logs.rs:2087-2100 sets `rlog_page_choice: PageChoice::Exhaustive` for the loader's router, and crates/ravel-ingest/src/log_shard.rs:415 passes it straight to the writer; changelog.d/3498.changed.md:3 does carry the exception.	not classified	none

Not approved: a finding from the previous round is not fixed and is not a nit.

No findings.

Verdict

Approve. One of the three previous findings is fixed; the two that stand are both nits (a doc omission and a commit-message overclaim), neither blocking. No new blocking or should_fix issue survived this pass.

Previous findings

1 — fixed. docs/log-segment-format.md:725 now names the writers: the server's logs ingest flush writes Estimated via the rlog_page_choice setting, ravel-cli load writes Exhaustive through the same flush, and compaction plus the erasure rewrite write Exhaustive. That is the whole of the change since the previous round (commit 3062625), and it says exactly what the finding asked for.

2 — open (nit). crates/ravel-ingest/src/log_shard.rs:429: to_logseg_record still does stream_attrs: rec.stream_attrs.to_vec(), so the shared Arc<[u8]> is copied once per record when the flush builds the columnar batch. The saving D3a buys is buffer residency, not total allocations across the record's life. The PR description now carries that caveat; the commit message of 8038ebc still reads "normalization allocates 1 time per record instead of 2" with no qualifier, and that message is what survives in git log.

3 — open (nit). docs/ingest.md:1023 still says the flush configures the writer with PageChoice::Estimated without noting that ravel-cli load drives the same flush with Exhaustive (services/ravel-cli/src/load/logs.rs, log_load_ingest_config). changelog.d/3498.changed.md:3 does carry the exception, and docs/log-segment-format.md:725 now does too, so docs/ingest.md is the one place left where a reader meets the flush and is told there is only one page choice.

Verified

check_stream_attrs reproduces the row writer's collision rule. RlogWriter's row-path directory check (writer.rs:494-512) walks self.records in push order and keeps the first blob seen per stream id; check_stream_attrs does the same with a HashMap<LogStreamId, &Arc<[u8]>>, with Arc::ptr_eq only as a fast path before the byte comparison, so a second Arc holding equal bytes is still accepted and a differing one still refused. The error travels as RlogEncodeError::Finish(LogSegError::InconsistentStreamAttrs) and reaches the same LogWriteError::StreamIdCollision and record_stream_id_collision() at log_shard.rs:1073 the row path used.
Byte identity for D2. The flush builds ColumnarLogBatch::try_from_records with no .with_dictionaries(), which is the same shape columnar_build_matches_row_build_byte_for_byte and dictionary_columns_match_row_build_byte_for_byte already pin in ravel-logseg, including max_dyn in 1..8 so the max_dynamic_columns overflow path is covered on both sides. build applies that budget identically for row and columnar input.
admit_batch_per_id is equivalent to per-record admission, not just cheaper. EpochIdSet::admit returns true forever once an id is in current, and cap.admits_one_more(active_count) only gets stricter within a batch because active_count only grows; a rejected id is never inserted, so a first-occurrence reject stays a reject for every later occurrence. One entry is still pushed per candidate and the counters still move per candidate, so the shed accounting is unchanged. The new proptest compares full post-state tuples, not just the verdict vector.
effective_flush_tick can only shorten the tick (min against the existing flush_tick), so no test or deployment that sets a small flush_tick changes behaviour, and flush_tick has no CLI flag, so nothing a user set is silently overridden. The millennium-tick config test now asserts real numbers (40_500_000_000 / 3_559_500_000_000) where it used to assert the i64::MAX / 0 saturation, which is the point of D6.
The carried byte estimate is the same number the budget charged. log_router.rs computes record_bytes once, folds it into the charge, and zips it into each shard's ShardRecords; nothing between those two points mutates the records. merge_rows_unstamped takes bytes_added from the struct instead of recomputing it, and impl From<Vec<NormalizedLogRecord>> recomputes via the same est_record_bytes for the paths that still build a bare Vec.
Sharing the stream blob is safe: no production path mutates stream_attrs, and the normalizer builds one blob per ScopeLogs and Arc::clones it, which the Arc::ptr_eq test pins.

Checks run

cargo test -p ravel-ingest --lib: 397 passed, 0 failed. All eight new tests execute and pass, including the_columnar_flush_writes_the_row_writers_bytes across both page choices and both sort settings.
Revert check: deleting the check_stream_attrs(&records).map_err(RlogEncodeError::Finish)? line makes the_flush_batch_refuses_diverging_stream_attrs_as_the_row_writer_does fail at log_shard.rs:3547 ("the flush batch refuses the records from finish"). The test earns its place. File restored; git status --porcelain shows only the untracked node_modules.

Smaller things

services/ravel-server/tests/label_charge_allocations.rs: wide_stream_log_request() changed from one shared resource with UNITS records to UNITS resources with one record each, because the projection charges per record while normalization now allocates one blob per scope. That keeps the test meaningful, but it means the realistic shape (many records under one resource) is no longer covered by an allocation assertion. The divergence is in the safe direction, since the projection over-charges and so sheds earlier than real memory warrants, and the PR says so. Worth a sentence in the test's module doc so the next reader does not restore the old fixture and find it failing.
FLOOR_PER_RECORD dropping 2 to 1 in crates/ravel-otlp/tests/logs_normalize_allocations.rs:41 is the measurement that backs D3a, and the module doc at lines 9-14 already explains why the per-scope allocation cancels out of the two-size difference. Nothing to change; noting it as the place a future regression would surface.

Not checked

I did not run the ravel-server or ravel-cli test suites, nor any feature lane (sql, flight-sql, esql, agents, mcp), nor clippy or fmt. CI covers these.
I did not open the mechanical .into() / .to_vec() fixture edits spread across the ravel-ingest tests, ravel-bench, ravel-failure-tests, and the ravel-cli export and test-support files. They follow from the ShardRecords and Arc<[u8]> type changes and the crate compiles and tests green, but I did not read each one.
I did not reproduce the logseg_encode CPU figure the description cites from #3500 / #3494, so the claimed size of the win is unverified here.

Not checked:

The ravel-server and ravel-cli test suites, and every feature lane (sql, flight-sql, esql, agents, mcp), plus clippy and fmt: not run, left to CI.
The mechanical .into() / .to_vec() fixture edits across the ravel-ingest tests, ravel-bench, ravel-failure-tests, and the ravel-cli export and test-support files: not read individually.
The logseg_encode CPU figure cited from #3500 / #3494: not reproduced, so the claimed size of the win is unverified.

Task ffa373c0-1e20-4c70-a269-6440e9af2034 on ravel-clickbench-ubuntu-2, model claude-opus-5, effort medium.

pmoust mentioned this pull request Oct 30, 2026

fix(ingest): the log hand-back merge drops stream-id collisions silently #3510

Open

claude-fleet Bot reviewed Oct 30, 2026

View reviewed changes

pmoust added this pull request to the merge queue Oct 30, 2026

Merged via the queue into main with commit cb5c951 Oct 30, 2026
40 checks passed

pmoust deleted the land/3498-ingest-per-record branch October 30, 2026 15:50

pmoust mentioned this pull request Oct 30, 2026

Epic: 120k log lines/s on 8 vCPU (ingest CPU per line) #3492

Open

7 tasks

Navigation Menu

Repository navigation

feat(ingest): cut per-record work on the OTLP logs ingest path - #3509

feat(ingest): cut per-record work on the OTLP logs ingest path#3509
pmoust merged 3 commits into
mainfrom
land/3498-ingest-per-record

pmoust commented Oct 30, 2026 •

edited

Loading

Uh oh!

pmoust commented Oct 30, 2026

Uh oh!

claude-fleet Bot commented Oct 30, 2026 •

edited

Loading

Uh oh!

claude-fleet Bot left a comment

Uh oh!

claude-fleet Bot Oct 30, 2026

Uh oh!

pmoust commented Oct 30, 2026

Uh oh!

claude-fleet Bot commented Oct 30, 2026 •

edited

Loading

Uh oh!

claude-fleet Bot left a comment

Uh oh!

claude-fleet Bot Oct 30, 2026

Uh oh!

claude-fleet Bot Oct 30, 2026

Uh oh!

pmoust commented Oct 30, 2026

Uh oh!

claude-fleet Bot commented Oct 30, 2026 •

edited

Loading

Uh oh!

claude-fleet Bot left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

pmoust commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmoust commented Oct 30, 2026

Uh oh!

claude-fleet Bot commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude-fleet Bot left a comment

Choose a reason for hiding this comment

Uh oh!

claude-fleet Bot Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

pmoust commented Oct 30, 2026

Uh oh!

claude-fleet Bot commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude-fleet Bot left a comment

Choose a reason for hiding this comment

Uh oh!

claude-fleet Bot Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

claude-fleet Bot Oct 30, 2026

Choose a reason for hiding this comment

Uh oh!

pmoust commented Oct 30, 2026

Uh oh!

claude-fleet Bot commented Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude-fleet Bot left a comment

Choose a reason for hiding this comment

Verdict

Previous findings

Verified

Checks run

Smaller things

Not checked

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

pmoust commented Oct 30, 2026 •

edited

Loading

claude-fleet Bot commented Oct 30, 2026 •

edited

Loading

claude-fleet Bot commented Oct 30, 2026 •

edited

Loading

claude-fleet Bot commented Oct 30, 2026 •

edited

Loading