refactor: prioritize code-based config over env vars in OTLP exporter

opentelemetry-otlp/CHANGELOG.md

@@ -17,6 +17,9 @@
  `Error` which contained many variants unrelated to building an exporter, the
  new one returns specific variants applicable to building an exporter. Some
  variants might be applicable only on select features.
+- *Breaking* `ExportConfig`'s `timeout` field is now optional, and the order of
+  priority changed in favor of compile time values. Additionally the `endpoint`
+  field prioritizes compile time values over environment variables.
 
 ## 0.28.0
 

opentelemetry-otlp/src/exporter/http/mod.rs

@@ -1,11 +1,8 @@
 use super::{
-    default_headers, default_protocol, parse_header_string, ExporterBuildError,
+    default_headers, default_protocol, parse_header_string, resolve_timeout, ExporterBuildError,
     OTEL_EXPORTER_OTLP_HTTP_ENDPOINT_DEFAULT,
 };
-use crate::{
-    ExportConfig, Protocol, OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS,
-    OTEL_EXPORTER_OTLP_TIMEOUT,
-};
+use crate::{ExportConfig, Protocol, OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS};
 use http::{HeaderName, HeaderValue, Uri};
 use opentelemetry_http::HttpClient;
 use opentelemetry_proto::transform::common::tonic::ResourceAttributesWithSchema;
@@ -112,19 +109,10 @@
         let endpoint = resolve_http_endpoint(
             signal_endpoint_var,
             signal_endpoint_path,
-            self.exporter_config.endpoint.clone(),
+            self.exporter_config.endpoint.as_deref(),
         )?;
 
-        let timeout = match env::var(signal_timeout_var)
-            .ok()
-            .or(env::var(OTEL_EXPORTER_OTLP_TIMEOUT).ok())
-        {
-            Some(val) => match val.parse() {
-                Ok(milli_seconds) => Duration::from_millis(milli_seconds),
-                Err(_) => self.exporter_config.timeout,
-            },
-            None => self.exporter_config.timeout,
-        };
+        let timeout = resolve_timeout(signal_timeout_var, self.exporter_config.timeout.as_ref());
 
         #[allow(unused_mut)] // TODO - clippy thinks mut is not needed, but it is
         let mut http_client = self.http_config.client.take();
@@ -371,30 +359,27 @@
 fn resolve_http_endpoint(
     signal_endpoint_var: &str,
     signal_endpoint_path: &str,
-    provided_endpoint: Option<String>,
+    provided_endpoint: Option<&str>,
 ) -> Result<Uri, ExporterBuildError> {
-    // per signal env var is not modified
-    if let Some(endpoint) = env::var(signal_endpoint_var)
+    // programmatic configuration overrides any value set via environment variables
+    if let Some(provider_endpoint) = provided_endpoint {
+        provider_endpoint
+            .parse()
+            .map_err(|er: http::uri::InvalidUri| {
+                ExporterBuildError::InvalidUri(provider_endpoint.to_string(), er.to_string())
+            })
+    } else 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)
+        // per signal env var is not modified
+        Ok(endpoint)
+    } else if let Some(endpoint) = env::var(OTEL_EXPORTER_OTLP_ENDPOINT)
         .ok()
         .and_then(|s| build_endpoint_uri(&s, signal_endpoint_path).ok())
     {
-        return Ok(endpoint);
-    }
-
-    if let Some(provider_endpoint) = provided_endpoint {
-        provider_endpoint
-            .parse()
-            .map_err(|er: http::uri::InvalidUri| {
-                ExporterBuildError::InvalidUri(provider_endpoint, er.to_string())
-            })
+        // if signal env var is not set, then we check if the OTEL_EXPORTER_OTLP_ENDPOINT env var is set
+        Ok(endpoint)
     } else {
         build_endpoint_uri(
             OTEL_EXPORTER_OTLP_HTTP_ENDPOINT_DEFAULT,
@@ -481,12 +466,9 @@
         run_env_test(
             vec![(OTEL_EXPORTER_OTLP_ENDPOINT, "http://example.com")],
             || {
-                let endpoint = resolve_http_endpoint(
-                    OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
-                    "/v1/traces",
-                    Some("http://localhost:4317".to_string()),
-                )
-                .unwrap();
+                let endpoint =
+                    resolve_http_endpoint(OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, "/v1/traces", None)
+                        .unwrap();
                 assert_eq!(endpoint, "http://example.com/v1/traces");
             },
         )
@@ -496,20 +478,36 @@
     fn test_not_append_signal_path_to_signal_env() {
         run_env_test(
             vec![(OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, "http://example.com")],
+            || {
+                let endpoint =
+                    resolve_http_endpoint(OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, "/v1/traces", None)
+                        .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_http_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
-                    Some("http://localhost:4317".to_string()),
+                    None,
                 )
                 .unwrap();
                 assert_eq!(endpoint, "http://example.com");
             },
-        )
+        );
     }
 
     #[test]
-    fn test_priority_of_signal_env_over_generic_env() {
+    fn test_priority_of_code_based_config_over_envs() {
         run_env_test(
             vec![
                 (OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, "http://example.com"),
@@ -519,24 +517,20 @@
                 let endpoint = super::resolve_http_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
-                    Some("http://localhost:4317".to_string()),
+                    Some("http://localhost:4317"),
                 )
                 .unwrap();
-                assert_eq!(endpoint, "http://example.com");
+                assert_eq!(endpoint, "http://localhost:4317");
             },
         );
     }
 
     #[test]
-    fn test_use_provided_or_default_when_others_missing() {
+    fn test_use_default_when_others_missing() {
         run_env_test(vec![], || {
-            let endpoint = super::resolve_http_endpoint(
-                "NON_EXISTENT_VAR",
-                "/v1/traces",
-                Some("http://localhost:4317".to_string()),
-            )
-            .unwrap();
-            assert_eq!(endpoint, "http://localhost:4317/");
+            let endpoint =
+                super::resolve_http_endpoint("NON_EXISTENT_VAR", "/v1/traces", None).unwrap();
+            assert_eq!(endpoint, "http://localhost:4318/v1/traces");
         });
     }
 
@@ -568,7 +562,7 @@
                 let endpoint = super::resolve_http_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
-                    Some("http://localhost:4317".to_string()),
+                    None,
                 )
                 .unwrap();
                 assert_eq!(endpoint, "http://example.com/v1/traces");
@@ -582,7 +576,7 @@
             let result = super::resolve_http_endpoint(
                 OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                 "/v1/traces",
-                Some("-*/*-/*-//-/-/yet-another-invalid-uri".to_string()),
+                Some("-*/*-/*-//-/-/yet-another-invalid-uri"),
             );
             assert!(result.is_err());
             // You may also want to assert on the specific error type if applicable
@@ -722,7 +716,7 @@
             let url = resolve_http_endpoint(
                 OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                 "/v1/traces",
-                exporter.exporter_config.endpoint,
+                exporter.exporter_config.endpoint.as_deref(),
             )
             .unwrap();
 
@@ -737,7 +731,7 @@
             let url = resolve_http_endpoint(
                 OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                 "/v1/traces",
-                exporter.exporter_config.endpoint,
+                exporter.exporter_config.endpoint.as_deref(),
             )
             .unwrap();
 

opentelemetry-otlp/src/exporter/mod.rs

@@ -9,6 +9,7 @@
 use crate::Protocol;
 #[cfg(feature = "serialize")]
 use serde::{Deserialize, Serialize};
+use std::env;
 use std::fmt::{Display, Formatter};
 use std::str::FromStr;
 use std::time::Duration;
@@ -76,19 +77,19 @@
     pub protocol: Protocol,
 
     /// The timeout to the collector.
-    pub timeout: Duration,
+    pub timeout: Option<Duration>,
 }
 
 impl Default for ExportConfig {
     fn default() -> Self {
         let protocol = default_protocol();
 
-        ExportConfig {
+        Self {
             endpoint: None,
             // don't use default_endpoint(protocol) here otherwise we
             // won't know if user provided a value
             protocol,
-            timeout: OTEL_EXPORTER_OTLP_TIMEOUT_DEFAULT,
+            timeout: None,
         }
     }
 }
@@ -250,7 +251,7 @@
     }
 
     fn with_timeout(mut self, timeout: Duration) -> Self {
-        self.export_config().timeout = timeout;
+        self.export_config().timeout = Some(timeout);
         self
     }
 
@@ -262,6 +263,27 @@
     }
 }
 
+fn resolve_timeout(signal_timeout_var: &str, provided_timeout: Option<&Duration>) -> Duration {
+    // programmatic configuration overrides any value set via environment variables
+    if let Some(timeout) = provided_timeout {
+        *timeout
+    } else if let Some(timeout) = env::var(signal_timeout_var)
+        .ok()
+        .and_then(|s| s.parse().ok())
+    {
+        // per signal env var is not modified
+        Duration::from_millis(timeout)
+    } else if let Some(timeout) = env::var(OTEL_EXPORTER_OTLP_TIMEOUT)
+        .ok()
+        .and_then(|s| s.parse().ok())
+    {
+        // if signal env var is not set, then we check if the OTEL_EXPORTER_OTLP_TIMEOUT env var is set
+        Duration::from_millis(timeout)
+    } else {
+        OTEL_EXPORTER_OTLP_TIMEOUT_DEFAULT
+    }
+}
+
 #[cfg(any(feature = "grpc-tonic", feature = "http-proto", feature = "http-json"))]
 fn parse_header_string(value: &str) -> impl Iterator<Item = (&str, String)> {
     value
@@ -342,7 +364,6 @@
     #[cfg(feature = "logs")]
     #[cfg(any(feature = "http-proto", feature = "http-json"))]
     #[test]
-    #[ignore = "Unstable due to interference from env variable tests. Re-enable after https://github.com/open-telemetry/opentelemetry-rust/issues/2818 is resolved."]
     fn export_builder_error_invalid_http_endpoint() {
         use std::time::Duration;
 
@@ -351,7 +372,7 @@
         let ex_config = ExportConfig {
             endpoint: Some("invalid_uri/something".to_string()),
             protocol: Protocol::HttpBinary,
-            timeout: Duration::from_secs(10),
+            timeout: Some(Duration::from_secs(10)),
         };
 
         let exporter_result = LogExporter::builder()
@@ -371,7 +392,6 @@
 
     #[cfg(feature = "grpc-tonic")]
     #[tokio::test]
-    #[ignore = "Unstable due to interference from env variable tests. Re-enable after https://github.com/open-telemetry/opentelemetry-rust/issues/2818 is resolved."]
     async fn export_builder_error_invalid_grpc_endpoint() {
         use std::time::Duration;
 
@@ -380,7 +400,7 @@
         let ex_config = ExportConfig {
             endpoint: Some("invalid_uri/something".to_string()),
             protocol: Protocol::Grpc,
-            timeout: Duration::from_secs(10),
+            timeout: Some(Duration::from_secs(10)),
         };
 
         let exporter_result = LogExporter::builder()
@@ -500,4 +520,44 @@
             )
         }
     }
+
+    #[test]
+    fn test_priority_of_signal_env_over_generic_env_for_timeout() {
+        run_env_test(
+            vec![
+                (crate::OTEL_EXPORTER_OTLP_TRACES_TIMEOUT, "3000"),
+                (super::OTEL_EXPORTER_OTLP_TIMEOUT, "2000"),
+            ],
+            || {
+                let timeout =
+                    super::resolve_timeout(crate::OTEL_EXPORTER_OTLP_TRACES_TIMEOUT, None);
+                assert_eq!(timeout.as_millis(), 3000);
+            },
+        );
+    }
+
+    #[test]
+    fn test_priority_of_code_based_config_over_envs_for_timeout() {
+        run_env_test(
+            vec![
+                (crate::OTEL_EXPORTER_OTLP_TRACES_TIMEOUT, "3000"),
+                (super::OTEL_EXPORTER_OTLP_TIMEOUT, "2000"),
+            ],
+            || {
+                let timeout = super::resolve_timeout(
+                    crate::OTEL_EXPORTER_OTLP_TRACES_TIMEOUT,
+                    Some(&std::time::Duration::from_millis(1000)),
+                );
+                assert_eq!(timeout.as_millis(), 1000);
+            },
+        );
+    }
+
+    #[test]
+    fn test_use_default_when_others_missing_for_timeout() {
+        run_env_test(vec![], || {
+            let timeout = super::resolve_timeout(crate::OTEL_EXPORTER_OTLP_TRACES_TIMEOUT, None);
+            assert_eq!(timeout.as_millis(), 10_000);
+        });
+    }
 }