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

Add command line option --LogModes #12919

Merged

Conversation

kiminuo
Copy link
Collaborator

@kiminuo kiminuo commented Apr 23, 2024

Contributes to #9805

This PR adds --LogModes command line option to specify that one wants to log to only specific logging targets1. Specifically one can omit file to avoid storing logging data.

See also #9805 (comment).

Testing

dotnet build && dotnet run --framework net8.0 -- --LogModes="" # Do not log at all
dotnet build && dotnet run --framework net8.0 -- --LogModes="file" # Log only to file
dotnet build && dotnet run --framework net8.0 -- --LogModes="console" # Log only to console
dotnet build && dotnet run --framework net8.0 -- --LogModes="file,console" # Log to console & file
dotnet build && dotnet run --framework net8.0 -- --LogModes="console,file" # Log to file & console (equivalent to the above line)
dotnet build && dotnet run --framework net8.0 -- --LogModes="file,file,file" # Should log once, not three times :)
dotnet build && dotnet run --framework net8.0 -- --LogModes=",,,,,," # Parsed as an empty string really

Footnotes

  1. --LogTargets might be a better name than log modes.

@kiminuo kiminuo changed the title Add command line option `--LogModes Add command line option --LogModes Apr 23, 2024
@kiminuo kiminuo force-pushed the feature/2024-04-23-cmd-LogModes branch from 93f2b9f to 29e25a2 Compare April 23, 2024 09:25
@kiminuo kiminuo force-pushed the feature/2024-04-23-cmd-LogModes branch from 29e25a2 to cc3b74f Compare April 23, 2024 10:19
@kiminuo kiminuo marked this pull request as ready for review April 23, 2024 11:47
@Szpoti
Copy link
Collaborator

Szpoti commented Apr 23, 2024

Right now If I misspell file or console, because of the thrown exception, Wasabi will crash with the crash reporter.
This happens even if I don't specify neither file or console so during parsing, x is "".

image

Regardless the empty string, I don't think Wasabi should crash, rather it should log a warning or an error that it couldn't parse the LogMode.

@kiminuo kiminuo force-pushed the feature/2024-04-23-cmd-LogModes branch from cc3b74f to e52966e Compare April 23, 2024 12:15
@kiminuo
Copy link
Collaborator Author

kiminuo commented Apr 23, 2024

Regardless the empty string, I don't think Wasabi should crash, rather it should log a warning or an error that it couldn't parse the LogMode.

Empty string should be parsed as "no logging targets". I changed it as such.

Right now If I misspell file or console, because of the thrown exception, Wasabi will crash with the crash reporter.

Ok, though real users do not see logs so if they make a typo, everything works except that it does not work as they want.

@Szpoti Another issue here is that we are here in the process of initializing the logger and then you say we should log something (before it's initialized).

@Szpoti
Copy link
Collaborator

Szpoti commented Apr 23, 2024

Ok, though real users do not see logs so if they make a typo, everything works except that it does not work as they want.

I get your point, but this feature can only be done via terminal, right? My logic is that if a user starts Wasabi from the command line with dotnet run, with LogModes argument, they are definitely not a regular user, they probably know what they are doing.
Though the issue you mentioned (loggin before initializing the logger) stands, one might expect something in the logs nevertheless, either if the logging was redirected, or if it couldn't be redirected in the desired way.

On the other hand, now that the client doesn't crash with empty string, I get the idea why it should, if the user e.g. misspells the LogMode. I leave it up to you.

@kiminuo
Copy link
Collaborator Author

kiminuo commented Apr 23, 2024

I get your point, but this feature can only be done via terminal, right? My logic is that if a user starts Wasabi from the command line with dotnet run, with LogModes argument, they are definitely not a regular user, they probably know what they are doing.

But from this perspective it's better to show an error than "hide" it from an experienced user, or not?

@Szpoti
Copy link
Collaborator

Szpoti commented Apr 23, 2024

But from this perspective it's better to show an error than "hide" it from an experienced user, or not?

Yeah, I think you're right with that.

yahiheb
yahiheb previously approved these changes Apr 23, 2024
Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

tACK

@kiminuo
Copy link
Collaborator Author

kiminuo commented Apr 24, 2024

Changed warning to exception in e8880b7.

@kiminuo kiminuo force-pushed the feature/2024-04-23-cmd-LogModes branch from af1ceb3 to e8880b7 Compare April 24, 2024 05:38
@kiminuo
Copy link
Collaborator Author

kiminuo commented Apr 25, 2024

I reimplemented this PR to align better with how we specify options in Config class in 5965740.

Also we don't log to TorLogs.txt if LogMode.File is not specified.

One gotcha is that if the Tor process is already running we don't modify the setting (so in that scenario we fail). In #12749 I implemented restarting Tor if settings are not as user required (specified bridges in that PR) so we can use that approach to improve this. An alternative is to change the way how we set the --Log Tor setting by setting it always once we have a handle of Tor process we want to work with (this approach seems preferrable but I need to check it out in detail how to actually do it). Any opinion?

@kiminuo kiminuo force-pushed the feature/2024-04-23-cmd-LogModes branch from 5965740 to cfa3e8e Compare April 25, 2024 08:06
@kiminuo kiminuo force-pushed the feature/2024-04-23-cmd-LogModes branch from cfa3e8e to 32d7d06 Compare April 25, 2024 11:27
Copy link
Collaborator

@turbolay turbolay left a comment

Choose a reason for hiding this comment

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

tACK

@turbolay turbolay requested a review from Szpoti April 25, 2024 18:17
Copy link
Collaborator

@yahiheb yahiheb left a comment

Choose a reason for hiding this comment

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

tACK

Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

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

cACK

@molnard molnard merged commit 59f6aac into WalletWasabi:master Apr 26, 2024
6 of 8 checks passed
@molnard
Copy link
Collaborator

molnard commented Apr 26, 2024

Kimi asked me to merge with Admin rights CF was blocking him.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants