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

Remove HTTP log spam on default settings #6097

Merged
merged 2 commits into from
Sep 18, 2023
Merged

Remove HTTP log spam on default settings #6097

merged 2 commits into from
Sep 18, 2023

Conversation

emlautarom1
Copy link
Contributor

Fixes #6017

Changes

  • Do not change the log levels of JsonWebAPI* rules.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Tested on VM using with and without the --log info flag.

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

Remarks

Passing --log info as a argument results in the same logging settings as not passing any argument.

@MarekM25
Copy link
Contributor

I would like to understand why it was needed. Why can't we rely on NLog.config? https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.Runner/NLog.config#L110

@emlautarom1
Copy link
Contributor Author

I would like to understand why it was needed. Why can't we rely on NLog.config? https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.Runner/NLog.config#L110

I'm not an expert on NLog, but the reason why we can't just use the NLog.config config file is that during initialization we perform some manual overrides to it (see: https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.Runner/Logging/NLogConfigurator.cs).

This change ensures that the manual override does not modify these JsonWebAPI* rules, so we keep the behavior defined in the config file.

@emlautarom1
Copy link
Contributor Author

emlautarom1 commented Sep 18, 2023

Similar to what was discussed on #6051 (comment), a better solution would be to not do this manual override, but instead rely on variables that we set up during initialization.

On NLog.config, this would look something like:

<variable name="LogLevel" value="Info" />
<!-- ... -->
<logger name="*" minlevel="Off" writeTo="seq" />
<logger name="*" minlevel="${LogLevel}" writeTo="file-async" />
<logger name="*" minlevel="${LogLevel}" writeTo="auto-colored-console-async" />

And then, during initialization:

NLog.LogManager.Configuration.Variables["LogLevel"] = logLevel; // parse and get the log level from CLI/Env/whatever.
NLog.LogManager.Configuration = NLog.LogManager.Configuration.Reload();

The reason why I didn't push forward with this type of change is that we would need to redo the whole NLog initialization code to use only variables for it to make sense. Would it make sense to push forward with this refactor @MarekM25?

Copy link
Contributor

@MarekM25 MarekM25 left a comment

Choose a reason for hiding this comment

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

Okay, so please add comments and we could move forward with this PR

@MarekM25 MarekM25 merged commit f373ffb into master Sep 18, 2023
61 checks passed
@MarekM25 MarekM25 deleted the fix/http_log_spam branch September 18, 2023 16:18
@kamilchodola
Copy link
Contributor

@MarekM25 @emlautarom1
Had that issue on 1.21.0 using Kurtosis setup - now checked master branch and looks like it is resolved! Was very problematic with huge amount of calls sent to Neth node to browse through logs.

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.

Logs spam in ethdocker and sedge
3 participants