Log viewer parity with headlamp (download + ALL lines) - #963
Tejhan Diallo (tejhan) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Download formatting, resource cleanup, and multi-pod ordering/attribution issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds full-log selection and download support to the AKS Desktop log viewer.
Changes:
- Adds an
Alllines option and omitstailLinesfor full logs. - Adds timestamped log downloads.
File summaries
| File | Summary and findings |
|---|---|
frontend/src/lib/k8s/api/v2/fetchLogs.tsx |
Supports fetching all log lines. |
frontend/src/components/common/LogsViewer/LogsViewer.tsx |
Adds the download action and All selector. Findings: downloaded lines are concatenated without newline delimiters (moderate, 3 votes); Blob URLs and anchors are not cleaned up (nit, 3 votes); workload downloads lose pod attribution and displayed timestamp ordering (moderate, 1 vote). |
Review details
Suppressed comments (1)
frontend/src/components/common/LogsViewer/LogsViewer.tsx:82
- For workload items,
rawLogsis keyed by pod name, but this flattening preserves pod-list insertion order and drops the pod name.useParsedLogssorts the same data by timestamp and labels multi-pod rows, so a downloaded workload log is neither in the displayed order nor attributable to its source pod; construct this branch with pod prefixes and the same timestamp ordering.
: Object.values(rawLogs).flat().join('');
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
31cabe7 to
ef18b35
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Address the two moderate issues affecting complete and attributable log downloads.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
frontend/src/lib/k8s/api/v2/fetchLogs.tsx:56
- The new
Allpath still cannot return the full log verbatim: the stream transform below only enqueues truthy lines (fetchLogs.tsx:82-84), so empty log records are discarded. Preserve empty records while distinguishing the final partial chunk; otherwise selectingAllstill loses blank lines in the downloaded file.
if (lines !== -1) {
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
ef18b35 to
52930d3
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Full-log downloads can lose blank lines, and the new paths lack regression coverage.
Review details
Suppressed comments (3)
frontend/src/components/common/LogsViewer/LogsViewer.tsx:87
- This serializer cannot produce a full log when the stream contains blank lines:
fetchLogsdrops empty records in its transform (fetchLogs.tsx:80-84), and joiningrawLogshere therefore removes those lines permanently. Preserve empty records in the fetch path or retain the original response text before creating the download; otherwise the new “full log” download is lossy.
const content = Array.isArray(rawLogs)
? rawLogs.join('\n')
: // Multi-pod (Logs tab): tag each line with its pod and sort by timestamp to match the view.
Object.entries(rawLogs)
.flatMap(([pod, podLogs]) => podLogs.map(log => ({ pod, log })))
.sort((a, b) => a.log.localeCompare(b.log))
.map(({ pod, log }) => `[${pod}] ${log}`)
.join('\n');
frontend/src/components/common/LogsViewer/LogsViewer.tsx:87
- The new download behavior has no automated coverage for either raw-log shape. In particular, the pod and multi-pod branches differ in line serialization and pod tagging, while the only related
LogsButtontest suite is skipped and there is noLogsViewertest; add a regression test that inspects the generated Blob/download URL so these formats cannot regress silently.
function downloadLogs() {
// Cuts off the last 5 digits of the timestamp to remove the milliseconds
const time = new Date().toISOString().replace(/:/g, '-').slice(0, -5);
const content = Array.isArray(rawLogs)
? rawLogs.join('\n')
: // Multi-pod (Logs tab): tag each line with its pod and sort by timestamp to match the view.
Object.entries(rawLogs)
.flatMap(([pod, podLogs]) => podLogs.map(log => ({ pod, log })))
.sort((a, b) => a.log.localeCompare(b.log))
.map(({ pod, log }) => `[${pod}] ${log}`)
.join('\n');
frontend/src/lib/k8s/api/v2/fetchLogs.tsx:59
- The new
lines === -1request branch has no regression test asserting thattailLinesis omitted for All while remaining present for bounded options. The v2 API layer has focused tests (for examplefetch.test.tsandmakeUrl.test.ts), so please add coverage for this query contract before relying on manual testing; otherwise a future change can silently break the full-log option.
// Negative tailLines parameter fetches all logs. If it's non negative, it fetches
// the tailLines number of logs.
if (lines !== -1) {
query.tailLines = String(lines);
}
const url = makeUrl(`/api/v1/namespaces/${namespace}/pods/${podName}/log`, query);
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
btw. I think we should probably remove the custom log viewer. The one in headlamp has gotten many more improvements, and there are many bugs AFAIK in this custom one? |
Description
This PR addresses #914, and updates our
LogsViewerto include the download button & also allow users to view all lines (previously capped at 2500).This PR re-uses logic from the base Headlamp's Logs tool, but we are keeping our logs tool distinct as initially intended, thus I've brought the missing pieces here so we can have closer parity & still be able to customize this for AKSD - related wants further down the line. (vs. completely duping HL's logs)
Type of Change
Related Issues
Closes #914
Changes Made
LogsViewertoolbar.Alloption (value={-1}) to the lines dropdown.fetchLogsto omit tailines when-1so All can return the full logTesting
To test, attempt to download logs. The logic is highly similar to existing, proven HL logic.