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 toggle to allow log lines to be JSON colorized #1778

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rm-hull
Copy link
Contributor

@rm-hull rm-hull commented Sep 25, 2022

I've got a tentative PR that adds colorized log lines when the line can be parsed as JSON (when it can't be parsed into JSON, the line is just returned as-is).

Colorizing can be applied permanently in the config, but by default is is toggled off, activated by pressing 'Ctrl-J':
Screenshot 2022-09-25 at 22 38 19

I've marked this as "Draft" for the moment because I'd like to add some tests over this behaviour, and I've also got an outstanding PR open against colorjson (see TylerBrock/colorjson#7) which would be nice to get merged rather than using my forked version, cc/ @TylerBrock.

I followed how the log timestamps were implemented, and added colorization to the model rather than the view. Initially, I had added it to the view and this didn't play nicely with filter or timestamps (this is how I discovered and came to fix #1776), whereas it works fine when located in the model.

Anyhow, hopefully this would be a useful feature on an already amazingly useful tool!

@derailed
Copy link
Owner

@rm-hull Very cool! This was a long awaited feature that many folks voiced. Thank you so much Richard for punching this thru and for the great explanation!! That said, I think we have to look in a bit closer about the cost of this implementation. Having to marshal/unmarshal each line is bound to run gc bunkers especially in this already less than optimal implementation. Let me noodle on this a bit more and take your branch out for a spin...

@derailed derailed added the question Further information is requested label Sep 26, 2022
@rm-hull
Copy link
Contributor Author

rm-hull commented Sep 26, 2022

I didn't look too closely into the model code, but does it trigger the render for every log line or just the ones currently visible in the box view? If the former then, yes I imagine it could incur quite an overhead.

I did start off trying to write a naive parser that maintained a bit of state to determine the current color (whether in a string, in curly brackets, in square brackets, etc + managing that recursively) but it got complicated quickly.

Marshalling to JSON and the delegating to a lib to add color seemed like the right thing to do.

If we can restrict the view to just rendering the visible log lines this might still be ok?

@thinkniko
Copy link
Contributor

@rm-hull Can you change the key to Shift-J for the json toggle?
When viewing logs, I use the J key for scrolling down after scrolling up with the K key.

@rm-hull
Copy link
Contributor Author

rm-hull commented Feb 11, 2023

@thinkniko .. I changed the toggle to Ctrl-J

internal/dao/log_item.go Show resolved Hide resolved
internal/dao/log_item.go Show resolved Hide resolved
@derailed derailed added enhancement New feature or request needs-tlc Pr needs additional updates labels Nov 12, 2023
@derailed
Copy link
Owner

@rm-hull Thanks for this PR Richard. Looks like we have conflicts. Could you update?

…-log

* 'feat/json-log' of github.com:rm-hull/k9s:
  Toggle JSON with Ctrl-J
  [feat] Add toggle to allow log lines to be JSON colorized
  Release v0.27.3 (derailed#1970)
  Add sort by pod count on node view (derailed#1961)
  fix: Show meaningful error message when kubectl exec fails (derailed#1966)
  set default sinceSeconds to 300 (derailed#1965)
  Match ${XXX} environment variables (derailed#1896)
  Bump github.com/cenkalti/backoff/v4 from 4.1.3 to 4.2.0 (derailed#1957)
  Bump k8s.io/klog/v2 from 2.80.1 to 2.90.0 (derailed#1958)
  Bump golang from 1.19.5-alpine3.16 to 1.20.0-alpine3.16 (derailed#1956)
  Bump github.com/fatih/color from 1.13.0 to 1.14.1 (derailed#1959)
  Added Nightfox theme (derailed#1960)
  fix: Add missing help menu to gruvbox-dark skin (derailed#1969)
  Bump helm.sh/helm/v3 from 3.11.0 to 3.11.1 (derailed#1963)
* 'master' of github.com:derailed/k9s: (130 commits)
  added flux suspended resources retrieval plugin (derailed#1584)
  Provide white blur so images work in dark modes (derailed#1597)
  Add context to get-all (derailed#1701)
  fix brew command in the readme (derailed#2012)
  Add support for using custom kubeconfig with log_full plugin (derailed#2014)
  feat: allow for multiple plugin files in $XDG_DATA_DIRS/k9s/plugins (derailed#2029)
  Clean up issues introduced by derailed#2125 (derailed#2289)
  Pod view resembles more the output of kubectl get pods -o wide (derailed#2125)
  Update README.md with snap install (derailed#2262)
  Add snapcraft config (derailed#2123)
  storageclasses view keeps the same output as kubectl get sc (derailed#2132)
  Fix merge issues with PR derailed#2168 (derailed#2288)
  Add colour config for container picker (derailed#2140)
  Add env var to disable node pod counts (derailed#2168)
  Use current k9s NS if new context has no default NS (derailed#2197)
  Bump actions/setup-go from 4.0.1 to 4.1.0 (derailed#2200)
  fix: trigger a single log refresh after changing 'since' (derailed#2202)
  Add crossplane plugin (derailed#2204)
  fix(derailed#1359): add option to keep missing clusters in config (derailed#2213)
  K9s release v0.28.2
  ...
@rm-hull rm-hull marked this pull request as ready for review November 12, 2023 22:22
@rm-hull
Copy link
Contributor Author

rm-hull commented Nov 12, 2023

@rm-hull Thanks for this PR Richard. Looks like we have conflicts. Could you update?

Conflicts fixed

@derailed derailed added needs-review PR needs to be reviewed and removed needs-tlc Pr needs additional updates question Further information is requested labels Nov 12, 2023
* 'master' of github.com:derailed/k9s: (169 commits)
  fix: align build image Go version with go.mod (derailed#2812)
  Bump github.com/docker/docker (derailed#2816)
  Bump golangci/golangci-lint-action from 6.0.1 to 6.1.0 (derailed#2813)
  Add comment about Escape keybinding (derailed#2817)
  proper handle OwnerReference for manually created job (derailed#2772)
  [derailed#2773] fix freebsd build failure (derailed#2775)
  install copyright file into correct location (derailed#2780)
  fix status for completed pods in workload view (derailed#2729)
  update deps
  [Maint] Bump grype rev
  K9s/release v0.32.5 (derailed#2740)
  --- (derailed#2707)
  fix view sorting being reset (derailed#2736)
  use policy/v1 instead of policy/v1beta1 (derailed#2732)
  Bump alpine from 3.19.1 to 3.20.0 (derailed#2721)
  fix: jump to namespaceless owner reference (derailed#2718)
  feat: Add plugins for argo-rollouts (derailed#2711)
  Bump golangci/golangci-lint-action from 5.1.0 to 6.0.1 (derailed#2702)
  allow jumping to the owner of the resource (derailed#2700)
  fix: job color based on failures (derailed#2686) (derailed#2698)
  ...
@rm-hull
Copy link
Contributor Author

rm-hull commented Aug 12, 2024

Hi @derailed, @slimus - I rebased and fixed merge conflicts again, so the PR should be up-to-date w.r.t. the main branch .. would be good to get this in 🙏

@KevinGimbel
Copy link

LGTM! I checked out the PR and tested it with a few pod logs, works as expected.

@derailed I think this can be merged with the next release.

@vparmeland
Copy link

Merge this PR ??

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-review PR needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants