Add integrity and MCP metrics to conclusion usage reporting - #58018
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
actions/setup/js/generate_usage_activity_summary.cjs:367: shrink: gateway accumulator/helper layer is heavier than the small parser it serves. Inline the counters in the parser and keep one local accumulator object.
net: -24 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #58018 · codex · mai10 · 6.3 AIC · ⌖ 0.568 AIC · ⊞ 13.5K
Comment /ponytail to run again
| try { | ||
| const content = fs.readFileSync(gatewayPath, "utf-8"); | ||
| const lines = content.split("\n"); | ||
| function createGatewayActivityAccumulator() { |
There was a problem hiding this comment.
actions/setup/js/generate_usage_activity_summary.cjs:367: shrink: gateway accumulator/helper layer is heavier than the small parser it serves. Inline the counters in the parser and keep one local accumulator object.
There was a problem hiding this comment.
Fixed in commit 4f1f4f7: the gateway activity accumulator is now initialized locally in the parser, removing the separate accumulator factory and its type indirection.
🏗️ ADR Required - draft added for PR #58018I enforced the ADR gate for this PR because it adds 597 new lines in default business-logic directories, which exceeds the 100-line threshold. Evidence used
Result
Next action for the author
|
There was a problem hiding this comment.
🟡 Changes recommended
Existing server rows do not receive missing average-duration metrics during compact-summary backfill.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds compact integrity, firewall, and MCP metrics to usage artifacts, logs reports, caches, and OTLP conclusion spans.
Changes:
- Aggregates MCP activity and integrity-filter metrics from gateway/RPC logs.
- Backfills and reports compact metrics across runs and cached reports.
- Emits and documents new OTLP attributes.
File summaries
| File | Description |
|---|---|
.changeset/minor-conclusion-integrity-mcp-metrics.md |
Records the minor feature change. |
actions/setup/js/generate_usage_activity_summary.cjs |
Generates compact MCP and integrity aggregates. |
actions/setup/js/generate_usage_activity_summary.test.cjs |
Tests gateway and RPC aggregation. |
actions/setup/js/send_otlp_span.cjs |
Emits conclusion-span usage attributes. |
actions/setup/js/send_otlp_span.test.cjs |
Tests the new OTLP attributes. |
docs/src/content/docs/reference/artifacts.md |
Documents compact usage metrics. |
docs/src/content/docs/reference/open-telemetry-attributes.mdx |
Documents OTLP attribute names. |
pkg/cli/audit_report.go |
Adds integrity summaries to MCP reports. |
pkg/cli/gateway_logs_rpc_result_test.go |
Tests MCP isError handling. |
pkg/cli/gateway_logs_types.go |
Recognizes error results in RPC responses. |
pkg/cli/logs_mcp_tool_usage_test.go |
Tests cross-run integrity aggregation. |
pkg/cli/logs_models.go |
Exposes aggregate integrity report data. |
pkg/cli/logs_report_mcp.go |
Merges MCP and integrity metrics across runs. |
pkg/cli/logs_run_processor.go |
Persists cache-healed activity metrics. |
pkg/cli/logs_usage_activity.go |
Backfills compact MCP and integrity data. |
pkg/cli/logs_usage_activity_test.go |
Tests backfill and cache healing. |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if activity, ok := activityServers[server.ServerName]; ok { | ||
| if server.TotalInputSize == 0 { | ||
| server.TotalInputSize = activity.TotalInputSize | ||
| } | ||
| if server.TotalOutputSize == 0 { | ||
| server.TotalOutputSize = activity.TotalOutputSize | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in commit 4f1f4f7: existing server rows now backfill AvgDuration from compact AvgDurationMS when missing.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Request changes
The new integrity aggregation path is currently counting the same filtered event twice when a run has both raw FilteredEvents and the compact Integrity fallback populated, so the cross-run metrics this PR adds are not trustworthy yet.
Blocking theme
The regression is in mergeRunIntegrityFilterSummary: it blindly prefers usage.Integrity when present, while the surrounding code also keeps the raw FilteredEvents list for the same run. Cache-healed runs can now carry both representations, and the tests currently lock in the inflated total instead of catching it. That turns the new logs summary and conclusion-span metrics into overcounts.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 68.6 AIC · ⌖ 8.43 AIC · ⊞ 21.8K
Comment /review to run again
| FilteredReasonCounts: make(map[string]int), | ||
| } | ||
| } | ||
| (*destination).TotalFiltered += runIntegrity.TotalFiltered |
There was a problem hiding this comment.
You're adding usage.Integrity specifically as a compact fallback for runs where the raw FilteredEvents list is unavailable, but mergeRunIntegrityFilterSummary counts both sources for the same run. In the common cache-healing path a run can carry FilteredEvents and the backfilled compact Integrity summary, so this inflates TotalFiltered and the per-server/tool/reason counters in cross-run reports. Build the run-level aggregate from exactly one source per run (prefer raw events when present, otherwise fall back to the compact summary) or dedupe before merging.
💡 Why this blocks the change
The new tests even encode the broken behavior by asserting TotalFiltered == 4 for only three underlying events. That means gh aw logs --artifacts usage will over-report integrity filtering as soon as a cached run is healed from usage/activity/summary.json, which is precisely the scenario this PR is trying to improve. The OTLP span and docs would then advertise incorrect counts.
A safe fix is to make mergeRunIntegrityFilterSummary prefer summarizeIntegrityFilterEvents(usage.FilteredEvents) when len(usage.FilteredEvents) > 0, and only read usage.Integrity when the raw event list is absent. Then update the test to expect the real total and keep RunsWithFilteredEvents keyed to runs, not duplicated sources.
There was a problem hiding this comment.
Fixed in commit 4f1f4f7: integrity aggregation now prefers raw filtered events for each run and only falls back to compact integrity data when raw events are absent, preventing double-counting.
There was a problem hiding this comment.
Review summary
This is a well-structured metrics addition (integrity-filter + detailed MCP metrics), and the Go-side merge/backfill logic and RPC isError handling look correct and well-tested.
Blocking issue found in actions/setup/js/generate_usage_activity_summary.cjs: the rewritten parseGatewayJSONL only recognizes event === "tool_call" (or method === "tools/call"), dropping the legacy "rpc_call" and "request" event names that the original parseGatewayLogs accepted and that other parsers in this codebase (gateway_logs_parsing.go, gateway_logs_timeline.go, unified_timeline.cjs) still support. This silently undercounts/loses gateway and integrity metrics for any gateway.jsonl using those legacy event names — see inline comment for details and suggested fix.
Everything else (Go merge helpers, RPC isError → failure mapping, OTLP attribute wiring, docs) looks correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 125.9 AIC · ⌖ 13.5 AIC · ⊞ 8.3K
Comments that could not be inline-anchored
actions/setup/js/generate_usage_activity_summary.cjs:285
Bug: legacy gateway.jsonl events silently dropped, undercounting total_calls/failed_calls.
The old parseGatewayLogs accepted event values "tool_call", "rpc_call", and "request" (see the removed code a few lines above: if (!["tool_call", "rpc_call", "request"].includes(event))). The new parseGatewayJSONL only matches event === "tool_call" or method === "tools/call":
if (event !== "tool_call" && method !== "tools/call") {
continue;
}Other code paths in …
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — solid feature addition with strong test coverage across JS and Go; a few design/behavior nuances worth a second look before merge.
📋 Key Themes & Highlights
Key Themes
- Shared-type coupling risk:
rpcResponsePayload.UnmarshalJSONnow synthesizes anErrorfor MCPisErrorresults, feeding the same code path used for guard-policy-block detection and transport-error metrics. Currently safe (zero-value code falls outside the guard-policy range), but implicit and worth hardening/testing explicitly. - Cache-hit backfill cost:
backfillCacheHitIfNeededdropped its earlier short-circuit and now always reads/parsesusage/activity/summary.json, even for fully-populated cached runs — a minor but avoidable I/O cost ongh aw logsre-runs over large histories. - Zero-vs-missing ambiguity: several backfill helpers (
backfillUsageActivityToolMetrics, server merge inbackfillUsageActivityMCPMetrics) treat== 0as "field is missing," which can't distinguish a legitimate zero from an absent value on repeated cache-hit passes.
Positive Highlights
- ✅ Excellent test coverage for the new RPC v2 parsing paths (JS
parseGatewayActivityand GoparseRPCMessages/buildToolCallsFromRPCMessages), including the isError→failure mapping and gateway.jsonl-precedence-over-rpc-messages.jsonl behavior. - ✅ Clean refactor of
buildMCPToolUsageSummaryinto small composable merge helpers (mergeMCPToolSummaries,mergeMCPServerStats,mergeRunIntegrityFilterSummary) — readable and consistent with existing weighted-average duration patterns. - ✅ Additive, backward-compatible schema changes (
gateway/integritysections, OTLP attributes) with docs updated in the same PR.
Nothing here is blocking; flagging as COMMENT for the author's consideration since the identified points are edge-case robustness rather than functional bugs.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 143.9 AIC · ⌖ 13.9 AIC · ⊞ 10.3K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/gateway_logs_types.go:207
[/codebase-design] rpcResponsePayload.UnmarshalJSON is a shared type used by every RPC-response consumer (metrics aggregation, guard-policy detection, tool-call building). Synthesizing an Error whenever result.isError is true means any MCP tool-level failure now flows through the same resp.Error != nil branch as transport/guard-policy errors, incrementing TotalErrors/ErrorCount for both cases.
<details>
<summary>💡 Why this matters</summary>
Since isGuardPolicyErrorCode(0)…
pkg/cli/logs_run_processor.go:1229
[/codebase-design] backfillCacheHitIfNeeded now unconditionally loads and re-applies usage/activity/summary.json on every cache hit, whereas previously it only did so when Turns, SafeItemsCount, or WorkingSet were still zero/nil. This adds a file read + parse for every cached run, even ones that already have complete data.
<details>
<summary>💡 Suggested guard</summary>
Consider restoring a cheap early-exit condition (e.g. also check whether result.MCPToolUsage.Integrity / gat…
pkg/cli/logs_usage_activity.go:1471
[/tdd] backfillUsageActivityToolMetrics/backfillUsageActivityMCPMetrics use == 0 as the signal that a field is missing and needs backfilling from the compact gateway summary. This is indistinguishable from a legitimately-zero value (e.g. a tool call with an empty JSON payload, total_input_size: 0), so on the next cache-hit pass a genuine zero gets silently overwritten by whatever the gateway summary reports.
<details>
<summary>💡 Suggested test</summary>
Add a regression test wher…
|
@copilot Please address the newest unresolved review feedback on this PR, refresh the branch if needed, and run the
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the requested integrity aggregation and MCP backfill fixes in commits f0dbd39 and the follow-up typecheck fix. Raw filtered events now take precedence per run, existing server durations are backfilled, and the activity parser accumulator is initialized locally. |
|
@copilot Please verify the latest fixes against the unresolved review feedback on this PR and run the
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Verified the latest branch state and local validation. The integrity and MCP backfill fixes are present, and the activity summary typecheck passes. Three review threads remain marked unresolved in GitHub (the first is outdated; the two current threads already have substantive replies), and the PR is blocked pending maintainer review. Current CI is still running/queued, so no additional code blocker remains. |
|
👏 Great work! This PR looks ready for review. The additions to integrity and MCP metrics reporting are well-scoped and thoroughly tested:
The diff is well-organized across the logging and metrics infrastructure. Ready for maintainer review! 🚀 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
@copilot Please address the current blocking review feedback, refresh the branch if needed, and run the
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
|
🎉 This pull request is included in a new release. Release: |
Conclusion artifacts lacked compact integrity-filter and detailed MCP activity metrics, leaving
gh aw logsand OTLP reporting incomplete. This adds those metrics while reusing existing firewall and total-run aggregates.Usage artifact
usage-activity-summary/v1schema.Logs reporting
runs_with_filtered_eventsalongside the existing total-run metric.result.isErrorresponses as failures.OTLP
Run: https://github.com/github/gh-aw/actions/runs/33679827993
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
PR Sous Chef branch refresh requested. Run: https://github.com/github/gh-aw/actions/runs/33682167508)
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.