From 0a40eea37a7dbde5fc4760d6333d81186dfee255 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Wed, 9 Sep 2026 17:41:41 +0200 Subject: [PATCH] feat(otel): support standard OTEL resource attribute env vars (#10974) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(otel): pick up standard OTEL_RESOURCE_ATTRIBUTES on the exported resource The OTEL resource was built with `Resource::builder_empty()`, which runs no resource detectors, so attributes injected through the standard `OTEL_RESOURCE_ATTRIBUTES` env var were silently dropped. Deployments that inject `k8s.pod.uid`, `k8s.container.name` or `service.namespace` saw none of them reach their backend. Use `Resource::builder()`, which seeds from the SDK's env detector. Windmill's own attributes keep being applied on top, so per the OTel resource spec the env var is the secondary resource and `service.name`, `service.version`, `host.name` and `deployment.environment*` stay authoritative. The EE change lives in windmill-ee-private; this carries the ee-repo-ref bump and a regression test pinning both halves of the contract. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013yUJaBDHPgjZPodP2u4aqR * test(otel): clear OTEL_HOST_NAME so the resource test is hermetic OTEL_HOST_NAME takes precedence over the hostname argument, so an ambient one failed the host.name assertion with a message pointing at the merge logic. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013yUJaBDHPgjZPodP2u4aqR * feat(otel): honor OTEL_SERVICE_NAME and OTEL_SERVICE_VERSION Deployments identify each pod from its own labels, e.g. through the Kubernetes downward API, so `service.name` and `service.version` must be settable per pod. Both were ignored: OTEL_SERVICE_NAME was read by the SDK and then overwritten, and because the two attributes are set in code they also outrank OTEL_RESOURCE_ATTRIBUTES, leaving no route to set them at all. The EE change lives in windmill-ee-private; this carries the ee-repo-ref bump and tests for the dedicated overrides. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013yUJaBDHPgjZPodP2u4aqR * fix(otel): keep service.version pinned to the build version OTEL_SERVICE_VERSION is not an OTel env var, and service.version identifies the build that produced the telemetry, which a deployment cannot state more precisely than GIT_VERSION already does. A deployment that wants its own release version in telemetry can carry it under its own key. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013yUJaBDHPgjZPodP2u4aqR * test(otel): pin OTEL_SERVICE_NAME above service.name in OTEL_RESOURCE_ATTRIBUTES The spec ranks OTEL_SERVICE_NAME above a service.name carried in OTEL_RESOURCE_ATTRIBUTES; that ordering was only checked by hand. The three candidate values are distinct, so the assertions fail if either ranking breaks. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013yUJaBDHPgjZPodP2u4aqR * feat(otel): honor OTEL_SERVICE_VERSION The spec defines no OTEL_SERVICE_VERSION, but deployments set it expecting it to work because it sits next to OTEL_SERVICE_NAME, and setting service.version in code blocks the OTEL_RESOURCE_ATTRIBUTES route, so there is otherwise no way to set it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013yUJaBDHPgjZPodP2u4aqR * test(otel): guard against unknown_service on a default deployment Resource::builder seeds SdkProvidedResourceDetector, which sets service.name to "unknown_service" when neither OTEL_SERVICE_NAME nor a service.name in OTEL_RESOURCE_ATTRIBUTES is present. Only our own attribute keeps that out of the exported resource, and no assertion covered the case where nothing is set at all — which is the default deployment. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013yUJaBDHPgjZPodP2u4aqR * test(otel): pin the empty-means-unset fallback for OTEL_SERVICE_VERSION The empty case asserted the fallback for service.name and host.name but not service.version, leaving one branch of the three-variable contract uncovered. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013yUJaBDHPgjZPodP2u4aqR * chore: update ee-repo-ref to b964f0caaae57dc526c7ac9dc54d753372989f63 This commit updates the EE repository reference after PR #779 was merged in windmill-ee-private. Previous ee-repo-ref: 62efa909aabdba4cb31ffabe9aae0e4909ca1e07 New ee-repo-ref: b964f0caaae57dc526c7ac9dc54d753372989f63 Automated by sync-ee-ref workflow. --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: windmill-internal-app[bot] --- backend/ee-repo-ref.txt | 2 +- backend/tests/otel.rs | 118 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 1 deletion(-) diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 8f428844ad..51977e3cac 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -81edd1382d951265ab3e9b67fc7ca7967676fd56 +b964f0caaae57dc526c7ac9dc54d753372989f63 diff --git a/backend/tests/otel.rs b/backend/tests/otel.rs index 3a81f31021..1a4040d390 100644 --- a/backend/tests/otel.rs +++ b/backend/tests/otel.rs @@ -627,3 +627,121 @@ async fn test_root_job_span_relocated_to_inbound_trace() { expected_uuid_trace ); } + +// ═══════════════════════════════════════════════════════════════════════ +// RESOURCE ATTRIBUTES (OTEL_RESOURCE_ATTRIBUTES) +// ═══════════════════════════════════════════════════════════════════════ + +fn resource_attrs() -> std::collections::HashMap { + otlp_service_resource( + &windmill_common::utils::Mode::Worker, + "fallback-host", + "dev", + ) + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect() +} + +#[test] +#[serial_test::serial] +fn test_otlp_resource_merges_env_attributes_without_losing_windmill_identity() { + // These take precedence over the hostname argument and over OTEL_RESOURCE_ATTRIBUTES, + // so clear them or an ambient one fails the assertions below for an unrelated reason. + for var in [ + "OTEL_HOST_NAME", + "OTEL_SERVICE_NAME", + "OTEL_SERVICE_VERSION", + ] { + std::env::remove_var(var); + } + std::env::set_var( + "OTEL_RESOURCE_ATTRIBUTES", + "k8s.pod.uid=abc-123,service.name=injected,host.name=injected", + ); + let attrs = resource_attrs(); + std::env::remove_var("OTEL_RESOURCE_ATTRIBUTES"); + + // Attributes the deployment injects reach the exporters. + assert_eq!( + attrs.get("k8s.pod.uid").map(String::as_str), + Some("abc-123") + ); + // OTEL_RESOURCE_ATTRIBUTES is the secondary resource, so Windmill's own values still win. + assert_eq!( + attrs.get("service.name").map(String::as_str), + Some("windmill-worker") + ); + assert_eq!( + attrs.get("host.name").map(String::as_str), + Some("fallback-host") + ); +} + +#[test] +#[serial_test::serial] +fn test_otlp_resource_dedicated_overrides_win() { + // A deployment sets these per pod, e.g. from Kubernetes downward-API labels. The + // competing service.name must lose: the spec ranks OTEL_SERVICE_NAME above it. + std::env::set_var("OTEL_RESOURCE_ATTRIBUTES", "service.name=should-lose"); + std::env::set_var("OTEL_SERVICE_NAME", "windmill-workers"); + std::env::set_var("OTEL_SERVICE_VERSION", "1.802.0"); + std::env::set_var("OTEL_HOST_NAME", "pod-7"); + let overridden = resource_attrs(); + + // An empty value means unset, which is what the downward API yields for a missing label. + for var in [ + "OTEL_SERVICE_NAME", + "OTEL_SERVICE_VERSION", + "OTEL_HOST_NAME", + ] { + std::env::set_var(var, ""); + } + let empty = resource_attrs(); + for var in [ + "OTEL_SERVICE_NAME", + "OTEL_SERVICE_VERSION", + "OTEL_HOST_NAME", + "OTEL_RESOURCE_ATTRIBUTES", + ] { + std::env::remove_var(var); + } + let unset = resource_attrs(); + + assert_eq!( + overridden.get("service.name").map(String::as_str), + Some("windmill-workers") + ); + assert_eq!( + overridden.get("service.version").map(String::as_str), + Some("1.802.0") + ); + assert_eq!( + overridden.get("host.name").map(String::as_str), + Some("pod-7") + ); + + assert_eq!( + empty.get("service.name").map(String::as_str), + Some("windmill-worker") + ); + assert_eq!( + empty.get("host.name").map(String::as_str), + Some("fallback-host") + ); + assert_eq!( + empty.get("service.version").map(String::as_str), + Some(windmill_common::utils::GIT_VERSION) + ); + + // With nothing set at all — the default deployment — SdkProvidedResourceDetector still + // contributes service.name = "unknown_service". Ours has to overwrite it. + assert_eq!( + unset.get("service.name").map(String::as_str), + Some("windmill-worker") + ); + assert_eq!( + unset.get("service.version").map(String::as_str), + Some(windmill_common::utils::GIT_VERSION) + ); +}