feat(server): querier OTel metrics + flip RFC 0016 green (rfc0016 .6) - #285
Conversation
Emit the querier role's OpenTelemetry metrics (RFC 0016 §3.6), filling the
last red arm (RFC0016.6) and flipping RFC 0016 to `green` (7/7 §5).
Per the OpenTelemetry usage/state convention (confirmed via the OTel docs:
record raw counts, derive ratios in the backend), the pruning signal is
emitted as raw scanned-vs-pruned row-group counts rather than a pre-computed
ratio histogram:
- `ourios.query.duration` (histogram, `s`) tagged by `ourios.query.kind`
(logs | drift), and with the upstream `error.type` attribute on a failed
query (the "recording errors on metrics" convention — no bespoke error
metric).
- `ourios.query.row_groups` (counter, `{row_group}`) split by
`ourios.query.row_group.state` (scanned | pruned). The two states partition
the candidate row groups, so the B1 pruned fraction derives in the backend
as `pruned / (scanned + pruned)`.
New names flow through the semconv registry + weaver (regenerated
ourios-semconv). Recorded in the server handler around run_query / run_drift,
built against the global meter (RFC 0001 §6.8).
RFC0016.6 lives in its own integration binary (its own process) since it
installs a process-global in-memory MeterProvider: a multi-hour corpus where
each file holds a distinct template_id, a selective `template_id ==` query
prunes the rest, and the test asserts both the response's row_groups_pruned
and the exported metric stream (the pruned-state counter == the response).
Also hardens RFC0016.5's disabled-role case: it now just confirms no listener
binds and reaps the process, rather than blind-waiting then asserting a clean
SIGTERM exit (a compactor-only server prints no readiness line, so the wait
raced signal-handler setup — graceful shutdown stays asserted in the enabled
case, which waits for the printed address first).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 28 minutes and 3 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughImplements RFC0016.6 observability by adding two OpenTelemetry metrics—a query-duration histogram and a row-group scanned/pruned counter—to the ChangesRFC0016.6 Query Observability Metrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Adds OpenTelemetry metrics for the querier role (RFC 0016 §3.6), updates semantic-convention registry + generated constants, and flips RFC 0016 to green by replacing the last red-gate stub with an end-to-end metrics integration test.
Changes:
- Define and emit
ourios.query.duration(histogram) andourios.query.row_groups(counter split by scanned/pruned) from the querier handler. - Extend semconv registry (metrics + attributes) and regenerate
ourios-semconvconstants accordingly. - Add a dedicated RFC0016.6 integration test binary that installs a process-global in-memory
MeterProviderand asserts exported metric data; adjust RFC0016.5 served-binary test to avoid flaky shutdown sequencing.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| semconv/registry/metrics.yaml | Registers the two new querier metrics and required attributes/units. |
| semconv/registry/attributes.yaml | Registers ourios.query.kind and ourios.query.row_group.state attribute enums. |
| docs/rfcs/0016-query-serving-endpoint.md | Marks RFC 0016 as green and documents the final metrics slice outcome. |
| crates/ourios-server/tests/rfc0016_query_endpoint.rs | Removes the RFC0016.6 ignored stub and points to the new binary test. |
| crates/ourios-server/tests/rfc0016_6_query_metrics.rs | New integration test validating pruning stats + exported OTel metric stream. |
| crates/ourios-server/tests/rfc0016_5_7_served_querier.rs | Updates disabled-role cleanup logic to avoid readiness/shutdown races. |
| crates/ourios-server/src/querier.rs | Implements querier OTel instruments and records success/error measurements. |
| crates/ourios-server/Cargo.toml | Adds OTel metrics API + semconv deps; adds dev-deps for in-memory exporter test. |
| crates/ourios-semconv/src/lib.rs | Regenerated constants for new metric and attribute keys. |
| Cargo.lock | Locks new dependencies (opentelemetry, opentelemetry_sdk, ourios-semconv). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/ourios-server/tests/rfc0016_5_7_served_querier.rs`:
- Around line 156-160: The `drop(disabled)` call only sends an asynchronous kill
signal without waiting for the process to actually exit, which can cause race
conditions when the enabled-phase process spawns immediately after. Instead of
just dropping the `disabled` variable, explicitly wait for the process to finish
exiting by calling `.wait()` on it (or using `timeout` as done elsewhere in the
test) before the enabled process is spawned at line 163, ensuring deterministic
test execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7857a0c3-eed3-4373-b43c-33a1254ee29c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
crates/ourios-semconv/src/lib.rscrates/ourios-server/Cargo.tomlcrates/ourios-server/src/querier.rscrates/ourios-server/tests/rfc0016_5_7_served_querier.rscrates/ourios-server/tests/rfc0016_6_query_metrics.rscrates/ourios-server/tests/rfc0016_query_endpoint.rsdocs/rfcs/0016-query-serving-endpoint.mdsemconv/registry/attributes.yamlsemconv/registry/metrics.yaml
#285) Replace `drop(disabled)` with `disabled.kill().await` (SIGKILL + wait) in RFC0016.5: `kill_on_drop` only fires a best-effort, un-awaited signal, so the compactor-only process could linger while the enabled-phase process spawns — a CI flake. `Child::kill` reaps it deterministically first. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The `ourios.query.row_groups` seeding is not attribute-free (the `state` attribute is required); reword to say it seeds once per state value, rather than the ingester's single attribute-free `add(0, &[])`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
What
RFC 0016 green slice 3 (final): emit the querier role's OpenTelemetry metrics (RFC0016.6), the last red arm — this flips RFC 0016 →
green(7/7 §5). Slices 1 (#283, handler) and 2 (#284, role env-gating + compose) preceded.OTel design (consulted the OpenTelemetry docs)
The OTel naming spec is explicit: record raw count components, derive ratios/utilization in the backend (the
*.usage+statepattern;*.utilizationis a derived/optional presentation metric). So instead of a pre-computed "pruning ratio" histogram:ourios.query.duration— histogram, units(the{operation}.durationconvention), tagged byourios.query.kind(logs|drift). A failed query carries the upstreamerror.typeattribute on this same metric (the "recording errors on metrics" convention — no bespoke error metric).ourios.query.row_groups— counter, unit{row_group}, split byourios.query.row_group.state(scanned|pruned). The two states partition the candidate row groups, so the B1 pruned fraction derives in the backend aspruned / (scanned + pruned).New names flow through the semconv registry (
semconv/registry/{metrics,attributes}.yaml) + weaver (regeneratedourios-semconv). Recorded in the server handler aroundrun_query/run_drift, built against the global meter (RFC 0001 §6.8).Tests
MeterProvider, single-threaded, mirroringourios-ingester'sperf_metrics): a multi-hour corpus where each file holds a distincttemplate_id; a selectivetemplate_id == 1query prunes the rest; asserts the response'srow_groups_pruned > 0and the exported metric stream (duration histogram count == 1; the pruned-state counter == the response's pruned count).rfc0003_16).cargo fmt --all --check,cargo clippy -p ourios-server -p ourios-semconv --all-targets --all-features -- -D warnings,cargo test -p ourios-server -p ourios-semconv,weaver registry check, regeneratedourios-semconvis in sync,mdbook build..5/.7ran 4× clean.Invariants / hazards
Observability (§6.3): the querier is now self-observable. New metric/attribute names went through the weaver registry (no hand-written flat names). No hot-path or on-disk-format change; multi-tenancy and the H6 boundary are unchanged from #283/#284.
RFC 0016 status
green— all seven §5 scenarios pass. gRPC and authn/z beyond tenant-scoping remain deferred (§7).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests