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

Control logging via command line or environment variable #1163

Merged
merged 1 commit into from
Sep 11, 2022

Conversation

foresto
Copy link
Contributor

@foresto foresto commented Aug 24, 2022

Nheko is very chatty in its log output, generating log noise (which complicates diagnostics) and needless disk writes (which affect power consumption and SSD life). This patch provides control over it, with command line options for:

  • Setting the global log level
  • Setting log levels for individual components (loggers)
  • Disabling log file writes
  • Disabling stderr writes (which are often written to ~/.xsession-errors)

The old --debug command line option still works, at least for now, and is overridden by the new fine-grained option.

Log levels and output type (file/stderr) can also be set by environment variables. (This is especially convenient for flatpak users, who can flatpak override --env= instead of having to copy and edit the .desktop file to add command line options.)

Partially addresses #665.

@foresto
Copy link
Contributor Author

foresto commented Aug 24, 2022

Looks like lint flagged a few formatting issues. I can easily reorder the #include lines, but the rest doesn't make sense to me.

Why does it want to indent with 2 spaces when the existing code is clearly using 4 spaces? Was lint misconfigured?

Why does it want to insert more than 30 spaces of indentation in the last flagged section of main.cpp? That wastes a lot of space and doesn't align with anything.

@foresto
Copy link
Contributor Author

foresto commented Aug 24, 2022

What version of lint is being used here? What configuration?

@redsky17
Copy link
Member

we use clang format for linting and our configuration is in the root of the repository under .clang-format. Our lint isn't misconfigured. I promise it's consistent. If it wasn't, the 'lint' step would mark everything inconsistent as needing to be fixed. You should be able to run make lint from the root nheko git directory and it'll fix everything fo ryou.

@foresto
Copy link
Contributor Author

foresto commented Aug 24, 2022

Ah, there it is.

ContinuationIndentWidth: 2
IndentWidth: 4

That's what I mean by inconsistent. (Maybe I should have said "internally inconsistent".) If that's the format you prefer, though, that's what I'll use. :)

Thanks for the info. I'll reformat and resubmit.

@deepbluev7
Copy link
Member

Can you update the manpage as well? I think that just mentions the debug switch atm.

src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
@foresto
Copy link
Contributor Author

foresto commented Aug 25, 2022

  • Rephrased the help text to be more descriptive, following your suggested change, and to be more specific about the syntax.
  • Updated the man page.
  • Added support for the mtx logger.

@foresto
Copy link
Contributor Author

foresto commented Sep 5, 2022

Changes since last draft:

  • --log-level instead of --loglevel.
  • --log=<type> instead of --nolog, --nostderr.
  • NHEKO_LOG environment variable. (The comma-separated --log=file,stderr syntax was superfluous before, since it was the default, but it can now be useful for overriding the environment variable.) Especially convenient for flatpak users, who can now flatpak override --env= instead of copying and editing the .desktop file to control logging.
  • Translatable command line help text.

I guess --log=file,stderr is fine, although I still prefer a common prefix like --log-to=file.

I went with --log because easy typing is important to me. Thanks for allowing it. However:

What about satisfying the desire for a common prefix while still allowing easy typing, by adding short forms of these options? For example:

-l --log-level
-L --log-to (or --log-type or --log-output)

(I'm a little embarrassed for not suggesting this earlier. In new projects, I usually reserve short command line options until I know what the most common operations will be, since there are only so many letters available. Since Nheko is already in real-world use and its common operations are all through the GUI, it's a non-issue here.)

@foresto foresto force-pushed the log-options branch 4 times, most recently from a1d666b to 4534333 Compare September 10, 2022 07:46
@foresto
Copy link
Contributor Author

foresto commented Sep 10, 2022

  • Rebased to master.
  • Implemented short & long options. (Both long options now use the --log- prefix).
  • Updated environment variable to match its corresponding renamed option.
  • Alphabetically ordered option help text.

src/main.cpp Outdated Show resolved Hide resolved
@deepbluev7
Copy link
Member

I think this looks pretty nice now, I would just prefer it if the different log level flags were not merged, but instead would overwrite each other. Otherwise it can be hard to troubleshoot, why something behaves weirdly, imo.

Nheko is very chatty in its log output, generating log noise (which
complicates diagnostics) and needless disk writes (which affect power
consumption and SSD life).  This patch introduces command line options
and environment variables to control log levels and output type.

The old --debug command line option still works, at least for now.
It is overridden by the new command line options when they are used.

Partially addresses Nheko-Reborn#665.
@deepbluev7
Copy link
Member

Awesome, thank you for your patience!

@deepbluev7 deepbluev7 merged commit 02adcfd into Nheko-Reborn:master Sep 11, 2022
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