Skip to content

Log viewer parity with headlamp (download + ALL lines) - #963

Open
Tejhan Diallo (tejhan) wants to merge 1 commit into
Azure:headlamp-downstreamfrom
tejhan:logs-viewer-parity
Open

Tejhan Diallo (tejhan) wants to merge 1 commit into
Azure:headlamp-downstreamfrom
tejhan:logs-viewer-parity

Conversation

@tejhan

Copy link
Copy Markdown
Collaborator

Description

This PR addresses #914, and updates our LogsViewer to 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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Related Issues

Closes #914

Changes Made

  • Added download button to LogsViewer toolbar.
  • Added All option (value={-1}) to the lines dropdown.
  • Updated v2 fetchLogs to omit tailines when -1 so All can return the full log
  • Mention any dependencies added/removed

Testing

  • Manual testing completed

To test, attempt to download logs. The logic is highly similar to existing, proven HL logic.

Copilot AI lite review requested due to automatic review settings September 11, 2026 16:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 All lines option and omits tailLines for 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, rawLogs is keyed by pod name, but this flattening preserves pod-list insertion order and drops the pod name. useParsedLogs sorts 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.

Comment thread frontend/src/components/common/LogsViewer/LogsViewer.tsx Outdated
Comment thread frontend/src/components/common/LogsViewer/LogsViewer.tsx Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 All path 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 selecting All still loses blank lines in the downloaded file.
  if (lines !== -1) {
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread frontend/src/components/common/LogsViewer/LogsViewer.tsx Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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: fetchLogs drops empty records in its transform (fetchLogs.tsx:80-84), and joining rawLogs here 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 LogsButton test suite is skipped and there is no LogsViewer test; 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 === -1 request branch has no regression test asserting that tailLines is omitted for All while remaining present for bounded options. The v2 API layer has focused tests (for example fetch.test.ts and makeUrl.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

@illume

Copy link
Copy Markdown
Collaborator

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants