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 logger to Zellij #592

Merged
merged 20 commits into from
Jul 21, 2021
Merged

Conversation

qepasa
Copy link
Contributor

@qepasa qepasa commented Jun 30, 2021

We add log4rs create for logging across Zellij. Additionally, we capture
stderr output from plugins and log it the same log file as other
Zellij logs.

We set up the logger in zellij-utils/src/logging.rs. The logger parses
configuration from zellij-utils/assets/config/log4rs.yml file. After calling
configure_logger from logging.rs the logger should be available in the
calling thread and all children threads.
Second part of this PR is the mechanism that captures stderr output from
plugins. We setup a customized drain (DecoratingPipe) for plugin
processes in zellij-server/src/wasm_vm.rs. The DecoratingPipe logs the
stderr output from plugins using the same logger as we will use everywhere
else but with custom format.

Implements: #561

Todo:

  • (kind of, see comment in DecoratingPipe::flush) make sure we detect byte boundaries correctly in DecoratingPipe::write
  • work on log format i.e.: add colors, pad certain parts (looks like there is no way to add colors to file for now)
  • cleanup

Note:
I didn't go with slog due to it not being fully compatible with standard rust log::info (warn, err etc.), instead that project prefers to use it's own slog::info (warn, err etc.). I tried to use adapter provided by slog project but it was difficult to share the logger between packages. Slog basically expects you to pass log instance across your codebase, which seems like a lot of effort in case of a project that wasn't initially created with slog usage in mind. Also, there is no official support for adding thread name to log, emitting colored log to file or filtering modules.

Sample from log file:

INFO   |zellij_utils::logging    | 2021-07-13 23:15:16.650 [main      ] [zellij-utils/src/logging.rs:56]: Zellij logger initialized 
INFO   |zellij                   | 2021-07-13 23:15:16.650 [main      ] [src/main.rs:26]: Starting Zellij! 
INFO   |zellij_utils::logging    | 2021-07-13 23:15:16.698 [main      ] [zellij-utils/src/logging.rs:56]: Zellij logger initialized 
INFO   |zellij                   | 2021-07-13 23:15:16.698 [main      ] [src/main.rs:26]: Starting Zellij! 
INFO   |zellij_server            | 2021-07-13 23:15:16.709 [main      ] [zellij-server/src/lib.rs:113]: starts server 
INFO   |zellij_server::wasm_vm   | 2021-07-13 23:15:16.762 [wasm      ] [zellij-server/src/wasm_vm.rs:61]: Wasm main thread starts 
INFO   |tab-bar                  | 2021-07-13 23:15:21.357 [id: 0     ] Creating decorating pipe for plugin: tab-bar! 
DEBUG  |tab-bar                  | 2021-07-13 23:15:21.396 [id: 0     ] [default-plugins/tab-bar/src/main.rs:23] "hello from plugin main :)" = "hello from plugin main :)" 
INFO   |strider                  | 2021-07-13 23:15:27.802 [id: 1     ] Creating decorating pipe for plugin: strider! 
DEBUG  |strider                  | 2021-07-13 23:15:27.843 [id: 1     ] [default-plugins/strider/src/main.rs:8] "hello from plugin main :)" = "hello from plugin main :)" 
DEBUG  |strider                  | 2021-07-13 23:15:27.843 [id: 1     ] [default-plugins/strider/src/main.rs:12] "hello from load2?" = "hello from load2?" 
INFO   |status-bar               | 2021-07-13 23:15:32.170 [id: 2     ] Creating decorating pipe for plugin: status-bar! 
DEBUG  |status-bar               | 2021-07-13 23:15:32.198 [id: 2     ] [default-plugins/status-bar/src/main.rs:22] "hello from plugin main :)" = "hello from plugin main :)" 
DEBUG  |status-bar               | 2021-07-13 23:15:32.199 [id: 2     ] [default-plugins/status-bar/src/main.rs:136] "hello from load" = "hello from load" 
INFO   |zellij_client            | 2021-07-13 23:24:45.110 [main      ] [zellij-client/src/lib.rs:304]: Bye from Zellij! 
INFO   |zellij_utils::logging    | 2021-07-13 23:27:27.729 [main      ] [zellij-utils/src/logging.rs:63]: Zellij logger initialized 
INFO   |zellij                   | 2021-07-13 23:27:27.729 [main      ] [src/main.rs:24]: Starting Zellij! 

We add log4rs create for logging across Zellij. Additionally, we capture
`stderr` output from plugins and log it the same log file as other
Zellij logs.
@TheLostLambda
Copy link
Member

This looks super cool! I'm still a bit homeless at the moment, as I'm between places and moving, but I'll try to give this a look either today or tomorrow! thanks so much for doing this!

Is there anything in particular you'd like me to look at or think about?

@qepasa
Copy link
Contributor Author

qepasa commented Jul 6, 2021

Hey, thanks! For now this is still a bit of WIP, but I think that the general mechanism for plugin logs will not change. Neither will the logging crate (unless you or other contributors suggest otherwise).

As for the initial review, feel free to have a look at it but please remember that it's a bit crude at the moment and might change a bit. I will try to finish it as fast as possible and ping on discord when it's ready for a proper, detailed review. 😃. I added a small guide in the initial comment to make the review as easy as possible.

@qepasa qepasa changed the title WIP: feat: Add logger to Zellij feat: Add logger to Zellij Jul 9, 2021
Copy link
Member

@TheLostLambda TheLostLambda left a comment

Choose a reason for hiding this comment

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

Thank you for all of your hard work! This is looking great! I left a couple of comments as a first pass.

The main things I think we should look into are making the plugin log-lines fit with the rest of the log lines (adding a date at least), and writing to the proper Zellij log directory in /tmp.

Overall, I'm very pleased with this!

.gitignore Outdated Show resolved Hide resolved
default-plugins/status-bar/src/main.rs Outdated Show resolved Hide resolved
zellij-server/src/decorating_pipe.rs Outdated Show resolved Hide resolved
zellij-server/src/decorating_pipe.rs Outdated Show resolved Hide resolved
zellij-server/src/decorating_pipe.rs Outdated Show resolved Hide resolved
zellij-server/src/decorating_pipe.rs Outdated Show resolved Hide resolved
zellij-server/src/wasm_vm.rs Outdated Show resolved Hide resolved
zellij-utils/assets/config/log4rs.yml Outdated Show resolved Hide resolved
zellij-utils/assets/config/log4rs.yml Outdated Show resolved Hide resolved
zellij-utils/assets/config/log4rs.yml Outdated Show resolved Hide resolved
…. Add plugin_id to plugin log. Move logger init from file to in-code initialization and change logging file to zellij directory. Change format of timestamp.
zellij-server/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@TheLostLambda TheLostLambda left a comment

Choose a reason for hiding this comment

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

I love it! I left a couple of small comments for you to address if you'd like, but I'm happy with merging this! Feel free to do any final cleanup (I'm not sure if you want to keep all of the test messages in?) and merge when ready!

zellij-server/src/logging_pipe.rs Outdated Show resolved Hide resolved
zellij-server/src/logging_pipe.rs Show resolved Hide resolved
zellij-server/src/logging_pipe.rs Outdated Show resolved Hide resolved
zellij-server/src/logging_pipe.rs Outdated Show resolved Hide resolved
zellij-server/src/logging_pipe.rs Show resolved Hide resolved
@TheLostLambda
Copy link
Member

@qepasa Let me know if you need any help with merge conflicts! I think it's just been a bump in the crate versions though, so it shouldn't be too difficult :)

@qepasa
Copy link
Contributor Author

qepasa commented Jul 21, 2021

@TheLostLambda Done 😃 Also, changed the LoggingPipe::Read to return error as discussed above. Please feel free to merge it

@TheLostLambda
Copy link
Member

@qepasa Looks stellar! Thank you for all of your hard work on this! It's been lovely working with you and I'm excited to see you around more!

I'll merge it now and update the changelog!

@TheLostLambda TheLostLambda merged commit b55c879 into zellij-org:main Jul 21, 2021
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