feat(tracing): add semantic span attributes to reconciler child spans - #10156
Paramesh324 wants to merge 6 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/kind feature |
|
/retest |
Address review feedback on PR tektoncd#10156: - Restore root-span identity attributes (taskrun/pipelinerun + namespace) on initTracing spans to keep root traces queryable - Replace tekton.* prefixed attribute names with existing convention (taskrun, pipelinerun, namespace) for consistency - Drop semconv k8s.namespace.name in favor of attribute.String ("namespace", ...) to avoid mixing naming conventions Fixes tektoncd#9785 Signed-off-by: Parameshwaran Krishnasamy <Parameshwaran.K@ibm.com>
There was a problem hiding this comment.
Pull request overview
This PR enhances OpenTelemetry tracing in the TaskRun and PipelineRun reconcilers by attaching attributes to previously “attribute-less” child spans so they become filterable/queryable in tracing backends.
Changes:
- Adds
span.SetAttributes(...)to multiple child spans intaskrunandpipelinerunreconcilers (e.g., step counts, done status, failure reason). - Adds additional PipelineRun child-span attributes for task/taskrun/customrun creation paths.
- Minor formatting refactors of existing
SetAttributescalls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/reconciler/taskrun/taskrun.go | Adds attributes to several TaskRun reconciler child spans (e.g., step counts, done status, failure reason). |
| pkg/reconciler/pipelinerun/pipelinerun.go | Adds attributes to several PipelineRun reconciler child spans and creation subroutines; some spans still miss namespace. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback on PR tektoncd#10156: - Restore root-span identity attributes (taskrun/pipelinerun + namespace) on initTracing spans to keep root traces queryable - Replace tekton.* prefixed attribute names with existing convention (taskrun, pipelinerun, namespace) for consistency - Drop semconv k8s.namespace.name in favor of attribute.String ("namespace", ...) to avoid mixing naming conventions Fixes tektoncd#9785 Signed-off-by: Parameshwaran Krishnasamy <Parameshwaran.K@ibm.com>
|
/retest |
Address review feedback on PR tektoncd#10156: - Restore root-span identity attributes (taskrun/pipelinerun + namespace) on initTracing spans to keep root traces queryable - Replace tekton.* prefixed attribute names with existing convention (taskrun, pipelinerun, namespace) for consistency - Drop semconv k8s.namespace.name in favor of attribute.String ("namespace", ...) to avoid mixing naming conventions Fixes tektoncd#9785 Signed-off-by: Parameshwaran Krishnasamy <Parameshwaran.K@ibm.com>
Address waveywaves review feedback on PR tektoncd#10156: - Add TestChildSpanAttributes in taskrun/tracing_test.go that asserts stopSidecars span carries taskrun, namespace, and pod attributes - Add TestChildSpanAttributes in pipelinerun/tracing_test.go that asserts durationAndCountMetrics span carries pipelinerun and done attributes - Uses tracetest.SpanRecorder to export and inspect child spans These tests lock in the attribute contract so any silent drift in span attributes will be caught. Fixes tektoncd#9785
f2bf4b2 to
edf3b68
Compare
Address review feedback on PR tektoncd#10156: - Restore root-span identity attributes (taskrun/pipelinerun + namespace) on initTracing spans to keep root traces queryable - Replace tekton.* prefixed attribute names with existing convention (taskrun, pipelinerun, namespace) for consistency - Drop semconv k8s.namespace.name in favor of attribute.String ("namespace", ...) to avoid mixing naming conventions Fixes tektoncd#9785 Signed-off-by: Parameshwaran Krishnasamy <Parameshwaran.K@ibm.com>
Address waveywaves review feedback on PR tektoncd#10156: - Add TestChildSpanAttributes in taskrun/tracing_test.go that asserts stopSidecars span carries taskrun, namespace, and pod attributes - Add TestChildSpanAttributes in pipelinerun/tracing_test.go that asserts durationAndCountMetrics span carries pipelinerun and done attributes - Uses tracetest.SpanRecorder to export and inspect child spans These tests lock in the attribute contract so any silent drift in span attributes will be caught. Fixes tektoncd#9785
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10156 +/- ##
==========================================
+ Coverage 90.73% 90.77% +0.03%
==========================================
Files 297 297
Lines 22569 22665 +96
==========================================
+ Hits 20478 20574 +96
Misses 2089 2089
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| ctx, span := c.tracerProvider.Tracer(TracerName).Start(ctx, "resolvePipelineState") | ||
| defer span.End() | ||
| span.SetAttributes( | ||
| attribute.String("pipelinerun", pr.Name), |
There was a problem hiding this comment.
Likewise, pipelinerun identifies the run rather than the referenced Pipeline. Since this PR closes #9785, which asks for Pipeline name or reference and status or outcome, could these spans expose a distinct Pipeline identity using the existing pipeline key and record outcome where known? pipelineMeta is already available in this path.
There was a problem hiding this comment.
Thanks for adding and testing the distinct pipeline attribute. The outcome portion remains. resolvePipelineState returns errors without recording them on the span, and reconcile likewise ends without outcome or error status. Could these paths use the existing recordSpanError helper or otherwise record the final outcome before we resolve this thread?
Add queryable attributes to all 22+ child spans in TaskRun and
PipelineRun reconcilers that previously carried zero attributes.
- Add tekton.taskrun.name, tekton.pipelinerun.name, tekton.pipeline.task.name,
tekton.task.step.count, tekton.taskrun.failure.reason, and other semantic
attributes to key spans (reconcile, createPod, failTaskRun, createTaskRun,
resolvePipelineState, runNextSchedulableTask, stopSidecars, etc.)
- Replace ad-hoc attribute.String("namespace", ...) with OTel semconv
k8s.namespace.name to align with the semconv v1.40.0 already vendored
- Remove duplicate SetAttributes from initTracing root spans that repeated
the same name/namespace attributes already set on ReconcileKind spans
Operators can now filter traces in Jaeger/Tempo by outcome, task name,
step count, or failure reason.
Fixes tektoncd#9785
Signed-off-by: Parameshwaran Krishnasamy <Parameshwaran.K@ibm.com>
Address review feedback on PR tektoncd#10156: - Restore root-span identity attributes (taskrun/pipelinerun + namespace) on initTracing spans to keep root traces queryable - Replace tekton.* prefixed attribute names with existing convention (taskrun, pipelinerun, namespace) for consistency - Drop semconv k8s.namespace.name in favor of attribute.String ("namespace", ...) to avoid mixing naming conventions Fixes tektoncd#9785 Signed-off-by: Parameshwaran Krishnasamy <Parameshwaran.K@ibm.com>
Address Copilot review feedback: add namespace attribute to createChildPipelineRuns, createTaskRuns, and createCustomRuns spans for consistent queryability with their child span counterparts. Fixes tektoncd#9785 Signed-off-by: Parameshwaran Krishnasamy <Parameshwaran.K@ibm.com>
Address waveywaves review feedback on PR tektoncd#10156: - Add TestChildSpanAttributes in taskrun/tracing_test.go that asserts stopSidecars span carries taskrun, namespace, and pod attributes - Add TestChildSpanAttributes in pipelinerun/tracing_test.go that asserts durationAndCountMetrics span carries pipelinerun and done attributes - Uses tracetest.SpanRecorder to export and inspect child spans These tests lock in the attribute contract so any silent drift in span attributes will be caught. Fixes tektoncd#9785
Address waveywaves review: PipelineRun/TaskRun names are namespace-scoped, so every span that carries a name attribute must also carry namespace for unambiguous querying. Adds namespace to durationAndCountMetrics, finishReconcileUpdateEmitEvents, updateLabelsAndAnnotations, and updateTaskRunWithDefaultWorkspaces spans in both reconcilers. Fixes tektoncd#9785 Signed-off-by: Parameshwaran Krishnasamy <Parameshwaran.K@ibm.com>
edf3b68 to
c0236b3
Compare
…spans Address waveywaves review feedback: - Add taskrun+namespace attributes to all 10 validation/substitution child spans (validateTaskSpecRequestResources, ValidateResolvedTask, ValidateEnumParam, ValidateParamArrayIndex, ValidateBindings, validateOverrides, validateTaskRunResults, applyParamsContextsResults- AndWorkspaces, ApplyParameters, ApplyWorkspaces) so they are queryable. - Add 'task' attribute (rtr.TaskName) to reconcile, validateTaskRunResults, and applyParamsContextsResultsAndWorkspaces spans for Task identity. - Add 'pipeline' attribute (pipelineMeta.Name) to reconcile and resolvePipelineState spans for Pipeline identity. - Extend TestChildSpanAttributes with subtests covering task and pipeline attributes. Fixes tektoncd#9785
c0236b3 to
9f9988f
Compare
There was a problem hiding this comment.
This PR adds operator visible tracing attributes and is labeled kind/feature, but the release note is still NONE. Please add a concise release note describing that PipelineRun and TaskRun spans now expose semantic attributes for filtering and diagnosis.
Please also squash the branch to a single commit after addressing the remaining feedback and before merge.
Changes
Add queryable attributes to all child spans in the TaskRun and PipelineRun reconcilers that previously carried zero attributes (22 of 26 spans were unqueryable timers).
What this PR does:
Add taskrun, pipelinerun, namespace, pipelinetask, step.count, failure.reason, done, pod, taskrun.count, child.pipelinerun, customrun, and other attributes to key spans (reconcile, createPod, failTaskRun, createTaskRun, resolvePipelineState, runNextSchedulableTask, stopSidecars, createChildPipelineRuns, createCustomRuns, etc.)
Use the existing attribute naming convention (taskrun/pipelinerun + namespace) consistently across all spans
Preserve root-span identity attributes on initTracing spans to keep root traces queryable
Operators can now filter and search traces in Jaeger/Tempo by task name, namespace, step count, or failure reason.
Fixes #9785
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes