Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add gather & download logs button in troubleshooting #5119

Merged
merged 9 commits into from Feb 8, 2024

Conversation

cdrage
Copy link
Contributor

@cdrage cdrage commented Dec 5, 2023

feat: Add gather & download logs button in troubleshooting

What does this PR do?

  • Adds a button to collect logs which is currently only hardcoded to
    retrieve the console logs
  • Adds a button to download the logs as a zip file with all logs stored
    within as a .txt file
  • Shows which files have been generated and what will be zipped.

Screenshot / video of UI

Screen.Recording.2023-12-05.at.2.23.40.PM.mov

What issues does this PR fix or reference?

Closes #5048

How to test this PR?

  1. Go to troubleshooting
  2. Click the collect logs & download logs buttons.

@cdrage cdrage requested review from benoitf and a team as code owners December 5, 2023 01:36
@cdrage cdrage requested review from jeffmaury and feloy and removed request for a team December 5, 2023 01:36
@cdrage cdrage marked this pull request as draft December 5, 2023 01:36
@benoitf
Copy link
Collaborator

benoitf commented Dec 5, 2023

includes on macOS

~/Library/Logs/Podman\ Desktop/launchd-stdout.log
~/Library/Logs/Podman\ Desktop/launchd-stderr.log

@cdrage cdrage force-pushed the add-grab-logs branch 3 times, most recently from 5ce4380 to b8e3aaf Compare December 5, 2023 19:16
@cdrage
Copy link
Contributor Author

cdrage commented Dec 5, 2023

Ready for review!

Would like some input on how to improve the UI as well as other logs we should be collecting (Windows and Linux specifically).

In a later PR we should implement methods for extensions to add what logs they'd also like to have in the .zip

@cdrage cdrage force-pushed the add-grab-logs branch 5 times, most recently from 145b098 to c6e6b35 Compare December 6, 2023 14:08
@cdrage cdrage marked this pull request as ready for review December 6, 2023 14:08
Copy link
Collaborator

@deboer-tim deboer-tim left a comment

Choose a reason for hiding this comment

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

This review is going to jump around a lot, sorry in advance. :)

+1 to all the basic parts (zip, file dialog, etc) being there. It works!

Title: I know this as 'must gather' from the past 20 years and the name kinda sticks, but like 'onboarding' I don't know if the average user knows the term. Maybe something like "Gather Log Files" would be enough with an expanded description? e.g. "Bundle all log files into a zip for future (or offline?) problem determination".

From a core/UI standpoint I don't know when we'd ever want just console logs or just system logs, plus we'll have extension logs, etc. I think it's probably simpler if index.ts just exposes a 'getLogs', and if we ever need something more fine-grained it could have optional parameters in the future.

Going a step further I think it's nice it shows you a list of files that it's going to zip and uses mono-font to list them - but I think the usage pattern is always to click one, then the other. I'm wondering if we should even bother having a Collect Logs button and exposing it through the API, or just have a single button? Or maybe a single button and it just gives you a summary of the files that it included after.

We're a desktop app, so a "Download" button is a bit out of place. Maybe just "Save"?

Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

Missing implementations

packages/main/src/plugin/troubleshooting.ts Outdated Show resolved Hide resolved
packages/main/src/plugin/troubleshooting.ts Outdated Show resolved Hide resolved
@cdrage cdrage force-pushed the add-grab-logs branch 5 times, most recently from a0f8ce7 to 457126a Compare December 19, 2023 22:03
@cdrage
Copy link
Contributor Author

cdrage commented Dec 19, 2023

Ready for another review 👍 / UI updated to use the new "tabbed" logs.

@cdrage
Copy link
Contributor Author

cdrage commented Feb 5, 2024

One button: 👍🏼

I was expecting one button to also mean there is only one exposed function from main though too, e.g. something like window.saveLogsToZip(filename). All of the logic to gather console/system logs would be in here, and the UI only gets to say where to save it.

I guess implicit in the previous comment is that IMHO it would be best if the main package wasn't doing UI-things like opening the save dialog and we left this in renderer - i.e. GatherLog page would open the dialog to pick a location and then call the window function.

Done! Ready for another review. Put it down to one API call (that also gets the console logs via memoryLogs).

Copy link
Collaborator

@deboer-tim deboer-tim left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes, this is much simpler and cleaner to me now. I'd still appreciate a couple more minor tweaks as listed.

packages/preload/src/index.ts Outdated Show resolved Hide resolved
packages/main/src/plugin/troubleshooting.ts Outdated Show resolved Hide resolved
@cdrage
Copy link
Contributor Author

cdrage commented Feb 6, 2024

Screenshot 2024-02-06 at 1 52 02 PM

Updated! I've also changed for it to retain the original file name extension, and if there is none, default to .txt

### What does this PR do?

* Adds a button to collect logs which is currently only hardcoded to
  retrieve the console logs
* Adds a button to download the logs as a zip file with all logs stored
  within as a .txt file
* Shows which files have been generated and what will be zipped.

### Screenshot / video of UI

<!-- If this PR is changing UI, please include
screenshots or screencasts showing the difference -->

### What issues does this PR fix or reference?

<!-- Include any related issues from Podman Desktop
repository (or from another issue tracker). -->

Closes containers#5048

### How to test this PR?

<!-- Please explain steps to reproduce -->

1. Go to troubleshooting
2. Click the collect logs & download logs buttons.

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Copy link
Collaborator

@deboer-tim deboer-tim left a comment

Choose a reason for hiding this comment

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

Thanks for all the changes, LGTM.

packages/main/src/plugin/troubleshooting.spec.ts Outdated Show resolved Hide resolved
packages/main/src/plugin/troubleshooting.ts Outdated Show resolved Hide resolved
packages/main/src/plugin/troubleshooting.ts Outdated Show resolved Hide resolved
packages/main/src/plugin/troubleshooting.ts Outdated Show resolved Hide resolved
packages/main/src/plugin/troubleshooting.ts Outdated Show resolved Hide resolved
packages/main/src/plugin/troubleshooting.ts Outdated Show resolved Hide resolved
cdrage and others added 7 commits February 7, 2024 10:12
Co-authored-by: Florent BENOIT <fbenoit@redhat.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Co-authored-by: Florent BENOIT <fbenoit@redhat.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Co-authored-by: Florent BENOIT <fbenoit@redhat.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Co-authored-by: Florent BENOIT <fbenoit@redhat.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
…rLogs.svelte

Co-authored-by: Florent BENOIT <fbenoit@redhat.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
…rLogs.svelte

Co-authored-by: Florent BENOIT <fbenoit@redhat.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Co-authored-by: Florent BENOIT <fbenoit@redhat.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@cdrage
Copy link
Contributor Author

cdrage commented Feb 7, 2024

@benoitf thank you for the review! implemented all the requested changes and it's ready for another 👍

@cdrage cdrage force-pushed the add-grab-logs branch 2 times, most recently from 1dd76fa to 3a05a8c Compare February 7, 2024 17:24
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

if you can update glitches like copyright date, formatting the license, and fileName -> filename before merging

else 👍

packages/main/src/plugin/troubleshooting.spec.ts Outdated Show resolved Hide resolved
packages/main/src/plugin/troubleshooting.ts Outdated Show resolved Hide resolved
packages/main/src/plugin/troubleshooting.spec.ts Outdated Show resolved Hide resolved
packages/main/src/plugin/troubleshooting.ts Outdated Show resolved Hide resolved
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@cdrage
Copy link
Contributor Author

cdrage commented Feb 8, 2024

Updated based on the changes, thank you for the reviews! Enabling auto-merge now 🎉

@cdrage cdrage merged commit 3d0e7d9 into containers:main Feb 8, 2024
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.8.0 milestone Feb 8, 2024
Copy link
Collaborator

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

I noticed that packages/renderer/src/lib/troubleshooting/TroubleshootingGatherLogs.svelte has no unit test

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.

Log collection
7 participants