Merge branch 'main' into validate-baggage

Author: cijothomas

.github/workflows/benchmark.yml

@@ -0,0 +1,54 @@
+# This workflow runs a Criterion benchmark on a PR and compares the results against the base branch.
+# It is triggered on a PR or a push to main.
+#
+# The workflow is gated on the presence of the "performance" label on the PR.
+#
+# The workflow runs on a self-hosted runner pool. We can't use the shared runners for this,
+# because they are only permitted to run on the default branch to preserve resources. 
+#
+# In the future, we might like to consider using bencher.dev or the framework used by otel-golang here. 
+on: 
+  pull_request:
+  push:
+    branches:
+      - main
+name: benchmark pull requests
+jobs:
+  runBenchmark:
+    name: run benchmark
+    permissions:
+      pull-requests: write
+
+    # If we're running on a PR, use ubuntu-latest - a shared runner. We can't use the self-hosted
+    # runners on arbitrary PRs, and we don't want to unleash that load on the pool anyway.     
+    # If we're running on main, use the OTEL self-hosted runner pool. 
+    runs-on: ${{ github.event_name == 'pull_request' && 'ubuntu-latest' || 'self-hosted' }}
+    if: ${{ (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'performance')) || github.event_name == 'push' }}
+    env:
+      # For PRs, compare against the base branch - e.g., 'main'. 
+      # For pushes to main, compare against the previous commit
+      BRANCH_NAME: ${{ github.event_name == 'pull_request' && github.base_ref || github.event.before }}
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          fetch-depth: 10  # Fetch current commit and its parent
+      - uses: arduino/setup-protoc@v3
+        with:
+          repo-token: ${{ secrets.GITHUB_TOKEN }}
+      - uses: dtolnay/rust-toolchain@master
+        with:
+          toolchain: stable
+      - uses: boa-dev/criterion-compare-action@v3
+        with:
+          cwd: opentelemetry
+          branchName: ${{ env.BRANCH_NAME }}
+      - uses: boa-dev/criterion-compare-action@v3
+        with:
+          cwd: opentelemetry-appender-tracing
+          features: spec_unstable_logs_enabled
+          branchName: ${{ env.BRANCH_NAME }}
+      - uses: boa-dev/criterion-compare-action@v3
+        with:
+          cwd: opentelemetry-sdk
+          features: rt-tokio,testing,metrics,logs,spec_unstable_metrics_views
+          branchName: ${{ env.BRANCH_NAME }}

.github/workflows/pr_criterion.yaml

@@ -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/CHANGELOG.md

@@ -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();