feat(core): configurable head-based sampling with always-sample-on-error - #907
feat(core): configurable head-based sampling with always-sample-on-error#907Ladas wants to merge 3 commits into
Conversation
|
Unsigned commits: 02d9688. Please sign your commits. |
Replace the fmt-only tracing subscriber with a layered tracing_subscriber::Registry combining: - fmt layer (always) — stdout text or JSON logging - OTLP layer (opt-in) — span export to OTel Collector via gRPC The OTLP layer is behind the `otel` cargo feature flag to keep the default binary lean. When compiled with `--features otel` and an endpoint is configured (via `telemetry.otlp_endpoint` in config or `OTEL_EXPORTER_OTLP_ENDPOINT` env var), spans are exported to any OTLP-compatible backend (OTel Collector, MLflow, Tempo, etc.). Changes: - Add opentelemetry 0.32, opentelemetry_sdk 0.32, opentelemetry-otlp 0.32, tracing-opentelemetry 0.33 as workspace dependencies - Add `registry` feature to tracing-subscriber - Add `otel` feature to core and server crates - New TelemetryConfig with otlp_endpoint field (deny_unknown_fields) - Refactor init_tracing to return TracingGuard (RAII shutdown) - Add example config and feature-gated integration test Closes praxis-proxy#315 Signed-off-by: Ladislav Smola <lsmola@redhat.com>
Resolve the OTLP endpoint once during init_tracing() via TelemetryConfig::resolve(), which returns a new TelemetryConfig with env var merged in. The resolved config is passed to build_otel_provider so downstream code never reads global state directly. Tests use TelemetryConfig::resolved() — a test-only constructor that takes explicit config and env values as parameters, eliminating unsafe set_var/remove_var calls and parallel test races. Addresses review feedback: - alexsnaps: env vars should be snapshotted at startup, not accessed over time as global mutable state - alexsnaps: unsafe set_var/remove_var in tests cannot guarantee thread safety Signed-off-by: Ladislav Smola <lsmola@redhat.com>
Add sampling_rate field to TelemetryConfig (0.0 to 1.0, default None = AlwaysOn for backward compat). When set, configures a ParentBased sampler wrapping TraceIdRatioBased on the SdkTracerProvider. - Validate rate at config load time (reject < 0.0 or > 1.0) - ParentBased respects incoming traceparent sampling decisions - Add 15 tests covering parsing, validation, boundary values, resolve - TODO: always-sample-on-error requires praxis-proxy#301 (root span) for error detection at span close time Closes praxis-proxy#311 Signed-off-by: Ladislav Smola <lsmola@redhat.com>
02d9688 to
3af7727
Compare
praxis-bot
left a comment
praxis-bot
left a comment
There was a problem hiding this comment.
Review Summary
Solid PR adding configurable head-based sampling with proper feature gating, clean TracingGuard RAII shutdown, and thorough test coverage. The architecture (optional otel feature behind praxis-core, propagated through server) is well-designed and backward-compatible.
Three medium findings around validation edge cases and test fidelity.
| Severity | Count |
|---|---|
| Critical | 0 |
| Large | 0 |
| Medium | 3 |
Reviewed by praxis-bot
| )); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
[Medium] validate() checks sampling_rate but does not reject empty or whitespace-only otlp_endpoint. Setting otlp_endpoint: "" in config (or exporting OTEL_EXPORTER_OTLP_ENDPOINT="") passes validation, survives resolve(), and reaches build_otel_provider which will attempt to build a tonic exporter with an empty URL -- failing at runtime rather than at config load.
Add empty-endpoint validation before the sampling_rate check:
if let Some(ep) = &self.otlp_endpoint
&& ep.trim().is_empty()
{
return Err(
"telemetry.otlp_endpoint must not be empty".to_owned(),
);
}| Self { | ||
| otlp_endpoint: config_endpoint.or(env_endpoint).map(ToOwned::to_owned), | ||
| sampling_rate: None, | ||
| } |
There was a problem hiding this comment.
[Medium] The resolved() test helper manually reimplements the merge logic from resolve() (config_endpoint.or(env_endpoint)). The three tests that use it (resolve_prefers_config_over_env, resolve_falls_back_to_env, resolve_none_when_both_unset) verify the helper's behavior, not the actual resolve() method. If resolve() is refactored, these tests would still pass.
The config-takes-precedence path is testable without env var mutation: construct a TelemetryConfig with otlp_endpoint: Some(...) and call .resolve() -- the config value is returned regardless of env. For the env fallback, consider temp_env or a thin indirection. At minimum, rename the tests to clarify they test the helper, not resolve().
|
|
||
| #[test] | ||
| fn validate_sampling_rate_above_one_rejected() { | ||
| let config = TelemetryConfig { |
There was a problem hiding this comment.
[Medium] YAML supports .nan, .inf, and -.inf as float literals, and serde_yaml will parse them into Option<f64>. The current !(0.0..=1.0).contains(&rate) check happens to reject all three (NaN comparisons are always false, infinity is outside the range), but there are no tests confirming this. Add explicit test cases to lock in the behavior:
#[test]
fn validate_sampling_rate_nan_rejected() {
let config = TelemetryConfig {
sampling_rate: Some(f64::NAN),
..Default::default()
};
assert!(
config.validate().is_err(),
"NaN sampling_rate should be rejected",
);
}
#[test]
fn validate_sampling_rate_infinity_rejected() {
let config = TelemetryConfig {
sampling_rate: Some(f64::INFINITY),
..Default::default()
};
assert!(
config.validate().is_err(),
"infinite sampling_rate should be rejected",
);
}2 participants
What
Add sampling_rate field to TelemetryConfig (0.0 to 1.0). When set, configures a ParentBased sampler wrapping TraceIdRatioBased on the SdkTracerProvider.
Why
Production deployments need to control trace volume. AlwaysOn sampling generates too much data; configurable rates (1-10%) with always-on-error preserve visibility while managing cost.
How
sampling_rate: Option<f64>on TelemetryConfigDepends on #315 (PR #836)
Closes #311