refactor: prioritize code-based config over env vars in OTLP exporter (#2827)

Co-authored-by: Lalit Kumar Bhasin <[email protected]>
Co-authored-by: Cijo Thomas <[email protected]>
Co-authored-by: Cijo Thomas <[email protected]>

Author: 3 people

opentelemetry-otlp/CHANGELOG.md

@@ -17,6 +17,12 @@
  `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(`Option<Duration>`)
+- **Breaking** Export configuration done via code is final. ENV variables cannot be used to override the code config.
+  Do not use code based config, if there is desire to control the settings via ENV variables.
+  List of ENV variables and corresponding setting being affected by this change.
+  - `OTEL_EXPORTER_OTLP_ENDPOINT` -> `ExportConfig.endpoint`
+  - `OTEL_EXPORTER_OTLP_TIMEOUT` -> `ExportConfig.timeout`
 
 ## 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 @@ impl HttpExporterBuilder {
         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 build_endpoint_uri(endpoint: &str, path: &str) -> Result<Uri, ExporterBuildEr
 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 @@ mod tests {
         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 @@ mod tests {
     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 @@ mod tests {
                 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 @@ mod tests {
                 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 @@ mod tests {
             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 @@ mod tests {
             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 @@ mod tests {
             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

@@ -68,27 +68,32 @@ pub(crate) mod tonic;
 /// Configuration for the OTLP exporter.
 #[derive(Debug)]
 pub struct ExportConfig {
-    /// The address of the OTLP collector. If it's not provided via builder or environment variables.
+    /// The address of the OTLP collector.
     /// Default address will be used based on the protocol.
+    ///
+    /// Note: Programmatically setting this will override any value set via the environment variable.
     pub endpoint: Option<String>,
 
     /// The protocol to use when communicating with the collector.
     pub protocol: Protocol,
 
     /// The timeout to the collector.
-    pub timeout: Duration,
+    /// The default value is 10 seconds.
+    ///
+    /// Note: Programmatically setting this will override any value set via the environment variable.
+    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,
         }
     }
 }
@@ -223,18 +228,24 @@ impl HasExportConfig for HttpExporterBuilder {
 /// ```
 pub trait WithExportConfig {
     /// Set the address of the OTLP collector. If not set or set to empty string, the default address is used.
+    ///
+    /// Note: Programmatically setting this will override any value set via the environment variable.
     fn with_endpoint<T: Into<String>>(self, endpoint: T) -> Self;
     /// Set the protocol to use when communicating with the collector.
     ///
     /// Note that protocols that are not supported by exporters will be ignored. The exporter
     /// will use default protocol in this case.
     ///
     /// ## Note
-    /// All exporters in this crate only support one protocol, thus choosing the protocol is an no-op at the moment.
+    /// All exporters in this crate only support one protocol, thus choosing the protocol is a no-op at the moment.
     fn with_protocol(self, protocol: Protocol) -> Self;
     /// Set the timeout to the collector.
+    ///
+    /// Note: Programmatically setting this will override any value set via the environment variable.
     fn with_timeout(self, timeout: Duration) -> Self;
-    /// Set export config. This will override all previous configuration.
+    /// Set export config. This will override all previous configurations.
+    ///
+    /// Note: Programmatically setting this will override any value set via environment variables.
     fn with_export_config(self, export_config: ExportConfig) -> Self;
 }
 
@@ -250,7 +261,7 @@ impl<B: HasExportConfig> WithExportConfig for B {
     }
 
     fn with_timeout(mut self, timeout: Duration) -> Self {
-        self.export_config().timeout = timeout;
+        self.export_config().timeout = Some(timeout);
         self
     }
 
@@ -262,6 +273,28 @@ impl<B: HasExportConfig> WithExportConfig for B {
     }
 }
 
+#[cfg(any(feature = "grpc-tonic", feature = "http-proto", feature = "http-json"))]
+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) = std::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) = std::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 +375,6 @@ mod tests {
     #[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 +383,7 @@ mod tests {
         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 +403,6 @@ mod tests {
 
     #[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 +411,7 @@ mod tests {
         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 +531,44 @@ mod tests {
             )
         }
     }
+
+    #[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);
+        });
+    }
 }