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: Enhanced pod logs viewer. Fixes #6199 #11000 #10166 #11030

Merged
merged 25 commits into from
Mar 7, 2023

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Oct 21, 2022

Fixes #6199
Fixes #11000
Fixes #10166

I've found the pod log viewer to be unusable. It's slow and works in a unintuitive manner. I can't sort my problems out with it.

  • I want to know why my container is broken.
  • I want to know why my container was broken.

This PR:

  • The log viewer now essentially works in two mode - "follow" or "historical".
  • In "follow" mode the logs are tailed.
  • In "historical" mode you can choose a "since time" and number of lines to view.
  • In both modes you can choose to filter by text.

The changes use the react-virtualization module so you can now scroll through thousands of log lines without crashing or slowing your browser.

image

image

@alexec alexec requested a review from rbreeze October 21, 2022 19:09
@codecov
Copy link

codecov bot commented Oct 22, 2022

Codecov Report

Patch coverage has no change and project coverage change: -0.02 ⚠️

Comparison is base (42d1c85) 47.79% compared to head (c0fad73) 47.78%.

❗ Current head c0fad73 differs from pull request most recent head cb52ad6. Consider uploading reports for the commit cb52ad6 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11030      +/-   ##
==========================================
- Coverage   47.79%   47.78%   -0.02%     
==========================================
  Files         246      246              
  Lines       41968    41921      -47     
==========================================
- Hits        20058    20031      -27     
+ Misses      19910    19891      -19     
+ Partials     2000     1999       -1     
Impacted Files Coverage Δ
applicationset/generators/matrix.go 69.74% <0.00%> (-7.76%) ⬇️
util/lua/lua.go 76.87% <0.00%> (-1.03%) ⬇️
util/git/creds.go 50.31% <0.00%> (-0.63%) ⬇️
cmpserver/plugin/plugin.go 78.51% <0.00%> (-0.50%) ⬇️
controller/appcontroller.go 52.62% <0.00%> (-0.42%) ⬇️
cmd/argocd/commands/app.go 21.49% <0.00%> (-0.02%) ⬇️
util/settings/settings.go 49.36% <0.00%> (ø)
applicationset/generators/cluster.go 80.27% <0.00%> (ø)
server/server.go 55.64% <0.00%> (+0.12%) ⬆️
pkg/apis/application/v1alpha1/types.go 59.02% <0.00%> (+0.48%) ⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@alexec alexec marked this pull request as ready for review October 22, 2022 21:40
@tooptoop4
Copy link

'Show the first 4 characters of the container name.' - what if 2 containers start with same prefix? current behavior of full name is handy

@alexec
Copy link
Contributor Author

alexec commented Nov 21, 2022

@rbreeze bump!

@alexec
Copy link
Contributor Author

alexec commented Dec 1, 2022

@rbreeze bump!

@crenshaw-dev crenshaw-dev self-assigned this Dec 20, 2022
@crenshaw-dev crenshaw-dev added this to the v2.7 milestone Dec 20, 2022
@alexec alexec changed the title feat: Tweak the pod logs viewer feat: Re-work the pod logs viewer Jan 5, 2023
@alexec alexec requested review from crenshaw-dev and removed request for rbreeze February 10, 2023 17:15
@alexec
Copy link
Contributor Author

alexec commented Feb 10, 2023

@michael12312 how do I escalate this PR? It's a small(ish) change. It is isolated to one part of the code. It'll make a big improvement is allow you to debug problems using Argo CD.

The current log viewer is nearly unusable.

Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
Copy link
Contributor Author

alexec commented Feb 18, 2023

Don't merge. Need to figure out what to do with the performance issue when many log lines.

Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
Copy link
Contributor Author

alexec commented Feb 18, 2023

@crenshaw-dev @rbreeze ready for review again.

@alexec alexec changed the title feat: Re-work the pod logs viewer feat: Enhanced pod logs viewer Feb 19, 2023
@crenshaw-dev
Copy link
Collaborator

How difficult would it be to re-enable color handling in the logs?
image

@crenshaw-dev
Copy link
Collaborator

I'm not sure the search box makes sense for these three controls.
image

Even if it does make sense for the container dropdown, I think it should be hidden when there's only one container.

Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec
Copy link
Contributor Author

alexec commented Feb 23, 2023

  • Fixed colors.
  • Changed the auto-completes into vanilla selects.
  • Only show container selector when there is more than one container.

@alexec
Copy link
Contributor Author

alexec commented Feb 23, 2023

image

Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec changed the title feat: Enhanced pod logs viewer feat: Enhanced pod logs viewer. Fixes #6199 #11000 #10166 Feb 26, 2023
@alexec
Copy link
Contributor Author

alexec commented Mar 1, 2023

7 days from chatting with Jesse would be 4:30pm tomorrow.

@crenshaw-dev
Copy link
Collaborator

@jessesuen @rbreeze are y'all in alignment that this is the way to go?

Signed-off-by: Alex Collins <alex_collins@intuit.com>
Copy link
Member

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

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

LGTM, just one comment that can be addressed later: I miss the ability to quickly highlight a line copy it to the clipboard.

@@ -41,6 +41,7 @@
"react-router": "^4.3.1",
"react-router-dom": "^4.2.2",
"react-svg-piechart": "^2.4.2",
"react-virtualized": "^9.22.3",
Copy link
Member

Choose a reason for hiding this comment

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

Where is this being used?

Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

Yep directionally when I spoke with @alexec, I liked how this is more in line with the kubectl experience, so I support the change. As for the UI implementation, 🤷‍♂️

Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec merged commit 0f500a5 into argoproj:master Mar 7, 2023
@alexec alexec deleted the plv branch March 7, 2023 18:11
@alexec
Copy link
Contributor Author

alexec commented Mar 7, 2023

🥳

@dhirschfeld
Copy link

These improvements look great & I can't wait to try them out! 🚀

Being able to search more than 100 lines will make the logs much more useful.

I would've loved if my issue #11455 could have made the cut, but that'll keep for in future...

@rodrigc
Copy link
Contributor

rodrigc commented May 16, 2023

@alexec I'm trying the enhanced log viewer in Argo 2.7.2 and it looks great. Thanks for doing this!

One comment that I have is that the log viewer has a button to enable Dark Mode, but just for the log viewer.

One suggestion that I have is that the default behavior of Dark Mode for the log viewer
should be to inherit the setting for Dark Mode from /settings/appearance.

It is a bit weird to enable Dark Mode for the entire ArgoCD UI, but then have the Log Viewer be not in Dark Mode.

@alexec
Copy link
Contributor Author

alexec commented Jul 26, 2023

@rodrigc that is strange! Historical. You wanna fix?

@rodrigc
Copy link
Contributor

rodrigc commented Jul 26, 2023

@alexec I love ArgoCD, but don't have the bandwidth to fix this

yyzxw pushed a commit to yyzxw/argo-cd that referenced this pull request Aug 9, 2023
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
7 participants