From 5617550363a5100853351379937125f758537062 Mon Sep 17 00:00:00 2001 From: David Katz Date: Fri, 22 Aug 2025 14:31:31 -0400 Subject: [PATCH 1/5] read from config --- crates/goose-cli/src/main.rs | 9 +++- crates/goose/src/tracing/otlp_layer.rs | 73 +++++++++++++++++++++++++- 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/crates/goose-cli/src/main.rs b/crates/goose-cli/src/main.rs index 0f54e53b76ca..8f15d9dd19b7 100644 --- a/crates/goose-cli/src/main.rs +++ b/crates/goose-cli/src/main.rs @@ -9,8 +9,13 @@ async fn main() -> Result<()> { let result = cli().await; - // Only wait for telemetry flush if OTLP is configured - if std::env::var("OTEL_EXPORTER_OTLP_ENDPOINT").is_ok() { + // Only wait for telemetry flush if OTLP is configured (check both env and config) + let should_wait = std::env::var("OTEL_EXPORTER_OTLP_ENDPOINT").is_ok() || + goose::config::Config::global() + .get_param::("otel_exporter_otlp_endpoint") + .is_ok(); + + if should_wait { // Use a shorter, dynamic wait with max timeout let max_wait = tokio::time::Duration::from_millis(500); let start = tokio::time::Instant::now(); diff --git a/crates/goose/src/tracing/otlp_layer.rs b/crates/goose/src/tracing/otlp_layer.rs index e54712da52ac..a2e51c7896a6 100644 --- a/crates/goose/src/tracing/otlp_layer.rs +++ b/crates/goose/src/tracing/otlp_layer.rs @@ -32,6 +32,7 @@ impl Default for OtlpConfig { impl OtlpConfig { pub fn from_env() -> Option { + // First check environment variable if let Ok(endpoint) = env::var("OTEL_EXPORTER_OTLP_ENDPOINT") { let mut config = Self { endpoint, @@ -49,6 +50,29 @@ impl OtlpConfig { None } } + + pub fn from_config() -> Option { + // Try to get from Goose config system (which also checks env vars) + let config = crate::config::Config::global(); + + // Try to get the endpoint from config + if let Ok(endpoint) = config.get_param::("otel_exporter_otlp_endpoint") { + let mut otlp_config = Self { + endpoint, + timeout: Duration::from_secs(10), + }; + + // Try to get timeout from config + if let Ok(timeout_ms) = config.get_param::("otel_exporter_otlp_timeout") { + otlp_config.timeout = Duration::from_millis(timeout_ms); + } + + Some(otlp_config) + } else { + // Fall back to checking env directly + Self::from_env() + } + } } pub fn init_otlp_tracing(config: &OtlpConfig) -> OtlpResult<()> { @@ -105,7 +129,7 @@ pub fn init_otlp_metrics(config: &OtlpConfig) -> OtlpResult<()> { pub fn create_otlp_tracing_layer() -> OtlpResult { let config = - OtlpConfig::from_env().ok_or("OTEL_EXPORTER_OTLP_ENDPOINT environment variable not set")?; + OtlpConfig::from_config().ok_or("OTEL_EXPORTER_OTLP_ENDPOINT not configured")?; let resource = Resource::new(vec![ KeyValue::new("service.name", "goose"), @@ -135,7 +159,7 @@ pub fn create_otlp_tracing_layer() -> OtlpResult { pub fn create_otlp_metrics_layer() -> OtlpResult { let config = - OtlpConfig::from_env().ok_or("OTEL_EXPORTER_OTLP_ENDPOINT environment variable not set")?; + OtlpConfig::from_config().ok_or("OTEL_EXPORTER_OTLP_ENDPOINT not configured")?; let resource = Resource::new(vec![ KeyValue::new("service.name", "goose"), @@ -268,4 +292,49 @@ mod tests { None => env::remove_var("OTEL_EXPORTER_OTLP_TIMEOUT"), } } + + #[test] + fn test_otlp_config_from_config() { + use tempfile::NamedTempFile; + + // Save original env vars + let original_endpoint = env::var("OTEL_EXPORTER_OTLP_ENDPOINT").ok(); + let original_timeout = env::var("OTEL_EXPORTER_OTLP_TIMEOUT").ok(); + + // Clear env vars to ensure we're testing config file + env::remove_var("OTEL_EXPORTER_OTLP_ENDPOINT"); + env::remove_var("OTEL_EXPORTER_OTLP_TIMEOUT"); + + // Create a test config file + let temp_file = NamedTempFile::new().unwrap(); + let test_config = crate::config::Config::new(temp_file.path(), "test-otlp").unwrap(); + + // Set values in config + test_config.set_param("otel_exporter_otlp_endpoint", serde_json::Value::String("http://config:4318".to_string())).unwrap(); + test_config.set_param("otel_exporter_otlp_timeout", serde_json::Value::Number(3000.into())).unwrap(); + + // Test that from_config reads from the config file + // Note: We can't easily test from_config() directly since it uses Config::global() + // But we can test that the config system works with our keys + let endpoint: String = test_config.get_param("otel_exporter_otlp_endpoint").unwrap(); + assert_eq!(endpoint, "http://config:4318"); + + let timeout: u64 = test_config.get_param("otel_exporter_otlp_timeout").unwrap(); + assert_eq!(timeout, 3000); + + // Test env var override still works + env::set_var("OTEL_EXPORTER_OTLP_ENDPOINT", "http://env:4317"); + let endpoint: String = test_config.get_param("otel_exporter_otlp_endpoint").unwrap(); + assert_eq!(endpoint, "http://env:4317"); + + // Restore original env vars + match original_endpoint { + Some(val) => env::set_var("OTEL_EXPORTER_OTLP_ENDPOINT", val), + None => env::remove_var("OTEL_EXPORTER_OTLP_ENDPOINT"), + } + match original_timeout { + Some(val) => env::set_var("OTEL_EXPORTER_OTLP_TIMEOUT", val), + None => env::remove_var("OTEL_EXPORTER_OTLP_TIMEOUT"), + } + } } From 403e684ac01fcdef513e2ef1e4219fb1a237f5ff Mon Sep 17 00:00:00 2001 From: David Katz Date: Fri, 22 Aug 2025 14:38:10 -0400 Subject: [PATCH 2/5] rm from env --- crates/goose/src/tracing/otlp_layer.rs | 49 +++++++------------------- 1 file changed, 13 insertions(+), 36 deletions(-) diff --git a/crates/goose/src/tracing/otlp_layer.rs b/crates/goose/src/tracing/otlp_layer.rs index a2e51c7896a6..43a407d6e10b 100644 --- a/crates/goose/src/tracing/otlp_layer.rs +++ b/crates/goose/src/tracing/otlp_layer.rs @@ -31,47 +31,24 @@ impl Default for OtlpConfig { } impl OtlpConfig { - pub fn from_env() -> Option { - // First check environment variable - if let Ok(endpoint) = env::var("OTEL_EXPORTER_OTLP_ENDPOINT") { - let mut config = Self { - endpoint, - timeout: Duration::from_secs(10), - }; - - if let Ok(timeout_str) = env::var("OTEL_EXPORTER_OTLP_TIMEOUT") { - if let Ok(timeout_ms) = timeout_str.parse::() { - config.timeout = Duration::from_millis(timeout_ms); - } - } - - Some(config) - } else { - None - } - } - pub fn from_config() -> Option { - // Try to get from Goose config system (which also checks env vars) + // Try to get from Goose config system (which checks env vars first, then config file) let config = crate::config::Config::global(); - // Try to get the endpoint from config - if let Ok(endpoint) = config.get_param::("otel_exporter_otlp_endpoint") { - let mut otlp_config = Self { - endpoint, - timeout: Duration::from_secs(10), - }; - - // Try to get timeout from config - if let Ok(timeout_ms) = config.get_param::("otel_exporter_otlp_timeout") { - otlp_config.timeout = Duration::from_millis(timeout_ms); - } + // Try to get the endpoint from config (checks OTEL_EXPORTER_OTLP_ENDPOINT env var first) + let endpoint = config.get_param::("otel_exporter_otlp_endpoint").ok()?; + + let mut otlp_config = Self { + endpoint, + timeout: Duration::from_secs(10), + }; - Some(otlp_config) - } else { - // Fall back to checking env directly - Self::from_env() + // Try to get timeout from config (checks OTEL_EXPORTER_OTLP_TIMEOUT env var first) + if let Ok(timeout_ms) = config.get_param::("otel_exporter_otlp_timeout") { + otlp_config.timeout = Duration::from_millis(timeout_ms); } + + Some(otlp_config) } } From 020815d39457c41cc385b20d37b038ced22c85b7 Mon Sep 17 00:00:00 2001 From: David Katz Date: Fri, 22 Aug 2025 14:38:57 -0400 Subject: [PATCH 3/5] rm test --- crates/goose/src/tracing/otlp_layer.rs | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/crates/goose/src/tracing/otlp_layer.rs b/crates/goose/src/tracing/otlp_layer.rs index 43a407d6e10b..25a50878903d 100644 --- a/crates/goose/src/tracing/otlp_layer.rs +++ b/crates/goose/src/tracing/otlp_layer.rs @@ -3,7 +3,6 @@ use opentelemetry::{global, KeyValue}; use opentelemetry_otlp::WithExportConfig; use opentelemetry_sdk::trace::{self, RandomIdGenerator, Sampler}; use opentelemetry_sdk::{runtime, Resource}; -use std::env; use std::time::Duration; use tracing::{Level, Metadata}; use tracing_opentelemetry::{MetricsLayer, OpenTelemetryLayer}; @@ -245,31 +244,6 @@ mod tests { assert_eq!(config.timeout, Duration::from_secs(10)); } - #[test] - fn test_otlp_config_from_env() { - let original_endpoint = env::var("OTEL_EXPORTER_OTLP_ENDPOINT").ok(); - let original_timeout = env::var("OTEL_EXPORTER_OTLP_TIMEOUT").ok(); - - env::remove_var("OTEL_EXPORTER_OTLP_ENDPOINT"); - assert!(OtlpConfig::from_env().is_none()); - - env::set_var("OTEL_EXPORTER_OTLP_ENDPOINT", "http://test:4317"); - env::set_var("OTEL_EXPORTER_OTLP_TIMEOUT", "5000"); - - let config = OtlpConfig::from_env().unwrap(); - assert_eq!(config.endpoint, "http://test:4317"); - assert_eq!(config.timeout, Duration::from_millis(5000)); - - match original_endpoint { - Some(val) => env::set_var("OTEL_EXPORTER_OTLP_ENDPOINT", val), - None => env::remove_var("OTEL_EXPORTER_OTLP_ENDPOINT"), - } - match original_timeout { - Some(val) => env::set_var("OTEL_EXPORTER_OTLP_TIMEOUT", val), - None => env::remove_var("OTEL_EXPORTER_OTLP_TIMEOUT"), - } - } - #[test] fn test_otlp_config_from_config() { use tempfile::NamedTempFile; From 6b6e28448c005172e81c6100283d2086a9ac258b Mon Sep 17 00:00:00 2001 From: David Katz Date: Fri, 22 Aug 2025 14:43:16 -0400 Subject: [PATCH 4/5] fmt --- crates/goose-cli/src/main.rs | 6 ++-- crates/goose/src/tracing/otlp_layer.rs | 42 +++++++++++++++++--------- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/crates/goose-cli/src/main.rs b/crates/goose-cli/src/main.rs index 8f15d9dd19b7..1f268d4c73b6 100644 --- a/crates/goose-cli/src/main.rs +++ b/crates/goose-cli/src/main.rs @@ -10,11 +10,11 @@ async fn main() -> Result<()> { let result = cli().await; // Only wait for telemetry flush if OTLP is configured (check both env and config) - let should_wait = std::env::var("OTEL_EXPORTER_OTLP_ENDPOINT").is_ok() || - goose::config::Config::global() + let should_wait = std::env::var("OTEL_EXPORTER_OTLP_ENDPOINT").is_ok() + || goose::config::Config::global() .get_param::("otel_exporter_otlp_endpoint") .is_ok(); - + if should_wait { // Use a shorter, dynamic wait with max timeout let max_wait = tokio::time::Duration::from_millis(500); diff --git a/crates/goose/src/tracing/otlp_layer.rs b/crates/goose/src/tracing/otlp_layer.rs index 25a50878903d..d61c907ceb21 100644 --- a/crates/goose/src/tracing/otlp_layer.rs +++ b/crates/goose/src/tracing/otlp_layer.rs @@ -33,10 +33,12 @@ impl OtlpConfig { pub fn from_config() -> Option { // Try to get from Goose config system (which checks env vars first, then config file) let config = crate::config::Config::global(); - + // Try to get the endpoint from config (checks OTEL_EXPORTER_OTLP_ENDPOINT env var first) - let endpoint = config.get_param::("otel_exporter_otlp_endpoint").ok()?; - + let endpoint = config + .get_param::("otel_exporter_otlp_endpoint") + .ok()?; + let mut otlp_config = Self { endpoint, timeout: Duration::from_secs(10), @@ -104,8 +106,7 @@ pub fn init_otlp_metrics(config: &OtlpConfig) -> OtlpResult<()> { } pub fn create_otlp_tracing_layer() -> OtlpResult { - let config = - OtlpConfig::from_config().ok_or("OTEL_EXPORTER_OTLP_ENDPOINT not configured")?; + let config = OtlpConfig::from_config().ok_or("OTEL_EXPORTER_OTLP_ENDPOINT not configured")?; let resource = Resource::new(vec![ KeyValue::new("service.name", "goose"), @@ -134,8 +135,7 @@ pub fn create_otlp_tracing_layer() -> OtlpResult { } pub fn create_otlp_metrics_layer() -> OtlpResult { - let config = - OtlpConfig::from_config().ok_or("OTEL_EXPORTER_OTLP_ENDPOINT not configured")?; + let config = OtlpConfig::from_config().ok_or("OTEL_EXPORTER_OTLP_ENDPOINT not configured")?; let resource = Resource::new(vec![ KeyValue::new("service.name", "goose"), @@ -247,7 +247,7 @@ mod tests { #[test] fn test_otlp_config_from_config() { use tempfile::NamedTempFile; - + // Save original env vars let original_endpoint = env::var("OTEL_EXPORTER_OTLP_ENDPOINT").ok(); let original_timeout = env::var("OTEL_EXPORTER_OTLP_TIMEOUT").ok(); @@ -259,23 +259,37 @@ mod tests { // Create a test config file let temp_file = NamedTempFile::new().unwrap(); let test_config = crate::config::Config::new(temp_file.path(), "test-otlp").unwrap(); - + // Set values in config - test_config.set_param("otel_exporter_otlp_endpoint", serde_json::Value::String("http://config:4318".to_string())).unwrap(); - test_config.set_param("otel_exporter_otlp_timeout", serde_json::Value::Number(3000.into())).unwrap(); + test_config + .set_param( + "otel_exporter_otlp_endpoint", + serde_json::Value::String("http://config:4318".to_string()), + ) + .unwrap(); + test_config + .set_param( + "otel_exporter_otlp_timeout", + serde_json::Value::Number(3000.into()), + ) + .unwrap(); // Test that from_config reads from the config file // Note: We can't easily test from_config() directly since it uses Config::global() // But we can test that the config system works with our keys - let endpoint: String = test_config.get_param("otel_exporter_otlp_endpoint").unwrap(); + let endpoint: String = test_config + .get_param("otel_exporter_otlp_endpoint") + .unwrap(); assert_eq!(endpoint, "http://config:4318"); - + let timeout: u64 = test_config.get_param("otel_exporter_otlp_timeout").unwrap(); assert_eq!(timeout, 3000); // Test env var override still works env::set_var("OTEL_EXPORTER_OTLP_ENDPOINT", "http://env:4317"); - let endpoint: String = test_config.get_param("otel_exporter_otlp_endpoint").unwrap(); + let endpoint: String = test_config + .get_param("otel_exporter_otlp_endpoint") + .unwrap(); assert_eq!(endpoint, "http://env:4317"); // Restore original env vars From 715302e2ca217c1008d814beed3021679f6da027 Mon Sep 17 00:00:00 2001 From: David Katz Date: Fri, 22 Aug 2025 15:09:31 -0400 Subject: [PATCH 5/5] lingering env --- crates/goose-cli/src/main.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/crates/goose-cli/src/main.rs b/crates/goose-cli/src/main.rs index 1f268d4c73b6..c301e30139a8 100644 --- a/crates/goose-cli/src/main.rs +++ b/crates/goose-cli/src/main.rs @@ -9,11 +9,10 @@ async fn main() -> Result<()> { let result = cli().await; - // Only wait for telemetry flush if OTLP is configured (check both env and config) - let should_wait = std::env::var("OTEL_EXPORTER_OTLP_ENDPOINT").is_ok() - || goose::config::Config::global() - .get_param::("otel_exporter_otlp_endpoint") - .is_ok(); + // Only wait for telemetry flush if OTLP is configured + let should_wait = goose::config::Config::global() + .get_param::("otel_exporter_otlp_endpoint") + .is_ok(); if should_wait { // Use a shorter, dynamic wait with max timeout