fix(tracing): NICo's Go tier drops W3C trace context in five places #5517
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,13 @@ spec: | |
| value: /app/config.yaml | ||
| - name: TEMPORAL_NAMESPACE | ||
| value: site | ||
| {{- range $k, $v := .Values.extraEnv }} | ||
| {{- if has $k (list "CONFIG_FILE_PATH" "TEMPORAL_NAMESPACE") }} | ||
| {{- fail (printf "extraEnv must not override chart-managed variable %s" $k) }} | ||
| {{- end }} | ||
| - name: {{ $k }} | ||
| value: {{ $v | quote }} | ||
| {{- end }} | ||
|
Comment on lines
+47
to
+53
Contributor
There was a problem hiding this comment. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: printf '%s\n' '--- repository conventions ---'
head -5 /tmp/coderabbit-repo-knowledge/nvidia-infra-controller-80992b25/*/*.md 2>/dev/null || true
printf '%s\n' '--- target template ---'
cat -n helm/rest/nico-rest/charts/nico-rest-workflow/templates/deployment-site-worker.yaml | sed -n '35,65p'
printf '%s\n' '--- relevant values and template references ---'
rg -n --glob '*.yaml' --glob '*.yml' --glob '*.tpl' 'extraEnv|TEMPORAL_QUEUE|CONFIG_FILE_PATH|TEMPORAL_NAMESPACE' helm/rest/nico-rest/charts/nico-rest-workflow
printf '%s\n' '--- changed-file summary ---'
git diff --stat -- helm/rest/nico-rest/charts/nico-rest-workflow/templates/deployment-site-worker.yamlRepository: NVIDIA/infra-controller Length of output: 29451 🏁 Script executed: printf '%s\n' '--- applicable Helm convention ---'
cat /tmp/coderabbit-repo-knowledge/nvidia-infra-controller-80992b25/conventions/helm.md
printf '%s\n' '--- applicable Helm learnings ---'
cat /tmp/coderabbit-repo-knowledge/nvidia-infra-controller-80992b25/learnings/helm.md
printf '%s\n' '--- chart files and validation hooks ---'
git ls-files helm/rest/nico-rest/charts/nico-rest-workflow | sed -n '1,120p'
printf '%s\n' '--- sibling worker template ---'
cat -n helm/rest/nico-rest/charts/nico-rest-workflow/templates/deployment-cloud-worker.yaml | sed -n '38,60p'
printf '%s\n' '--- values and schema ---'
cat -n helm/rest/nico-rest/charts/nico-rest-workflow/values.yaml | sed -n '75,100p'
fd -i 'values.schema.json' helm/rest/nico-rest/charts/nico-rest-workflow || trueRepository: NVIDIA/infra-controller Length of output: 3160 🌐 Web query:
💡 Result: In Kubernetes, while environment variables within a container's env list are conceptually keyed by name, the system historically allowed the persistence of duplicate names in the spec [1][2]. However, defining duplicate environment variable names is strongly discouraged, as it causes significant issues with patching operations, such as those performed by kubectl apply [1][3][4]. Validation Behavior Historically, Kubernetes allowed duplicate environment variable names to be persisted [1][2]. Attempts to tighten validation have been made over time to prevent this behavior [5], but consistency can vary depending on the Kubernetes version and the specific mechanism used to create or update the resource. Because the environment variable name is treated as a merge key in strategic merge patches, the presence of duplicates creates ambiguity [1][2]. Specifically, operations that remove or update a single entry may inadvertently affect all entries with that duplicate name [2][4]. Precedence and Behavior 1. Precedence: When duplicate keys are present in the env list, the last defined value typically takes precedence because the list is processed sequentially [6][7][8]. However, relying on this behavior is unsafe due to the aforementioned tooling conflicts. 2. Tooling Conflicts: Using kubectl apply or other strategic merge patch-based tools on objects with duplicate environment variable names is known to be problematic [1][3][4]. Because the patch mechanism uses the variable name as a unique identifier, it may fail, remove all occurrences, or cause unpredictable behavior when attempting to manage these duplicates [2][3][4]. 3. Recommended Practice: The authoritative recommendation is to avoid duplicate environment variable names entirely [3][4]. If multiple definitions are needed, ensure unique naming, or use distinct sources (like separate ConfigMaps or Secrets) where the last source defined generally overrides previous ones [6][7]. Server-Side Apply (SSA) is generally better at handling these conflicts than client-side apply [3][4]. Citations:
Reject When 🤖 Prompt for AI AgentsSource: Path instructions |
||
| - name: TEMPORAL_QUEUE | ||
| value: site | ||
| volumeMounts: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| // Package tracing installs the process-wide OpenTelemetry propagator and exporter. | ||
| package tracing | ||
|
|
||
| import ( | ||
| "go.opentelemetry.io/otel" | ||
| "go.opentelemetry.io/otel/propagation" | ||
| ) | ||
|
|
||
| // InstallPropagator sets the global text-map propagator to W3C trace context | ||
| // plus baggage. Call it first in main, before anything captures the global. | ||
| func InstallPropagator() { | ||
| otel.SetTextMapPropagator(propagation.NewCompositeTextMapPropagator( | ||
| propagation.TraceContext{}, | ||
| propagation.Baggage{}, | ||
| )) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package tracing | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "os" | ||
|
|
||
| "github.com/rs/zerolog/log" | ||
| "go.opentelemetry.io/otel" | ||
| "go.opentelemetry.io/otel/attribute" | ||
| "go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc" | ||
| "go.opentelemetry.io/otel/sdk/resource" | ||
| sdktrace "go.opentelemetry.io/otel/sdk/trace" | ||
| ) | ||
|
|
||
| // InstallTracerProvider installs an OTLP exporter, or a no-op when | ||
| // OTEL_EXPORTER_OTLP_ENDPOINT is unset. Returns its shutdown func. | ||
| func InstallTracerProvider(ctx context.Context, serviceName string) (func(context.Context) error, error) { | ||
| noop := func(context.Context) error { return nil } | ||
| if os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") == "" && | ||
| os.Getenv("OTEL_EXPORTER_OTLP_TRACES_ENDPOINT") == "" { | ||
| return noop, nil | ||
| } | ||
|
|
||
| exp, err := otlptracegrpc.New(ctx) | ||
| if err != nil { | ||
| return noop, fmt.Errorf("creating OTLP trace exporter: %w", err) | ||
| } | ||
|
|
||
| res, err := resource.Merge(resource.Default(), | ||
| resource.NewWithAttributes(resource.Default().SchemaURL(), | ||
| attribute.String("service.name", serviceName))) | ||
| if err != nil { | ||
| return noop, fmt.Errorf("building trace resource for %s: %w", serviceName, err) | ||
| } | ||
|
|
||
| tp := sdktrace.NewTracerProvider(sdktrace.WithBatcher(exp), sdktrace.WithResource(res)) | ||
| otel.SetTracerProvider(tp) | ||
| return tp.Shutdown, nil | ||
| } | ||
|
|
||
| // InstallExporter is InstallTracerProvider for a main(): one line, errors logged. | ||
| func InstallExporter(serviceName string) func() { | ||
| shutdown, err := InstallTracerProvider(context.Background(), serviceName) | ||
| if err != nil { | ||
| log.Warn().Err(err).Str("service", serviceName). | ||
| Msg("tracing: no TracerProvider installed; spans will not be exported") | ||
| return func() {} | ||
| } | ||
| return func() { _ = shutdown(context.Background()) } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,9 +11,15 @@ import ( | |
| "github.com/NVIDIA/infra-controller/rest-api/cert-manager/pkg/core" | ||
| "github.com/NVIDIA/infra-controller/rest-api/site-manager/pkg/sitemgr" | ||
| cli "github.com/urfave/cli/v2" | ||
|
|
||
| "github.com/NVIDIA/infra-controller/rest-api/common/pkg/tracing" | ||
|
There was a problem hiding this comment.
When the documented AGENTS.md reference: rest-api/AGENTS.md:L81-L85 Useful? React with 👍 / 👎.
Author
There was a problem hiding this comment. Correct. I updated Fixed in a96bd1d. |
||
| ) | ||
|
|
||
| func main() { | ||
| // First: interceptors and handlers below capture the global propagator. | ||
| tracing.InstallPropagator() | ||
| // No-op unless OTEL_EXPORTER_OTLP_ENDPOINT is set. | ||
| defer tracing.InstallExporter("site-manager")() | ||
| cmd := sitemgr.NewCommand() | ||
| app := &cli.App{ | ||
| Name: cmd.Name, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.