feat: add gh-aw.run.attempt to setup and conclusion OTel spans#24670
feat: add gh-aw.run.attempt to setup and conclusion OTel spans#24670
Conversation
…isolation Agent-Logs-Url: https://github.com/github/gh-aw/sessions/d84e6711-7aa2-4c62-9412-183d931e9405 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds gh-aw.run.attempt as an OTLP span attribute so re-run attempts within the same GitHub Actions run can be distinguished in tracing dashboards/metrics.
Changes:
- Emit
gh-aw.run.attempton job setup spans fromGITHUB_RUN_ATTEMPT(defaulting to"1"). - Emit
gh-aw.run.attempton job conclusion spans, preferringaw_info.json’srun_attemptthen falling back toGITHUB_RUN_ATTEMPT/"1". - Extend unit tests to cover explicit attempt values and default fallback behavior.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/send_otlp_span.cjs | Adds gh-aw.run.attempt attribute to setup and conclusion span attribute sets. |
| actions/setup/js/send_otlp_span.test.cjs | Updates env teardown and adds assertions/tests for attempt emission and defaulting. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
actions/setup/js/send_otlp_span.cjs:506
sendJobConclusionSpannow consumesGITHUB_RUN_ATTEMPT(and prefersawInfo.run_attempt), but the JSDoc “Environment variables consumed” list doesn’t mentionGITHUB_RUN_ATTEMPT. Please update the doc comment so the inputs that influence span attributes are accurately documented.
const workflowName = awInfo.workflow_name || "";
const engineId = awInfo.engine_id || "";
const model = awInfo.model || "";
const jobName = process.env.INPUT_JOB_NAME || "";
const runId = process.env.GITHUB_RUN_ID || "";
const runAttempt = awInfo.run_attempt || process.env.GITHUB_RUN_ATTEMPT || "1";
const actor = process.env.GITHUB_ACTOR || "";
actions/setup/js/send_otlp_span.test.cjs:947
- In the conclusion span tests, attributes are read via
a.value.stringValuedirectly. Since OTLP attribute values are a union (string/int/bool), this makes the test fragile if the underlying attribute type changes. Consider using a helper (similar toattrValue()in the setup span tests) to extract the scalar value in a type-agnostic way.
const body = JSON.parse(mockFetch.mock.calls[0][1].body);
const span = body.resourceSpans[0].scopeSpans[0].spans[0];
const attrs = Object.fromEntries(span.attributes.map(a => [a.key, a.value.stringValue]));
expect(attrs["gh-aw.run.attempt"]).toBe("3");
});
it("defaults gh-aw.run.attempt to '1' when neither awInfo nor GITHUB_RUN_ATTEMPT is set", async () => {
const mockFetch = vi.fn().mockResolvedValue({ ok: true, status: 200, statusText: "OK" });
vi.stubGlobal("fetch", mockFetch);
process.env.OTEL_EXPORTER_OTLP_ENDPOINT = "https://traces.example.com";
await sendJobConclusionSpan("gh-aw.job.conclusion");
const body = JSON.parse(mockFetch.mock.calls[0][1].body);
const span = body.resourceSpans[0].scopeSpans[0].spans[0];
const attrs = Object.fromEntries(span.attributes.map(a => [a.key, a.value.stringValue]));
expect(attrs["gh-aw.run.attempt"]).toBe("1");
});
- Files reviewed: 2/2 changed files
- Comments generated: 3
| const serviceName = process.env.OTEL_SERVICE_NAME || "gh-aw"; | ||
| const jobName = process.env.INPUT_JOB_NAME || ""; | ||
| const workflowName = process.env.GH_AW_INFO_WORKFLOW_NAME || process.env.GITHUB_WORKFLOW || ""; | ||
| const engineId = process.env.GH_AW_INFO_ENGINE_ID || ""; | ||
| const runId = process.env.GITHUB_RUN_ID || ""; | ||
| const runAttempt = process.env.GITHUB_RUN_ATTEMPT || "1"; | ||
| const actor = process.env.GITHUB_ACTOR || ""; |
There was a problem hiding this comment.
sendJobSetupSpan now consumes GITHUB_RUN_ATTEMPT, but the function’s JSDoc “Environment variables consumed” list doesn’t mention it. Please update the doc comment to include GITHUB_RUN_ATTEMPT so callers know it affects emitted attributes.
This issue also appears on line 500 of the same file.
|
|
||
| const body = JSON.parse(mockFetch.mock.calls[0][1].body); | ||
| const span = body.resourceSpans[0].scopeSpans[0].spans[0]; | ||
| const attrs = Object.fromEntries(span.attributes.map(a => [a.key, a.value.stringValue])); |
There was a problem hiding this comment.
The new tests extract attributes via a.value.stringValue directly. This is brittle if an attribute is ever emitted as intValue/boolValue (or if buildAttr changes), and it’s inconsistent with the attrValue() helper already used in this describe block. Consider using attrValue(a) here as well to keep the tests resilient to OTLP union value types.
This issue also appears on line 928 of the same file.
| const attrs = Object.fromEntries(span.attributes.map(a => [a.key, a.value.stringValue])); | |
| const attrs = Object.fromEntries(span.attributes.map(a => [a.key, attrValue(a)])); |
| const jobName = process.env.INPUT_JOB_NAME || ""; | ||
| const runId = process.env.GITHUB_RUN_ID || ""; | ||
| const runAttempt = awInfo.run_attempt || process.env.GITHUB_RUN_ATTEMPT || "1"; | ||
| const actor = process.env.GITHUB_ACTOR || ""; |
There was a problem hiding this comment.
sendJobConclusionSpan prefers awInfo.run_attempt over GITHUB_RUN_ATTEMPT, but there’s no test covering that precedence. Adding a test that stubs fs.readFileSync to return an aw_info.json payload with run_attempt (and sets a different GITHUB_RUN_ATTEMPT) would lock in the intended behavior and prevent regressions.
GITHUB_RUN_ATTEMPTwas never emitted as a span attribute, making it impossible to isolate re-run attempts in OTel dashboards — all attempts share the samegh-aw.run.id, so error spans from failed attempt 1 pollute metrics even when attempt 2 succeeded.Changes
sendJobSetupSpan: readsprocess.env.GITHUB_RUN_ATTEMPT || "1"and emits it asgh-aw.run.attemptsendJobConclusionSpan: readsawInfo.run_attempt || process.env.GITHUB_RUN_ATTEMPT || "1"— prefers the value already persisted toaw_info.jsonbygenerate_aw_info.cjsfor consistency with what the agent job recordedGITHUB_RUN_ATTEMPTadded toenvKeysteardown in both describe blocks; new assertions for explicit attempt values and"1"default fallback in both setup and conclusion span tests