Skip to content

feat(semconv): add the ourios.miner.* metric registry + spec-first miner telemetry (§3.1.2/RFC0001.8/H2.2) - #160

Merged
jensholdgaard merged 23 commits into
mainfrom
rfc0001-miner-semconv
Jun 8, 2026
Merged

feat(semconv): add the ourios.miner.* metric registry + spec-first miner telemetry (§3.1.2/RFC0001.8/H2.2)#160
jensholdgaard merged 23 commits into
mainfrom
rfc0001-miner-semconv

Conversation

@jensholdgaard

@jensholdgaard jensholdgaard commented Jun 8, 2026

Copy link
Copy Markdown
Owner

Redo of #159 (closed for inverting the process) as a spec-first change: the weaver/semconv registry design is the main theme, authored and audited before any instrumentation. History reads spec-first — commit 1 is the registry + regenerated ourios-semconv; commit 2 is the code built on the generated constants.

1. OTel formulation rules applied (registry audit)

Authored against the OpenTelemetry semantic-conventions naming rules (docs/general/naming.md, docs/general/metrics.md) and the weaver registry policies (weaver registry check --future, clean):

  • Counters carry no _total suffix (Prometheus-ism; OTel reserves it for delta-backend monotonic sums). Names are bare nouns with singular UCUM {annotation} units — merges {merge}, parse_failures {failure}, params.overflow {overflow}, template.version_changes {change}. The annotation matches grammatical number ({merge}, not {merges}).
  • Fraction-of-total gauges use the conventional .utilization segment (unit 1), not a bespoke .ratio. OTel names a "fraction out of a total" utilization with the dimensionless unit 1.
  • Elapsed-time histogram is duration (ourios.miner.duration, UCUM s) — the conventional segment for a discrete operation's elapsed time — not latency.
  • Units live in metadata, never in the name (so miner_latency_secondsourios.miner.duration + unit s).
  • Lowercase dotted namespaces, snake_case within a segment; every entry carries stability and brief.
  • Required vs recommended attributes: ourios.tenant is required (always present); ourios.service is recommended (a line may carry no source service.name) — an honest contract rather than over-promising required.

What changed vs the closed #159

#159 was treated as an un-audited starting reference. The audit changed three names:

#159 this PR why
ourios.miner.body_retention.ratio ourios.miner.body_retention.utilization .utilization is the OTel segment for a fraction-of-total
ourios.miner.params.overflow.ratio ourios.miner.params.overflow.utilization same
ourios.miner.latency ourios.miner.duration duration is the OTel segment for operation elapsed time

The confidence.p50 / confidence.p01 gauges are kept as in-process named views of the confidence histogram (RFC0001.8); the histogram-vs-backend-quantile fork stays deferred to its own review, as the RFC requires.

2. Registry entries (semconv/registry/)

Metrics group ourios.miner.* (alongside the compaction set):

  • counters: merges (attr ourios.miner.template_change), parse_failures, params.overflow, template.version_changes
  • histograms: confidence (unit 1), duration (unit s)
  • observable gauges: template.count, body_retention.utilization, params.overflow.utilization, confidence.p50, confidence.p01

Attributes: reuse ourios.tenant; add ourios.service; add the ourios.miner.template_change enum (widened / type_expanded, wire values template_widened / template_type_expanded matching the audit payloads).

weaver registry check -r semconv/registry --future is clean; weaver registry generate ... && cargo fmt -p ourios-semconv is a verified no-diff (the CI invariant).

3. Generated constants (ourios-semconv)

Regenerated from the registry; consumed in code as use ourios_semconv as semconv; — no flat/hand-written name strings anywhere. The exports test locks the new dotted names.

4. Instrumentation built on the constants (ourios-miner)

