diff --git a/opentelemetry-otlp/CHANGELOG.md b/opentelemetry-otlp/CHANGELOG.md index 9ad9f267ac..2df1611562 100644 --- a/opentelemetry-otlp/CHANGELOG.md +++ b/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`) +- **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 diff --git a/opentelemetry-otlp/src/exporter/http/mod.rs b/opentelemetry-otlp/src/exporter/http/mod.rs index 6f874d4856..77eb5cefb2 100644 --- a/opentelemetry-otlp/src/exporter/http/mod.rs +++ b/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, + provided_endpoint: Option<&str>, ) -> Result { - // 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(); diff --git a/opentelemetry-otlp/src/exporter/mod.rs b/opentelemetry-otlp/src/exporter/mod.rs index 2472290134..28676e4566 100644 --- a/opentelemetry-otlp/src/exporter/mod.rs +++ b/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, /// 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, } 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,6 +228,8 @@ 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>(self, endpoint: T) -> Self; /// Set the protocol to use when communicating with the collector. /// @@ -230,11 +237,15 @@ pub trait WithExportConfig { /// 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 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 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 { 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); + }); + } } diff --git a/opentelemetry-otlp/src/exporter/tonic/mod.rs b/opentelemetry-otlp/src/exporter/tonic/mod.rs index a69180e003..776c2d904a 100644 --- a/opentelemetry-otlp/src/exporter/tonic/mod.rs +++ b/opentelemetry-otlp/src/exporter/tonic/mod.rs @@ -1,7 +1,6 @@ use std::env; use std::fmt::{Debug, Formatter}; use std::str::FromStr; -use std::time::Duration; use http::{HeaderMap, HeaderName, HeaderValue}; use opentelemetry::otel_debug; @@ -12,12 +11,12 @@ use tonic::transport::Channel; #[cfg(feature = "tls")] use tonic::transport::ClientTlsConfig; -use super::ExporterBuildError; use super::{default_headers, parse_header_string, OTEL_EXPORTER_OTLP_GRPC_ENDPOINT_DEFAULT}; +use super::{resolve_timeout, ExporterBuildError}; use crate::exporter::Compression; use crate::{ ExportConfig, OTEL_EXPORTER_OTLP_COMPRESSION, OTEL_EXPORTER_OTLP_ENDPOINT, - OTEL_EXPORTER_OTLP_HEADERS, OTEL_EXPORTER_OTLP_TIMEOUT, + OTEL_EXPORTER_OTLP_HEADERS, }; #[cfg(feature = "logs")] @@ -197,16 +196,7 @@ impl TonicExporterBuilder { let endpoint = Channel::from_shared(endpoint) .map_err(|op| ExporterBuildError::InvalidUri(endpoint_clone.clone(), op.to_string()))?; - 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(_) => config.timeout, - }, - None => config.timeout, - }; + let timeout = resolve_timeout(signal_timeout_var, config.timeout.as_ref()); #[cfg(feature = "tls")] let channel = match self.tonic_config.tls_config { @@ -231,14 +221,16 @@ impl TonicExporterBuilder { // 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()) - } + // + // programmatic configuration overrides any value set via environment variables + if let Some(endpoint) = provided_endpoint { + endpoint + } else if let Ok(endpoint) = env::var(default_endpoint_var) { + endpoint + } else if let Ok(endpoint) = env::var(OTEL_EXPORTER_OTLP_ENDPOINT) { + endpoint + } else { + OTEL_EXPORTER_OTLP_GRPC_ENDPOINT_DEFAULT.to_string() } } @@ -456,7 +448,7 @@ mod tests { use crate::exporter::tonic::WithTonicConfig; #[cfg(feature = "grpc-tonic")] use crate::exporter::Compression; - use crate::{TonicExporterBuilder, WithExportConfig, OTEL_EXPORTER_OTLP_TRACES_ENDPOINT}; + use crate::{TonicExporterBuilder, 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}; @@ -522,6 +514,54 @@ mod tests { assert!(tonic::codec::CompressionEncoding::try_from(Compression::Zstd).is_err()); } + #[test] + fn test_priority_of_signal_env_over_generic_env_for_compression() { + run_env_test( + vec![ + (crate::OTEL_EXPORTER_OTLP_TRACES_COMPRESSION, "zstd"), + (super::OTEL_EXPORTER_OTLP_COMPRESSION, "gzip"), + ], + || { + let builder = TonicExporterBuilder::default(); + + let compression = builder + .resolve_compression(crate::OTEL_EXPORTER_OTLP_TRACES_COMPRESSION) + .unwrap(); + assert_eq!(compression, Some(tonic::codec::CompressionEncoding::Zstd)); + }, + ); + } + + #[test] + fn test_priority_of_code_based_config_over_envs_for_compression() { + run_env_test( + vec![ + (crate::OTEL_EXPORTER_OTLP_TRACES_COMPRESSION, "gzip"), + (super::OTEL_EXPORTER_OTLP_COMPRESSION, "gzip"), + ], + || { + let builder = TonicExporterBuilder::default().with_compression(Compression::Zstd); + + let compression = builder + .resolve_compression(crate::OTEL_EXPORTER_OTLP_TRACES_COMPRESSION) + .unwrap(); + assert_eq!(compression, Some(tonic::codec::CompressionEncoding::Zstd)); + }, + ); + } + + #[test] + fn test_use_default_when_others_missing_for_compression() { + run_env_test(vec![], || { + let builder = TonicExporterBuilder::default(); + + let compression = builder + .resolve_compression(crate::OTEL_EXPORTER_OTLP_TRACES_COMPRESSION) + .unwrap(); + assert!(compression.is_none()); + }); + } + #[test] fn test_parse_headers_from_env() { run_env_test( @@ -581,29 +621,45 @@ 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, - ); + fn test_priority_of_signal_env_over_generic_env_for_endpoint() { + run_env_test( + vec![ + (OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, "http://localhost:1234"), + (super::OTEL_EXPORTER_OTLP_ENDPOINT, "http://localhost:2345"), + ], + || { + let url = TonicExporterBuilder::resolve_endpoint( + OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, + None, + ); + assert_eq!(url, "http://localhost:1234"); + }, + ); + } - assert_eq!(url, "http://localhost:4317"); - }); + #[test] + fn test_priority_of_code_based_config_over_envs_for_endpoint() { + run_env_test( + vec![ + (OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, "http://localhost:1234"), + (super::OTEL_EXPORTER_OTLP_ENDPOINT, "http://localhost:2345"), + ], + || { + let url = TonicExporterBuilder::resolve_endpoint( + OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, + Some("http://localhost:3456".to_string()), + ); + assert_eq!(url, "http://localhost:3456"); + }, + ); + } - // if builder endpoint is set, it should not use default. + #[test] + fn test_use_default_when_others_missing_for_endpoint() { 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"); + let url = + TonicExporterBuilder::resolve_endpoint(OTEL_EXPORTER_OTLP_TRACES_ENDPOINT, None); + assert_eq!(url, "http://localhost:4317"); }); } }