feat(logs): summarize entries by service and level - #491
Conversation
Closes #488 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Website Preview Your PR preview is ready! 📎 Preview URL: https://jongio.github.io/azd-app/pr/491/ This preview will be automatically cleaned up when the PR is closed. |
wbreza
left a comment
There was a problem hiding this comment.
Code Review — Approve ✅
Clean, well-scoped addition of azd app logs --summary. The summary builders are pure and unit-tested, level mapping is consistent with the existing logLevelToString, and the --summary/--follow conflict is validated. No Critical, High, or Medium issues found.
What was verified
normalizeLogSummaryLevelreturn values (info/warn/error/debug/unknown) match theswitchcases inbuildLogSummaryagainst the real constants (logLevelWarn="warn",statusError="error",logLevelDebug="debug"). ✔- Timestamp min/max tracking correctly copies the loop value and yields nil pointers (omitted in JSON, skipped in text) when no entry has a timestamp. ✔
- Summary short-circuit is placed after the output writer is set up, so
--summary --file <path>and--output jsonboth work. ✔ - Tests cover both builders, text + JSON rendering, flag registration, and the follow conflict.
Non-blocking observations (Low / nitpick)
logLevelToStringreturns"info"for unrecognizedservice.LogLevelvalues (itsdefault), so theUnknowncolumn/field is effectively dead for the non-context (buildLogSummaryFromEntries) path — only the context-log path (raw string levels) can ever populateUnknown. Consider mapping unknown enum values to"unknown"here if you want the column to be meaningful for local logs, otherwise it's harmlessly always zero.displayLogSummaryswallows the JSON encode error by writing toos.Stderrand returning nothing; the caller inexecute()also discards it. Fine in practice (encoding this struct won't fail), but returning the error would be more consistent with the surrounding code.- Service column is
%-20s; service names longer than 20 chars will misalign the text table. Purely cosmetic. - Minor test-style nit:
TestValidateLogsOptionsspecial-casesif tt.name == "summary with follow"to set fields rather than driving them from table columns — works, just slightly asymmetric with the rest of the table.
Nice work — approving.
errcheck flagged the unchecked fmt.Fprintf and fmt.Fprintln return values in displayLogSummary. Discard them explicitly with the same _, _ = form the rest of the commands package already uses for io.Writer output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
wbreza
left a comment
There was a problem hiding this comment.
Code Review (re-review @ d93bc02) — Approve ✅
Re-reviewed after new commits landed since my previous approval at 9a51cf8. Two additions:
- Merge of
origin/maininto the branch — verified viagit diff origin/main...HEAD: the net feature delta is unchanged (logs.go+14,logs_filtering.go+4,logs_summary.go+159). No behavioral impact on the summary feature from the merge. fix(logs): check writer errors in log summary output(d93bc02) — prepends_, _ =to thefmt.Fprintf/Fprintlncalls in the text branch ofdisplayLogSummaryto explicitly discard writer return values (satisfies errcheck).
The core logic I validated in my prior pass (level normalization vs the real constants, nil-timestamp handling, JSON/text rendering, the --summary/--follow guard, and test coverage) is unchanged and still correct.
No Critical, High, or Medium issues found — approving.
Non-blocking observations (Low / nitpick)
- The commit is titled "check writer errors" but the change actually discards them (
_, _ =) rather than checking/propagating. Cosmetic naming mismatch; behavior is fine for stdout/buffer writers. - The JSON branch is left as-is (still logs an encode error to
os.Stderrand returns), so writer-error handling is now slightly asymmetric between the JSON and text paths. Harmless in practice. - Prior nits still stand and remain non-blocking:
Unknowncolumn is effectively always zero for the local-log path (logLevelToStringdefaults unknown enums to"info"), and the%-20sservice column can misalign for names >20 chars.
Nice iteration — approving.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #491 +/- ##
==========================================
+ Coverage 62.14% 62.23% +0.08%
==========================================
Files 225 226 +1
Lines 29762 29859 +97
==========================================
+ Hits 18497 18583 +86
- Misses 9996 10002 +6
- Partials 1269 1274 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
🚀 Test This PRA preview build ( 🌐 Website PreviewLive Preview: https://jongio.github.io/azd-app/pr/491/ One-Line Install (Recommended)PowerShell (Windows): iex "& { $(irm https://raw.githubusercontent.com/jongio/azd-app/main/cli/scripts/install-pr.ps1) } -PrNumber 491 -Version 0.20.0-pr491"Bash (macOS/Linux): curl -fsSL https://raw.githubusercontent.com/jongio/azd-app/main/cli/scripts/install-pr.sh | bash -s 491 0.20.0-pr491UninstallWhen you're done testing: PowerShell (Windows): iex "& { $(irm https://raw.githubusercontent.com/jongio/azd-app/main/cli/scripts/uninstall-pr.ps1) } -PrNumber 491"Bash (macOS/Linux): curl -fsSL https://raw.githubusercontent.com/jongio/azd-app/main/cli/scripts/uninstall-pr.sh | bash -s 491Build Info:
What to Test: |
Resolves logs test conflicts by keeping the min-level, no-timestamps, and summary test cases from both sides. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
wbreza
left a comment
There was a problem hiding this comment.
Code Review (re-review @ 373c99e) — Approve ✅
Re-reviewed after a new commit landed since my previous approval at d93bc02:
chore: merge main into idea/logs-summary(373c99e) — merges the latestorigin/maininto the branch.
Verification of the merge:
- The PR's net diff vs the merge base (
4e6c9c3) is unchanged — still exactly the summary feature across the same 5 files (logs.go+14,logs_command_test.go+6/-1,logs_filtering.go+4,logs_summary.go+159,logs_summary_test.go+102). logs_summary.goat this head resolves to the same blob (bdda7315) I approved atd93bc02— byte-identical.- No conflict markers in any feature file; the merge integrated cleanly.
- Unrelated features that arrived on
main(--min-level,--no-timestamps) are present equally in base and head, so they net out of this PR and are not part of its contribution.
The summary logic I validated in prior passes (level normalization vs the real constants, nil-timestamp handling, JSON/text rendering, the --summary/--follow guard, and test coverage) is unchanged and still correct.
No Critical, High, or Medium issues found — approving.
Non-blocking observations (Low / nitpick — carried from prior reviews, still applicable)
Unknowncolumn is effectively always zero on the local-log path, sincelogLevelToStringmaps unknown enum values to"info".- The
%-20sservice column can misalign for service names longer than 20 characters (cosmetic). - Writer-error handling is asymmetric: the text path discards
Fprintfreturns (_, _ =) while the JSON path logs an encode error to stderr. Harmless in practice.
Nice work — approving.
Closes #488
Summary
azd app logs --summary--output jsonis usedVerification
go test .\src\cmd\app\commands -run "Test(BuildLogSummary|DisplayLogSummary|ValidateLogsOptions|LogsCommandStructure)"mage testmage build