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

Let logging to file respect log level #8264

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

hubertp
Copy link
Collaborator

@hubertp hubertp commented Nov 8, 2023

Pull Request Description

This change fixes a regression introduced in #7918, which prevented the execution from setting the right log level either via env var or parameter.

Now passing either of the options returns logs of the expected level in the log file:

  • ENSO_LOG_TO_FILE_LOG_LEVEL = trace
  • ... -vv ...

Fixes #8274

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@hubertp hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 8, 2023
Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

When I use

diff --git a/engine/language-server/src/main/resources/application.conf b/engine/language-server/src/main/resources/application.conf
index c2d9e1ded..eb6ac846c 100644
--- a/engine/language-server/src/main/resources/application.conf
+++ b/engine/language-server/src/main/resources/application.conf
@@ -56,7 +56,7 @@ logging-service {
   default-appender = socket
   default-appender = ${?ENSO_APPENDER_DEFAULT}
   log-to-file {
-    enable = false
+    enable = true
     enable = ${?ENSO_LOG_TO_FILE}
     log-level = debug
     log-level = ${?ENSO_LOG_TO_FILE_LOG_LEVEL}

I still have 5 GBs of output in my stdout, even though the default log level to console should be INFO. What is the purpose of log-to-file.enable property then?

Note that I like to set the log-to-file.log-level = trace in application.conf rather than via env variable. That is why I also tried to set log-to-file.enable = true.

#org.enso.languageserver.protocol.json.JsonConnectionController = debug
#org.enso.jsonrpc.JsonRpcServer = debug
#org.enso.languageserver.runtime.RuntimeConnector = debug
org.enso.languageserver.protocol.json.JsonConnectionController = debug
Copy link
Member

Choose a reason for hiding this comment

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

Nice to see that this is uncommented by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I got fed up with it. In trace you can't see anything because of it.

@hubertp
Copy link
Collaborator Author

hubertp commented Nov 10, 2023

When I use
...
I still have 5 GBs of output in my stdout, even though the default log level to console should be INFO. What is the purpose of ?log-to-file.enable property then?

It's a problem unrelated to the fact what this PR is fixing but I've been thinking how to best tackle this anyway.

As noted internally setting log-to-file for language server that is being started by project-manager makes little sense when using socket appender. It probably would make sense if language server was started separately. For now, just hardcoded the fact that language server should always reset the LoggerContext thus ensuring console is not flooded by logs. I would only spend more time on this if there is a real use-case to support it.

@hubertp hubertp requested a review from Akirathan November 10, 2023 11:30
@hubertp hubertp added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Nov 10, 2023
This change fixes a regression introduced in #7918, which prevented the
execution from setting the right log level either via env var or
parameter.

Now passing either of the options returns logs of the expected level
in the log file:
- `ENSO_LOG_TO_FILE_LOG_LEVEL = trace`
- ... `-vv` ...
It makes little sense in the current environment to log to file while
also logging to socket, because the latter most of the time is doing
that anyway.
This change prevents from flodding console with logs.

It would make sense to support such feature if language server was
started independently (without `project-manager`).

Left out the section in language server's `application.conf` in case
someone wants to use an appender different from socket one.
@hubertp hubertp force-pushed the wip/hubert/log-to-file-log-level-2 branch from 0e31d20 to 11a4a07 Compare November 10, 2023 13:25
@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Nov 13, 2023
@mergify mergify bot merged commit c649ed8 into develop Nov 14, 2023
32 of 34 checks passed
@mergify mergify bot deleted the wip/hubert/log-to-file-log-level-2 branch November 14, 2023 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting logging to verbose does not increase the verbosity of logs in log file
3 participants