feat(telemetry)!: configure traces via universal OTel env vars (RFC 0038 slice 4 → green) - #619
Conversation
…038 slice 4 → green) Slice 4 leans traces entirely on the standard OpenTelemetry SDK env vars instead of a bespoke Ourios config surface — a deletion, not an addition: - Drop TelemetryConfig.trace_sample_ratio and the forced .with_sampler() in init(); the SDK resolves the sampler from OTEL_TRACES_SAMPLER / OTEL_TRACES_SAMPLER_ARG (default parentbased_always_on). Invalid values are logged + ignored by the SDK per spec — no bespoke validation or precedence. - Disable is the standard per-signal OTEL_TRACES_EXPORTER=none, mapped in the server to traces_enabled=false (the one programmatic flag kept). This completes RFC 0038: all six §5 criteria pass, so the RFC flips to green. §3.4 amended to the universal-env-var approach (maintainer decision). Metrics/logs carry the same coupling (export_interval vs OTEL_METRIC_EXPORT_INTERVAL; no OTEL_*_EXPORTER=none) — tracked as a follow-up in #618. BREAKING CHANGE: TelemetryConfig.trace_sample_ratio is removed. Trace sampling is now configured with the standard OTEL_TRACES_SAMPLER / OTEL_TRACES_SAMPLER_ARG environment variables, resolved by the OTel SDK. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
|
Warning Review limit reached
Next review available in: 47 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)
📝 WalkthroughWalkthroughTelemetry sampling is delegated to standard OTel environment variables. Server startup disables the traces pipeline only for ChangesOTel trace configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
This PR completes RFC 0038 slice 4 by removing the bespoke trace-sampler configuration surface and delegating trace sampling entirely to the OpenTelemetry SDK’s standard environment variables, while mapping the standard OTEL_TRACES_EXPORTER=none switch to disabling trace installation in Ourios. It also updates RFC 0038’s status to green and amends §3.4 accordingly.
Changes:
- Remove
TelemetryConfig.trace_sample_ratioand stop forcing a sampler inourios-telemetryso sampling is resolved viaOTEL_TRACES_SAMPLER/OTEL_TRACES_SAMPLER_ARG. - Add server-side mapping
OTEL_TRACES_EXPORTER=none→TelemetryConfig.traces_enabled=false, with a unit test for the mapping behavior. - Update RFC 0038 documentation to reflect the universal-env-var approach and mark the RFC as
green.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/rfcs/0038-self-tracing.md | Marks RFC 0038 as green and updates §3.4 + RFC0038.4 scenario to use universal OTel env vars. |
| crates/ourios-telemetry/src/lib.rs | Drops the custom sampler knob and stops overriding sampler selection so the SDK can resolve it from env. |
| crates/ourios-server/src/main.rs | Adds OTEL_TRACES_EXPORTER=none → disable mapping (traces_enabled()), wires it into startup, and tests it. |
Comments suppressed due to low confidence (1)
crates/ourios-server/src/main.rs:958
- This assertion message reads like
otlp,consoleis a supported exporter configuration, but Ourios only uses this env var to detect the specialnonedisable value. Consider rewording the message so it doesn’t imply other exporter selectors are honored.
assert!(traces_enabled(Some("otlp")), "otlp → on");
assert!(traces_enabled(Some("otlp,console")), "a real exporter → on");
assert!(!traces_enabled(Some("none")), "none → off");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 294-308: Add a crate-local unit test adjacent to init that sets
OTEL_TRACES_SAMPLER and OTEL_TRACES_SAMPLER_ARG in isolation, initializes
tracing, and verifies the resulting sampler follows those environment settings
rather than a hard-coded value. Use an in-memory exporter or isolated process
and restore environment state to keep the test independent.
- Around line 294-308: Update the shared SDK initialization in
crates/ourios-telemetry/src/lib.rs around the logger and tracer provider
construction to check OTEL_SDK_DISABLED=true first and route both signals
through a common no-op path without constructing exporters or providers; add
tests covering all-signals-disabled behavior. In
crates/ourios-server/src/main.rs lines 813-818, propagate the setting instead of
only mapping OTEL_TRACES_EXPORTER. Update docs/rfcs/0038-self-tracing.md lines
158-180 to preserve the documented all-signals no-op contract.
🪄 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: 14df65d4-8380-48de-9999-c69fdb876e8a
📒 Files selected for processing (3)
crates/ourios-server/src/main.rscrates/ourios-telemetry/src/lib.rsdocs/rfcs/0038-self-tracing.md
…t a selector Per review: the helper/test docs implied `OTEL_TRACES_EXPORTER` behaves like a full exporter selector. Ourios only honors it as an on/off switch (`none` → off); when traces are on it always exports over OTLP, so `console` (or any non-`none` value) is treated as on, not as a different exporter. Reworded both doc comments and pinned it with a `console` test case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
RFC 0038 slice 4 — traces via universal OTel env vars (RFC 0038 → green)
The last slice. It deletes the bespoke sampler-config coupling and leans traces entirely on the standard OpenTelemetry SDK env vars — the config contract operators already know.
Changes
TelemetryConfig.trace_sample_ratioand the forced.with_sampler()ininit().SdkTracerProvider::builder()seeds its config from an env-readingConfig::default(), so not setting a sampler means the SDK resolvesOTEL_TRACES_SAMPLER/OTEL_TRACES_SAMPLER_ARG(defaultparentbased_always_on). Invalid values are logged + ignored by the SDK per the env-var spec — no bespoke validation or file-vs-env precedence.OTEL_TRACES_EXPORTER=none, mapped in the server (traces_enabled()helper) totraces_enabled=false— the one programmatic flag the library keeps.OTEL_SDK_DISABLEDstill turns off all three signals together.telemetry.traces.*config-file section, no precedence, no hand-rolled validation).Tests (RFC0038.4)
rfc0038_4_otel_traces_exporter_none_disables_traces(server unit):OTEL_TRACES_EXPORTER→ install decision (noneoff; unset /otlp/ other on). Sampler resolution is the SDK's universal, upstream-tested behaviour that Ourios no longer overrides — nothing Ourios-specific left to test there.RFC 0038 is now green
All six §5 criteria pass — RFC0038.1 (#614/#615/#616/#617), .2 (#615), .3 (#617), .4 (this PR), .5/.6 (#614). Status flipped
specified→green.Follow-up
Metrics + logs carry the same coupling (
export_intervalvsOTEL_METRIC_EXPORT_INTERVAL; noOTEL_METRICS_EXPORTER/OTEL_LOGS_EXPORTER=none) — tracked in #618.BREAKING CHANGE:
TelemetryConfig.trace_sample_ratioremoved; sampling is now the standardOTEL_TRACES_SAMPLER/_ARGenv vars.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
OTEL_TRACES_EXPORTER=none, regardless of capitalization or surrounding whitespace.OTEL_TRACES_SAMPLERandOTEL_TRACES_SAMPLER_ARG.Documentation