fix: URLs in OTLP exporters (#1706)

TommyCpp · cijothomas · web-flow · commit 19d3223e247b · 2024-05-08T11:52:00.000-04:00
Co-authored-by: Cijo Thomas &lt;cijo.thomas@gmail.com&gt;
diff --git a/opentelemetry-otlp/CHANGELOG.md b/opentelemetry-otlp/CHANGELOG.md
@@ -5,6 +5,8 @@
 ### Fixed
 
 - URL encoded values in `OTEL_EXPORTER_OTLP_HEADERS` are now correctly decoded. [#1578](https://github.com/open-telemetry/opentelemetry-rust/pull/1578)
+- OTLP exporter will not change the URL added through `ExportConfig` [#1706](https://github.com/open-telemetry/opentelemetry-rust/pull/1706)
+- Default grpc endpoint will not have path based on signal(e.g `/v1/traces`) [#1706](https://github.com/open-telemetry/opentelemetry-rust/pull/1706)
 
 ### Added
 
diff --git a/opentelemetry-otlp/src/exporter/http/mod.rs b/opentelemetry-otlp/src/exporter/http/mod.rs
@@ -1,4 +1,7 @@
-use super::{default_headers, default_protocol, parse_header_string};
+use super::{
+    default_headers, default_protocol, parse_header_string,
+    OTEL_EXPORTER_OTLP_HTTP_ENDPOINT_DEFAULT,
+};
 use crate::{
     ExportConfig, Protocol, OTEL_EXPORTER_OTLP_ENDPOINT, OTEL_EXPORTER_OTLP_HEADERS,
     OTEL_EXPORTER_OTLP_TIMEOUT,
@@ -150,7 +153,7 @@ impl HttpExporterBuilder {
         signal_timeout_var: &str,
         signal_http_headers_var: &str,
     ) -> Result<OtlpHttpClient, crate::Error> {
-        let endpoint = resolve_endpoint(
+        let endpoint = resolve_http_endpoint(
             signal_endpoint_var,
             signal_endpoint_path,
             self.exporter_config.endpoint.as_str(),
@@ -373,10 +376,10 @@ fn build_endpoint_uri(endpoint: &str, path: &str) -> Result<Uri, crate::Error> {
 }
 
 // see https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#endpoint-urls-for-otlphttp
-fn resolve_endpoint(
+fn resolve_http_endpoint(
     signal_endpoint_var: &str,
     signal_endpoint_path: &str,
-    provided_or_default_endpoint: &str,
+    provided_endpoint: &str,
 ) -> Result<Uri, crate::Error> {
     // per signal env var is not modified
     if let Some(endpoint) = env::var(signal_endpoint_var)
@@ -394,8 +397,14 @@ 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)
+    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)
+    }
 }
 
 #[allow(clippy::mutable_key_type)] // http headers are not mutated
@@ -411,16 +420,19 @@ 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_http_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_http_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
                     "http://localhost:4317",
@@ -436,7 +448,7 @@ mod tests {
         run_env_test(
             vec![(OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, "http://example.com")],
             || {
-                let endpoint = super::resolve_endpoint(
+                let endpoint = super::resolve_http_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
                     "http://localhost:4317",
@@ -455,7 +467,7 @@ mod tests {
                 (OTEL_EXPORTER_OTLP_ENDPOINT, "http://wrong.com"),
             ],
             || {
-                let endpoint = super::resolve_endpoint(
+                let endpoint = super::resolve_http_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
                     "http://localhost:4317",
@@ -469,10 +481,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_http_endpoint(
+                "NON_EXISTENT_VAR",
+                "/v1/traces",
+                "http://localhost:4317",
+            )
+            .unwrap();
+            assert_eq!(endpoint, "http://localhost:4317/");
         });
     }
 
@@ -501,7 +516,7 @@ mod tests {
                 (OTEL_EXPORTER_OTLP_ENDPOINT, "http://example.com"),
             ],
             || {
-                let endpoint = super::resolve_endpoint(
+                let endpoint = super::resolve_http_endpoint(
                     OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                     "/v1/traces",
                     "http://localhost:4317",
@@ -515,7 +530,7 @@ mod tests {
     #[test]
     fn test_all_invalid_urls_falls_back_to_error() {
         run_env_test(vec![], || {
-            let result = super::resolve_endpoint(
+            let result = super::resolve_http_endpoint(
                 OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
                 "/v1/traces",
                 "-*/*-/*-//-/-/yet-another-invalid-uri",
@@ -610,4 +625,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_http_endpoint(
+                OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
+                "/v1/traces",
+                exporter.exporter_config.endpoint.as_str(),
+            )
+            .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_http_endpoint(
+                OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
+                "/v1/traces",
+                exporter.exporter_config.endpoint.as_str(),
+            )
+            .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,7 +70,8 @@ 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.
+    /// 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,
 
     /// The protocol to use when communicating with the collector.
@@ -84,7 +86,9 @@ impl Default for ExportConfig {
         let protocol = default_protocol();
 
         ExportConfig {
-            endpoint: default_endpoint(protocol),
+            endpoint: "".to_string(),
+            // don't use default_endpoint(protocol) here otherwise we
+            // won't know if user provided a value
             protocol,
             timeout: Duration::from_secs(OTEL_EXPORTER_OTLP_TIMEOUT_DEFAULT),
         }
@@ -128,15 +132,6 @@ fn default_protocol() -> Protocol {
     }
 }
 
-/// default endpoint for protocol
-fn default_endpoint(protocol: Protocol) -> String {
-    match protocol {
-        Protocol::Grpc => OTEL_EXPORTER_OTLP_GRPC_ENDPOINT_DEFAULT.to_string(),
-        Protocol::HttpBinary => OTEL_EXPORTER_OTLP_HTTP_ENDPOINT_DEFAULT.to_string(),
-        Protocol::HttpJson => OTEL_EXPORTER_OTLP_HTTP_ENDPOINT_DEFAULT.to_string(),
-    }
-}
-
 /// default user-agent headers
 #[cfg(any(feature = "grpc-tonic", feature = "http-proto", feature = "http-json"))]
 fn default_headers() -> std::collections::HashMap<String, String> {
@@ -183,7 +178,7 @@ impl HasExportConfig for HttpExporterBuilder {
 /// # }
 /// ```
 pub trait WithExportConfig {
-    /// Set the address of the OTLP collector. If not set, the default address is used.
+    /// Set the address of the OTLP collector. If not set or set to empty string, the default address is used.
     fn with_endpoint<T: Into<String>>(self, endpoint: T) -> Self;
     /// Set the protocol to use when communicating with the collector.
     ///
@@ -297,21 +292,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, "");
     }
 
     #[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, "");
     }
 
     #[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,
@@ -213,7 +212,6 @@ impl TonicExporterBuilder {
     fn build_channel(
         self,
         signal_endpoint_var: &str,
-        signal_endpoint_path: &str,
         signal_timeout_var: &str,
         signal_compression_var: &str,
         signal_headers_var: &str,
@@ -255,12 +253,24 @@ 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 => format!("{}{signal_endpoint_path}", config.endpoint),
+            None => {
+                if config.endpoint.is_empty() {
+                    OTEL_EXPORTER_OTLP_GRPC_ENDPOINT_DEFAULT.to_string()
+                } else {
+                    config.endpoint
+                }
+            }
         };
 
         let endpoint = Channel::from_shared(endpoint).map_err(crate::Error::from)?;
@@ -300,7 +310,6 @@ impl TonicExporterBuilder {
 
         let (channel, interceptor, compression) = self.build_channel(
             crate::logs::OTEL_EXPORTER_OTLP_LOGS_ENDPOINT,
-            "/v1/logs",
             crate::logs::OTEL_EXPORTER_OTLP_LOGS_TIMEOUT,
             crate::logs::OTEL_EXPORTER_OTLP_LOGS_COMPRESSION,
             crate::logs::OTEL_EXPORTER_OTLP_LOGS_HEADERS,
@@ -323,7 +332,6 @@ impl TonicExporterBuilder {
 
         let (channel, interceptor, compression) = self.build_channel(
             crate::metric::OTEL_EXPORTER_OTLP_METRICS_ENDPOINT,
-            "/v1/metrics",
             crate::metric::OTEL_EXPORTER_OTLP_METRICS_TIMEOUT,
             crate::metric::OTEL_EXPORTER_OTLP_METRICS_COMPRESSION,
             crate::metric::OTEL_EXPORTER_OTLP_METRICS_HEADERS,
@@ -347,7 +355,6 @@ impl TonicExporterBuilder {
 
         let (channel, interceptor, compression) = self.build_channel(
             crate::span::OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,
-            "/v1/traces",
             crate::span::OTEL_EXPORTER_OTLP_TRACES_TIMEOUT,
             crate::span::OTEL_EXPORTER_OTLP_TRACES_COMPRESSION,
             crate::span::OTEL_EXPORTER_OTLP_TRACES_HEADERS,
