feat(telemetry)!: per-signal disable via OTEL_{METRICS,LOGS}_EXPORTER=none (closes #618) - #621
Conversation
…=none (closes #618) Completes #618 — the metrics/logs half of "lean telemetry config into the universal OTel SDK env vars", mirroring the traces disable from RFC 0038 §3.4: - TelemetryConfig gains metrics_enabled + logs_enabled (default on, like traces_enabled). init() skips the meter provider / logger when disabled; TelemetryGuard.provider becomes Option (matching tracer: Option). Metrics off → instruments resolve to the global no-op; logs off → the tracing→OTel bridge is dropped but the stderr fmt layer stays. - The server maps the standard OTEL_METRICS_EXPORTER / OTEL_LOGS_EXPORTER / OTEL_TRACES_EXPORTER (`=none` → off) through one shared signal_enabled() helper — the three signals share the on/off mapping. With #620 (metric interval → OTEL_METRIC_EXPORT_INTERVAL) already merged, all three signals now configure through universal OTel env vars, no bespoke knobs. BREAKING CHANGE: TelemetryConfig gains the metrics_enabled and logs_enabled fields; exhaustive struct-literal constructors must set them. TelemetryConfig::new sets all three signal flags to true, so ::new() callers are unaffected. 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>
📝 WalkthroughWalkthroughThe server now maps standard OpenTelemetry exporter variables to independent signal toggles. Telemetry initialization conditionally creates metrics, logs, and tracing providers, with optional guard ownership, shutdown, flushing, subscriber wiring, and tests updated accordingly. ChangesOpenTelemetry signal controls
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant main
participant signal_enabled
participant init
participant telemetry_providers
main->>signal_enabled: Read per-signal OTEL exporter variables
signal_enabled-->>main: Return enabled or disabled signals
main->>init: Pass TelemetryConfig
init->>telemetry_providers: Create enabled providers
telemetry_providers-->>init: Return optional providers
init-->>main: Install subscriber and return TelemetryGuard
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
Extends Ourios telemetry bootstrap to support per-signal disabling of metrics and logs via standard OpenTelemetry env vars (OTEL_METRICS_EXPORTER=none, OTEL_LOGS_EXPORTER=none), matching the existing traces behavior and keeping Ourios’ telemetry configuration aligned with upstream OTel conventions.
Changes:
- Add
metrics_enabledandlogs_enabledflags toTelemetryConfig(defaulttrue) and make the meter provider optional (TelemetryGuard.provider: Option<SdkMeterProvider>). - Update
ourios_telemetry::init()to skip installing the metrics provider when metrics are disabled and to skip constructing the OTLP logger + tracing→OTel-logs bridge when logs are disabled. - Replace the traces-only env mapping helper with a shared
signal_enabled()that applies the…_EXPORTER=none→ off mapping for traces/metrics/logs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/ourios-telemetry/src/lib.rs | Add per-signal enable flags; make metrics provider optional; conditionally build/install metrics + logs pipelines based on flags. |
| crates/ourios-server/src/main.rs | Map OTEL_{TRACES,METRICS,LOGS}_EXPORTER via shared signal_enabled() into TelemetryConfig flags; update unit test accordingly. |
Comments suppressed due to low confidence (1)
crates/ourios-telemetry/src/lib.rs:303
- The new
logs_enabledbranch is also untested. A focused unit test can catch accidental reintroduction of the OTLP logger/bridge when disabled (e.g., use an invalidotlp_endpointand assertinit()still succeeds whenlogs_enabled=falsebut fails whenlogs_enabled=true).
// guard to shut down. `None` when disabled (`OTEL_LOGS_EXPORTER=none`);
// the stderr `fmt` layer stays, so only the OTel Logs signal is dropped.
let logger: Option<SdkLoggerProvider> = if config.logs_enabled {
let mut builder = LogExporter::builder().with_tonic();
if let Some(endpoint) = &config.otlp_endpoint {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…lines) Per review: the metrics/logs disable path was untested. Add init_with_signals_disabled_installs_no_pipelines — with all three signals off, init() succeeds and the guard holds no meter/logger/tracer provider (each is built inside its `if <signal>_enabled` block, so a regression that installed a disabled pipeline fails the test). 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>
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-telemetry/src/lib.rs`:
- Around line 268-315: Update TelemetryConfig::new and init so
OTEL_SDK_DISABLED=true overrides the individual signal-enabled flags before
building metric, logger, or tracer pipelines. Ensure no OTLP providers are
constructed and all corresponding TelemetryGuard fields are None, while
preserving existing per-signal exporter behavior when the global flag is unset;
add a regression test covering the disabled configuration.
🪄 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: f13b10dd-092e-4575-9987-a757efd72ee4
📒 Files selected for processing (2)
crates/ourios-server/src/main.rscrates/ourios-telemetry/src/lib.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/ourios-telemetry/src/lib.rs:272
- After introducing
metrics_enabled/logs_enabled/traces_enabled,initcan legitimately install only a subset of pipelines (or none). The rustdoc immediately aboveinitstill states it always builds the OTLPMeterProviderand OTLPLoggerProvider, which is now misleading.
pub fn init(config: &TelemetryConfig) -> Result<TelemetryGuard, TelemetryError> {
let resource = resource(&config.service_name);
// Metrics: `None` when disabled (`OTEL_METRICS_EXPORTER=none` → the server
// clears `metrics_enabled`), so instruments resolve to the global no-op and
…ion fields Per review: - init() now honors the standard OTEL_SDK_DISABLED kill-switch: a new otel_sdk_disabled() helper reads the boolean, and each signal's effective enable is `config.<signal>_enabled && !sdk_disabled` computed before any provider is built. `OTEL_SDK_DISABLED=true` forces all three off (no OTLP providers, guard fields all None) — the outcome the disable test asserts. - TelemetryGuard + init() docs no longer imply metrics/logs are always installed: each provider is an Option that may be absent when the signal is disabled. 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>
… mapping
Verified with the OTel MCP that the OTEL_{TRACES,METRICS,LOGS}_EXPORTER
selector is an autoconfigure/autoexport concern (Go's contrib/exporters/
autoexport, Java's sdk-extension-autoconfigure), not the core SDK — and the
Rust SDK has no autoexport, so its manual exporter construction reads none of
those vars. Rather than map them in the server, ourios-telemetry now plays that
autoconfigure role itself:
- init() reads OTEL_{TRACES,METRICS,LOGS}_EXPORTER (+ OTEL_SDK_DISABLED)
directly via exporter_selected()/signal_enabled(); a signal installs only
when its config flag, the global kill-switch, and the per-signal selector all
agree. The config flags stay as programmatic overrides (default on, testable).
- The server drops its signal_enabled mapping — it just calls
init(TelemetryConfig::new(...)). The operator uses only the universal env
vars; there is no bespoke Ourios telemetry config to map.
exporter_selected is unit-tested in ourios-telemetry (the mapping moved out of
the server). RFC 0038 §3.4 updated.
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>
Per-signal disable via the universal
OTEL_*_EXPORTERenv vars (closes #618)The metrics/logs half of #618 — the same "lean into universal OTel SDK env vars" move RFC 0038 slice 4 (#619) made for traces, completing all three signals. With #620 (metric interval →
OTEL_METRIC_EXPORT_INTERVAL) merged, this closes #618.ourios-telemetryis the autoconfigure layerConfirmed with the OTel MCP: the
OTEL_{TRACES,METRICS,LOGS}_EXPORTERselector (incl.none) is an autoconfigure/autoexport responsibility — Go'scontrib/exporters/autoexport, Java'ssdk-extension-autoconfigure— not the core SDK, and Rust has no autoexport crate (grepped everyopentelemetry*crate: zero matches). Since Ourios builds its exporters manually (RFC 0001 §6.8 split),init()now plays that autoconfigure role itself:init()readsOTEL_{TRACES,METRICS,LOGS}_EXPORTER(+OTEL_SDK_DISABLED) directly: a signal installs only when itsconfig.*_enabledflag, the global kill-switch, and the per-signal selector all agree.none→ off; unset /otlp/ any other → on (always OTLP — Ourios doesn't implement the full selector).init(TelemetryConfig::new(...)). Operators use only the universal env vars; no bespoke Ourios telemetry config to map.TelemetryGuard.providerbecomesOption<SdkMeterProvider>(matchingtracer: Option); metrics off → instruments resolve to the global no-op, logs off → thetracing→OTel bridge is dropped but the stderrfmtlayer stays. The*_enabledflags remain as programmatic overrides (default on, testable).Tests
exporter_selected_treats_none_as_off— the pure selector mapping (none→ off; else on).init_with_signals_disabled_installs_no_pipelines— all flags off → guard owns no meter/logger/tracer provider.OTEL_EXPORTER_OTLP_*still tune the transport.BREAKING CHANGE:
TelemetryConfiggainsmetrics_enabled/logs_enabledand drops nothing callers set (::newdefaults all three flags to true, so::new()callers are unaffected); exhaustive struct-literal constructors must set the new fields.🤖 Generated with Claude Code