feat: make endpoint resolution responsibility more explicit

Author: pitoniak32

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

@@ -127,7 +127,7 @@ impl HttpExporterBuilder {
         let endpoint = resolve_http_endpoint(
             signal_endpoint_var,
             signal_endpoint_path,
-            self.exporter_config.endpoint.as_str(),
+            self.exporter_config.endpoint.clone(),
         )?;
 
         let timeout = match env::var(signal_timeout_var)
@@ -340,7 +340,7 @@ fn build_endpoint_uri(endpoint: &str, path: &str) -> Result<Uri, crate::Error> {
 fn resolve_http_endpoint(
     signal_endpoint_var: &str,
     signal_endpoint_path: &str,
-    provided_endpoint: &str,
+    provided_endpoint: Option<String>,
 ) -> Result<Uri, crate::Error> {
     // per signal env var is not modified
     if let Some(endpoint) = env::var(signal_endpoint_var)
@@ -358,14 +358,14 @@ fn resolve_http_endpoint(
         return Ok(endpoint);
     }
 
-    if provided_endpoint.is_empty() {
-        build_endpoint_uri(
-            OTEL_EXPORTER_OTLP_HTTP_ENDPOINT_DEFAULT,
-            signal_endpoint_path,
-        )
-    } else {
-        provided_endpoint.parse().map_err(From::from)
-    }
+    provided_endpoint
+        .map(|e| e.parse().map_err(From::from))
+        .unwrap_or_else(|| {
+            build_endpoint_uri(
+                OTEL_EXPORTER_OTLP_HTTP_ENDPOINT_DEFAULT,
+                signal_endpoint_path,
+            )
+        })
 }
 
 #[allow(clippy::mutable_key_type)] // http headers are not mutated
@@ -449,7 +449,7 @@ mod tests {
                 let endpoint = resolve_http_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
-                    "http://localhost:4317",
+                    Some("http://localhost:4317".to_string()),
                 )
                 .unwrap();
                 assert_eq!(endpoint, "http://example.com/v1/traces");
@@ -465,7 +465,7 @@ mod tests {
                 let endpoint = super::resolve_http_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
-                    "http://localhost:4317",
+                    Some("http://localhost:4317".to_string()),
                 )
                 .unwrap();
                 assert_eq!(endpoint, "http://example.com");
@@ -484,7 +484,7 @@ mod tests {
                 let endpoint = super::resolve_http_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
-                    "http://localhost:4317",
+                    Some("http://localhost:4317".to_string()),
                 )
                 .unwrap();
                 assert_eq!(endpoint, "http://example.com");
@@ -498,7 +498,7 @@ mod tests {
             let endpoint = super::resolve_http_endpoint(
                 "NON_EXISTENT_VAR",
                 "/v1/traces",
-                "http://localhost:4317",
+                Some("http://localhost:4317".to_string()),
             )
             .unwrap();
             assert_eq!(endpoint, "http://localhost:4317/");
@@ -533,7 +533,7 @@ mod tests {
                 let endpoint = super::resolve_http_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
-                    "http://localhost:4317",
+                    Some("http://localhost:4317".to_string()),
                 )
                 .unwrap();
                 assert_eq!(endpoint, "http://example.com/v1/traces");
@@ -547,7 +547,7 @@ mod tests {
             let result = super::resolve_http_endpoint(
                 OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                 "/v1/traces",
-                "-*/*-/*-//-/-/yet-another-invalid-uri",
+                Some("-*/*-/*-//-/-/yet-another-invalid-uri".to_string()),
             );
             assert!(result.is_err());
             // You may also want to assert on the specific error type if applicable
@@ -644,8 +644,8 @@ mod tests {
     fn test_http_exporter_builder_with_header() {
         use std::collections::HashMap;
         // Arrange
-        let initial_headers = [("k1".to_string(), "v1".to_string())];
-        let extra_headers = [("k2".to_string(), "v2".to_string())];
+        let initial_headers = HashMap::from([("k1".to_string(), "v1".to_string())]);
+        let extra_headers = HashMap::from([("k2".to_string(), "v2".to_string())]);
         let expected_headers = initial_headers.iter().chain(extra_headers.iter()).fold(
             HashMap::new(),
             |mut acc, (k, v)| {
@@ -656,13 +656,13 @@ mod tests {
         let builder = HttpExporterBuilder {
             http_config: HttpConfig {
                 client: None,
-                headers: Some(HashMap::from([("k1".to_string(), "v1".to_string())])),
+                headers: Some(initial_headers),
             },
             exporter_config: crate::ExportConfig::default(),
         };
 
         // Act
-        let builder = builder.with_headers(HashMap::from([("k2".to_string(), "v2".to_string())]));
+        let builder = builder.with_headers(extra_headers);
 
         // Assert
         assert_eq!(
@@ -684,7 +684,7 @@ mod tests {
             let url = resolve_http_endpoint(
                 OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                 "/v1/traces",
-                exporter.exporter_config.endpoint.as_str(),
+                exporter.exporter_config.endpoint,
             )
             .unwrap();
 
@@ -699,7 +699,7 @@ mod tests {
             let url = resolve_http_endpoint(
                 OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                 "/v1/traces",
-                exporter.exporter_config.endpoint.as_str(),
+                exporter.exporter_config.endpoint,
             )
             .unwrap();
 

opentelemetry-otlp/src/exporter/mod.rs

@@ -69,7 +69,7 @@ pub(crate) mod tonic;
 pub struct ExportConfig {
     /// The address of the OTLP collector. If it's not provided via builder or environment variables.
     /// Default address will be used based on the protocol.
-    pub endpoint: String,
+    pub endpoint: Option<String>,
 
     /// The protocol to use when communicating with the collector.
     pub protocol: Protocol,
@@ -83,7 +83,7 @@ impl Default for ExportConfig {
         let protocol = default_protocol();
 
         ExportConfig {
-            endpoint: "".to_string(),
+            endpoint: None,
             // don't use default_endpoint(protocol) here otherwise we
             // won't know if user provided a value
             protocol,
@@ -199,7 +199,7 @@ pub trait WithExportConfig {
 
 impl<B: HasExportConfig> WithExportConfig for B {
     fn with_endpoint<T: Into<String>>(mut self, endpoint: T) -> Self {
-        self.export_config().endpoint = endpoint.into();
+        self.export_config().endpoint = Some(endpoint.into());
         self
     }
 
@@ -295,15 +295,15 @@ mod tests {
     fn test_default_http_endpoint() {
         let exporter_builder = crate::HttpExporterBuilder::default();
 
-        assert_eq!(exporter_builder.exporter_config.endpoint, "");
+        assert_eq!(exporter_builder.exporter_config.endpoint, None);
     }
 
     #[cfg(feature = "grpc-tonic")]
     #[test]
     fn test_default_tonic_endpoint() {
         let exporter_builder = crate::TonicExporterBuilder::default();
 
-        assert_eq!(exporter_builder.exporter_config.endpoint, "");
+        assert_eq!(exporter_builder.exporter_config.endpoint, None);
     }
 
     #[test]

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

@@ -188,24 +188,7 @@ impl TonicExporterBuilder {
 
         let config = self.exporter_config;
 
-        // resolving endpoint string
-        // grpc doesn't have a "path" like http(See https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md)
-        // the path of grpc calls are based on the protobuf service definition
-        // so we won't append one for default grpc endpoints
-        // If users for some reason want to use a custom path, they can use env var or builder to pass it
-        let endpoint = match env::var(signal_endpoint_var)
-            .ok()
-            .or(env::var(OTEL_EXPORTER_OTLP_ENDPOINT).ok())
-        {
-            Some(val) => val,
-            None => {
-                if config.endpoint.is_empty() {
-                    OTEL_EXPORTER_OTLP_GRPC_ENDPOINT_DEFAULT.to_string()
-                } else {
-                    config.endpoint
-                }
-            }
-        };
+        let endpoint = Self::resolve_endpoint(signal_endpoint_var, config.endpoint);
 
         let endpoint = Channel::from_shared(endpoint).map_err(crate::Error::from)?;
         let timeout = match env::var(signal_timeout_var)
@@ -235,6 +218,23 @@ impl TonicExporterBuilder {
         Ok((channel, interceptor, compression))
     }
 
+    fn resolve_endpoint(default_endpoint_var: &str, provided_endpoint: Option<String>) -> String {
+        // resolving endpoint string
+        // grpc doesn't have a "path" like http(See https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md)
+        // the path of grpc calls are based on the protobuf service definition
+        // so we won't append one for default grpc endpoints
+        // If users for some reason want to use a custom path, they can use env var or builder to pass it
+        match env::var(default_endpoint_var)
+            .ok()
+            .or(env::var(OTEL_EXPORTER_OTLP_ENDPOINT).ok())
+        {
+            Some(val) => val,
+            None => {
+                provided_endpoint.unwrap_or(OTEL_EXPORTER_OTLP_GRPC_ENDPOINT_DEFAULT.to_string())
+            }
+        }
+    }
+
     fn resolve_compression(
         &self,
         env_override: &str,
@@ -441,7 +441,7 @@ mod tests {
     use crate::exporter::tonic::WithTonicConfig;
     #[cfg(feature = "grpc-tonic")]
     use crate::exporter::Compression;
-    use crate::TonicExporterBuilder;
+    use crate::{TonicExporterBuilder, WithExportConfig, OTEL_EXPORTER_OTLP_TRACES_ENDPOINT};
     use crate::{OTEL_EXPORTER_OTLP_HEADERS, OTEL_EXPORTER_OTLP_TRACES_HEADERS};
     use http::{HeaderMap, HeaderName, HeaderValue};
     use tonic::metadata::{MetadataMap, MetadataValue};
@@ -564,4 +564,31 @@ mod tests {
             },
         );
     }
+
+    #[test]
+    fn test_tonic_exporter_endpoint() {
+        // default endpoint for grpc should not add signal path.
+        run_env_test(vec![], || {
+            let exporter = TonicExporterBuilder::default();
+
+            let url = TonicExporterBuilder::resolve_endpoint(
+                OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
+                exporter.exporter_config.endpoint,
+            );
+
+            assert_eq!(url, "http://localhost:4317");
+        });
+
+        // if builder endpoint is set, it should not use default.
+        run_env_test(vec![], || {
+            let exporter = TonicExporterBuilder::default().with_endpoint("http://localhost:1234");
+
+            let url = TonicExporterBuilder::resolve_endpoint(
+                OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
+                exporter.exporter_config.endpoint,
+            );
+
+            assert_eq!(url, "http://localhost:1234");
+        });
+    }
 }