fix(tracing): direct OTel SDK setup for chain-coherent sampling#2756
fix(tracing): direct OTel SDK setup for chain-coherent sampling#2756ci-operator wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions Pipelines-as-Code tracing configuration from a custom ConfigMap to standard OpenTelemetry environment variables (such as OTEL_EXPORTER_OTLP_ENDPOINT and OTEL_TRACES_SAMPLER). It introduces a new tracing package to initialize and manage the tracer provider lifecycle, updating both the controller and adapter to initialize and shut down the provider correctly. A critical issue was identified in pkg/tracing/provider.go where passing the application's cancellation context to the OTLP exporter prevents final spans from being flushed during shutdown; using context.Background() is recommended instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return noopProvider() | ||
| } | ||
|
|
||
| exporter, err := newExporter(ctx, logger) |
There was a problem hiding this comment.
Passing the application's cancellation context (ctx) to the OTLP exporter creation means that when the application context is cancelled to initiate shutdown, the exporter's connection/client is also cancelled immediately. This prevents the final flush of spans during tp.Shutdown(shutdownCtx) from succeeding, leading to lost spans. Using context.Background() ensures the exporter remains functional during the shutdown flush.
| exporter, err := newExporter(ctx, logger) | |
| exporter, err := newExporter(context.Background(), logger) |
There was a problem hiding this comment.
newExporter uses context.Background() so the exporter's connection stays open through tp.Shutdown(shutdownCtx) and queued spans flush. The ctx parameter on tracing.New is dropped since nothing else used it.
ci-operator
force-pushed
the
distributed-tracing
branch
from
ff868b1 to
50ad9c1
Compare
| # tracing-protocol specifies the trace export protocol. | ||
| # Supported values: "grpc", "http/protobuf", "none". | ||
| # Default is "none" (tracing disabled). | ||
| # tracing-protocol: "none" | ||
|
|
||
| # tracing-endpoint specifies the OTLP collector endpoint. | ||
| # Required when tracing-protocol is "grpc" or "http/protobuf". | ||
| # The OTEL_EXPORTER_OTLP_ENDPOINT env var takes precedence if set. | ||
| # tracing-endpoint: "http://otel-collector.observability.svc.cluster.local:4317" | ||
|
|
||
| # tracing-sampling-rate controls the fraction of traces sampled. | ||
| # 0.0 = none, 1.0 = all. Default is 0 (none). | ||
| # tracing-sampling-rate: "1.0" | ||
|
|
There was a problem hiding this comment.
While the configmap keys are removed, I see no change in controller. Does that mean the configmap based tracing continue to work even with these changes?
There was a problem hiding this comment.
The Knative tracing wrapper that consumed these _example keys is replaced; the new tracer in pkg/tracing/provider.go reads the OTel-standard env vars directly. The _example block was just documentation for keys nothing reads anymore.
This is not the same ConfigMap as pipelines-as-code (the main one), which holds the tracing-label-action|application|component operator label-name mappings.
On the "no change in controller" observation - we verified end-to-end that with neither OTEL_EXPORTER_OTLP_ENDPOINT nor OTEL_TRACES_SAMPLER set, PaC falls back to a noop tracer and emits no spans. If you're seeing tracing behavior unchanged from the pre-PR state, could you share how to reproduce so we can dig in?
There was a problem hiding this comment.
By changes in controller, I intended to ask if the knative base (eventing/adapter) that pac uses, reads these observability configmap keys for any arbitrary configuration/operations. Ref.
There was a problem hiding this comment.
Yes - the Knative eventing-adapter still consumes config-observability for non-tracing observability (metrics/profiling/etc. via evadapter.NewObservabilityConfiguratorFromConfigMap() at the line you linked, logging is read from config-logging separately). What this PR changes is specifically the tracing portion: PaC's old Knative tracing wrapper (added in bd9f468) read tracing-protocol / tracing-endpoint / tracing-sampling-rate from this ConfigMap; the new pkg/tracing/provider.go reads the OTel-standard env vars directly and that wrapper is gone, so those three keys went with the _example block. The Knative-base reads for non-tracing observability are unchanged.
There was a problem hiding this comment.
@ci-operator as it looks breaking change and @theakshaypant is considerate about, can't we introduce env based configuration as fallback to configmap so that we're keeping both, considering other users may want to use configmap?
| Both `OTEL_EXPORTER_OTLP_ENDPOINT` and `OTEL_TRACES_SAMPLER` must be set to opt in to tracing. If either is unset PaC falls back to a noop tracer that emits no spans and incurs no exporter cost. Changes to any of these env vars take effect on the next pod restart. | ||
|
|
||
| ### Example | ||
| PaC honors inbound `traceparent` headers on incoming webhook requests via the W3C TraceContext propagator. OTel Baggage is intentionally not honored; per [ADR 0061](https://github.com/konflux-ci/architecture/blob/main/ADR/0061-distributed-tracing.md), this delivery-tracing chain does not use Baggage for cross-service attribute propagation because all required attributes are locally available at every emission point. |
There was a problem hiding this comment.
We should not link a downstream project's design doc in PaC's documentation.
There was a problem hiding this comment.
is github.com/konflux-ci considered downstream?? I think NO it's another open source repository.
There was a problem hiding this comment.
Downstream would be anything that builds on or utilizes PaC and if a change here is affecting konflux-ci, then it does qualify as a downstream project.
Even so, point still stands. We should not link a design doc from another project.
There was a problem hiding this comment.
yes please remove konflux references, we are not a dep on this
There was a problem hiding this comment.
Removed the konflux link entirely. Rewrote the Baggage paragraph to stand on its own (every emission point already has the attributes it needs from the local PipelineRun and webhook event, so no cross-service propagation channel is needed). Also corrected a stale tracing-endpoint reference further down the file - the OTel SDK reads OTEL_EXPORTER_OTLP_ENDPOINT automatically, so the docs just needed to point at the standard env var name.
| case protocolGRPC: | ||
| return otlptracegrpc.New(ctx, otlptracegrpc.WithEndpointURL(endpoint)) | ||
| default: | ||
| logger.Errorw("unsupported OTLP protocol; falling back to grpc", "protocol", protocolFromEnv()) |
There was a problem hiding this comment.
[nit] redundant protocolFromEnv call
There was a problem hiding this comment.
Fixed. Hoisted to a local.
zakisk
force-pushed
the
distributed-tracing
branch
from
50ad9c1 to
5b41561
Compare
|
/ok-to-test |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2756 +/- ##
==========================================
+ Coverage 59.73% 59.78% +0.05%
==========================================
Files 210 211 +1
Lines 21117 21218 +101
==========================================
+ Hits 12615 12686 +71
- Misses 7707 7733 +26
- Partials 795 799 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This feelds vibe coded, have you tested this properly? have you properly reviewed it? |
ci-operator
force-pushed
the
distributed-tracing
branch
from
5b41561 to
311babd
Compare
|
Sorry for the churn here. A number of findings surfaced while working through the integration-service and release-service side of this work, and the OTel SDK swap in this PR turned out to be necessary to avoid a fractional-sampling fragmentation bug (Knative's tracing wrapper made per-service sampling decisions independently of the root span). The unrelated cleanups landed in the same commit, but the overall behavior is functionally identical to what shipped in #2605 (#2605): same spans, same attribute schema, same opt-in env vars, just plumbed through the OTel SDK directly so The PR was validated end-to-end across more than a dozen scenarios covering each fix and each error class it mitigates:
|
zakisk
force-pushed
the
distributed-tracing
branch
from
311babd to
43cabca
Compare
| Changes to `tracing-protocol`, `tracing-endpoint`, and `tracing-sampling-rate` require restarting the controller and watcher pods. The trace exporter is created once at startup from the ConfigMap values at that time. Set `tracing-protocol` to `none` or remove the tracing keys to disable tracing. | ||
|
|
||
| The controller and watcher locate this ConfigMap by name via the `CONFIG_OBSERVABILITY_NAME` environment variable set in their deployment manifests. Operator-based installations may manage this differently; consult the operator documentation for details. | ||
| The `parentbased_*` sampler family honors the parent span's sample decision carried in the W3C `traceparent` flag bit. When PaC, Tekton, integration-service, and release-service all use parent-based samplers, the root span's sampling decision propagates through the whole delivery chain: every service either keeps its spans or drops them based on what the root chose. Flat-rate samplers (`traceidratio` without parent-based) cause each service to roll independently, which at fractional sampling fragments the chain into orphaned spans whose `parent_spanID` references a span that was dropped. `parentbased_always_on` keeps everything; `parentbased_traceidratio` with a numeric argument samples a coherent fraction. |
There was a problem hiding this comment.
[warning/docs] Remaining Konflux-specific service references
The earlier reviewer feedback asked to remove Konflux references. The ADR link was removed, but "integration-service" and "release-service" on this line are Konflux-CI-specific service names that leaked through. PaC is upstream and shouldn't assume a specific deployment topology in its docs.
Consider generalizing:
| The `parentbased_*` sampler family honors the parent span's sample decision carried in the W3C `traceparent` flag bit. When PaC, Tekton, integration-service, and release-service all use parent-based samplers, the root span's sampling decision propagates through the whole delivery chain: every service either keeps its spans or drops them based on what the root chose. Flat-rate samplers (`traceidratio` without parent-based) cause each service to roll independently, which at fractional sampling fragments the chain into orphaned spans whose `parent_spanID` references a span that was dropped. `parentbased_always_on` keeps everything; `parentbased_traceidratio` with a numeric argument samples a coherent fraction. | |
| The `parentbased_*` sampler family honors the parent span's sample decision carried in the W3C `traceparent` flag bit. When every service in the delivery chain uses parent-based samplers, the root span's sampling decision propagates end to end: each service either keeps its spans or drops them based on what the root chose. Flat-rate samplers (`traceidratio` without parent-based) cause each service to roll independently, which at fractional sampling fragments the chain into orphaned spans whose `parent_spanID` references a span that was dropped. `parentbased_always_on` keeps everything; `parentbased_traceidratio` with a numeric argument samples a coherent fraction. |
There was a problem hiding this comment.
Right, I missed those alongside the ADR link. Reworded the sentence to be topology-neutral.
| otel.SetTracerProvider(tp) | ||
| otel.SetTextMapPropagator(propagation.TraceContext{}) | ||
|
|
||
| logger.Infow("tracing initialized", "endpoint", os.Getenv(EnvOTLPEndpoint), "protocol", protocolFromEnv()) |
There was a problem hiding this comment.
[nit/code-quality] Redundant protocolFromEnv() — acknowledged but not yet pushed
This was flagged by a prior reviewer, and you replied "Fixed. Hoisted to a local." — but the fix hasn't been pushed yet. protocolFromEnv() is called here (line 70) and again inside newExporter() (line 91). No functional impact (env vars don't change mid-process), just redundant work.
To hoist: add proto := protocolFromEnv() before the newExporter call, pass it into newExporter as a parameter, and use the local proto in this log line.
There was a problem hiding this comment.
Sorry, I was on wrong branch and never actually pushed it. Pulled proto := protocolFromEnv() up before the log line and pass it into newExporter so it only runs once.
There was a problem hiding this comment.
[AI-assisted review]
Solid work — well-structured, correct, and good test coverage. Two minor items inline.
What this does: Replaces Knative's config-observability ConfigMap-based tracing (which uses a flat tracing-sampling-rate causing per-service independent sampling) with direct OTel SDK initialization. This enables parentbased_* samplers that honor the root span's W3C traceparent flag bit, keeping the entire trace chain coherent.
What's good:
- Clean self-contained
tracingpackage with a clearNew()/Shutdown()lifecycle - Correct use of
context.Background()for the exporter — the gRPC/HTTP connection survives context cancellation so theBatchSpanProcessorcan flush during shutdown - Dual opt-in guard (both
OTEL_EXPORTER_OTLP_ENDPOINTandOTEL_TRACES_SAMPLERmust be set) with clear log messages on the noop fallback path - Thorough test suite: all 6 sampler families, noop fallback paths, protocol precedence, shutdown idempotency. The
saveGlobalProviderhelper properly restores global OTel state - Uses
gotest.tools/v3/assertand table-driven tests per project conventions samplerFromEnvlogs a helpful warning when a ratio sampler is selected withoutOTEL_TRACES_SAMPLER_ARG(defaults to 0%)
Findings (1 warning, 1 nit):
docs/content/docs/operations/tracing.md:23— warning/docs: "integration-service" and "release-service" are Konflux-specific service names that survived the earlier Konflux-reference cleanup. Suggest generalizing to "every service in the delivery chain."pkg/tracing/provider.go:70— nit/code-quality: RedundantprotocolFromEnv()call was acknowledged by the author as fixed ("Hoisted to a local.") but the fix hasn't been pushed yet.
Prior review feedback status:
- ✅ Gemini's
context.Background()for exporter creation — fixed - ✅ Konflux ADR link removed, Baggage paragraph rewritten
- ✅ ConfigMap clarification — Knative base still reads non-tracing keys
⚠️ RedundantprotocolFromEnv()— acknowledged but not yet pushed
ci-operator
force-pushed
the
distributed-tracing
branch
from
43cabca to
ee2356a
Compare
|
/ok-to-test |
zakisk
force-pushed
the
distributed-tracing
branch
from
ee2356a to
d495d68
Compare
|
/ok-to-test |
theakshaypant
force-pushed
the
distributed-tracing
branch
from
d495d68 to
6820cf1
Compare
|
/ok-to-test |
| log := logging.FromContext(ctx) | ||
|
|
||
| tp := tracing.New(log) | ||
| go func() { |
There was a problem hiding this comment.
Please run make lint and make test locally to avoid CI failures.
There was a problem hiding this comment.
Suppressed gosec false positive on OTel shutdown goroutine where Background is needed; lint and test pass now.
Knative's config-observability ConfigMap only exposes a flat tracing-sampling-rate, so at fractional rates each service in the chain rolls independently — PaC can drop a trace while Tekton keeps it, leaving execution spans whose parent_spanID points at nothing. Switching to the OTel SDK opens up OTEL_TRACES_SAMPLER's parentbased_* family, which honors the root span's sample decision in the W3C traceparent flag so the whole chain is kept or dropped together. Controller and watcher call tracing.New() at startup. Tracing is opt-in: both OTEL_EXPORTER_OTLP_ENDPOINT and OTEL_TRACES_SAMPLER must be set, otherwise PaC falls back to noop (matching the prior tracing-sampling-rate "0" default). Resource service.name is pipelines-as-code. Propagator is W3C TraceContext only; Baggage is intentionally not honored per Konflux-CI ADR 0061. otlptracegrpc and otlptracehttp promoted from indirect to direct dependencies. Assisted-by: Claude Code Signed-off-by: Josiah England <jengland@redhat.com>
ci-operator
force-pushed
the
distributed-tracing
branch
from
6820cf1 to
7fe6ead
Compare
|
Pushed gosec suppression on OTel shutdown goroutine and a small cleanup. |
There was a problem hiding this comment.
Since the configmap based tracing would no longer work, can we add a callout for any existing users.
Something like this.
| <-ctx.Done() | ||
| shutdownCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer cancel() | ||
| _ = tp.Shutdown(shutdownCtx) |
There was a problem hiding this comment.
shouldn't we log the shutdown error?
| } | ||
|
|
||
| proto := protocolFromEnv() | ||
| exporter, err := newExporter(context.Background(), logger, proto) |
There was a problem hiding this comment.
as per some AI review, does timeout handling needed here??
|
@ci-operator the whole point to emit traces via OTEL Sdk and drop the knative configmap is to use |
If yes, so then AFAIK, hierarchy of services is PaC controller -> Tekon Controller -> PaC watcher (please correct if I am wrong...). and I think tektoncd/pipeline still uses knative based tracing and using |
6 participants
📝 Description of the Change
PR #2605 ran PaC's tracing through Knative's
config-observabilityflattracing-sampling-rate. At fractional rates each service in the chain rollsindependently — PaC can drop a trace while Tekton keeps it, leaving execution
spans whose
parent_spanIDpoints at nothing.This PR switches to the OTel SDK directly.
OTEL_TRACES_SAMPLERopens theparentbased_*family, which honors the root span's sample decision in theW3C
traceparentflag bit so the whole chain is kept or dropped together.Controller and watcher call
tracing.New()at startup; tracing is opt-in(both
OTEL_EXPORTER_OTLP_ENDPOINTandOTEL_TRACES_SAMPLERmust be set).The
tracing-*keys are dropped from the_exampleConfigMap block sincethey no longer apply. Resource
service.nameispipelines-as-code.Propagator is W3C TraceContext only; Baggage is intentionally not honored
per Konflux-CI ADR 0061.
🔗 Linked GitHub Issue
Fixes #
🧪 Testing Strategy
🤖 AI Assistance
AI assistance can be used for various tasks, such as code generation,
documentation, or testing.
Please indicate whether you have used AI assistance
for this PR and provide details if applicable.
Important
Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Claude noreply@anthropic.com
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.