feat(bench): RFC 0031 — Loki container integration, RFC0031.1 green - #471
Merged
Conversation
The single source of truth for the RFC0031.1 equivalence check, in its own commit so it reviews in isolation (container wiring follows). - `FixtureRecord` + `comparative_fixture(base_ns)` — a deterministic 3-record set (INFO/INFO/ERROR, distinct timestamps). `base_ns` is a parameter because Loki's default reject_old_samples refuses old lines, so the container test stamps near now while local tests use any base. - `fixture_logs_data` — the records as one OTLP `LogsData` (one resource, `service.name = comparative-fixture`; one scope). Both systems' inputs derive from this one value: the Ourios corpus line and the Loki OTLP push, so `(timestamp, body)` is byte-identical by construction. - `fixture_jsonl` — the `LogsData` as one OTLP/JSON Lines corpus line (serde against the same with-serde type `ingest_otlp_jsonl` parses, so it round-trips by construction). - `fixture_line_keys` — the expected `LineKey`s a query matching every fixture record must return from either system. Local proof (no container): `fixture_round_trips_through_the_ourios_side` drives the fixture through the registry-bearing comparative store and the in-process query, and asserts the extracted LineKeys equal `fixture_line_keys` — the same expected keys the Loki container run is compared against. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesRFC0031.1 Loki equivalence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ComparativeFixture
participant OuriosComparativeStore
participant LokiContainer
participant EquivalenceAssertions
ComparativeFixture->>OuriosComparativeStore: provide JSONL fixture
OuriosComparativeStore->>EquivalenceAssertions: return Ourios LineKeys
ComparativeFixture->>LokiContainer: push OTLP LogsData
LokiContainer->>EquivalenceAssertions: return broad and narrow query results
EquivalenceAssertions->>EquivalenceAssertions: compare broad equality and narrow non-equality
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The final slice of the equivalence harness: RFC0031.1 becomes a real
end-to-end test on a live Loki container (grafana/loki 3.5.3,
digest-pinned like Dex), run by the new required `loki-interop` CI job
via --ignored --exact — the dex-oidc / collector-interop pattern; there
is no local Docker, so validation is intentionally CI-only.
The test: the shared OTLP fixture (commit 1) is ingested by BOTH
systems — Ourios through the registry-bearing comparative store +
in-process query (the locally-proven path), Loki through its native
OTLP endpoint (the same LogsData value, prost-encoded to
/otlp/v1/logs) — then queried equivalently (logs DSL "severity >= 0"
vs LogQL {service_name="comparative-fixture"}), and the two LineKey
multisets must be identical via compare_lines. A deliberately narrower
LogQL (|= `logged in`, 2 of 3 lines) asserts the mismatch arm reports
Mismatch rather than silently passing.
Structure notes:
- Plain #[test]: ourios_query_lines owns its own runtime, so the Ourios
half runs sync and only the container half runs inside block_on
(nesting would panic).
- The fixture is stamped near now (Loki's reject_old_samples window).
- Readiness polls /ready and surfaces the container's stderr on timeout
(a config rejection must not read as a bare timeout).
- New dev-deps: testcontainers-modules, reqwest (no TLS — plain http to
the mapped port), prost 0.14 (the opentelemetry-proto 0.32
generation), tokio rt-multi-thread + macros.
- CI: loki-interop job added and wired into ci-success's needs.
Default `cargo test` stays green (0 passed; 11 ignored — the live test
is #[ignore]d for Docker, ten red stubs remain for the later slices).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the RFC 0031.1 “result-set equivalence” slice end-to-end by adding a shared OTLP fixture and a Loki testcontainers-based interop test, then wiring that ignored test into CI via a dedicated required job.
Changes:
- Adds a single-source-of-truth comparative fixture (
LogsData+ JSONL rendering + expectedLineKeys) and a local Ourios-side round-trip proof test. - Converts
RFC0031.1from a red stub to a real ignored test that runs Loki in a container, pushes OTLP protobuf, queries LogQL, parses results, and compares viacompare_lines. - Introduces a new
loki-interopGitHub Actions job to run the ignored container test in CI.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-bench/tests/rfc0031_comparative.rs | Implements the Loki container-based RFC0031.1 interop test and equivalence assertion. |
| crates/ourios-bench/src/lib.rs | Re-exports new comparative fixture helpers/types for bench/test use. |
| crates/ourios-bench/src/comparative.rs | Adds the shared OTLP fixture API and a local Ourios-side round-trip test. |
| crates/ourios-bench/Cargo.toml | Adds dev-dependencies needed for Loki container interop (testcontainers/reqwest/prost/tokio features). |
| Cargo.lock | Locks new dev-dependency additions. |
| .github/workflows/ci.yml | Adds required loki-interop CI job to run the ignored RFC0031.1 test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nes) CI clippy: the RFC0031.1 test fn hit too_many_lines (140/100). Extract the container half as `loki_round_trip` (container + readiness + OTLP push + both queries) and `loki_query_range` (one query_range call → LineKeys). No behavior change — the loki-interop job already PASSED on the previous head; this only restructures for the lint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ourios-bench/tests/rfc0031_comparative.rs (1)
125-125: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to the
reqwest::Clientto prevent indefinite CI hangs.
Client::new()has no request timeout. If Loki accepts a connection but never responds (e.g., config rejection, deadlock),.send().awaitblocks forever — the deadline-based polling loops never reach their timeout checks because they run after the request returns. The OTLP push has no deadline at all. A hung request would block the job until GitHub's 6-hour default timeout.♻️ Proposed fix
- let http = reqwest::Client::new(); + let http = reqwest::Client::builder() + .timeout(Duration::from_secs(30)) + .build() + .expect("http client");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/ourios-bench/tests/rfc0031_comparative.rs` at line 125, Update the reqwest client initialization in the comparative benchmark test to configure a finite request timeout instead of using Client::new(). Ensure the timeout applies to both Loki polling requests and the OTLP push, using the existing project convention or an appropriate bounded duration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/ourios-bench/tests/rfc0031_comparative.rs`:
- Line 125: Update the reqwest client initialization in the comparative
benchmark test to configure a finite request timeout instead of using
Client::new(). Ensure the timeout applies to both Loki polling requests and the
OTLP push, using the existing project convention or an appropriate bounded
duration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 85e35305-24a1-4c51-aeb6-310f594ad21a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/ci.ymlcrates/ourios-bench/Cargo.tomlcrates/ourios-bench/src/comparative.rscrates/ourios-bench/src/lib.rscrates/ourios-bench/tests/rfc0031_comparative.rs
- tokio dev-dep: declare the `time` feature explicitly (sleep was compiling only via transitive feature unification). - reqwest client: 10 s per-request timeout so a wedged container/network fails the request instead of hanging the CI job past its deadlines. - Readiness panic surfaces BOTH container streams (Loki writes startup errors to stdout too, not just stderr). - loki_query_range checks the HTTP status before parsing — a non-2xx body may not be streams JSON, and "parse failed" would mask the real error. - fixture_logs_data rustdoc: proper FIXTURE_SERVICE intra-doc link. No behavior change on the green path (the loki-interop job already passed); these harden the failure modes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot review: the narrow line-filter used LogQL backtick (raw-string) quoting — valid, and the passing loki-interop run proves Loki accepted it — but the conventional form is double quotes; switch. More substantively, the comment surfaced a latent weakness: the mismatch arm asserted only inequality, so a silently-broken filter matching 0 lines would still "pass". Pin the narrow result to exactly the two "logged in" lines. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot review: the 30 s backdate used bare subtraction, which would panic on debug-build underflow if the clock were absurdly near the Unix epoch. saturating_sub keeps it total. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
What
The final slice of the RFC 0031 equivalence-harness: the Loki testcontainers integration that flips the
RFC0031.1stub green. Structured per review request:comparative_fixture(base_ns)→ one OTLPLogsData(service.name = comparative-fixture), rendered two ways:fixture_jsonl(the Ourios corpus line, serde against the samewith-serdetype the loader parses — round-trips by construction) and the sameLogsDataprotobuf-encoded for Loki's OTLP push.fixture_line_keysis the expected answer both systems must return. Local proof included:fixture_round_trips_through_the_ourios_sidedrives the fixture through the registry-bearing store + in-process query and asserts the extractedLineKeys equalfixture_line_keys.grafana/loki(testcontainers, Dex-style), push the fixture over OTLP (/otlp/v1/logs, protobuf),query_rangethe equivalent LogQL ({service_name="comparative-fixture"}),parse_loki_streams→compare_linesagainst the Ourios side → theRFC0031.1stub becomes a real#[ignore]d test run by a newloki interop (testcontainers)CI job (modeled oncollector-interop), plus a deliberate-mismatch arm (a narrower LogQL must yieldMismatch).ourios-bench:testcontainers-modules,reqwest,prost(protobuf encode),tokiotest features.Intentionally CI-only (like Dex)
The container test is
#[ignore]d and executed by the CI job via--ignored --exact— there is no local Docker in the dev environment, matching thedex oidc/collector interopprecedent. Everything around the container is already proven locally (comparator #467, Ourios extraction #468, registry-bearing store #469, Loki response parser #470, and commit 1's fixture round-trip); the container run validates only the last-mile Loki specifics.The two known subtleties — validated through the container run
LogsDatavalue, so(timestamp, body)is byte-identical by construction; the container run proves Loki's OTLP ingest → LogQL → response path preserves it (nanosecond timestamps and body bytes) end to end.body_bytesdeliberately errors on structured/absent bodies rather than collapsing them. ExtendingLineKeywith a body-kind discriminator lands with the OTLP-native L2/L3/L4 gates, not here.Scope
Focused on flipping
RFC0031.1green with one query pair. Nicer LogQL, more pairs, and the bytes-read metrics are follow-ups per the RFC's slice plan.🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores