Preserve OTEL resource attribution and normalize agent token counters#36450
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes an observability attribution gap by ensuring OTLP resource spans retain workflow/run identity from OTEL_RESOURCE_ATTRIBUTES, and hardens token roll-ups by normalizing agent_usage.json counters (including numeric strings) before emitting gen_ai.usage.* attributes.
Changes:
- Merge and upsert
OTEL_RESOURCE_ATTRIBUTESinto emitted OTLP resource attributes (preserving escaping) while keeping GitHubgithub.*metadata authoritative. - Normalize token counters (including string-valued counters) via the shared runtime token normalization path before emitting
gen_ai.usage.*. - Add regression tests for merged resource attributes and string token counters on the agent span.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/send_otlp_span.cjs | Adds parsing/merging of OTEL_RESOURCE_ATTRIBUTES into resource attributes and expands token normalization to accept numeric strings. |
| actions/setup/js/send_otlp_span.test.cjs | Adds regression tests for resource-attribute merging and string-valued token counters. |
| .github/workflows/agentic-token-trend-audit.lock.yml | Updates the workflow lock metadata hash. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 2
| for (const char of raw) { | ||
| if (escaping) { | ||
| current += char; | ||
| escaping = false; | ||
| continue; |
| it("merges OTEL_RESOURCE_ATTRIBUTES into resource attributes", async () => { | ||
| const mockFetch = vi.fn().mockResolvedValue({ ok: true, status: 200, statusText: "OK" }); | ||
| vi.stubGlobal("fetch", mockFetch); | ||
|
|
||
| process.env.GH_AW_OTLP_ENDPOINTS = JSON.stringify([{ url: "https://traces.example.com" }]); | ||
| process.env.GITHUB_REPOSITORY = "owner/repo"; | ||
| process.env.GITHUB_RUN_ID = "987654321"; | ||
| process.env.OTEL_RESOURCE_ATTRIBUTES = | ||
| "gh-aw.workflow.name=Daily\\, Token\\= Report,gh-aw.run.id=987654321,gh-aw.engine.id=claude,service.version=1.2.3"; | ||
|
|
||
| await sendJobConclusionSpan("gh-aw.job.conclusion"); | ||
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #36450 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (requires_adr_by_default_volume=false, threshold=100). Neither Condition A nor Condition B is met. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — changes are correct and targeted; three minor issues worth addressing before merge.
📋 Key Themes & Highlights
Key Themes
- Test env pollution:
OTEL_RESOURCE_ATTRIBUTESis set by the new integration test but not included in theenvKeyssave/restore list — will bleed into later tests in the samedescribeblock. - Parser under-tested:
parseOTELResourceAttributesimplements non-trivial OTEL escape rules but lacks a dedicated unit test suite for edge cases. - Silent fallback change: Passing
agent_usage.jsonthroughnormalizeRuntimeTokenUsagechanges the fallback behavior for files with unrecognized fields; the change is probably correct but untested.
Positive Highlights
- ✅ Root cause correctly identified and fixed — merging
OTEL_RESOURCE_ATTRIBUTESbefore upsertinggithub.*attributes is exactly the right precedence model. - ✅
normalizeTokenCounteris clean and handles both numeric and string values without duplicating logic. - ✅ Two regression tests added that directly cover the two fixed gaps — good signal that future breakage will be caught.
- ✅ OTEL escape spec (
,,=,\) correctly implemented inparseOTELResourceAttributes.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 2.2M
Comments that could not be inline-anchored
actions/setup/js/send_otlp_span.test.cjs:1165
[/tdd] OTEL_RESOURCE_ATTRIBUTES is missing from the envKeys save/restore list, so the new test at line 1498 sets this env var without cleanup — it will leak into every subsequent test in this describe block.
<details>
<summary>💡 Fix</summary>
Add "OTEL_RESOURCE_ATTRIBUTES" to the envKeys array here (and to the equivalent array in the sendJobConclusionSpan describe block ~line 2480). Without this, tests that run after the new integration test will see unexpected resource attri…
actions/setup/js/send_otlp_span.cjs:470
[/tdd] parseOTELResourceAttributes is a non-trivial parser with custom escape rules but is only exercised through the high-level integration test. Consider adding a small dedicated describe('parseOTELResourceAttributes') block to cover edge cases directly.
<details>
<summary>💡 Uncovered edge cases</summary>
\\(literal backslash in a value) — e.g.key=a\\bshould yielda\b- Pair with no
=— e.g.noequals,key=val— should the keyless token be silently dropped? - Trailing …
actions/setup/js/send_otlp_span.cjs:2202
[/diagnose] Wrapping readJSONIfExists(...) in normalizeRuntimeTokenUsage introduces a subtle fallback: if agent_usage.json exists but contains an object with no recognized token fields (e.g. {"model":"claude-3"} only), the function returns undefined and execution falls through to runtimeMetrics.tokenUsage. Previously the raw file object would have been used directly.
This is likely fine, but the behavior change is unintentional and untested — a test with a file containing only …
There was a problem hiding this comment.
Three issues found across the two changed files. None are data-loss or security bugs, but two will cause test environment pollution and the third is a silent protocol-conformance bug.
### Findings summary
1. OTEL_RESOURCE_ATTRIBUTES not cleaned up in afterEach (test.cjs:1498) — env var leaks into all subsequent tests in the describe block; test results become execution-order dependent.
2. parseOTELResourceAttributes two-pass design breaks \= in key names (cjs:479) — the outer loop strips backslashes before splitPair runs, making splitPair's own escape handling dead code. A key like my\=key=value is silently mis-parsed as key my, value key=value. Fix: collapse both passes into one.
3. New test placed inside wrong describe block (test.cjs:1491) — calls sendJobConclusionSpan while scoped to sendJobSetupSpan hooks; conclusion-span env keys are not reset, creating potential for cross-test state bleed.
The production-side logic changes (normalizeRuntimeTokenUsage string-normalization, upsertResourceAttr for OTEL attribute merging) look correct.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 254K
| process.env.GH_AW_OTLP_ENDPOINTS = JSON.stringify([{ url: "https://traces.example.com" }]); | ||
| process.env.GITHUB_REPOSITORY = "owner/repo"; | ||
| process.env.GITHUB_RUN_ID = "987654321"; | ||
| process.env.OTEL_RESOURCE_ATTRIBUTES = |
There was a problem hiding this comment.
OTEL_RESOURCE_ATTRIBUTES leaks into subsequent tests: this env var is set here but not included in the envKeys cleanup array, so afterEach will not delete it.
💡 Suggested fix
Add "OTEL_RESOURCE_ATTRIBUTES" to the envKeys array at line ~1142:
const envKeys = [
"GH_AW_OTLP_ENDPOINTS",
// ... existing keys ...
"OTEL_RESOURCE_ATTRIBUTES", // add this
];Without this, every test that runs after "merges OTEL_RESOURCE_ATTRIBUTES" will see gh-aw.workflow.name, gh-aw.engine.id, and service.version injected into resource attributes. Tests that use toContainEqual will likely still pass (they only assert presence), but the env pollution makes test results non-deterministic with respect to execution order and can mask regressions.
| pairs.push(current); | ||
| } | ||
|
|
||
| const splitPair = pair => { |
There was a problem hiding this comment.
splitPair's escape-handling is dead code and masks a real parsing bug for \\= in key names: the outer loop strips all backslashes before pairs reach splitPair, so the escaped flag inside splitPair can never be set, and \\= in a key position is silently mis-parsed.
💡 Details and fix
What the outer loop does
For input key\\=1=value, the outer loop (lines 455–471) sets escaping=true on \\, then appends = literally and resets escaping. The resulting pair string is key=1=value — the backslash is gone.
splitPair then sees key=1=value, finds the first = and produces:
- key:
"key" - value:
"1=value"
The correct result per the OTel spec is key: "key=1", value: "value".
The escape-handling code inside splitPair (the escaped variable and its branches) is therefore unreachable dead code — it will never fire because the outer loop already consumed every backslash.
Fix
Do the whole parse in a single pass so escape context is shared between comma-splitting and equals-splitting:
function parseOTELResourceAttributes(raw) {
if (!raw || !raw.trim()) return [];
const result = [];
let key = "", value = "", seenEq = false, escaped = false;
const flush = () => {
const k = key.trim();
if (k) result.push(buildAttr(k, value.trim()));
key = ""; value = ""; seenEq = false;
};
for (const ch of raw) {
if (escaped) { (seenEq ? (value += ch) : (key += ch)); escaped = false; continue; }
if (ch === "\\") { escaped = true; continue; }
if (ch === ",") { flush(); continue; }
if (ch === "=" && !seenEq) { seenEq = true; continue; }
seenEq ? (value += ch) : (key += ch);
}
flush();
return result;
}This correctly handles \\,, \\=, and \\\\ in a single pass and eliminates the dead code.
| expect(resourceAttrs).toContainEqual({ key: "github.run_id", value: { stringValue: "987654321" } }); | ||
| }); | ||
|
|
||
| it("merges OTEL_RESOURCE_ATTRIBUTES into resource attributes", async () => { |
There was a problem hiding this comment.
Test placed in the wrong describe block: this test is inside sendJobSetupSpan but exercises sendJobConclusionSpan.
💡 Details
The sendJobSetupSpan describe block wraps this test, but line 1501 calls sendJobConclusionSpan("gh-aw.job.conclusion"). The beforeEach/afterEach hooks for sendJobSetupSpan save/restore env vars that are relevant to setup spans, not conclusion spans — e.g., GH_AW_INFO_STAGED, GH_AW_SETUP_WORKFLOW_NAME. Conclusion-span-specific keys like GH_AW_AGENT_CONCLUSION or GH_AW_EFFECTIVE_TOKENS are not reset, so this test can see state leaked by earlier conclusion-span tests.
Move this test into the sendJobConclusionSpan describe block and ensure OTEL_RESOURCE_ATTRIBUTES is added to that block's envKeys array as well.
|
@copilot review all comments and reviews. fix https://github.com/github/gh-aw/actions/runs/26824926231/job/79092092107?pr=36450 . apply copilot-review skill |
|
@mnkiefer please review |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in e894421. I reviewed the in-scope feedback, fixed OTEL_RESOURCE_ATTRIBUTES parsing for escaped |
|
|
Daily token reporting showed 151.3M tokens across 5,939 model calls, but workflow-level attribution was missing because token-bearing spans and workflow identity were emitted on disjoint span populations. In practice,
gh-aw.workflow.namewas absent from token-bearing spans, and theagent_usage.jsonroll-up path was too strict about token counter types.Preserve workflow/run attribution on OTLP resource spans
OTEL_RESOURCE_ATTRIBUTESinto the JavaScript OTLP resource builder instead of rebuilding resource metadata from scratch.\,,\=,\\) so workflow names and other contextual fields survive round-trip parsing.github.*resource attributes after merging.Harden token roll-up emission
agent_usage.jsonthrough the same token-usage path used for runtime log parsing before emittinggen_ai.usage.*.input_tokens,output_tokens,cache_read_tokens, andcache_write_tokens.gen_ai.usage.total_tokensfrom normalized input/output totals.Add regressions for the observability gap
OTEL_RESOURCE_ATTRIBUTESon emitted resource spans.Example of the new resource-attribute handling: