fix: endpoint urls for otlp http exporter. (#1210)

TommyCpp · web-flow · commit f66e9626cb34 · 2023-08-28T11:14:40.000-04:00
diff --git a/opentelemetry-otlp/src/exporter/http/mod.rs b/opentelemetry-otlp/src/exporter/http/mod.rs
@@ -144,14 +144,11 @@ impl HttpExporterBuilder {
         signal_endpoint_path: &str,
         signal_timeout_var: &str,
     ) -> Result<OtlpHttpClient, crate::Error> {
-        let endpoint = match env::var(signal_endpoint_var)
-            .ok()
-            .or(env::var(OTEL_EXPORTER_OTLP_ENDPOINT).ok())
-            .and_then(|s| s.parse().ok())
-        {
-            Some(val) => val,
-            None => format!("{}{signal_endpoint_path}", self.exporter_config.endpoint).parse()?,
-        };
+        let endpoint = resolve_endpoint(
+            signal_endpoint_var,
+            signal_endpoint_path,
+            self.exporter_config.endpoint.as_str(),
+        )?;
 
         let timeout = match env::var(signal_timeout_var)
             .ok()
@@ -265,3 +262,152 @@ impl OtlpHttpClient {
         }
     }
 }
+
+// see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#endpoint-urls-for-otlphttp
+fn resolve_endpoint(
+    signal_endpoint_var: &str,
+    signal_endpoint_path: &str,
+    provided_or_default_endpoint: &str,
+) -> Result<Uri, crate::Error> {
+    // per signal env var is not modified
+    if let Some(endpoint) = env::var(signal_endpoint_var)
+        .ok()
+        .and_then(|s| s.parse().ok())
+    {
+        return Ok(endpoint);
+    }
+
+    // if signal env var is not set, then we check if the OTEL_EXPORTER_OTLP_ENDPOINT is set
+    if let Some(endpoint) = env::var(OTEL_EXPORTER_OTLP_ENDPOINT)
+        .ok()
+        .and_then(|s| format!("{s}{signal_endpoint_path}").parse().ok())
+    {
+        return Ok(endpoint);
+    }
+
+    // if neither works, we use the one provided in pipeline. If user never provide one, we will use the default one
+    format!("{provided_or_default_endpoint}{signal_endpoint_path}")
+        .parse()
+        .map_err(From::from)
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::{OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_TRACES_ENDPOINT};
+    use std::sync::Mutex;
+
+    // Make sure env tests are not running concurrently
+    static ENV_LOCK: Mutex<()> = Mutex::new(());
+
+    fn run_env_test<T, F>(env_vars: T, f: F)
+    where
+        F: FnOnce(),
+        T: Into<Vec<(&'static str, &'static str)>>,
+    {
+        let _env_lock = ENV_LOCK.lock().expect("env test lock poisoned");
+        let env_vars = env_vars.into();
+        for (k, v) in env_vars.iter() {
+            std::env::set_var(k, v);
+        }
+        f();
+        for (k, _) in env_vars {
+            std::env::remove_var(k);
+        }
+    }
+
+    #[test]
+    fn test_append_signal_path_to_generic_env() {
+        run_env_test(
+            vec![(OTEL_EXPORTER_OTLP_ENDPOINT, "http://example.com")],
+            || {
+                let endpoint = super::resolve_endpoint(
+                    OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
+                    "/v1/traces",
+                    "http://localhost:4317",
+                )
+                .unwrap();
+                assert_eq!(endpoint, "http://example.com/v1/traces");
+            },
+        )
+    }
+
+    #[test]
+    fn test_not_append_signal_path_to_signal_env() {
+        run_env_test(
+            vec![(OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, "http://example.com")],
+            || {
+                let endpoint = super::resolve_endpoint(
+                    OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
+                    "/v1/traces",
+                    "http://localhost:4317",
+                )
+                .unwrap();
+                assert_eq!(endpoint, "http://example.com");
+            },
+        )
+    }
+
+    #[test]
+    fn test_priority_of_signal_env_over_generic_env() {
+        run_env_test(
+            vec![
+                (OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, "http://example.com"),
+                (OTEL_EXPORTER_OTLP_ENDPOINT, "http://wrong.com"),
+            ],
+            || {
+                let endpoint = super::resolve_endpoint(
+                    OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
+                    "/v1/traces",
+                    "http://localhost:4317",
+                )
+                .unwrap();
+                assert_eq!(endpoint, "http://example.com");
+            },
+        );
+    }
+
+    #[test]
+    fn test_use_provided_or_default_when_others_missing() {
+        run_env_test(vec![], || {
+            let endpoint =
+                super::resolve_endpoint("NON_EXISTENT_VAR", "/v1/traces", "http://localhost:4317")
+                    .unwrap();
+            assert_eq!(endpoint, "http://localhost:4317/v1/traces");
+        });
+    }
+
+    #[test]
+    fn test_invalid_uri_in_signal_env_falls_back_to_generic_env() {
+        run_env_test(
+            vec![
+                (
+                    OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
+                    "-*/*-/*-//-/-/invalid-uri",
+                ),
+                (OTEL_EXPORTER_OTLP_ENDPOINT, "http://example.com"),
+            ],
+            || {
+                let endpoint = super::resolve_endpoint(
+                    OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
+                    "/v1/traces",
+                    "http://localhost:4317",
+                )
+                .unwrap();
+                assert_eq!(endpoint, "http://example.com/v1/traces");
+            },
+        );
+    }
+
+    #[test]
+    fn test_all_invalid_urls_falls_back_to_error() {
+        run_env_test(vec![], || {
+            let result = super::resolve_endpoint(
+                OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
+                "/v1/traces",
+                "-*/*-/*-//-/-/yet-another-invalid-uri",
+            );
+            assert!(result.is_err());
+            // You may also want to assert on the specific error type if applicable
+        });
+    }
+}
diff --git a/opentelemetry-otlp/src/exporter/mod.rs b/opentelemetry-otlp/src/exporter/mod.rs
@@ -186,13 +186,12 @@ pub trait WithExportConfig {
     ///
     /// Note that protocols that are not supported by exporters will be ignore. The exporter
     /// will use default protocol in this case.
+    ///
+    /// ## Note
+    /// All exporters in this crate are only support one protocol thus choosing the protocol is an no-op at the moment
     fn with_protocol(self, protocol: Protocol) -> Self;
     /// Set the timeout to the collector.
     fn with_timeout(self, timeout: Duration) -> Self;
-    /// Set the trace provider configuration from the given environment variables.
-    ///
-    /// If the value in environment variables is illegal, will fall back to use default value.
-    fn with_env(self) -> Self;
     /// Set export config. This will override all previous configuration.
     fn with_export_config(self, export_config: ExportConfig) -> Self;
 }
@@ -213,184 +212,10 @@ impl<B: HasExportConfig> WithExportConfig for B {
         self
     }
 
-    fn with_env(mut self) -> Self {
-        let protocol = match std::env::var(OTEL_EXPORTER_OTLP_PROTOCOL)
-            .unwrap_or_else(|_| OTEL_EXPORTER_OTLP_PROTOCOL_DEFAULT.to_string())
-            .as_str()
-        {
-            OTEL_EXPORTER_OTLP_PROTOCOL_HTTP_PROTOBUF => Protocol::HttpBinary,
-            OTEL_EXPORTER_OTLP_PROTOCOL_GRPC => Protocol::Grpc,
-            _ => default_protocol(),
-        };
-
-        self.export_config().protocol = protocol;
-
-        let endpoint = match std::env::var(OTEL_EXPORTER_OTLP_ENDPOINT) {
-            Ok(val) => val,
-            Err(_) => default_endpoint(protocol),
-        };
-        self.export_config().endpoint = endpoint;
-
-        let timeout = match std::env::var(OTEL_EXPORTER_OTLP_TIMEOUT) {
-            Ok(val) => u64::from_str(&val).unwrap_or(OTEL_EXPORTER_OTLP_TIMEOUT_DEFAULT),
-            Err(_) => OTEL_EXPORTER_OTLP_TIMEOUT_DEFAULT,
-        };
-        self.export_config().timeout = Duration::from_secs(timeout);
-        self
-    }
-
     fn with_export_config(mut self, exporter_config: ExportConfig) -> Self {
         self.export_config().endpoint = exporter_config.endpoint;
         self.export_config().protocol = exporter_config.protocol;
         self.export_config().timeout = exporter_config.timeout;
         self
     }
 }
-
-#[cfg(test)]
-#[cfg(feature = "grpc-tonic")]
-mod tests {
-    // If an env test fails then the mutex will be poisoned and the following error will be displayed.
-    const LOCK_POISONED_MESSAGE: &str = "one of the other pipeline builder from env tests failed";
-
-    use crate::exporter::{
-        default_endpoint, default_protocol, WithExportConfig, OTEL_EXPORTER_OTLP_ENDPOINT,
-        OTEL_EXPORTER_OTLP_GRPC_ENDPOINT_DEFAULT, OTEL_EXPORTER_OTLP_HTTP_ENDPOINT_DEFAULT,
-        OTEL_EXPORTER_OTLP_PROTOCOL_GRPC, OTEL_EXPORTER_OTLP_PROTOCOL_HTTP_PROTOBUF,
-        OTEL_EXPORTER_OTLP_TIMEOUT, OTEL_EXPORTER_OTLP_TIMEOUT_DEFAULT,
-    };
-    use crate::{new_exporter, Compression, Protocol, OTEL_EXPORTER_OTLP_PROTOCOL};
-    use std::str::FromStr;
-    use std::sync::Mutex;
-
-    // Make sure env tests are not running concurrently
-    static ENV_LOCK: Mutex<usize> = Mutex::new(0);
-
-    #[test]
-    fn test_pipeline_builder_from_env_default_vars() {
-        let _env_lock = ENV_LOCK.lock().expect(LOCK_POISONED_MESSAGE);
-        let exporter_builder = new_exporter().tonic().with_env();
-        assert_eq!(
-            exporter_builder.exporter_config.protocol,
-            default_protocol()
-        );
-        assert_eq!(
-            exporter_builder.exporter_config.endpoint,
-            default_endpoint(default_protocol())
-        );
-        assert_eq!(
-            exporter_builder.exporter_config.timeout,
-            std::time::Duration::from_secs(OTEL_EXPORTER_OTLP_TIMEOUT_DEFAULT)
-        );
-    }
-
-    #[test]
-    fn test_pipeline_builder_from_env_endpoint() {
-        let _env_lock = ENV_LOCK.lock().expect(LOCK_POISONED_MESSAGE);
-        std::env::set_var(OTEL_EXPORTER_OTLP_ENDPOINT, "http://example.com");
-        let exporter_builder = new_exporter().tonic().with_env();
-        assert_eq!(
-            exporter_builder.exporter_config.endpoint,
-            "http://example.com"
-        );
-        std::env::remove_var(OTEL_EXPORTER_OTLP_ENDPOINT);
-        assert!(std::env::var(OTEL_EXPORTER_OTLP_ENDPOINT).is_err());
-    }
-
-    #[test]
-    fn test_pipeline_builder_from_env_protocol_http_protobuf() {
-        let _env_lock = ENV_LOCK.lock().expect(LOCK_POISONED_MESSAGE);
-        std::env::set_var(
-            OTEL_EXPORTER_OTLP_PROTOCOL,
-            OTEL_EXPORTER_OTLP_PROTOCOL_HTTP_PROTOBUF,
-        );
-        let exporter_builder = new_exporter().tonic().with_env();
-        assert_eq!(
-            exporter_builder.exporter_config.protocol,
-            Protocol::HttpBinary
-        );
-        assert_eq!(
-            exporter_builder.exporter_config.endpoint,
-            OTEL_EXPORTER_OTLP_HTTP_ENDPOINT_DEFAULT
-        );
-
-        std::env::remove_var(OTEL_EXPORTER_OTLP_PROTOCOL);
-        assert!(std::env::var(OTEL_EXPORTER_OTLP_PROTOCOL).is_err());
-    }
-
-    #[test]
-    fn test_pipeline_builder_from_env_protocol_grpc() {
-        let _env_lock = ENV_LOCK.lock().expect(LOCK_POISONED_MESSAGE);
-        std::env::set_var(
-            OTEL_EXPORTER_OTLP_PROTOCOL,
-            OTEL_EXPORTER_OTLP_PROTOCOL_GRPC,
-        );
-        let exporter_builder = new_exporter().tonic().with_env();
-        assert_eq!(exporter_builder.exporter_config.protocol, Protocol::Grpc);
-        assert_eq!(
-            exporter_builder.exporter_config.endpoint,
-            OTEL_EXPORTER_OTLP_GRPC_ENDPOINT_DEFAULT
-        );
-
-        std::env::remove_var(OTEL_EXPORTER_OTLP_PROTOCOL);
-        assert!(std::env::var(OTEL_EXPORTER_OTLP_PROTOCOL).is_err());
-    }
-
-    #[test]
-    fn test_pipeline_builder_from_env_bad_protocol() {
-        let _env_lock = ENV_LOCK.lock().expect(LOCK_POISONED_MESSAGE);
-        std::env::set_var(OTEL_EXPORTER_OTLP_PROTOCOL, "bad_protocol");
-        let exporter_builder = new_exporter().tonic().with_env();
-        assert_eq!(
-            exporter_builder.exporter_config.protocol,
-            default_protocol()
-        );
-        assert_eq!(
-            exporter_builder.exporter_config.endpoint,
-            default_endpoint(default_protocol())
-        );
-
-        std::env::remove_var(OTEL_EXPORTER_OTLP_PROTOCOL);
-        assert!(std::env::var(OTEL_EXPORTER_OTLP_PROTOCOL).is_err());
-    }
-
-    #[test]
-    fn test_pipeline_builder_from_env_timeout() {
-        let _env_lock = ENV_LOCK.lock().expect(LOCK_POISONED_MESSAGE);
-        std::env::set_var(OTEL_EXPORTER_OTLP_TIMEOUT, "60");
-        let exporter_builder = new_exporter().tonic().with_env();
-        assert_eq!(
-            exporter_builder.exporter_config.timeout,
-            std::time::Duration::from_secs(60)
-        );
-
-        std::env::remove_var(OTEL_EXPORTER_OTLP_TIMEOUT);
-        assert!(std::env::var(OTEL_EXPORTER_OTLP_TIMEOUT).is_err());
-    }
-
-    #[test]
-    fn test_pipeline_builder_from_env_bad_timeout() {
-        let _env_lock = ENV_LOCK.lock().expect(LOCK_POISONED_MESSAGE);
-        std::env::set_var(OTEL_EXPORTER_OTLP_TIMEOUT, "bad_timeout");
-
-        let exporter_builder = new_exporter().tonic().with_env();
-        assert_eq!(
-            exporter_builder.exporter_config.timeout,
-            std::time::Duration::from_secs(OTEL_EXPORTER_OTLP_TIMEOUT_DEFAULT)
-        );
-
-        std::env::remove_var(OTEL_EXPORTER_OTLP_TIMEOUT);
-        assert!(std::env::var(OTEL_EXPORTER_OTLP_TIMEOUT).is_err());
-    }
-
-    #[test]
-    fn test_compression_parse() {
-        assert_eq!(Compression::from_str("gzip").unwrap(), Compression::Gzip);
-        Compression::from_str("bad_compression").expect_err("bad compression");
-    }
-
-    #[test]
-    fn test_compression_to_str() {
-        assert_eq!(Compression::Gzip.to_string(), "gzip");
-    }
-}
diff --git a/opentelemetry-otlp/src/exporter/tonic/mod.rs b/opentelemetry-otlp/src/exporter/tonic/mod.rs
@@ -145,7 +145,10 @@ impl Default for TonicExporterBuilder {
         };
 
         TonicExporterBuilder {
-            exporter_config: ExportConfig::default(),
+            exporter_config: ExportConfig {
+                protocol: crate::Protocol::Grpc,
+                ..Default::default()
+            },
             tonic_config,
             channel: Option::default(),
             interceptor: Option::default(),
