feat(telemetry): dogfood our own logs over OTLP (tracing → OTel Logs signal) - #334
Merged
Conversation
…signal)
Workstream D (CLAUDE.md §6.3): Ourios's own logs now ship as the OTel Logs
signal over OTLP — the same protocol its users' logs arrive on — instead of
bare eprintln! to stderr.
ourios-telemetry: `init` also builds an OTLP `SdkLoggerProvider` (batch
exporter — the simple/synchronous processor deadlocks inside tonic request
contexts) and installs the tracing subscriber stack:
- an `OpenTelemetryTracingBridge` layer (the OTel-recommended Rust bridge;
Rust has no end-user OTel logging API) carrying a loop-guard filter that
mutes the export stack's own crates (tonic/hyper/h2/tower/opentelemetry*),
per the OTel self-observability guidelines — exporter-internal events must
not re-enter the exporter (telemetry-induced-telemetry);
- a `fmt` layer keeping a human-readable copy on **stderr**, RUST_LOG-filtered
(default info). stdout stays reserved for the machine-parsed start-up lines —
three integration suites read the bound-port announcements from it.
The guard now owns and flushes both providers; `try_init` tolerates an
already-installed subscriber (test harnesses). Unit test drives a tracing
event across the bridge into an in-memory log exporter (scoped subscriber, no
globals).
ourios-server: every eprintln!/stopgap site in main.rs + receiver.rs is now
tracing::error!/warn!/info! — except the post-telemetry-shutdown report, which
stays on stderr by necessity (the pipeline a tracing event would need is the
one being torn down). The "structured logging is a follow-up" notes are gone.
Verified end-to-end with a live self-ingestion smoke: a second ourios-server
process with OTEL_EXPORTER_OTLP_ENDPOINT pointed at a first instance's OTLP
receiver emitted its startup info event; the first instance ingested it
(WAL → Parquet on drain) and its querier returned it — 1 row, the exact line
("compaction disabled for this process …"), reconstruction=faithful. Ourios
ingested and served its own log.
Also: cargo test -p ourios-server -p ourios-telemetry --all-features (all
green; no test parsed the converted stderr text), clippy pedantic, fmt, and a
happy-path boot proving stdout carries only the port lines.
OTEL_* stays the SDK's env surface (RFC 0020 §3.8) — no new Ourios config.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 44 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 (1)
✨ 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 |
Owner
Author
|
Additional smoke (maintainer-requested): single-instance self-ingestion against the local receiver — one process with
{ "rows": 1,
"lines": ["compaction disabled for this process (OURIOS_COMPACTION_ENABLED)"],
"reconstruction": ["faithful"] }The §6.3 clause — "every hot path emits structured logs (via Ourios, when we bootstrap)" — now holds in its literal, single-process form. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates Ourios’s telemetry bootstrap to dogfood the OpenTelemetry Logs signal over OTLP by installing a tracing subscriber stack that bridges tracing events into OTel log records, while preserving the existing “stdout is machine-parsed” contract for ourios-server.
Changes:
- Add an OTLP
SdkLoggerProvider+OpenTelemetryTracingBridgealongside the existing OTLPSdkMeterProviderinourios-telemetry::init, and ensure shutdown flushes both pipelines. - Replace
eprintln!/println!stopgaps inourios-serverwithtracing::{info,warn,error}!(keeping stderr only for the post-telemetry shutdown report). - Add new dependencies (
tracing,tracing-subscriber,opentelemetry-appender-tracing) and updateCargo.lock.
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-telemetry/src/lib.rs | Build/install OTLP logger provider + tracing bridge + stderr fmt layer; extend guard to own/shutdown logs + metrics; add bridge unit test. |
| crates/ourios-telemetry/Cargo.toml | Enable OTel SDK/logs + OTLP/logs, add tracing-subscriber and opentelemetry-appender-tracing. |
| crates/ourios-server/src/receiver.rs | Convert stderr warnings to tracing::warn! for recovery/flush-related warnings. |
| crates/ourios-server/src/main.rs | Convert startup/shutdown and compaction notices from println/eprintln to tracing (except final telemetry shutdown error). |
| crates/ourios-server/Cargo.toml | Add tracing as the structured logging frontend dependency. |
| Cargo.lock | Lockfile updates for newly introduced telemetry/logging dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot's review of #334: - The loop-guard filter's `opentelemetry-otlp=off` directive was invalid — tracing targets are module paths, so the crate's target is `opentelemetry_otlp`. With the directive ignored, that crate's internal events were NOT muted from the bridge, leaving a telemetry-induced-telemetry window during OTLP export failures. Fixed to `opentelemetry_otlp=off`. - `TelemetryError::Exporter`'s Display said "metric exporter" but the variant now also covers the log exporter — reworded to "an OTLP exporter" (variant doc aligned). - `init`'s doc claimed a second call "logs a no-op"; nothing is emitted — reworded to "silently keeps the first subscriber". Verified: cargo test -p ourios-telemetry --all-features, clippy pedantic, fmt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot follow-up on #334: the enum-level doc still said "metrics pipeline" and the Shutdown variant doc + Display still named only the meter provider, though both now also cover the logger provider. Reworded to signal-agnostic ("the telemetry pipelines (metrics and logs)"; "a provider (meter or logger)"; "shutting down a telemetry provider failed"). Flush stays meter-specific — `force_flush` only flushes the meter provider, so its text is accurate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard
added a commit
that referenced
this pull request
Jul 2, 2026
…e logger on lost try_init Copilot follow-up on #334, both parts accepted: - The bridge layer was hard-coded to `info`, so `RUST_LOG` shaped only the stderr copy — exported volume couldn't be turned down (`warn`) or up (`debug`). The bridge filter now starts from `RUST_LOG` (default `info`) with the loop-guard `off` directives appended on top, so the guard always wins regardless of what `RUST_LOG` requests for those crates. - When `try_init` loses to an already-installed subscriber, the bridge was never wired but the `SdkLoggerProvider`'s batch pipeline stayed alive for the process lifetime. It is now shut down and the guard carries `logger: None`. Verified: cargo test -p ourios-telemetry --all-features, clippy pedantic, fmt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ger on lost try_init Copilot follow-up on #334, both parts accepted: - The bridge layer was hard-coded to `info`, so RUST_LOG shaped only the stderr copy — exported volume couldn't be turned down (warn) or up (debug). The bridge filter now starts from RUST_LOG (default info) with the loop-guard `off` directives appended on top, so the guard always wins regardless of what RUST_LOG requests for those crates. - When `try_init` loses to an already-installed subscriber, the bridge was never wired but the `SdkLoggerProvider`'s batch pipeline stayed alive for the process lifetime. It is now shut down and the guard carries `logger: None`. Verified: cargo test -p ourios-telemetry --all-features, clippy pedantic, fmt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard
force-pushed
the
dogfood-otlp-logs
branch
from
July 2, 2026 21:26
a904781 to
855895f
Compare
![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
🧹 Nitpick comments (3)
crates/ourios-telemetry/src/lib.rs (2)
237-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
EnvFilter::try_from_default_env()construction.The bridge filter (lines 237-238) and the
fmtlayer filter (line 259) each independently callEnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new("info")). Extracting this into a small helper would remove the duplication.♻️ Proposed refactor
+fn default_env_filter() -> EnvFilter { + EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new("info")) +} + ... - let mut bridge_filter = - EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new("info")); + let mut bridge_filter = default_env_filter(); for directive in [ ... let fmt = tracing_subscriber::fmt::layer() .with_writer(std::io::stderr) - .with_filter(EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new("info"))); + .with_filter(default_env_filter());🤖 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-telemetry/src/lib.rs` around lines 237 - 259, The filter construction is duplicated between the bridge setup and the `fmt` layer in `ourios-telemetry`’s initialization. Extract the repeated `EnvFilter::try_from_default_env().unwrap_or_else(|_| EnvFilter::new("info"))` into a small helper and reuse it for both `OpenTelemetryTracingBridge::new(...).with_filter(...)` and `tracing_subscriber::fmt::layer().with_filter(...)` to keep the behavior identical while removing duplication.
228-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe loop-guard filter itself has no dedicated test.
The new
tracing_events_bridge_to_otel_log_recordstest (lines 365-401) verifies a plain event bridges to an OTel log record, but nothing exercises the loop-guard directives (hyper=off,tonic=off, etc.) that are this PR's core self-observability safety mechanism. Given the maintainer's own emphasis on the guard keeping "amplification at zero," a regression here (e.g., an accidental typo in a directive string, or a future change to directive-precedence semantics) would go undetected. Consider extracting the directive-building loop into a small testable helper and adding a test that asserts an event targeting one of the muted crates (e.g.tracing::info!(target: "hyper", "noise")) does not produce a log record while a normal event still does.Based on path instructions, "Unit tests must be next to the code and are mandatory for anything non-trivial."
🤖 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-telemetry/src/lib.rs` around lines 228 - 253, The bridge filter assembly in the telemetry setup has no test coverage for the loop-guard directives that mute noisy crates. Extract the directive-building logic around the EnvFilter setup and OpenTelemetryTracingBridge::new into a small helper, then add a unit test beside it that verifies muted targets like hyper or tonic do not emit OTEL log records while a normal tracing event still bridges successfully. Ensure the test exercises the exact directive list used by the bridge so typos or precedence regressions are caught.Source: Path instructions
crates/ourios-telemetry/Cargo.toml (1)
48-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExact-pinned dev-dependency vs caret dependency elsewhere.
tracing = "=0.1.44"here is an exact pin, whileourios-server'stracing = "0.1"(Cargo.toml lines 55-58) is a caret range. Since both crates share one workspaceCargo.lock, the exact pin effectively locks the whole workspace'stracingresolution to0.1.44, silently blocking patch bumps until this pin is manually updated. If there's no specific reason to require exactly0.1.44for the bridge test, consider using the same range asourios-server.🤖 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-telemetry/Cargo.toml` around lines 48 - 49, The bridge test dependency in ourios-telemetry is exact-pinned, which can force the workspace to stay on one tracing patch version. Update the tracing dev-dependency in the Cargo.toml entry used by the bridge test to match the caret-style range used elsewhere in the workspace, unless there is a hard requirement for the exact 0.1.44 version. Keep the change scoped to the tracing dependency declaration so the workspace can accept patch upgrades normally.
🤖 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-telemetry/src/lib.rs`:
- Around line 209-226: `init` is installing the meter provider globally too
early, before the fallible log exporter is successfully built. Move
`global::set_meter_provider(provider.clone())` to after
`LogExporter::builder().with_tonic()` and `builder.build()?` succeed, so a
failed log-exporter construction leaves no partially initialized global state.
Keep the change localized in `ourios-telemetry::init`, using the existing
`provider`, `builder`, and `logger` setup flow.
---
Nitpick comments:
In `@crates/ourios-telemetry/Cargo.toml`:
- Around line 48-49: The bridge test dependency in ourios-telemetry is
exact-pinned, which can force the workspace to stay on one tracing patch
version. Update the tracing dev-dependency in the Cargo.toml entry used by the
bridge test to match the caret-style range used elsewhere in the workspace,
unless there is a hard requirement for the exact 0.1.44 version. Keep the change
scoped to the tracing dependency declaration so the workspace can accept patch
upgrades normally.
In `@crates/ourios-telemetry/src/lib.rs`:
- Around line 237-259: The filter construction is duplicated between the bridge
setup and the `fmt` layer in `ourios-telemetry`’s initialization. Extract the
repeated `EnvFilter::try_from_default_env().unwrap_or_else(|_|
EnvFilter::new("info"))` into a small helper and reuse it for both
`OpenTelemetryTracingBridge::new(...).with_filter(...)` and
`tracing_subscriber::fmt::layer().with_filter(...)` to keep the behavior
identical while removing duplication.
- Around line 228-253: The bridge filter assembly in the telemetry setup has no
test coverage for the loop-guard directives that mute noisy crates. Extract the
directive-building logic around the EnvFilter setup and
OpenTelemetryTracingBridge::new into a small helper, then add a unit test beside
it that verifies muted targets like hyper or tonic do not emit OTEL log records
while a normal tracing event still bridges successfully. Ensure the test
exercises the exact directive list used by the bridge so typos or precedence
regressions are caught.
🪄 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: de55623a-c0ee-4966-8cc3-0b33dc799d8c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/ourios-server/Cargo.tomlcrates/ourios-server/src/main.rscrates/ourios-server/src/receiver.rscrates/ourios-telemetry/Cargo.tomlcrates/ourios-telemetry/src/lib.rs
…it steps Copilot + CodeRabbit both flagged it: `global::set_meter_provider` ran before the fallible log-exporter build, so a failed log build returned Err while leaving an installed meter provider (and its periodic-reader task) running with no TelemetryGuard to shut it down. Reorder: build both pipelines first, install globally only after every fallible step has succeeded. Verified: cargo test -p ourios-telemetry --all-features, clippy pedantic, fmt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard
added a commit
that referenced
this pull request
Jul 2, 2026
…CI gate (#335) * feat(semconv): registry-backed log event names + upstream registry dependency Strict weaver-registry usage extended to the Logs signal (maintainer, 2026-07-02: keep the maturity high). A first `weaver registry live-check` of the real binary showed every Ourios metric already registry-backed (seen_non_registry_metrics = {}) and exactly two violation classes; this closes both: - The SDK resource attributes (service.name, telemetry.sdk.*) are OTel's own — the fix is declaring the upstream semantic-conventions registry (v1.42.0) as a dependency in semconv/registry/manifest.yaml, not defining them ourselves. Codegen is unaffected (no-diff holds: only referenced groups generate). - The #334 logs bridge emitted tracing's default event names (`event <file>:<line>`). Every log call site (10, across main.rs + receiver.rs) now carries an explicit registry-backed name: a new semconv/registry/events.yaml defines the ourios.* events, the codegen template gains an events section, and the call sites reference the generated `ourios_semconv::EVENT_*` constants directly (tracing's `name:` accepts consts, so the registry stays the single source — no literal duplication). Verified: weaver registry check --future; regen no-diff; cargo test/clippy/fmt on ourios-server + ourios-semconv; and a live-check of the running binary now exits 0 with zero violations (only stability=development improvement advice, correct pre-1.0). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: gate emitted telemetry on weaver live-check New required job `live-check (weaver)`: boot the real ourios-server with its OTLP export (metrics + the dogfooded logs) pointed at `weaver registry live-check`'s OTLP listener, drive a query, shutdown-flush, and gate on weaver's exit code — violation-level advice (an unregistered metric/attribute/event name) fails the job; improvement-level advice (stability=development, expected pre-1.0) passes. `--include-unreferenced` resolves the SDK resource attributes from the upstream registry the manifest depends on. The JSON report uploads as an artifact on every run. Complements the `semconv` job: that validates the registry definitions; this validates what the binary actually emits against them. Same pinned weaver (v0.23.0 + sha256). Added to `ci-success`'s needs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: harden the live-check step — poll readiness, propagate the server's exit Copilot's review of #335, both accepted: - The probe query ran after a fixed `sleep 3`, racing server startup on slow runners. It's now a 30x1s poll; never-ready fails the job explicitly. - `wait "$server_pid" || true` masked a non-zero server exit (panic, failed drain). Both statuses are now collected — weaver's first, so the JSON report still prints on violations — and the step passes only if the server exited 0 (graceful SIGTERM path) AND weaver found no violations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: best-effort SIGTERM in the live-check step Copilot follow-up on #335: under `set -e`, `kill -TERM` on a server that already exited aborts the step and skips the report/status handling. The kill is now best-effort; the subsequent wait + server_status check is what enforces the clean exit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * ci: end the live-check session via /stop; inactivity timeout as backstop only Copilot's timing finding on #335 accepted: a 15s inactivity timeout could expire during a slow startup's quiet period (the probe loop alone may run 30s), ending the weaver session before the server's shutdown flush — a false pass. The session now ends deterministically: after the server exits (its flush delivered), the step POSTs weaver's admin /stop (verified locally: HTTP 200, graceful exit, report written); the inactivity timeout is bumped to 120s and serves only as a backstop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 24, 2026
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
Workstream D — logs dogfooding (CLAUDE.md §6.3). Ourios's own logs now ship
as the OTel Logs signal over OTLP — the same protocol its users' logs
arrive on — replacing the
eprintln!stopgaps.ourios-telemetry
initbuilds an OTLPSdkLoggerProvideralongside the existing MeterProvider(batch exporter — the simple/synchronous processor deadlocks inside tonic
request contexts) and installs the
tracingsubscriber stack:OpenTelemetryTracingBridge(the OTel-recommended Rust bridge — Rust hasno end-user OTel logging API; verified via the OpenTelemetry MCP) with a
loop-guard filter muting the export stack's own crates
(
tonic/hyper/h2/tower/opentelemetry*), per the OTelself-observability guidelines: exporter-internal events must not re-enter the
exporter (telemetry-induced-telemetry).
fmtlayer → stderr for humans,RUST_LOG-filtered (defaultinfo).stdout stays reserved for the machine-parsed start-up lines — three
integration suites read the bound-port announcements from it.
The guard owns and flushes both providers;
try_inittolerates analready-installed subscriber. A unit test drives a
tracingevent across thebridge into an in-memory log exporter (scoped subscriber — no global state).
ourios-server
All
eprintln!sites inmain.rs+receiver.rs→tracing::error!/warn!/info!, except the post-telemetry-shutdown report, which stays on stderr bynecessity (the pipeline a tracing event would need is the one being torn down).
The proof — Ourios ingested its own log
Live self-ingestion smoke: instance B (compactor role) with
OTEL_EXPORTER_OTLP_ENDPOINTpointed at instance A's OTLP receiver emittedits startup
info!event → A ingested it (WAL → Parquet on drain) → A'squerier returned it:
{ "rows": 1, "first": { "kind": "rendered", "line": "compaction disabled for this process (OURIOS_COMPACTION_ENABLED)", "reconstruction": "faithful" } }Invariants / notes
OTEL_*remains the SDK's envsurface (RFC 0020 §3.8) — no new Ourios config keys.
the port lines; the
compaction disablednotice moved from stdout totracing (nothing parses it — checked).
tracing-subscriber+opentelemetry-appender-tracing(bothMIT/Apache ecosystems;
tracingitself was already in the tree). cargo-denygreen.
Verification
cargo test -p ourios-server -p ourios-telemetry --all-features✅ (no testasserted on the converted stderr text — checked)
cargo clippy --all-targets --all-features -- -D warnings,cargo fmt✅🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes