Fix open-telemetry#1618

TommyCpp · TommyCpp · commit 5ca76f0690cb · 2024-05-05T19:55:24.000-07:00
diff --git a/opentelemetry-otlp/examples/basic-otlp-http/src/main.rs b/opentelemetry-otlp/examples/basic-otlp-http/src/main.rs
@@ -46,7 +46,7 @@ fn init_tracer() -> Result<sdktrace::Tracer, TraceError> {
 
 fn init_metrics() -> Result<(), MetricsError> {
     let export_config = opentelemetry_otlp::ExportConfig {
-        endpoint: "http://localhost:4318/v1/metrics".to_string(),
+        endpoint: Some("http://localhost:4318/v1/metrics".to_string()),
         ..opentelemetry_otlp::ExportConfig::default()
     };
     let provider = opentelemetry_otlp::new_pipeline()
diff --git a/opentelemetry-otlp/examples/basic-otlp/src/main.rs b/opentelemetry-otlp/examples/basic-otlp/src/main.rs
@@ -33,7 +33,7 @@ fn init_tracer() -> Result<sdktrace::Tracer, TraceError> {
 
 fn init_metrics() -> Result<(), MetricsError> {
     let export_config = ExportConfig {
-        endpoint: "http://localhost:4317".to_string(),
+        endpoint: Some("http://localhost:4317".to_string()),
         ..ExportConfig::default()
     };
     let provider = opentelemetry_otlp::new_pipeline()
diff --git a/opentelemetry-otlp/src/exporter/http/mod.rs b/opentelemetry-otlp/src/exporter/http/mod.rs
@@ -20,6 +20,8 @@ use std::str::FromStr;
 use std::sync::{Arc, Mutex};
 use std::time::Duration;
 
+use super::{default_headers, parse_header_string, OTEL_EXPORTER_OTLP_HTTP_ENDPOINT_DEFAULT};
+
 #[cfg(feature = "metrics")]
 mod metrics;
 
@@ -153,7 +155,7 @@ impl HttpExporterBuilder {
         let endpoint = resolve_endpoint(
             signal_endpoint_var,
             signal_endpoint_path,
-            self.exporter_config.endpoint.as_str(),
+            self.exporter_config.endpoint.as_deref(),
         )?;
 
         let timeout = match env::var(signal_timeout_var)
@@ -376,7 +378,7 @@ fn build_endpoint_uri(endpoint: &str, path: &str) -> Result<Uri, crate::Error> {
 fn resolve_endpoint(
     signal_endpoint_var: &str,
     signal_endpoint_path: &str,
-    provided_or_default_endpoint: &str,
+    provided_endpoint: Option<&str>,
 ) -> Result<Uri, crate::Error> {
     // per signal env var is not modified
     if let Some(endpoint) = env::var(signal_endpoint_var)
@@ -394,8 +396,13 @@ fn resolve_endpoint(
         return Ok(endpoint);
     }
 
-    // if neither works, we use the one provided in pipeline. If user never provides one, we will use the default one
-    build_endpoint_uri(provided_or_default_endpoint, signal_endpoint_path)
+    match provided_endpoint {
+        Some(endpoint) => build_endpoint_uri(endpoint, ""),
+        None => build_endpoint_uri(
+            OTEL_EXPORTER_OTLP_HTTP_ENDPOINT_DEFAULT,
+            signal_endpoint_path,
+        ),
+    }
 }
 
 #[allow(clippy::mutable_key_type)] // http headers are not mutated
@@ -411,19 +418,22 @@ fn add_header_from_string(input: &str, headers: &mut HashMap<HeaderName, HeaderV
 #[cfg(test)]
 mod tests {
     use crate::exporter::tests::run_env_test;
-    use crate::{OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_TRACES_ENDPOINT};
+    use crate::{
+        new_exporter, WithExportConfig, OTEL_EXPORTER_OTLP_ENDPOINT,
+        OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
+    };
 
-    use super::build_endpoint_uri;
+    use super::{build_endpoint_uri, resolve_endpoint};
 
     #[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(
+                let endpoint = resolve_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
-                    "http://localhost:4317",
+                    Some("http://localhost:4317"),
                 )
                 .unwrap();
                 assert_eq!(endpoint, "http://example.com/v1/traces");
@@ -439,7 +449,7 @@ mod tests {
                 let endpoint = super::resolve_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
-                    "http://localhost:4317",
+                    Some("http://localhost:4317"),
                 )
                 .unwrap();
                 assert_eq!(endpoint, "http://example.com");
@@ -458,7 +468,7 @@ mod tests {
                 let endpoint = super::resolve_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
-                    "http://localhost:4317",
+                    Some("http://localhost:4317"),
                 )
                 .unwrap();
                 assert_eq!(endpoint, "http://example.com");
@@ -469,10 +479,13 @@ mod tests {
     #[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");
+            let endpoint = super::resolve_endpoint(
+                "NON_EXISTENT_VAR",
+                "/v1/traces",
+                Some("http://localhost:4317"),
+            )
+            .unwrap();
+            assert_eq!(endpoint, "http://localhost:4317/");
         });
     }
 
@@ -504,7 +517,7 @@ mod tests {
                 let endpoint = super::resolve_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
-                    "http://localhost:4317",
+                    Some("http://localhost:4317"),
                 )
                 .unwrap();
                 assert_eq!(endpoint, "http://example.com/v1/traces");
@@ -518,7 +531,7 @@ mod tests {
             let result = super::resolve_endpoint(
                 OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                 "/v1/traces",
-                "-*/*-/*-//-/-/yet-another-invalid-uri",
+                Some("-*/*-/*-//-/-/yet-another-invalid-uri"),
             );
             assert!(result.is_err());
             // You may also want to assert on the specific error type if applicable
@@ -610,4 +623,37 @@ mod tests {
             }
         }
     }
+
+    #[test]
+    fn test_http_exporter_endpoint() {
+        // default endpoint should add signal path
+        run_env_test(vec![], || {
+            let exporter = new_exporter().http();
+
+            let url = resolve_endpoint(
+                OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
+                "/v1/traces",
+                exporter.exporter_config.endpoint.as_deref(),
+            )
+            .unwrap();
+
+            assert_eq!(url, "http://localhost:4318/v1/traces");
+        });
+
+        // if builder endpoint is set, it should not add signal path
+        run_env_test(vec![], || {
+            let exporter = new_exporter()
+                .http()
+                .with_endpoint("http://localhost:4318/v1/tracesbutnotreally");
+
+            let url = resolve_endpoint(
+                OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
+                "/v1/traces",
+                exporter.exporter_config.endpoint.as_deref(),
+            )
+            .unwrap();
+
+            assert_eq!(url, "http://localhost:4318/v1/tracesbutnotreally");
+        });
+    }
 }
diff --git a/opentelemetry-otlp/src/exporter/mod.rs b/opentelemetry-otlp/src/exporter/mod.rs
@@ -58,6 +58,7 @@ pub const OTEL_EXPORTER_OTLP_TIMEOUT: &str = "OTEL_EXPORTER_OTLP_TIMEOUT";
 pub const OTEL_EXPORTER_OTLP_TIMEOUT_DEFAULT: u64 = 10;
 
 // Endpoints per protocol https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md
+#[cfg(feature = "grpc-tonic")]
 const OTEL_EXPORTER_OTLP_GRPC_ENDPOINT_DEFAULT: &str = "http://localhost:4317";
 const OTEL_EXPORTER_OTLP_HTTP_ENDPOINT_DEFAULT: &str = "http://localhost:4318";
 
@@ -69,8 +70,9 @@ pub(crate) mod tonic;
 /// Configuration for the OTLP exporter.
 #[derive(Debug)]
 pub struct ExportConfig {
-    /// The base address of the OTLP collector. If not set, the default address is used.
-    pub endpoint: String,
+    /// 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: Option<String>,
 
     /// The protocol to use when communicating with the collector.
     pub protocol: Protocol,
@@ -84,7 +86,7 @@ impl Default for ExportConfig {
         let protocol = default_protocol();
 
         ExportConfig {
-            endpoint: default_endpoint(protocol),
+            endpoint: None,
             protocol,
             timeout: Duration::from_secs(OTEL_EXPORTER_OTLP_TIMEOUT_DEFAULT),
         }
@@ -201,7 +203,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
     }
 
@@ -297,21 +299,15 @@ mod tests {
     fn test_default_http_endpoint() {
         let exporter_builder = crate::new_exporter().http();
 
-        assert_eq!(
-            exporter_builder.exporter_config.endpoint,
-            "http://localhost:4318"
-        );
+        assert_eq!(exporter_builder.exporter_config.endpoint, None,);
     }
 
     #[cfg(feature = "grpc-tonic")]
     #[test]
     fn test_default_tonic_endpoint() {
         let exporter_builder = crate::new_exporter().tonic();
 
-        assert_eq!(
-            exporter_builder.exporter_config.endpoint,
-            "http://localhost:4317"
-        );
+        assert_eq!(exporter_builder.exporter_config.endpoint, None);
     }
 
     #[test]
diff --git a/opentelemetry-otlp/src/exporter/tonic/mod.rs b/opentelemetry-otlp/src/exporter/tonic/mod.rs
@@ -11,7 +11,7 @@ use tonic::transport::Channel;
 #[cfg(feature = "tls")]
 use tonic::transport::ClientTlsConfig;
 
-use super::{default_headers, parse_header_string};
+use super::{default_headers, parse_header_string, OTEL_EXPORTER_OTLP_GRPC_ENDPOINT_DEFAULT};
 use crate::exporter::Compression;
 use crate::{
     ExportConfig, OTEL_EXPORTER_OTLP_COMPRESSION, OTEL_EXPORTER_OTLP_ENDPOINT,
@@ -149,7 +149,6 @@ impl Default for TonicExporterBuilder {
         TonicExporterBuilder {
             exporter_config: ExportConfig {
                 protocol: crate::Protocol::Grpc,
-                endpoint: crate::exporter::default_endpoint(crate::Protocol::Grpc),
                 ..Default::default()
             },
             tonic_config,
@@ -260,7 +259,12 @@ impl TonicExporterBuilder {
             .or(env::var(OTEL_EXPORTER_OTLP_ENDPOINT).ok())
         {
             Some(val) => val,
-            None => format!("{}{signal_endpoint_path}", config.endpoint),
+            None => format!(
+                "{}{signal_endpoint_path}",
+                config
+                    .endpoint
+                    .unwrap_or(OTEL_EXPORTER_OTLP_GRPC_ENDPOINT_DEFAULT.to_string())
+            ),
         };
 
         let endpoint = Channel::from_shared(endpoint).map_err(crate::Error::from)?;
diff --git a/opentelemetry-otlp/src/lib.rs b/opentelemetry-otlp/src/lib.rs
@@ -162,7 +162,7 @@
 //!     # #[cfg(all(feature = "metrics", feature = "grpc-tonic"))]
 //!     # {
 //!     let export_config = ExportConfig {
-//!         endpoint: "http://localhost:4317".to_string(),
+//!         endpoint: Some("http://localhost:4317".to_string()),
 //!         timeout: Duration::from_secs(3),
 //!         protocol: Protocol::Grpc
 //!     };
