Repository navigation
Fix unknown and duplicated AIC telemetry attribution - #70864
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: approve
No actionable defects found in the changed lines. The exporter now preserves unknown AIC values, keeps explicit zero numeric, and places cost data on the owning span; the consumer guidance consistently treats missing values as unknown. Static review only; tests were not run in this review.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · 1.6 AIC · ⌖ 0.493 AIC · ⊞ 1.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd (per pr-triage: change_type=bug_fix). The core duplication and zero-fill fixes in send_otlp_span.cjs are sound and well-tested; one documentation consistency nit flagged inline.
📋 Key Themes & Highlights
Key Themes
- Duplication fix verified: the single
gh-aw.aicemission site was correctly moved intousageAttrs, which is attached to exactly one of the agent span or the conclusion span (never both) based onhasDedicatedAgentSpan— confirmed by tracing the code and by the newotel_contract.test.cjsassertion (toHaveLength(1)). - Unknown-vs-zero fix verified: removing the trailing
?? 0from theaiCreditsfallback chain lets the value stayundefinedwhen no valid cost source exists, correctly gated by thetypeof aiCredits === "number"check before emission. Explicit zeros (file orGH_AW_AICenv var) are still preserved. - Test coverage is solid: the new
it.each(6 invalid-input cases) plus explicit-zero-from-file and explicit-zero-from-env tests cover the regression well; existing detection/engine-job tests remain unaffected. - Minor doc gap:
daily-token-consumption-report.mdstill references "recognized AIC fields" (lines 103, 115) after the defining bullet list was removed from Step 4 — see inline comment.
Investigation note
Could not execute the test suite directly in this sandbox (network-restricted, no cached vitest binary) — verification was done via careful manual code tracing rather than empirical test runs. The PR description states 408 tests pass, which is consistent with the logic reviewed.
Positive Highlights
- ✅ Clean, minimal diff that isolates the emission-site move without touching unrelated logic
- ✅ New contract test directly guards against the regression class this PR fixes (single-span assertion)
- ✅ Consumer workflow instructions correctly updated to match the new unknown-vs-zero semantics
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 159.9 AIC · ⌖ 14.9 AIC · ⊞ 10.2K
Comment /matt to run again
Comments that could not be inline-anchored
.github/workflows/daily-token-consumption-report.md:103
[/grill-with-docs] "recognized AIC fields" is now a dangling term — the "Recognized AIC fields" bullet list this phrase referred to was removed from Step 4 in this PR, but Steps 2 and 3 (lines 103, 115) still use the phrase without a definition.
Content redacted to fit size limit.
@copilot please address this.
There was a problem hiding this comment.
🟡 Changes recommended
Targeted checks confirmed unresolved cost-availability and job-attribution defects.
3 open issues
What changed in this PR
Corrects AI-credit (AIC) attribution in gh-aw’s telemetry exporter and daily reporting workflow.
Changes:
- Removes the exporter’s default zero for unavailable cost.
- Routes AIC through the token-usage span boundary to avoid duplicate emission.
- Updates reporting guidance, regression tests, documentation, and release metadata.
| File | Description |
|---|---|
| docs/src/content/docs/reference/open-telemetry-attributes.mdx | Documents cost ownership and aggregation semantics. |
| actions/setup/js/send_otlp_span.test.cjs | Covers missing, invalid, and explicit-zero costs. |
| actions/setup/js/send_otlp_span.cjs | Changes cost selection and span placement. |
| actions/setup/js/otel_contract.test.cjs | Checks single cost-bearing spans across exports. |
| .github/workflows/daily-token-consumption-report.md | Updates field discovery, numeric accounting, and deduplication. |
| .github/workflows/daily-token-consumption-report.lock.yml | Refreshes body and admission hashes. |
| .changeset/fix-unknown-and-duplicated-otel-aic.md | Adds a patch release note. |
Review effort: Max
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: e8ebfbe
|
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
In response to #70864 (comment): Accepted the approval; no additional change was requested by this review. The separate correctness findings are addressed in this batch with focused regressions. |
|
In response to #70864 (comment): No code change is needed for this status-only report, and its no-op outcome is not treated as validation evidence. The batch was independently validated with 653 tests, typechecking, and the repository pre-publication gate. |
|
In response to #70864 (comment): No code change is needed because this completion notice contains no actionable finding. Publication and correctness were verified separately rather than inferred from the reviewer no-op. |
|
In response to #70864 (comment): Accepted the associated review's clarity finding. Steps 2 and 3 now explicitly reference finite non-negative numeric gh-aw.aic or legacy aliases confirmed under Step 4, replacing the dangling 'recognized AIC fields' wording. |
…banzo Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Summary
Correct three concrete, locally reproduced AIC attribution problems without changing optimizer policy or backend infrastructure:
gh-aw.aicwhen no valid source exists rather than synthesizing zero; keep explicitly reported zero encoded asdoubleValue.gh-aw.workflow.name, use confirmed numeric costs only, preserve unknown on both backends, deduplicate mirrored spans, and distinguish backend field/query failures from producer omissions.Update the attribute reference, patch changeset, and generated workflow lock. The lock diff contains only body/admission hash updates; permissions, MCP imports, tool allowlists, destinations and runtime steps are unchanged.
Evidence and boundaries
Discussion #69681 reports unqueryable cost, not zero usage and not proof of an emission defect. The corrections above are grounded independently in the current producer and consumer code and regression fixtures.
The canonical cost-owning span carries
gh-aw.aic,gh-aw.workflow.name,gh-aw.run.idandgh-aw.repository. Missing, invalid and unreadable cost sources now omit AIC. A0.125fixture produces one cost-bearing span in both the JSONL mirror and HTTP exports, not two.Live Sentry/Tempo verification remains unavailable: this session exposes neither backend's query tools. A representative raw cost-owning production span, Sentry custom-field discovery/indexing, and Tempo attribute discovery/search are still needed to establish backend queryability. The current Grafana import already allowlists the expected Tempo tools; no permission or infrastructure expansion is justified by the historical report alone. No workflow was dispatched.
Validation
make fmt-cjsandmake lint-cjspassed.make recompile: all 346 workflows compiled; unrelated generated maintenance output was excluded.PATH=/opt/homebrew/bin:$PATH make agent-report-progresspassed, including impacted tests and full workflow lock drift checks. Modern Bash was needed because macOS's default Bash lacksmapfile.Markdown-source security overview
The original branch name was retained because the app-managed rename encountered a shared
packed-refs.lock; that lock was not removed.