Skip to content

tracing: configure SDK explicitly instead of mutating process env - #2298

Open
satyamg1620 wants to merge 2 commits into
llm-d:mainfrom
satyamg1620:trace-explicit-sdk-config
Open

tracing: configure SDK explicitly instead of mutating process env#2298
satyamg1620 wants to merge 2 commits into
llm-d:mainfrom
satyamg1620:trace-explicit-sdk-config

Conversation

@satyamg1620

@satyamg1620 satyamg1620 commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind bug

What this PR does / why we need it:

InitTracing applied its defaults by calling os.Setenv for OTEL_SERVICE_NAME and OTEL_EXPORTER_OTLP_ENDPOINT, relying on the SDK to read them back. Mutating the process environment affects any other code in the process that reads those variables, is not reentrant, and leaves tests interfering with each other — InitTracing had no test coverage at all as a result.

The resource is now built directly via resource.New. Detectors are applied in order and later ones win, so placing WithFromEnv last keeps OTEL_SERVICE_NAME and OTEL_RESOURCE_ATTRIBUTES overriding the built-in service name.

This also fixes trace attribution. The previous resource was built with resource.NewWithAttributes, which never consults the environment, so neither service.name nor the chart's OTEL_RESOURCE_ATTRIBUTES (k8s.namespace.name, k8s.node.name, k8s.pod.name) ever reached a span — the os.Setenv calls had no effect on the exported resource. Spans now carry them.

Three implementation notes:

  • The plaintext loopback default is pinned, conditionally. The SDK defaults the gRPC endpoint to localhost:4317 but negotiates TLS for it, and tracing: honor OTLP TLS and auth environment variables #2312 removed the hardcoded WithInsecure. Simply dropping the old default therefore pointed the local development path at TLS against a plaintext collector. localCollectorOptions supplies the endpoint and the insecure transport only when the environment sets none of the endpoint, insecure or certificate variables; as soon as one is set the options are dropped, so an operator's configuration is never overridden.

  • A partial resource no longer aborts startup. resource.New reports a malformed OTEL_RESOURCE_ATTRIBUTES entry alongside a resource holding everything it could parse. Returning that error made a single stray comma in a chart value fail InitTracing, which the EPP runner turns into a process exit. The error is logged and the degraded resource is used.

  • resource.Default() is not merged in. It carries semconv v1.41.0 while this package uses v1.37.0, and merging conflicting schema URLs drops the schema URL from the result. Aligning the semconv version is a separate change; the reason is recorded in a comment so it is not re-added by accident.

Which issue(s) this PR fixes:

Fixes #2297

Part of #1632.

Test plan:

  • service.name defaults to the caller-supplied name when the environment is empty
  • OTEL_SERVICE_NAME and OTEL_RESOURCE_ATTRIBUTES override that default, with OTEL_SERVICE_NAME winning between the two
  • Chart-injected attributes land alongside the defaults, and the schema URL is preserved
  • InitTracing leaves every OTEL_* variable untouched in the environment
  • A malformed OTEL_RESOURCE_ATTRIBUTES entry keeps the parseable attributes and lets initialization succeed
  • With no transport variables set, the exporter reaches a plaintext collector on the loopback default
  • An operator's endpoint, insecure flag and certificate still decide the transport (existing OTLP coverage from tracing: honor OTLP TLS and auth environment variables #2312 unchanged)
  • Tests pass with OTEL_SERVICE_NAME, OTEL_RESOURCE_ATTRIBUTES and OTEL_EXPORTER_OTLP_ENDPOINT already set in the environment
  • Verified on a kind dev stack with an OpenTelemetry collector sidecar on the pod loopback: spans arrive carrying service.name=llm-d-epp with no OTEL_SERVICE_NAME set, plus k8s.namespace.name, k8s.node.name and k8s.pod.name, at schema URL 1.37.0

Release note (write NONE if no user-facing change):

Spans exported by EPP and the disaggregated sidecar now carry `service.name` and the attributes from `OTEL_RESOURCE_ATTRIBUTES`, including the `k8s.namespace.name`, `k8s.node.name` and `k8s.pod.name` values set by the Helm chart. These were previously never applied to the exported resource, so spans reached the collector without service identity.

@satyamg1620
satyamg1620 requested a review from a team as a code owner August 6, 2026 08:28
@satyamg1620
satyamg1620 requested review from ahg-g and elevran August 6, 2026 08:28
@github-actions github-actions Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. kind/cleanup and removed kind/cleanup labels Aug 6, 2026
mwaykole pushed a commit to mwaykole/llm-d-inference-scheduler that referenced this pull request Aug 6, 2026
* rename epp standalone to standalone

* fixed typo

* fixed name

* fixed typo

_, ok = os.LookupEnv("OTEL_EXPORTER_OTLP_ENDPOINT")
if !ok {
os.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "http://localhost:4317")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this default value applied now?

@satyamg1620 satyamg1620 Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ahg-g for the review.

The SDK supplies the endpoint: NewGRPCConfig seeds localhost:4317 before it applies env config or explicit options (options.go#L115-L131). But it negotiates TLS for that default, and #2312 removed the hardcoded WithInsecure — so the http:// scheme in the old os.Setenv value was what selected plaintext. Dropping it outright broke the local collector path: tls: first record does not look like a TLS handshake, 0 spans delivered.

Pushed a fix that pins the endpoint and the insecure transport, but only when the environment sets none of the endpoint, insecure or certificate variables. As soon as one is set the options are dropped, so an operator's OTEL_EXPORTER_OTLP_ENDPOINT and its scheme still decide where spans go and how the connection is secured.

@satyamg1620
satyamg1620 force-pushed the trace-explicit-sdk-config branch from 30e67a5 to f545f72 Compare August 8, 2026 12:01
@github-actions github-actions Bot added kind/bug Categorizes issue or PR as related to a bug. and removed kind/cleanup labels Aug 8, 2026
@satyamg1620
satyamg1620 requested a review from ahg-g August 8, 2026 18:14
Signed-off-by: satyamg1620 <Satyam.Gupta.3@ibm.com>
Signed-off-by: satyamg1620 <Satyam.Gupta.3@ibm.com>
@satyamg1620
satyamg1620 force-pushed the trace-explicit-sdk-config branch from 83854fb to 66cf37e Compare August 9, 2026 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tracing: configure the SDK explicitly instead of mutating process environment

2 participants