feat(miner): expose the §6.8 OTel metric set (§3.1.2/RFC0001.8/H2.2) - #159
feat(miner): expose the §6.8 OTel metric set (§3.1.2/RFC0001.8/H2.2)#159jensholdgaard wants to merge 2 commits into
Conversation
Register the full RFC 0001 §6.8 mandatory metric set on the
process-global `ourios.miner` meter (API-only dep per the §6.8
export-architecture split; SDK/OTLP stay in ourios-telemetry) and
flip three red-gate stubs into real AAA tests.
Metrics now exposed (flat pre-redesign names per §6.8, matching the
existing alias_* precedent — the dotted-ourios.* semconv conversion
is the deferred §6.8 redesign, not this slice):
- counters: merges_total{tenant_id,event_type},
parse_failures_total{tenant_id,service},
params_overflow_total{tenant_id,service},
template_version_changes_total{tenant_id}
- histograms: confidence{tenant_id,service},
miner_latency_seconds{tenant_id}
- observable gauges: template_count{tenant_id},
confidence_p50/_p01{tenant_id,service},
body_retention_ratio{tenant_id},
params_overflow_ratio{tenant_id,service}
The atomic counters stay as the in-process read path (test
accessors); the OTel instruments are driven in lockstep at the same
centralized emission sites. Init-seeded so the full set surfaces at
zero traffic (§3.1.2 collect-on-read guarantee).
confidence_p50/_p01 are observable gauges over an exact nearest-rank
quantile of a bounded (1024-sample) per-(tenant,service) reservoir;
the confidence histogram remains the §6.8 source of truth. §6.8 does
not pin the quantile mechanism/window — flagged for the maintainer's
OTel review (RFC already names the gauge-vs-backend-derived fork).
params_overflow_ratio = overflow_lines / lines per (tenant,service);
H2.2's "alert" is the gauge crossing the documented 0.01 threshold
(we ship the metric + rule, not an alerting engine).
semconv weaver job unaffected: semconv/registry/ and ourios-semconv
untouched; the miner's flat-named metrics were never in the registry.
Touches §3.1 (mandatory telemetry) / §6.3 (observability).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 31 minutes and 47 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. 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 ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR implements OpenTelemetry metrics instrumentation for the miner cluster per RFC 0001 §6.8, adding a canonical event-type mapping to ChangesRFC 6.8 Telemetry Metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The changes span multiple files with mixed density: the metrics module introduces substantial logic (instruments, reservoirs, quantile computation, shared state management), the cluster integration involves numerous call-site updates and refactoring of helpers throughout ingest/emit paths, and the test suite requires understanding of telemetry exporter/gauge/counter inspection patterns. The PR's cohesion—all changes work together to implement one RFC—partially offsets the breadth, but the variety of metrics types (counters, histograms, gauges, quantiles) and ingest-path complexity demand careful review. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Implements the RFC 0001 miner telemetry slice by registering and driving the RFC §6.8 OpenTelemetry metric set from ourios-miner (API-only), and flips the red-gate test stubs for §3.1.2, RFC0001.8, and H2.2 to green.
Changes:
- Add
MinerMetricswith §6.8 counters/histograms plus observable gauges backed by in-process state (including a bounded confidence reservoir). - Wire telemetry emission into the miner hot paths (
ingest, merge emission, parse/tokenizer failure paths, overflow retention, and record emission). - Add in-memory OTel SDK-based integration tests for mandatory metric exposure, confidence quantile gauges, and per-service overflow ratio threshold behavior.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ourios-miner/src/metrics.rs | New miner metric instrument registration + callbacks + state/reservoir implementation. |
| crates/ourios-miner/src/cluster.rs | Hooks metric recording into ingest/emission sites and merge events. |
| crates/ourios-miner/src/lib.rs | Adds the internal metrics module. |
| crates/ourios-miner/Cargo.toml | Adds OTel API dependency + dev deps for in-memory SDK testing. |
| crates/ourios-miner/tests/invariants.rs | Implements §3.1.2 mandatory metric-set exposure test using in-memory exporter. |
| crates/ourios-miner/tests/rfc_internal.rs | Implements RFC0001.8 p50/p01 gauge test with cross-check vs sink samples. |
| crates/ourios-miner/tests/hazards.rs | Implements H2.2 per-service overflow ratio threshold test. |
| crates/ourios-core/src/audit.rs | Adds TemplateChange::event_type() for merges_total event_type attribute. |
| Cargo.lock | Records new (dev) dependencies used by miner tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
….* + fix observable handles Migrates the RFC 0001 §6.8 miner metric set off flat names (template_count, merges_total, …) onto the dotted-`ourios.miner.*` weaver registry alongside the compaction set, regenerating `ourios-semconv` and consuming the generated constants in `crate::metrics`. Adds the `ourios.service` attribute and the `ourios.miner.template_change` enum (widened / type_expanded) to the registry; the merges change-kind moves from the flat `event_type` attribute onto it. The §3.1.2 / RFC0001.8 / H2.2 tests assert the dotted names via the generated constants. Also folds the Copilot correctness fixes: - retain the five observable-gauge handles in `MinerMetrics` so their collection callbacks stay registered (a dropped handle deregisters the callback, dropping the gauge from the exported stream); - recover a poisoned lock in the four collection callbacks rather than panic (a metrics callback must never bring the process down); - pre-allocate the confidence reservoir to RESERVOIR_CAP. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| let tenant_event = [ | ||
| KeyValue::new(semconv::OURIOS_TENANT, INIT_SENTINEL), | ||
| KeyValue::new(semconv::OURIOS_MINER_TEMPLATE_CHANGE, INIT_SENTINEL), | ||
| ]; |
| .with_callback(move |obs| { | ||
| let st = s.lock().unwrap_or_else(std::sync::PoisonError::into_inner); | ||
| let mut emitted = false; | ||
| for ((tenant, service), tally) in &st.by_service { | ||
| if let Some(v) = tally.confidence.quantile(q) { | ||
| obs.observe(v, &service_attrs(tenant, service)); | ||
| emitted = true; | ||
| } | ||
| } | ||
| if !emitted { | ||
| obs.observe( | ||
| 0.0, | ||
| &[ | ||
| KeyValue::new(semconv::OURIOS_TENANT, INIT_SENTINEL), | ||
| KeyValue::new(semconv::OURIOS_SERVICE, INIT_SENTINEL), | ||
| ], | ||
| ); | ||
| } | ||
| }) |
| pub(crate) fn record_line(&self, tenant: &TenantId, service: &str, confidence: f64) { | ||
| self.confidence | ||
| .record(confidence, &service_attrs(tenant, service)); | ||
| let mut st = self.state.lock().expect("metrics state mutex poisoned"); |
| } | ||
| self.params_overflow_total | ||
| .add(count, &service_attrs(tenant, service)); | ||
| let mut st = self.state.lock().expect("metrics state mutex poisoned"); |
| /// Record one body-retention event for the | ||
| /// `body_retention_ratio` numerator (§6.3 retention paths). | ||
| pub(crate) fn record_body_retention(&self, tenant: &TenantId) { | ||
| let mut st = self.state.lock().expect("metrics state mutex poisoned"); |
| /// Mirror a tenant's current template count into the state the | ||
| /// `template_count` observable gauge reads. | ||
| pub(crate) fn set_template_count(&self, tenant: &TenantId, count: u64) { | ||
| let mut st = self.state.lock().expect("metrics state mutex poisoned"); |
|
Closing to redo this spec-first. This slice was built code-first (the instrumentation with flat names, then the weaver registry retrofitted) — which inverts the standing process: new OTel metrics/attributes must be designed in the Reopening as a fresh PR whose main theme is the semconv/weaver registry design (so the review reads spec-first), with the §6.8 |
Summary
Implements the RFC 0001 miner telemetry slice: exposes the mandatory §6.8 OpenTelemetry metric set and flips three red-gate stubs — §3.1.2 (mandatory metric set exposed), RFC0001.8 (confidence_p50/p01 gauges), H2.2 (per-service params-overflow-rate alert).
Instruments are registered on the process-global
ourios.minermeter via the lightweightopentelemetryAPI crate only — the heavy SDK/OTLP exporter stays inourios-telemetryper the §6.8 export-architecture split. No new crate (theourios-telemetrycommitment already exists).§6.8 metrics now exposed
Flat, pre-redesign names + data-point attributes exactly as the §6.8 table pins (matching the existing
alias_*precedent; the dotted-ourios.*semconv conversion is the deferred §6.8 redesign, not this slice):template_counttenant_idmerges_totaltenant_id,event_typeconfidencetenant_id,serviceconfidence_p50/confidence_p01tenant_id,servicebody_retention_ratiotenant_idparse_failures_totaltenant_id,serviceparams_overflow_totaltenant_id,serviceparams_overflow_ratiotenant_id,servicetemplate_version_changes_totaltenant_idminer_latency_secondstenant_id(
alias_assertions_total/alias_retractions_totalare already implemented inourios-core::alias.)The existing atomic counters stay as the in-process read path (test accessors like
merges_total()); the OTel instruments are driven in lockstep at the same centralized emission sites (apply_overflow_retention,record_parse_failure,record_tokenizer_failure, the merge site,emit_record,ingest). Synchronous instruments are init-seeded with a zero-add/ sentinel record so the full mandatory set surfaces in the first collection cycle at zero traffic (§3.1.2's collect-on-read guarantee); observable gauges always emit at least one sentinel point.Quantile approach (flagged OTel decision)
confidence_p50/confidence_p01are observable gauges over an exact nearest-rank quantile of a bounded 1024-sample per-(tenant, service)reservoir. Theconfidencehistogram remains the §6.8 source of truth; the gauges are the named in-process views per RFC0001.8.Open question for the maintainer's OpenTelemetry review: §6.8 does not pin the quantile mechanism (sketch vs. exact), the window size, or whether p50/p01 should stay in-process gauges vs. become collector-/backend-derived quantiles over the exported histogram (the OTLP-native idiom). The RFC itself flags this exact fork as a contract change deferred to the dotted-semconv redesign. I picked the simplest defensible option (exact-over-bounded-window) and isolated it in
Reservoirso the redesign can swap it under its own review.Overflow ratio / H2.2 alert
params_overflow_ratio = overflow_lines / linesper(tenant_id, service)(a line is counted once toward the numerator regardless of how many of its params overflowed;params_overflow_totalstill counts per-param). H2.2's "alert" is the gauge crossing the documented0.01threshold — Ourios ships the metric + the alert rule, not an alerting engine. The test drives a service to ~1.48% overflow and asserts the gauge exceeds 0.01 while a clean sibling service stays at 0.0 (per-service isolation).Invariants / hazards touched
No existing passing test was weakened. Added
TemplateChange::event_type()(mirrorsAuditPayload::event_type) for themerges_totalevent_typeattribute.semconv weaver
Unaffected.
semconv/registry/and the generatedourios-semconvcrate are untouched; the miner's flat-named metrics were never in the weaver registry (same asalias_*). The dotted-semconv registry entries are the deferred §6.8 redesign. otel-weaver semantic-conventions integration tests remain a deferred follow-up, not added here.Verification (local)
cargo test --all-features— all binaries pass, 0 failed, exit 0 (the three flipped stubs now green; lib unit tests include 5 newmetrics.rstests).cargo fmt --all --check— clean.cargo clippy --all-targets --all-features -- -D warnings— clean.weaveris not installed locally; reasoned from the untouched registry + the CI job definition rather than running it.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests