Skip to content

feat(core): configurable head-based sampling with always-sample-on-error #907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Ladas wants to merge 1 commit into
base: main
Choose a base branch
from
+320 −4
Draft

feat(core): configurable head-based sampling with always-sample-on-error #907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
230 changes: 229 additions & 1 deletion core/src/config/telemetry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const OTLP_ENDPOINT_ENV_VAR: &str = "OTEL_EXPORTER_OTLP_ENDPOINT";
///
/// let telemetry = TelemetryConfig::default();
/// assert!(telemetry.otlp_endpoint.is_none());
/// assert!(telemetry.sampling_rate.is_none());
///
/// let telemetry: TelemetryConfig =
/// serde_yaml::from_str("otlp_endpoint: \"http://localhost:4317\"").unwrap();
Expand All @@ -36,7 +37,7 @@ const OTLP_ENDPOINT_ENV_VAR: &str = "OTEL_EXPORTER_OTLP_ENDPOINT";
/// Some("http://localhost:4317")
/// );
/// ```
#[derive(Debug, Clone, Default, Deserialize, Serialize)]
#[derive(Clone, Debug, Default, Deserialize, Serialize)]
#[serde(default, deny_unknown_fields)]
pub struct TelemetryConfig {
/// OTLP collector endpoint (e.g. `http://localhost:4317`).
Expand All @@ -45,6 +46,19 @@ pub struct TelemetryConfig {
/// feature). Falls back to `OTEL_EXPORTER_OTLP_ENDPOINT` env var
/// if not set in config.
pub otlp_endpoint: Option<String>,

/// Head-based trace sampling rate between `0.0` (drop all) and `1.0`
/// (sample all).
///
/// When set, configures a `ParentBased(TraceIdRatioBased(rate))`
/// sampler: root spans are sampled at the given rate while child
/// spans respect the parent's sampling decision (W3C `traceparent`).
///
/// When `None` (the default), the `OTel` default sampler is used
/// (`ParentBased(AlwaysOn)`), preserving backward compatibility.
// TODO(#311): always-sample-on-error requires #301 (root span) to
// detect errors at span close time and override the sampling decision.
pub sampling_rate: Option<f64>,
}

impl TelemetryConfig {
Expand All @@ -59,14 +73,41 @@ impl TelemetryConfig {
.otlp_endpoint
.clone()
.or_else(|| std::env::var(OTLP_ENDPOINT_ENV_VAR).ok()),
sampling_rate: self.sampling_rate,
}
}

/// Validate telemetry configuration values.
///
/// Returns an error if `otlp_endpoint` is empty/whitespace-only or
/// `sampling_rate` is outside the `0.0..=1.0` range (including NaN/Inf).
pub(crate) fn validate(&self) -> Result<(), String> {
if let Some(endpoint) = &self.otlp_endpoint
&& endpoint.trim().is_empty()
{
return Err("telemetry.otlp_endpoint must not be empty or whitespace-only".to_owned());
}
if let Some(rate) = self.sampling_rate
&& (!rate.is_finite() || !(0.0..=1.0).contains(&rate))
{
return Err(format!(
"telemetry.sampling_rate must be between 0.0 and 1.0, got {rate}"
));
}
Ok(())
}

@praxis-bot praxis-bot Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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(),
    );
}


/// Build from explicit values (for testing without env var mutation).
///
/// This deliberately bypasses [`resolve()`](Self::resolve) to avoid
/// mutating process-wide environment variables in tests. It tests
/// the *merge precedence* logic (config > env) in isolation. The
/// real `resolve()` path is exercised by `resolve_preserves_sampling_rate`.
#[cfg(test)]
fn resolved(config_endpoint: Option<&str>, env_endpoint: Option<&str>) -> Self {
Self {
otlp_endpoint: config_endpoint.or(env_endpoint).map(ToOwned::to_owned),
sampling_rate: None,
}

@praxis-bot praxis-bot Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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().

}
}
Expand Down Expand Up @@ -95,13 +136,26 @@ mod tests {
);
}

#[test]
fn defaults_to_no_sampling_rate() {
let telemetry = TelemetryConfig::default();
assert!(
telemetry.sampling_rate.is_none(),
"sampling_rate should default to None"
);
}

#[test]
fn parse_empty_yields_defaults() {
let telemetry: TelemetryConfig = serde_yaml::from_str("{}").unwrap();
assert!(
telemetry.otlp_endpoint.is_none(),
"empty yaml should default otlp_endpoint to None"
);
assert!(
telemetry.sampling_rate.is_none(),
"empty yaml should default sampling_rate to None"
);
}

#[test]
Expand All @@ -114,6 +168,38 @@ mod tests {
);
}

#[test]
fn parse_explicit_sampling_rate() {
let telemetry: TelemetryConfig = serde_yaml::from_str("sampling_rate: 0.5").unwrap();
assert_eq!(
telemetry.sampling_rate,
Some(0.5),
"explicit sampling_rate should be parsed"
);
}

#[test]
fn parse_sampling_rate_zero() {
let telemetry: TelemetryConfig = serde_yaml::from_str("sampling_rate: 0.0").unwrap();
assert_eq!(telemetry.sampling_rate, Some(0.0), "sampling_rate 0.0 should be parsed");
}

#[test]
fn parse_sampling_rate_one() {
let telemetry: TelemetryConfig = serde_yaml::from_str("sampling_rate: 1.0").unwrap();
assert_eq!(telemetry.sampling_rate, Some(1.0), "sampling_rate 1.0 should be parsed");
}

#[test]
fn parse_sampling_rate_one_percent() {
let telemetry: TelemetryConfig = serde_yaml::from_str("sampling_rate: 0.01").unwrap();
assert_eq!(
telemetry.sampling_rate,
Some(0.01),
"sampling_rate 0.01 (1%) should be parsed"
);
}

#[test]
fn reject_unknown_field() {
let result = serde_yaml::from_str::<TelemetryConfig>("bogus_field: true");
Expand Down Expand Up @@ -156,4 +242,146 @@ mod tests {
"should return None when both are unset"
);
}

#[test]
fn resolve_preserves_sampling_rate() {
let config = TelemetryConfig {
otlp_endpoint: None,
sampling_rate: Some(0.5),
};
let resolved = config.resolve();
assert_eq!(
resolved.sampling_rate,
Some(0.5),
"resolve should preserve sampling_rate"
);
}

// -------------------------------------------------------------------------
// Validation
// -------------------------------------------------------------------------

#[test]
fn validate_none_sampling_rate_ok() {
let config = TelemetryConfig::default();
assert!(config.validate().is_ok(), "None sampling_rate should pass validation");
}

#[test]
fn validate_sampling_rate_zero_ok() {
let config = TelemetryConfig {
sampling_rate: Some(0.0),
..Default::default()
};
assert!(config.validate().is_ok(), "sampling_rate 0.0 should pass validation");
}

#[test]
fn validate_sampling_rate_one_ok() {
let config = TelemetryConfig {
sampling_rate: Some(1.0),
..Default::default()
};
assert!(config.validate().is_ok(), "sampling_rate 1.0 should pass validation");
}

#[test]
fn validate_sampling_rate_mid_range_ok() {
let config = TelemetryConfig {
sampling_rate: Some(0.01),
..Default::default()
};
assert!(config.validate().is_ok(), "sampling_rate 0.01 should pass validation");
}

#[test]
fn validate_sampling_rate_negative_rejected() {
let config = TelemetryConfig {
sampling_rate: Some(-0.1),
..Default::default()
};
let err = config.validate().unwrap_err();
assert!(
err.contains("between 0.0 and 1.0"),
"negative rate should be rejected: {err}"
);
}

#[test]
fn validate_sampling_rate_above_one_rejected() {
let config = TelemetryConfig {

@praxis-bot praxis-bot Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[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",
    );
}

sampling_rate: Some(1.5),
..Default::default()
};
let err = config.validate().unwrap_err();
assert!(
err.contains("between 0.0 and 1.0"),
"rate above 1.0 should be rejected: {err}"
);
}

#[test]
fn validate_sampling_rate_nan_rejected() {
let config = TelemetryConfig {
sampling_rate: Some(f64::NAN),
..Default::default()
};
let err = config.validate().unwrap_err();
assert!(
err.contains("between 0.0 and 1.0"),
"NaN sampling_rate should be rejected: {err}"
);
}

#[test]
fn validate_sampling_rate_infinity_rejected() {
let config = TelemetryConfig {
sampling_rate: Some(f64::INFINITY),
..Default::default()
};
let err = config.validate().unwrap_err();
assert!(
err.contains("between 0.0 and 1.0"),
"Inf sampling_rate should be rejected: {err}"
);
}

#[test]
fn validate_sampling_rate_neg_infinity_rejected() {
let config = TelemetryConfig {
sampling_rate: Some(f64::NEG_INFINITY),
..Default::default()
};
let err = config.validate().unwrap_err();
assert!(
err.contains("between 0.0 and 1.0"),
"-Inf sampling_rate should be rejected: {err}"
);
}

#[test]
fn validate_empty_otlp_endpoint_rejected() {
let config = TelemetryConfig {
otlp_endpoint: Some(String::new()),
..Default::default()
};
let err = config.validate().unwrap_err();
assert!(
err.contains("must not be empty"),
"empty otlp_endpoint should be rejected: {err}"
);
}

#[test]
fn validate_whitespace_otlp_endpoint_rejected() {
let config = TelemetryConfig {
otlp_endpoint: Some(" ".to_owned()),
..Default::default()
};
let err = config.validate().unwrap_err();
assert!(
err.contains("must not be empty"),
"whitespace-only otlp_endpoint should be rejected: {err}"
);
}
}
1 change: 1 addition & 0 deletions core/src/config/validate/rules.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ impl Config {
validate_subrequest_circuit_breaker(self.runtime.subrequest_circuit_breaker.as_ref())?;
validate_global_queue_interval(self.runtime.global_queue_interval)?;
validate_shutdown_timeout(self.shutdown_timeout_secs)?;
self.telemetry.validate().map_err(ProxyError::Config)?;

Ok(())
}
Expand Down
Loading
Loading