logs: track per-run download duration/size and render end-of-run stats summary - #60951
Conversation
…ary to logs command 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.
🟡 Changes recommended
Normal runs omit the summary, and the current duration, size, and API-cost calculations can produce inaccurate results.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds per-run artifact download metrics and end-of-run statistics to gh aw logs.
Changes:
- Records download duration and directory size.
- Persists metrics in JSON/JSONL output.
- Renders aggregate timing, size, and API-cost estimates.
File summaries
| File | Description |
|---|---|
schemas/logs.schema.json |
Adds download metric fields. |
schemas/logs-jsonl.schema.json |
Adds cached JSONL metric fields. |
pkg/cli/logs_run_processor.go |
Measures download timing and size. |
pkg/cli/logs_report.go |
Exposes metrics in run reports. |
pkg/cli/logs_orchestrator.go |
Renders single-target statistics. |
pkg/cli/logs_orchestrator_unit_test.go |
Tests aggregation and formatting. |
pkg/cli/logs_orchestrator_stdin.go |
Renders stdin-driven statistics. |
pkg/cli/logs_orchestrator_download.go |
Aggregates and formats statistics. |
pkg/cli/logs_multi.go |
Renders multi-target statistics. |
pkg/cli/logs_models.go |
Stores metrics on workflow runs. |
Review details
Suppressed comments (1)
pkg/cli/logs_orchestrator_download.go:172
- The rate-limit delta includes discovery, cache-hit, failed/skipped, and successful-download requests, but the denominator counts only successful fresh downloads. For example, 99 cache hits plus one download attributes the entire command's API usage to one run. Divide by
discoveredRuns(or another count covering all contributing runs) rather thandownloadCount.
if calls, ok := gitHubAPIRateLimitCostEstimate(reports); ok {
msg += fmt.Sprintf("; GitHub API cost estimate: ~%.1f requests/run", float64(calls)/float64(count))
- Files reviewed: 10/10 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| finishGitHubAPIRateLimitReports(activeCtx, allAPIRateLimits, opts.JSONOutput) | ||
| cacheGitHubAPIRateLimitReports(opts.cachedJSONLWriter, allAPIRateLimits...) | ||
| apiRateLimit, apiRateLimits := partitionGitHubAPIRateLimitReports(allAPIRateLimits) | ||
| renderLogsDownloadStatsSummary(opts.collectionStats, allAPIRateLimits...) |
There was a problem hiding this comment.
Fixed in 686e9e4 (superseded by the follow-up commit) — opts.collectionStats is now allocated unconditionally in DownloadWorkflowLogsForTargets before collectLogsTargets runs, regardless of --cached-jsonl.
| } | ||
| renderLogsCollectionStats(opts.collectionStats) | ||
| finishGitHubAPIRateLimitReport(ctx, apiRateLimit, opts.JSONOutput) | ||
| renderLogsDownloadStatsSummary(opts.collectionStats, apiRateLimit) |
There was a problem hiding this comment.
Fixed — opts.collectionStats is now allocated unconditionally at the top of DownloadWorkflowLogs, independent of --cached-jsonl.
| diff := report.End.Used - report.Start.Used | ||
| if diff < 0 { | ||
| // The rate-limit window reset mid-run; fall back to the ending value as a | ||
| // lower-bound approximation rather than reporting a negative cost. | ||
| diff = report.End.Used | ||
| } |
There was a problem hiding this comment.
Fixed — gitHubAPIRateLimitCostEstimate now also falls back to End.Used when report.End.Reset != report.Start.Reset, catching the case where a reset happened but End.Used still came out >= Start.Used.
| ) { | ||
| writeWorkflowRunFolderLocation(run.DatabaseID, runOutputDir) | ||
| logsOrchestratorLog.Printf("Downloading artifacts for run %d: owner=%s, repo=%s", run.DatabaseID, perRunParams.dlOwner, perRunParams.dlRepo) | ||
| downloadStart := time.Now() |
There was a problem hiding this comment.
Fixed — the timer now starts after waitForConfiguredRateLimit/MkdirAll (right before the metadata fetch) and stops after the evals fallback, before analyzeRunArtifacts runs.
| result.Run.DownloadDuration = time.Since(downloadStart) | ||
| if size, sizeErr := logsDirectorySize(runOutputDir); sizeErr == nil { | ||
| result.Run.DownloadSizeBytes = size |
There was a problem hiding this comment.
Fixed — size is now a before/after delta measured around the actual download (post-MkdirAll, pre-analysis), so preexisting bytes from an earlier pass or locally generated files no longer inflate the reported size.
| // DownloadDurationMS is the wall-clock time (milliseconds) spent by `gh aw logs` | ||
| // downloading this run's artifacts from GitHub. Zero when the run was served | ||
| // from the on-disk cache instead of being freshly downloaded. | ||
| DownloadDurationMS int64 `json:"download_duration_ms,omitempty" console:"-"` |
There was a problem hiding this comment.
Updated the comments to clarify that zero means "not recorded this invocation" while a cached JSONL record can retain the original nonzero measurement from when the run was first downloaded.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. 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.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "proxy.golang.org"See Network Configuration for more information.
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
There was a problem hiding this comment.
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
clients2.google.commtalk.google.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "clients2.google.com"
- "mtalk.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · claude · sonnet46 · 69.1 AIC · ⌖ 18 AIC · ⊞ 7.8K
Comment /smoke-claude to run again
| // DownloadDuration is the wall-clock time spent downloading this run's artifacts | ||
| // from GitHub. It is zero for runs served from the on-disk cache (no download | ||
| // was performed). | ||
| DownloadDuration time.Duration |
There was a problem hiding this comment.
Nice addition of DownloadDuration — tracking wall-clock download time per run will help identify slow artifact transfers. Consider also exposing this in the JSON output for post-processing.
| // avg/max summary can be rendered at the end of the run. | ||
| downloadCount atomic.Int64 | ||
| totalDownloadNanos atomic.Int64 | ||
| maxDownloadNanos atomic.Int64 |
There was a problem hiding this comment.
The atomicMaxInt64 helper is a clean approach for lock-free max tracking. Worth adding a brief comment explaining the compare-and-swap retry loop for future readers unfamiliar with atomic patterns.
🏗️ ADR Required - 2026-09-15ResultAn ADR was required for this PR and none was present in the PR body, linked issue references, or Evidence used
Draft ADR added
Inferred decisionThis PR makes download-performance telemetry a first-class part of Next actionPlease review and refine the draft ADR so it accurately captures the long-term architectural intent before merge. NoteI attempted the required progress validation step with Warning Firewall blocked 2 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"
- "proxy.golang.org"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes because the headline feature (end-of-run download stats) does not fire for the common gh aw logs invocation without --cached-jsonl.
📋 Key Themes & Highlights
Key Themes
- Feature doesn't activate on the primary path:
opts.collectionStatsis only allocated when--cached-jsonlis set (prepareCachedLogsJSONL,logs_cached_json.go:206-212). All the recording/rendering code is nil-safe, so for a plaingh aw logsrun the new "Download stats: ..." summary silently never prints — confirmed by tracing every call site ofcollectionStatsinlogs_orchestrator.go/logs_multi.go/logs_orchestrator_download.go. Other automated review comments on this PR (ids 4010789954, 4010789996) already flag the same root cause onlogs_multi.go/logs_orchestrator.go; this needs to be fixed before merge. - Timer scope inflates duration:
downloadAndTimeRunArtifactstimes the entirerunDownloadDeferredReservedclosure (rate-limit wait + metadata fetch + artifact download + evals fallback + analysis), not just the network transfer — flagged already atlogs_run_processor.go:398. - Test coverage gap: no test drives the real (non-cached-JSONL)
DownloadWorkflowLogspath and asserts the summary line is printed — the existing entry-point test only covers the--cached-jsonlcase, which is exactly why the nil-collectionStatsregression wasn't caught.
Positive Highlights
- ✅
atomicMaxInt64correctly uses a CAS retry loop for lock-free max tracking. - ✅
gitHubAPIRateLimitCostEstimatehandles the rate-limit-window-reset edge case explicitly (documented in code and tests), even if the fallback (usingEnd.Usedalone) is an approximation worth calling out in the summary output. - ✅ Good extraction of
downloadAndTimeRunArtifactsout ofprocessSingleRunDownload, keeping the diff readable. - ✅ Schema files (
logs.schema.json,logs-jsonl.schema.json) were correctly regenerated alongside the newRunDatafields.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 67.2 AIC · ⌖ 15.3 AIC · ⊞ 10.4K
Comment /matt to run again
| } | ||
| renderLogsCollectionStats(opts.collectionStats) | ||
| finishGitHubAPIRateLimitReport(ctx, apiRateLimit, opts.JSONOutput) | ||
| renderLogsDownloadStatsSummary(opts.collectionStats, apiRateLimit) |
There was a problem hiding this comment.
[/diagnosing-bugs] This new download-stats summary silently never fires for the common case: opts.collectionStats is only allocated in prepareCachedLogsJSONL when --cached-jsonl is set (logs_cached_json.go:206-212). A plain gh aw logs run (no --cached-jsonl) leaves opts.collectionStats nil, and since recordDiscovered/recordResult/renderLogsDownloadStatsSummary are all nil-safe no-ops, the feature this PR adds silently never prints for the vast majority of invocations — exactly the case the PR description highlights.
💡 Suggested fix
Allocate opts.collectionStats unconditionally at the top of DownloadWorkflowLogs (and the analogous entry points in logs_multi.go), independent of whether --cached-jsonl is set, e.g.:
if opts.collectionStats == nil {
opts.collectionStats = &logsCollectionStats{}
}This matches feedback already left by another reviewer bot on this same PR (comment ids 4010789954 / 4010789996) — worth confirming both single-target and multi-target paths are fixed together since they share the same root cause.
@copilot please address this.
There was a problem hiding this comment.
Fixed — opts.collectionStats is now allocated unconditionally (nil check) right after prepareCachedLogsJSONL in both DownloadWorkflowLogs and DownloadWorkflowLogsForTargets, independent of --cached-jsonl.
| assert.Equal(t, 20, calls) | ||
| } | ||
|
|
||
| // TestDownloadWorkflowLogsReportsCollectionStatsForJSONLAndDiskCacheHits verifies |
There was a problem hiding this comment.
[/tdd] All new tests (TestLogsCollectionStatsRecordsDownloadDurationAndSize, TestRenderLogsDownloadStatsSummaryReportsAvgMaxAndRateLimitCost) exercise logsCollectionStats and renderLogsDownloadStatsSummary directly, and the one entry-point test that runs DownloadWorkflowLogs (TestDownloadWorkflowLogsReportsCollectionStatsForJSONLAndDiskCacheHits) only asserts on the discovered/downloaded/skipped counts, not the new download-stats line. There is no test that drives DownloadWorkflowLogs through an actual (non-cached-JSONL) download and asserts the "Download stats: ..." line appears in stderr.
💡 Why this matters
A test at the DownloadWorkflowLogs/DownloadWorkflowLogsForTargets entry-point level (without --cached-jsonl) would have caught the nil-collectionStats issue flagged elsewhere in this review, since it exercises the real wiring rather than only the unit-level helpers. Per /tdd, the missing coverage is precisely the gap that let the regression slip through.
@copilot please address this.
There was a problem hiding this comment.
Added TestDownloadWorkflowLogsRendersDownloadStatsForFreshDownloadWithoutCachedJSONL, which drives DownloadWorkflowLogs through a real (non-cached-JSONL) download via a fake gh binary and asserts the "Download stats: ..." line appears in stderr.
There was a problem hiding this comment.
Impeccable review (mode: harden, audit)
Change type: mixed_unclear (backend metrics feature, no UI) — applied harden (missing/degenerate-state checks) and audit (correctness of the reported numbers).
I verified the existing automated review comments against the code and confirm they are accurate and blocking:
opts.collectionStatsis nil unless--cached-jsonlis passed (logs_cached_json.go:210-212is the only initializer). This makesrenderLogsDownloadStatsSummary— and the pre-existingrenderLogsCollectionStats— silent no-ops for the defaultgh aw logsinvocation described in the PR's own example. No test exercises the non---cached-jsonlpath end-to-end to catch this; existing tests only callrenderLogsDownloadStatsSummarydirectly with a manually constructed non-nilstats.- The download timer in
downloadAndTimeRunArtifacts(logs_run_processor.go:398-430) spans rate-limit waiting, metadata fetch, evals fallback, full artifact analysis, and summary-writing — not just the network transfer — so "Download stats" overstates actual download time. logsDirectorySize(runOutputDir)(logs_run_processor.go:431) measures the whole run directory post-analysis, which can include artifacts reused from a prior incremental pass (logs_download.go:184-191) plus locally generatedrun_summary.json/analysis files, inflating the reported "size".
No new distinct issues found beyond what's already flagged inline on the same lines. Not duplicating those comments.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 110.8 AIC · ⌖ 13.7 AIC · ⊞ 8.4K
|
@copilot Please take another pass on this PR and run the
Please address the reported issues or summarize blockers in the PR.
|
…ize delta, and stale cache comment Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all of the reviewer feedback in this pass:
The two failing runs you linked (Design Decision Gate succeeded; PR Code Quality Reviewer's |
|
@copilot Please take another pass on this PR and run the Run: https://github.com/github/gh-aw/actions/runs/34922574085 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.
|
No further action needed from my side. Status check:
Nothing else is actionable from the agent side — this is ready for human review/merge. |
- Add copilot_org_billing_error failure category term - Update Operational Value Grader entry: metrics retain native scale instead of being normalized (PR #60888) - Add blankassigncomma custom Go linter entry (PR #60893) - Add Download Stats Summary term for gh aw logs download telemetry (PR #60951) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
The
gh aw logscommand didn't capture how long artifact downloads took or how much data they transferred, making it hard to diagnose slow runs or estimate GitHub API usage.Data collection
DownloadDuration/DownloadSizeBytestoWorkflowRun, populated by timing the download and measuring the resulting artifact directory size (zero for cache hits).downloadAndTimeRunArtifactsfromprocessSingleRunDownloadto isolate the timing/sizing logic.Cached-jsonl persistence
DownloadDurationMS/DownloadSizeBytestoRunData, populated inapplyGitHubMetadataToRunData.cachedLogsJSONLRunDataembedsRunData) and round-trip correctly on cache reload.schemas/logs.schema.jsonandschemas/logs-jsonl.schema.jsonto reflect the new fields.End-of-run summary
logsCollectionStatswith atomic counters for total/max download duration and size.gitHubAPIRateLimitCostEstimate, computing an average per-run API request cost from existingGitHubAPIRateLimitReportstart/end usage (handles rate-limit window resets).renderLogsDownloadStatsSummary, printed after each logs run:logs_orchestrator.go), multi-target (logs_multi.go), and stdin-driven (logs_orchestrator_stdin.go).✨ PR Review Safe Output Test - Run 34912620912
Warning
Firewall blocked 2 domains
The following domains were blocked by the firewall during workflow execution:
clients2.google.commtalk.google.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.
Run: https://github.com/github/gh-aw/actions/runs/34915242801
run: https://github.com/github/gh-aw/actions/runs/34922574085
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.laiyagushi.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.