Propagate a global setup parent span ID across jobs to preserve a single OTEL trace tree#31193
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot there should be a global parent span id , and setup is a child of this span as well |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in 1c7d06a. I added a propagated global setup parent span ID ( |
There was a problem hiding this comment.
Pull request overview
Propagates a stable “global setup parent span id” across workflow jobs so all gh-aw.<job>.setup spans remain in a coherent single OTEL trace tree (consistent parentage across activation/agent/downstream jobs).
Changes:
- Extended setup-step generation to optionally pass a
parent-span-idand added a reusableneeds.*expression helper to wire parent span propagation. - Updated built-in jobs to emit
setup-span-id/setup-parent-span-idjob outputs and to pass the upstream parent span into downstream setup steps. - Extended the
actions/setupcontract and implementation to acceptparent-span-idand to outputspan-idand resolvedparent-span-id; updated wasm golden fixtures accordingly.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/threat_detection.go | Passes activation-derived parent span into detection job setup step. |
| pkg/workflow/repo_memory.go | Passes activation-derived parent span into repo-memory job setup step. |
| pkg/workflow/publish_assets.go | Passes activation-derived parent span into publish-assets job setup step. |
| pkg/workflow/notify_comment.go | Passes activation-derived parent span into conclusion/notify job setup step. |
| pkg/workflow/compiler_unlock_job.go | Passes activation-derived parent span into unlock job setup step. |
| pkg/workflow/compiler_safe_outputs_job.go | Passes activation-derived parent span into safe-outputs setup/count paths. |
| pkg/workflow/cache.go | Passes activation-derived parent span into cache job setup step. |
| pkg/workflow/compiler_experiments.go | Passes activation-derived parent span into experiments push job setup step. |
| pkg/workflow/compiler_main_job.go | Emits setup span outputs from main/agent job and passes parent span into setup. |
| pkg/workflow/compiler_pre_activation_job.go | Emits setup-span-id and setup-parent-span-id from pre-activation job. |
| pkg/workflow/compiler_activation_job_builder.go | Wires pre-activation → activation parent span propagation and emits setup span outputs. |
| pkg/workflow/compiler_yaml_step_generation.go | Adds setupParentSpanNeedsExpr helper + adds parent-span-id support to generated setup steps. |
| pkg/workflow/setup_step_version_test.go | Updates setup-step generation tests for new signature and adds parent-span-id assertion. |
| actions/setup/action.yml | Adds parent-span-id input; adds span-id and parent-span-id outputs. |
| actions/setup/index.js | Plumbs INPUT_PARENT_SPAN_ID through setup.sh and action OTLP emission path. |
| actions/setup/setup.sh | Passes INPUT_PARENT_SPAN_ID into action_setup_otlp.cjs in script mode. |
| actions/setup/js/send_otlp_span.cjs | Resolves parent span id (options/input/context) and includes it in payload/return value. |
| actions/setup/js/action_setup_otlp.cjs | Reads parent span input, passes it to sendJobSetupSpan, and writes span-id/parent-span-id outputs. |
| actions/setup/js/action_setup_otlp.test.cjs | Updates mocks and adds coverage for parent-span-id output writing. |
| actions/setup/js/action_otlp.test.cjs | Extends expectations to include span-id output and adds parent-span behavior test. |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden | Golden fixture updates for new setup outputs/inputs (and updated embedded versions). |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden | Golden fixture updates for new setup outputs/inputs (and updated embedded versions). |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden | Golden fixture updates for new setup outputs/inputs (and updated embedded versions). |
| pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden | Golden fixture updates for new setup outputs/inputs (and updated embedded versions). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 24/24 changed files
- Comments generated: 3
| // - destination: The destination path where files should be copied (e.g., SetupActionDestination) | ||
| // - enableArtifactClient: Whether to install @actions/artifact so upload_artifact.cjs can upload via REST API directly | ||
| // - traceID: Optional OTLP trace ID expression for cross-job span correlation (e.g., "${{ needs.activation.outputs.setup-trace-id }}"). Empty string means a new trace ID is generated. | ||
| // - parentSpanID: Optional OTLP parent span ID expression for setup-span nesting (e.g., setupParentSpanNeedsExpr("activation")). Empty string means setup span is emitted as root. |
| * (and specific comment) that triggered the workflow | ||
| * | ||
| * @param {SendJobSetupSpanOptions} [options] | ||
| * @returns {Promise<{ traceId: string, spanId: string }>} The trace and span IDs used. | ||
| * @returns {Promise<{ traceId: string, spanId: string, parentSpanId: string }>} The trace/span IDs used and resolved parent span ID. | ||
| */ |
| // Patch the shared CJS module exports — run() re-destructures on every call | ||
| sendOtlpModule.sendJobSetupSpan = mockSendJobSetupSpan; | ||
| mockSendJobSetupSpan.mockResolvedValue({ traceId: VALID_TRACE_ID, spanId: VALID_SPAN_ID }); | ||
| mockSendJobSetupSpan.mockResolvedValue({ traceId: VALID_TRACE_ID, spanId: VALID_SPAN_ID, parentSpanId: VALID_PARENT_SPAN_ID }); | ||
|
|
🧪 Test Quality Sentinel ReportTest Quality Score: 70/100
Test Classification Details
Additionally, three Flagged Tests — Requires Review
|
| Component | Score | Notes |
|---|---|---|
| Behavioral Coverage (40 pts) | 40/40 | All 3 tests verify observable outputs |
| Error/Edge Case Coverage (30 pts) | 0/30 | No error paths tested in new functions |
| Low Duplication (20 pts) | 20/20 | No duplicate clusters detected |
| Proportional Growth (10 pts) | 10/10 | Test additions are justified by multi-file production changes |
Total: 70/100
Language Support
Tests analyzed:
- 🐹 Go (
*_test.go): 1 test function (//go:build !integration✅) - 🟨 JavaScript (
*.test.cjs): 2 newit()blocks + 3 inline assertion additions (vitest)
Verdict
✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). No coding-guideline violations detected. Score: 70/100 — consider adding at least one error/edge-case path to each new test function to improve coverage.
📖 Understanding Test Classifications
Design Tests (High Value) verify what the system does:
- Assert on observable outputs, return values, or state changes
- Cover error paths and boundary conditions
- Would catch a behavioral regression if deleted
- Remain valid even after internal refactoring
Implementation Tests (Low Value) verify how the system does it:
- Assert on internal function calls (mocking internals)
- Only test the happy path with typical inputs
- Break during legitimate refactoring even when behavior is correct
- Give false assurance: they pass even when the system is wrong
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
References: §25601261888
🧪 Test quality analysis by Test Quality Sentinel · ● 5.2M · ◷
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 70/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). All 3 new tests verify behavioral contracts. No coding-guideline violations detected. Suggestion: add error/edge-case paths to the new test functions to push toward 80+.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd (new feature with test additions) and a light /zoom-out pass (cross-cutting OTLP concern spanning JS, shell, and Go layers).
Key Themes
- Test coverage gaps on the
contextParentSpanIdresolution path — the third priority in the new||chain was the only pre-existing mechanism; a regression test for it is missing. - Script-mode path untested for
parent-span-id— the new Go testTestGenerateSetupStepIncludesParentSpanIDvalidates action mode only; the parallelINPUT_PARENT_SPAN_IDenv-var emission in script mode has no coverage. - End-to-end
GITHUB_OUTPUTwrite missing from integration test —action_otlp.test.cjsverifies the OTLP payload'sparentSpanIdfield but not that the output is written toGITHUB_OUTPUT.
Positive Highlights
- ✅ Clean, DRY helper
setupParentSpanNeedsExprcentralises the fallback expression across all job builders — easy to find and update. - ✅ Consistent three-layer resolution order (options → input → context) is well-documented in JSDoc.
- ✅ Golden fixture updates are included, keeping compiled expectations in sync with the change.
- ✅ The
setup-parent-span-id || setup-span-idfallback in job outputs is a safe degradation path for old cached runs.
Verdict
No blocking issues — this is a well-structured telemetry improvement. The inline comments flag test gaps that are worth closing to protect the pre-existing contextParentSpanId path from silent regressions.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 5.7M
| fetchSpy.mockRestore(); | ||
| } finally { | ||
| fs.rmSync(tmpOut, { force: true }); | ||
| } |
There was a problem hiding this comment.
[/tdd] The new test verifies that span?.parentSpanId in the OTLP payload is correct, but it does not assert that parent-span-id is also written to GITHUB_OUTPUT. The action_setup_otlp.test.cjs file has a dedicated unit test for this, but this integration-level test should also confirm the end-to-end plumbing:
const contents = fs.readFileSync(tmpOut, 'utf8');
expect(contents).toMatch(/^parent-span-id=abcdef1234567890$/m);Without this, the path from resolved parentSpanId → GITHUB_OUTPUT is only exercised through mocks, not the real appendFileSync code path.
| if !strings.Contains(combined, "parent-span-id: "+parentExpr) { | ||
| t.Fatalf("expected setup step to include parent-span-id input, got:\n%s", combined) | ||
| } | ||
| } |
There was a problem hiding this comment.
[/tdd] TestGenerateSetupStepIncludesParentSpanID only exercises the action mode path (when setupActionRef is non-empty and c.actionMode.IsScript() is false). The generateSetupStep function also has a script mode path that emits INPUT_PARENT_SPAN_ID as an environment variable rather than as an action input parent-span-id:. Consider adding a parallel sub-test:
t.Run("script mode", func(t *testing.T) {
c := NewCompiler()
c.actionMode = ActionModeScript // or however script mode is set
lines := c.generateSetupStep(data, "", "${{ runner.temp }}/gh-aw", false, "", parentExpr)
combined := strings.Join(lines, "")
assert.Contains(t, combined, "INPUT_PARENT_SPAN_ID: "+parentExpr)
})Without this, a regression in the script-mode branch would go undetected.
| @@ -853,6 +861,7 @@ async function sendJobSetupSpan(options = {}) { | |||
| const commentId = typeof awInfo.context?.comment_id === "string" ? awInfo.context.comment_id : ""; | |||
|
|
|||
There was a problem hiding this comment.
[/tdd] The parentSpanId resolution chain now has three sources:
const parentSpanId = optionsParentSpanId || inputParentSpanId || contextParentSpanId || "";The new test in action_otlp.test.cjs covers the inputParentSpanId path, and the mock test in action_setup_otlp.test.cjs covers optionsParentSpanId indirectly. However, the contextParentSpanId path (propagated from aw_info.context.otel_parent_span_id) has no dedicated test. This was the only path that existed before this PR, so a regression here — e.g. the || short-circuit shadowing the context value when inputParentSpanId is empty-but-set — would go undetected. Consider a test that sets GH_AW_INFO with otel_parent_span_id and omits INPUT_PARENT_SPAN_ID.
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (110 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started. It has been committed to this branch: 📄 Draft ADR: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why an ADR for this PR?This change extends the public contract of 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References:
|
|
@copilot review all comments |
Reviewed and addressed all actionable review comments in commit |
Uh oh!
There was an error while loading. Please reload this page.