crate::metrics instruments the miner through the global ourios.miner meter (API-only dep per the §6.8 export-architecture split). Correctness baked in from the start (these were post-hoc review fixes on #159):

  • the five ObservableGauge handles are retained on MinerMetrics (a dropped handle deregisters its callback);
  • collection callbacks recover a poisoned lock via lock().unwrap_or_else(PoisonError::into_inner) — never panic inside collect;
  • the confidence reservoir is VecDeque::with_capacity(RESERVOIR_CAP).

Wired into cluster.rs; the existing atomic counters feed the OTel instruments at the same sites. Three red-gate stubs flipped to AAA tests over an opentelemetry_sdk in-memory MeterProvider, asserting via the generated constants: §3.1.2 (mandatory set exposed at zero traffic), RFC0001.8 (p50/p01 gauges), H2.2 (per-(tenant,service) overflow utilization > 0.01 with sibling isolation).

Invariants / hazards touched (CLAUDE.md §3 / §4)

  • §3.1 / H1template_change enum keeps widening vs type-expansion distinct in the merges series; no silent merge introduced.
  • §3.2 / H2params.overflow counter + params.overflow.utilization gauge are the per-service overflow signal; H2.2 asserts the >1% alert threshold per (tenant, service).
  • §3.5 / H5template.version_changes counter tracks schema evolution.
  • §3.7 — every instrument is keyed by ourios.tenant.

Verification (all green, reproduces CI)

  • weaver registry check -r semconv/registry --future — clean; generate + fmt — no-diff
  • cargo test --all-features549 passed; 0 failed; 33 ignored (unrelated RFC red gates)
  • cargo fmt --all --check — clean
  • cargo clippy --all-targets --all-features -- -D warnings — clean
  • mdbook build — only the benign mdbook-mermaid version warning

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added comprehensive OpenTelemetry metrics for the template miner (duration, template count, confidence histogram + p50/p01 gauges, merges, parse failures, params overflow, body-retention, template-version changes, alias assertions/retractions). Metrics include tenant and optional service dimensions and a template-change attribute.
  • Tests

    • Added tests validating mandatory metric exposure, per-service overflow alerting, and confidence p50/p01 correctness.
  • Documentation

    • RFC and semconv registry updated to dotted ourios.miner.* metric and attribute names.

Author the §6.8 template-miner metric group in the weaver registry
alongside the compaction set, audited against the OpenTelemetry
semantic-conventions naming rules and the weaver registry policies:

- counters carry no `_total` suffix and singular `{annotation}` units
  (`merges` `{merge}`, `parse_failures` `{failure}`,
  `params.overflow` `{overflow}`, `template.version_changes`
  `{change}`);
- fraction-of-total gauges use the conventional `.utilization` segment
  with the dimensionless unit `1`
  (`body_retention.utilization`, `params.overflow.utilization`),
  not the non-conventional `.ratio`;
- elapsed-time histogram is `duration` (UCUM `s`), not `latency`;
- `confidence` is a histogram (unit `1`); `confidence.p50` / `.p01`
  stay in-process named gauges per RFC0001.8 (the
  histogram-vs-backend-quantile fork remains deferred);
- new attributes: `ourios.service` (the log's source service, distinct
  from Ourios's own `service.name` resource attribute) and the
  `ourios.miner.template_change` enum (widened / type_expanded, wire
  values matching the audit `template_widened` / `template_type_expanded`).
  `ourios.tenant` is `required`; `ourios.service` is `recommended`
  (it may be absent on lines that carry no source service.name).

`weaver registry check -r semconv/registry --future` is clean; the
generated `ourios-semconv` is regenerated (generate + `cargo fmt` is a
no-diff). The exports test locks the new dotted names.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@jensholdgaard
jensholdgaard requested a review from Copilot June 8, 2026 06:48
@jensholdgaard

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c7720850-04be-4823-8288-274ad9533a21

📥 Commits

Reviewing files that changed from the base of the PR and between 0c1edd2 and 12b6ef2.

📒 Files selected for processing (5)
  • crates/ourios-miner/src/cluster.rs
  • crates/ourios-miner/src/metrics.rs
  • crates/ourios-miner/tests/invariants.rs
  • docs/rfcs/0001-template-miner.md
  • semconv/registry/metrics.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
  • semconv/registry/metrics.yaml
  • crates/ourios-miner/tests/invariants.rs
  • docs/rfcs/0001-template-miner.md
  • crates/ourios-miner/src/cluster.rs

📝 Walkthrough

Walkthrough

This PR exposes the template-miner §6.8 OpenTelemetry metric set: it adds semconv metric/attribute definitions and constants, implements MinerMetrics with reservoir-backed p50/p01 gauges, wires metrics through the ingest/attach paths (including merge event_type), and replaces three stub tests with running telemetry validations.

Changes

Miner OpenTelemetry Metrics

Layer / File(s) Summary
Semantic convention attributes, metric definitions, and generated constants
semconv/registry/attributes.yaml, semconv/registry/metrics.yaml, crates/ourios-semconv/src/lib.rs, crates/ourios-semconv/tests/exports.rs, crates/ourios-core/src/audit.rs, crates/ourios-core/src/alias.rs, crates/ourios-core/Cargo.toml
Registry adds ourios.service and ourios.miner.template_change attributes and defines the ourios.miner.* metric set. Generated ourios-semconv constants expose those names. TemplateChange::event_type() maps variants to canonical EVENT_TYPE strings. Alias module switches to semconv metric/attribute constants.
Metrics instrumentation module with reservoir and observable gauges
crates/ourios-miner/Cargo.toml, crates/ourios-miner/src/lib.rs, crates/ourios-miner/src/metrics.rs
Adds opentelemetry API and ourios-semconv deps. MinerMetrics registers counters/histograms and observable gauges under ourios.miner, maintains shared MinerMetricsState behind a mutex, and implements a bounded per-(tenant,service) Reservoir with exact nearest-rank quantiles for p50/p01. Hot-path APIs accept service: Option<&str> and record metrics and state in lockstep.
Cluster integration: MinerMetrics wiring through ingest pipeline
crates/ourios-miner/src/cluster.rs
MinerCluster holds MinerMetrics. Ingest resolves service once per record (via service_of), threads service into emit_record, overflow/parse/tokenizer helpers, and attach_and_maybe_widen; merge emission derives event_type from TemplateChange and calls metrics.record_merge(...). Ingest records duration and updates template-count observable after mining.
Test implementation and RFC documentation
crates/ourios-miner/tests/invariants.rs, crates/ourios-miner/tests/rfc_internal.rs, crates/ourios-miner/tests/hazards.rs, docs/rfcs/0001-template-miner.md
Three previously-stubbed tests were implemented: mandatory metric set exposure, confidence p50/p01 gauge validation against nearest-rank, and per-service params-overflow-utilization alert (H2.2). RFC updated to dotted ourios.miner.* metric names and attribute keys and documents the collect-on-read behavior and quantile design decision.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • jensholdgaard/ourios#46: Adds TemplateChange::event_type() mappings which align with this PR’s merge-event observability changes.
  • jensholdgaard/ourios#105: Introduces/generated ourios-semconv constants crate used by this PR’s instrumentation and tests.

🐰 Metrics now hum where logs once lay,
Reservoirs count each confidence array.
P50, P01 — the gauges sing true,
Overflow alarms for the noisy crew.
From RFC to tests, the miner hops new.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: adding the ourios.miner.* metric registry and implementing spec-first miner telemetry with references to relevant RFC sections and scenario numbers.
Description check ✅ Passed The description comprehensively covers the spec-first approach, OTel rules applied, registry entries, generated constants, instrumentation details, invariants touched, and verification results. All key sections from the template are addressed with substantial detail.
Linked Issues check ✅ Passed The PR fully addresses the linked issue #159 objectives: implements RFC0001 §6.8 miner telemetry, exposes the mandatory metric set via spec-first registry design with audited OTel naming rules, implements confidence p50/p01 gauges, and converts three red-gate stubs to AAA tests for §3.1.2, RFC0001.8, and H2.2.
Out of Scope Changes check ✅ Passed All changes are directly scoped to implementing miner telemetry per RFC0001 §6.8: registry entries, generated constants, metrics instrumentation, red-gate test implementations, and updated audit.rs with event_type() method. No unrelated changes detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch rfc0001-miner-semconv

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Builds the §6.8 miner telemetry on the weaver-generated ourios_semconv
constants (spec-first; the registry + generated module land in the prior
commit). Retains observable-gauge handles, poison-safe collection-callback
locks, and a pre-sized quantile reservoir. Flips §3.1.2 (mandatory metric
set exposed), RFC0001.8 (confidence p50/p01 gauges), H2.2 (per-service
params-overflow utilization alert).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jensholdgaard
jensholdgaard force-pushed the rfc0001-miner-semconv branch from 1dba5ff to b76eca1 Compare June 8, 2026 06:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a spec-first OpenTelemetry semantic-conventions (weaver) registry for the RFC 0001 miner metrics and wires miner instrumentation/tests to consume the generated ourios-semconv constants (dotted ourios.miner.* names + ourios.* attribute keys).

Changes:

  • Define ourios.miner.* metrics and supporting attributes/enums in semconv/registry/*.
  • Regenerate and test ourios-semconv exports for the new metric/attribute keys.
  • Implement miner telemetry (crates/ourios-miner/src/metrics.rs), integrate it into MinerCluster, and flip RFC red-gate stubs to in-memory OTel SDK tests.

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
semconv/registry/metrics.yaml Adds the ourios.miner.* metric definitions (names, kinds, units, attributes).
semconv/registry/attributes.yaml Adds ourios.service and ourios.miner.template_change attribute definitions.
docs/rfcs/0001-template-miner.md Updates RFC text/table to reflect dotted-semconv naming (but still has some older name references elsewhere).
crates/ourios-semconv/tests/exports.rs Extends exports test coverage to lock miner metric/attribute names to the registry.
crates/ourios-semconv/src/lib.rs Adds generated constants for the miner metrics + new attribute keys.
crates/ourios-miner/tests/rfc_internal.rs Implements RFC0001.8 test asserting p50/p01 gauges match in-process quantiles.
crates/ourios-miner/tests/invariants.rs Implements §3.1.2 test asserting mandatory metric set appears at zero traffic.
crates/ourios-miner/tests/hazards.rs Implements H2.2 per-service overflow-utilization alert-threshold test (minor message/name nits noted).
crates/ourios-miner/src/metrics.rs New miner telemetry implementation (counters/histograms + observable gauges + init-seeding).
crates/ourios-miner/src/lib.rs Exposes new internal metrics module.
crates/ourios-miner/src/cluster.rs Integrates metrics into ingest/emit paths; records duration, confidence, overflow, parse failures, etc.
crates/ourios-miner/Cargo.toml Adds opentelemetry + ourios-semconv; adds test-only ourios-telemetry, tokio, opentelemetry_sdk.
crates/ourios-core/src/audit.rs Adds TemplateChange::event_type() for ourios.miner.template_change attribute emission.
Cargo.lock Locks new dependency graph for added OTel + test crates.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/ourios-miner/tests/hazards.rs Outdated
Comment thread crates/ourios-miner/tests/hazards.rs Outdated
Comment thread crates/ourios-miner/tests/hazards.rs Outdated
Comment thread crates/ourios-miner/src/cluster.rs Outdated
Comment thread crates/ourios-miner/src/cluster.rs Outdated
Comment thread crates/ourios-miner/src/cluster.rs Outdated
Comment thread crates/ourios-miner/src/metrics.rs Outdated
Comment thread crates/ourios-miner/src/metrics.rs Outdated
Comment thread docs/rfcs/0001-template-miner.md

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@docs/rfcs/0001-template-miner.md`:
- Around line 1850-1864: Rename the two metrics to follow the dotted-semconv:
change alias_assertions_total to ourios.miner.alias.assertions and
alias_retractions_total to ourios.miner.alias.retractions (remove the _total
suffix and add the ourios.miner alias namespace) in the table and update any
cross-references (e.g., H5, §6.7) or documentation mentions that refer to the
old names so they match the new metric identifiers.
🪄 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: e7126aec-61e1-431e-b649-9d041f37a705

📥 Commits

Reviewing files that changed from the base of the PR and between 5b336ba and 1dba5ff.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (13)
  • crates/ourios-core/src/audit.rs
  • crates/ourios-miner/Cargo.toml
  • crates/ourios-miner/src/cluster.rs
  • crates/ourios-miner/src/lib.rs
  • crates/ourios-miner/src/metrics.rs
  • crates/ourios-miner/tests/hazards.rs
  • crates/ourios-miner/tests/invariants.rs
  • crates/ourios-miner/tests/rfc_internal.rs
  • crates/ourios-semconv/src/lib.rs
  • crates/ourios-semconv/tests/exports.rs
  • docs/rfcs/0001-template-miner.md
  • semconv/registry/attributes.yaml
  • semconv/registry/metrics.yaml

Comment thread docs/rfcs/0001-template-miner.md
jensholdgaard and others added 3 commits June 8, 2026 09:36
…names

Reconcile the pre-semconv flat metric/attribute names lingering in
doc-comments, test panic/expect messages, and RFC 0001 §5 scenario
text with the landed dotted-`ourios.miner.*` registry scheme:

- metrics.rs: `parse_failures_total` / the `latency` histogram
  sentinel / `template_count` gauge prose → `ourios.miner.parse_failures`
  / `ourios.miner.duration` / `ourios.miner.template.count`.
- tests/hazards.rs: H2.2 doc comment + the three gauge-read
  panic/expect messages → `ourios.miner.params.overflow.utilization`
  with `ourios.tenant` / `ourios.service` attributes.
- RFC 0001 §5/§6.3/§6.4/§6.5/§6.7/§6.9: every remaining flat name
  (`template_count`, `merges_total`, `params_overflow_ratio`,
  `parse_failures_total`, `miner_latency_seconds`, the `confidence_p*`
  views, bare `tenant_id`/`service` attributes) reconciled to the
  dotted §6.8 table so the RFC is internally consistent with the
  generated `ourios-semconv` constants.

Comment/doc/message only — no metric or attribute name, registry, or
generated constant changed (weaver registry check stays clean).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`service_of(&record.resource_attributes)` scans the resource
attributes and allocates a fresh `String` every call. It was invoked
from each hot-path helper (overflow retention, parse-failure,
tokenizer-failure, emit), so a single ingested line re-scanned and
re-allocated the service identity up to four times.

Resolve it once at the top of `ingest` and thread it as `&str`
through `ingest_string` / `ingest_structured` / `attach_and_maybe_widen`
and the four helpers. Behaviour is identical — the same per-(tenant,
service) instruments are driven with the same value — but the ingest
hot path (pillar #2) now does one scan + one alloc per line.

Also aligns the cluster.rs telemetry doc-comments touched by these
helpers to the dotted `ourios.miner.*` names.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`alias_assertions_total` / `alias_retractions_total` were the last two
rows in the §6.8 table still carrying the Prometheus `_total` suffix
and missing the `ourios.miner.` prefix, contradicting the amendment's
own naming rule. Rename to `ourios.miner.alias.assertions` /
`ourios.miner.alias.retractions` (and the §6.7 prose mention). These
are future §6.7 metrics not yet in the registry, so this is a doc-only
consistency fix.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated 7 comments.

Comment thread crates/ourios-miner/src/metrics.rs Outdated
Comment thread crates/ourios-miner/src/metrics.rs Outdated
Comment thread crates/ourios-miner/src/metrics.rs Outdated
Comment thread crates/ourios-miner/src/metrics.rs Outdated
Comment thread crates/ourios-miner/src/metrics.rs Outdated
Comment thread docs/rfcs/0001-template-miner.md
Comment thread docs/rfcs/0001-template-miner.md
jensholdgaard and others added 4 commits June 8, 2026 09:49
Add metric.ourios.miner.alias.assertions and
metric.ourios.miner.alias.retractions to the weaver registry, both
counters carrying the required ourios.tenant attribute, matching the
RFC 0001 §6.8 dotted names. Regenerate ourios-semconv.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…* constants

Replace the flat alias_assertions_total / alias_retractions_total metric
names and the flat tenant_id attribute key with the generated
ourios_semconv constants (OURIOS_MINER_ALIAS_ASSERTIONS /
OURIOS_MINER_ALIAS_RETRACTIONS / OURIOS_TENANT), so the alias counters
match the dotted RFC 0001 §6.8 names and are registry-backed. Add
ourios-semconv as a dependency of ourios-core. Names only; the
increments and tenant attribute value are unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review pass: the metrics-state write paths (record_line/overflow/
body_retention/set_template_count) now recover from a poisoned mutex like
the collection callbacks (telemetry stays best-effort, never crashes the
ingest hot path). The merges counter is init-seeded without the
template_change attribute — that enum has no sentinel member, so a value
appears only on a real merge (no out-of-contract time series).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/rfcs/0001-template-miner.md (1)

1839-1842: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Mandatory zero-traffic guarantee conflicts with alias-counter behavior.

Line 1839 says every mandatory instrument is init-seeded for zero-traffic visibility, and Line 1851 includes alias counters in that mandatory set. But crates/ourios-core/src/alias.rs (Line 205-209) explicitly documents those alias counters are not zero-seeded. Please align the contract (either seed alias counters or scope them out of the zero-traffic mandatory guarantee).

Also applies to: 1851-1861

🤖 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 `@docs/rfcs/0001-template-miner.md` around lines 1839 - 1842, Mismatch between
the RFC's "init-seeded mandatory instruments" claim and alias.rs's doc that
alias counters are not zero-seeded; reconcile by either (A) updating the RFC
text around the "full mandatory set is exposed" / "init-seeded" language (and
the table rows around the alias counters at the 1851–1861 section) to explicitly
exclude alias counters from the zero-traffic mandatory guarantee, or (B)
changing the alias implementation in alias.rs to perform initial seeding of
alias counters at init (i.e., hook them into the same init-seed emission path
used for other mandatory instruments) and update alias.rs documentation
accordingly so both the RFC and alias.rs consistently state the same behavior.
Ensure references to "alias counters" and "init-seeded/zero-seeded" are updated
in both places.
🤖 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.

Outside diff comments:
In `@docs/rfcs/0001-template-miner.md`:
- Around line 1839-1842: Mismatch between the RFC's "init-seeded mandatory
instruments" claim and alias.rs's doc that alias counters are not zero-seeded;
reconcile by either (A) updating the RFC text around the "full mandatory set is
exposed" / "init-seeded" language (and the table rows around the alias counters
at the 1851–1861 section) to explicitly exclude alias counters from the
zero-traffic mandatory guarantee, or (B) changing the alias implementation in
alias.rs to perform initial seeding of alias counters at init (i.e., hook them
into the same init-seed emission path used for other mandatory instruments) and
update alias.rs documentation accordingly so both the RFC and alias.rs
consistently state the same behavior. Ensure references to "alias counters" and
"init-seeded/zero-seeded" are updated in both places.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0f3d558e-3151-41ed-80ca-b957b2129543

📥 Commits

Reviewing files that changed from the base of the PR and between 1dba5ff and 229d971.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (13)
  • crates/ourios-core/Cargo.toml
  • crates/ourios-core/src/alias.rs
  • crates/ourios-core/src/audit.rs
  • crates/ourios-miner/Cargo.toml
  • crates/ourios-miner/src/cluster.rs
  • crates/ourios-miner/src/lib.rs
  • crates/ourios-miner/src/metrics.rs
  • crates/ourios-miner/tests/hazards.rs
  • crates/ourios-miner/tests/invariants.rs
  • crates/ourios-miner/tests/rfc_internal.rs
  • crates/ourios-semconv/src/lib.rs
  • docs/rfcs/0001-template-miner.md
  • semconv/registry/metrics.yaml
✅ Files skipped from review due to trivial changes (2)
  • crates/ourios-miner/src/lib.rs
  • crates/ourios-semconv/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (8)
  • crates/ourios-miner/Cargo.toml
  • semconv/registry/metrics.yaml
  • crates/ourios-miner/tests/invariants.rs
  • crates/ourios-miner/src/metrics.rs
  • crates/ourios-core/src/audit.rs
  • crates/ourios-miner/tests/hazards.rs
  • crates/ourios-miner/tests/rfc_internal.rs
  • crates/ourios-miner/src/cluster.rs

jensholdgaard and others added 2 commits June 8, 2026 10:05
Per review: replace the eight .unwrap_or_else(PoisonError::into_inner)
call sites with a single lock_state() helper that matches on the
LockResult (Ok guard / Err -> into_inner). Same poison-recovery
behavior, expressed as an explicit match and DRYed to one place.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace partial_cmp(...).expect("never NaN") with f64::total_cmp — a total
order with no panic on a telemetry hot path (no .expect() in non-test
code).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.

Comment thread crates/ourios-miner/src/metrics.rs Outdated
Comment thread crates/ourios-miner/src/metrics.rs Outdated
Comment thread crates/ourios-miner/src/metrics.rs Outdated
Comment thread crates/ourios-miner/src/metrics.rs Outdated
…nknown

Drop the INIT_SENTINEL "__init__" attribute value: init-seeds are now
attribute-less zero points, so the mandatory §6.8 set still surfaces at
zero traffic (§3.1.2) without a sentinel that could collide with a real
(unvalidated) tenant/service or violate the required template_change enum.
Real series carry the registry-required attributes on real traffic.
service_of now folds an empty service.name into SERVICE_UNKNOWN. Reservoir
doc updated (dotted-semconv landed; the open fork is backend-derived
quantiles, naming-independent).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 6 comments.

Comment thread crates/ourios-miner/src/metrics.rs Outdated
Comment thread crates/ourios-miner/src/metrics.rs Outdated
Comment thread crates/ourios-miner/src/cluster.rs
Comment thread crates/ourios-miner/src/cluster.rs
Comment thread crates/ourios-miner/tests/rfc_internal.rs Outdated
Comment thread crates/ourios-miner/tests/rfc_internal.rs Outdated
jensholdgaard and others added 2 commits June 8, 2026 13:29
…ion; cut per-line alloc

service_of now returns Option<String> (None when service.name is absent
or empty); ourios.service is recommended, so a service-less line is
attributed to ourios.tenant alone rather than synthesized to "unknown".
Drops the SERVICE_UNKNOWN sentinel. The per-tenant state splits named
services (HashMap<String, _>, borrowed &str probe) from a dedicated
no_service slot, so the common ingest path clones a key only on first
sight of a (tenant, service) pair instead of every line. record_latency
-> record_duration for grep-consistency with the histogram it records.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nconditionally

The module doc said "with no provider installed every record/add is a
cheap no-op", implying the hot path is free when telemetry is disabled.
The observable-gauge state updates (mutex + tally maps) run on every
ingest regardless of the installed provider; document that this cost is
intentional (in-process derived gauges) but bounded (clone-on-first-sight
key, capped reservoir).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

docs/rfcs/0001-template-miner.md:1892

  • The RFC text here says p50/p01 are recomputed on a background ticker and cached so export never blocks, and implies quantiles are evaluated over the histogram. The current implementation computes quantiles on collection from a bounded in-process reservoir (no ticker/caching). Please align the RFC with the implemented mechanism (or implement the ticker/caching described here).
named views derived from it in-process. The miner recomputes them
on a short ticker (default 10 s, configurable; the cost is one
quantile evaluation over the histogram per tenant per service per
tick — negligible relative to the hot path) and caches the value
between ticks so a metric export cycle never blocks on

Comment thread crates/ourios-miner/src/metrics.rs
jensholdgaard and others added 2 commits June 8, 2026 13:45
The quantile gauge callback held the metrics Mutex while sorting each
per-(tenant,service) reservoir (O(n log n) inside the tenant×service
loop), stalling the ingest hot path during collection. Now it snapshots
the sample windows under the lock (cheap O(n) copy) and sorts + computes
the nearest-rank quantile via a free quantile_of() AFTER releasing the
lock. Reservoir::quantile (now redundant) removed; tests use
quantile_of(snapshot).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up to the off-lock quantile move: the test module references the
free quantile_of via super:: (it was unqualified, breaking the lib-test
build).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.

Comment thread docs/rfcs/0001-template-miner.md
…ttribute

Consistency with the dotted-semconv migration (the rest of the RFC +
registry use ourios.tenant; wal_replay_progress is a future §6.9 WAL
metric, documented not yet implemented).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.

Comment thread crates/ourios-miner/src/cluster.rs Outdated
Comment thread semconv/registry/metrics.yaml Outdated
The cluster "Not yet emitted" list was stale — three-zone confidence,
type-expansion, reconstruction, overflow, and OTel exposition are all
implemented; it now lists only the genuinely-pending items (Parquet
records, WAL-backed sink, snapshot/recovery). The template.count registry
brief now says tree leaves + structured-body templates, matching the
exported count.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.

Comment thread crates/ourios-miner/src/metrics.rs
Comment thread crates/ourios-miner/src/metrics.rs
Comment thread crates/ourios-miner/src/metrics.rs
…gauges

body_lines / body_retentions / template_counts updated via get_mut +
insert-on-miss (bump_tenant / set_tenant helpers) instead of
entry(tenant.clone()) / insert(tenant.clone()) — no per-line TenantId
alloc on the ingest hot path for an already-seen tenant, matching the
by_service path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

docs/rfcs/0001-template-miner.md:1125

  • The RFC pseudocode note says empty/whitespace-only input is "not a parse failure", but the current miner implementation treats masked_strs.is_empty() as a parse failure (emits a lossy record, increments parse failures, and returns NO_TEMPLATE). This should be reconciled so the spec matches the shipped behavior and the telemetry/tests (which use empty input to exercise ourios.miner.parse_failures).
        # Note: an empty-after-whitespace string (the AnyValue
        # carries `""` or only whitespace) is not a parse failure
        # — it has zero tokens and the miner short-circuits with
        # the cluster's `NO_TEMPLATE` sentinel rather than
        # descending the tree. The pre-amendment cluster code

Comment thread crates/ourios-miner/src/metrics.rs Outdated
Comment thread crates/ourios-miner/src/metrics.rs Outdated
… split structure

Doc-only: the ServiceTally comment now names the dotted
ourios.miner.confidence.p50/.p01 gauges, and the by_service field doc
describes the ServiceTallies { by_name, no_service } split rather than a
stale inner Option<String> key.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants