From 845ad1fa16092780052423ec0654aafcc2407838 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= <anzhengchao@gmail.com>
Date: Wed, 11 Mar 2026 01:32:46 +0800
Subject: [PATCH] fix(obs): avoid panic in telemetry init and clamp sampler
 boundaries (#2118)

Signed-off-by: houseme <housemecn@gmail.com>
Co-authored-by: houseme <housemecn@gmail.com>
---
 crates/obs/src/telemetry/local.rs | 23 +++++++++++++++++-
 crates/obs/src/telemetry/otel.rs  | 40 +++++++++++++++++++++++++++----
 2 files changed, 57 insertions(+), 6 deletions(-)

diff --git a/crates/obs/src/telemetry/local.rs b/crates/obs/src/telemetry/local.rs
index 4ca69a1e7..6e8964106 100644
--- a/crates/obs/src/telemetry/local.rs
+++ b/crates/obs/src/telemetry/local.rs
@@ -206,7 +206,7 @@ fn init_file_logging_internal(
 
         builder
             .build(log_directory)
-            .expect("failed to initialize rolling file appender")
+            .map_err(|e| TelemetryError::Io(format!("failed to initialize rolling file appender: {e}")))?
     };
 
     let (non_blocking, guard) = tracing_appender::non_blocking(file_appender);
@@ -417,3 +417,24 @@ fn spawn_cleanup_task(
         }
     })
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::config::OtelConfig;
+    use tempfile::tempdir;
+
+    #[test]
+    fn test_init_file_logging_invalid_filename_does_not_panic() {
+        let temp_dir = tempdir().expect("create temp dir");
+        let temp_path = temp_dir.path().to_str().expect("temp dir path is utf-8");
+        let config = OtelConfig {
+            log_filename: Some("invalid\0name.log".to_string()),
+            ..OtelConfig::default()
+        };
+
+        let result = init_file_logging_internal(&config, temp_path, "info", true);
+
+        assert!(result.is_err());
+    }
+}
diff --git a/crates/obs/src/telemetry/otel.rs b/crates/obs/src/telemetry/otel.rs
index c6cc1c476..620ca85d4 100644
--- a/crates/obs/src/telemetry/otel.rs
+++ b/crates/obs/src/telemetry/otel.rs
@@ -96,11 +96,7 @@ pub(super) fn init_observability_http(
     let service_name = config.service_name.as_deref().unwrap_or(APP_NAME).to_owned();
     let use_stdout = config.use_stdout.unwrap_or(!is_production);
     let sample_ratio = config.sample_ratio.unwrap_or(SAMPLE_RATIO);
-    let sampler = if (0.0..1.0).contains(&sample_ratio) {
-        Sampler::TraceIdRatioBased(sample_ratio)
-    } else {
-        Sampler::AlwaysOn
-    };
+    let sampler = build_tracer_sampler(sample_ratio);
 
     // ── Endpoint resolution ───────────────────────────────────────────────────
     // Each signal may have a dedicated endpoint; if absent, fall back to the
@@ -239,6 +235,14 @@ fn build_tracer_provider(
     Ok(Some(provider))
 }
 
+fn build_tracer_sampler(sample_ratio: f64) -> Sampler {
+    if sample_ratio.is_finite() && (0.0..=1.0).contains(&sample_ratio) {
+        Sampler::TraceIdRatioBased(sample_ratio)
+    } else {
+        Sampler::AlwaysOn
+    }
+}
+
 /// Build an optional [`SdkMeterProvider`] for the given metrics endpoint.
 ///
 /// Returns `None` when the endpoint is empty or metric export is disabled.
@@ -360,3 +364,29 @@ fn create_periodic_reader(interval: u64) -> PeriodicReader<opentelemetry_stdout:
         .with_interval(Duration::from_secs(interval))
         .build()
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_build_tracer_sampler_uses_trace_ratio_for_valid_values() {
+        let sampler = build_tracer_sampler(0.0);
+        assert!(format!("{sampler:?}").contains("TraceIdRatioBased"));
+
+        let sampler = build_tracer_sampler(1.0);
+        assert!(format!("{sampler:?}").contains("TraceIdRatioBased"));
+
+        let sampler = build_tracer_sampler(0.5);
+        assert!(format!("{sampler:?}").contains("TraceIdRatioBased"));
+    }
+
+    #[test]
+    fn test_build_tracer_sampler_rejects_invalid_ratio_with_always_on() {
+        let sampler = build_tracer_sampler(-0.1);
+        assert!(format!("{sampler:?}").contains("AlwaysOn"));
+
+        let sampler = build_tracer_sampler(1.2);
+        assert!(format!("{sampler:?}").contains("AlwaysOn"));
+    }
+}