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

Fix logging in CLI #11472

Merged
merged 8 commits into from
Nov 7, 2024
Merged

Fix logging in CLI #11472

merged 8 commits into from
Nov 7, 2024

Conversation

hubertp
Copy link
Contributor

@hubertp hubertp commented Nov 1, 2024

Pull Request Description

Previously, unless --logger-connect was used, CLI would always fallback to console logging. In addition it would be misconfigured if application.conf was provided with logging configuration.

This change makes sure that CLI uses the same logging infrastructure as the rest of the system.
As a result, CLI will now by default not only log to the console and respect the provided configuration but also log to the file system, with exceptions being printed in the latter.

Closes #11265.

Checklist

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

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

Previously, unless `--logger-connect` was used, CLI would always
fallback to console logging. In addition it would be misconfigured if
`application.conf` was provided with logging configuration.

This change makes sure that CLI uses the same logging infrastructure as
the rest of the system.
As a result, CLI will now by default not only log to the console and
respect the provided configuration but also log to the file system.
@hubertp hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label Nov 4, 2024
@hubertp hubertp marked this pull request as ready for review November 4, 2024 16:20
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

I'd like the documentation to show a piece of code for following common use-cases:

  • log a message into console
  • log a message into console only if verbosity is increased
  • log an exception (and/or its stacktrace) to the console
  • log an exception (and/or its stacktrace) to the console only if verbosity is increased
  • log an exception stacktrace to the console only if verbosity is increased, but log a message with default log setup

These are the basic use-cases I'd like to see covered in the documentation. In addition I'd like to know:

  • how development build (e.g. -ea) influences the above?
  • what CLI options to use to enable verbose logging of a single logger like org.enso.compiler.pass.PassManager?

I am primarily interested in configuring the logging without recompilation - e.g. without changing .conf files in src/main/resources. I assume such a configuration can be done with CLI options like --log-level and --vm.D or some environment variables.

@hubertp
Copy link
Contributor Author

hubertp commented Nov 5, 2024

  • log a message into console

Added

  • log a message into console only if verbosity is increased

Added

  • log an exception (and/or its stacktrace) to the console

Added

  • log an exception (and/or its stacktrace) to the console only if verbosity is increased

Not supported atm, feels like a feature request.

  • log an exception stacktrace to the console only if verbosity is increased, but log a message with default log setup

Not supported atm, feels like a feature request.

  • how development build (e.g. -ea) influences the above?

Sorry, I don't see the relation of this PR to -ea.

  • what CLI options to use to enable verbose logging of a single logger like org.enso.compiler.pass.PassManager?

This has been supported all along and documented.

Re not supported features - we could probably enable that with Logback's evaluators but it was never an official requirement so was not implemented.

Logger logger = LoggerFactory.getLogger(HelloWorld.class);
logger.info("Hello World");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I am probably going to use this from an Enso code, not for a a separate class with main main method.

Where will this message appear when I run ./enso --run hello_world.enso? Console? Somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really don't understand your request. You asked for examples of using logging infrastructure so I essentially copy pasted API examples from SLF4J because that's it is to it.
Now you ask for logging in Enso which is a different thing and has been quickly developed by libs team and has not been integrated with backend at all AFAIK.

Copy link
Member

@JaroslavTulach JaroslavTulach Nov 7, 2024

Choose a reason for hiding this comment

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

I am probably going to use this from an Enso code

I beg your pardon. I mean logging from Enso runtime code (like PassManager).

logging in Enso ... developed by libs team

I am sorry for causing confusion. No, logging from Enso code isn't what I am interested in right now.

Logger logger = LoggerFactory.getLogger(HelloWorld.class);
if (logger.isTraceEnabled()) {
logger.info("Hello World");
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this the right pattern? I would expect simple logger.trace("Hello World"). How do I need to launch the CLI for such a message to appear in the console?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You asked for log a message into console only if verbosity is increased. Of course one can use .trace but that's different from your request.

Copy link
Member

@JaroslavTulach JaroslavTulach Nov 7, 2024

Choose a reason for hiding this comment

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

You asked for log a message into console only if verbosity is increased. Of course one can use .trace but that's different from your request.

I have asked for a way to "log a message to a console if a verbosity is increased". Does:

logger.trace("Hello World")

appear on the console when enso is invoked (and the logger code is performed)? Does the Hello World message appear on the console when enso is invoked with some logging parameters? If the answers are no to 1st question and yes to 2nd question, then logger.trace + description of the CLI arguments to enable seeing the message on console is a solution.

PS: Using isTraceEnabled() is probably only good for performance reasons, isn't it? If so, then your example might be:

    Logger logger = LoggerFactory.getLogger(HelloWorld.class);
    if (logger.isTraceEnabled()) {
      logger.trace("Hello World");
    }

e.g. using isTraceEnabled and .trace. Combining it with .info is pretty weird and not very intuitive from my perspective.


public class HelloWorld {

public static void main(String[] args) {
Copy link
Member

Choose a reason for hiding this comment

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

Again, example of logging from Enso (including how to launch the CLI) would be more useful than standalone class unrelated to Enso. Is it guaranteed that ./enso --run hello.enso would print exception logged this way to console? Is there a test for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You clearly haven't read the documentation. We don't log stacktraces in console, on purpose. You can of course change that by removing %noexp but we are not going to change that for users. It's all documented.

Copy link
Member

Choose a reason for hiding this comment

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

We don't log stacktraces in console, on purpose.

Then your answer should be: It is not possible. I am not OK with such an answer, but that's a unrelated to the answer.

@JaroslavTulach
Copy link
Member

  • log an exception stacktrace to the console only if verbosity is increased, but log a message with default log setup

I expect I believe this may be achieved by two log statements:

logger.info(ex.getMessage());
logger.trace(ex);

Not supported atm, feels like a feature request.

I don't see it as a feature request. I believe it is one of the most essential scenarios when logging.

  • log an exception (and/or its stacktrace) to the console only if verbosity is increased
    Not supported atm, feels like a feature request.

This is even more essential case than the previous use-case! How am I supposed to use the Enso logging when we have no guideline how to log a stacktrace (supressed by default) and instructions how to make it appear in the console?

@JaroslavTulach
Copy link
Member

  • how development build (e.g. -ea) influences the above?

Sorry, I don't see the relation of this PR to -ea.

There is a need for logging in CI and not in production. To do that we need to differentiate between run in production and run in the CI. One way that a CI run differs from production is -ea being on.

To rephrase my question: How does one log in development build, but not in production? Can you provide example snippets for doing so (message, exception, with/without stack trace)? Can you include recommended CLI parameters to turn such logging on?

@JaroslavTulach
Copy link
Member

  • what CLI options to use to enable verbose logging of a single logger like org.enso.compiler.pass.PassManager?

This has been supported all along and documented.

I don't find such documentation satisfactory. According to current documentation I would expect that to "enable verbose logging of a single logger like org.enso.compiler.pass.PassManager" I should:

$ enso --vm.D=org.enso.compiler.pass.PassManager.level=trace --run f.enso

however, that's not enough! Nothing gets logged with 2024.5.1-nightly.2024.11.5. I need instructions how to use CLI to activate such kind of logging.

@hubertp
Copy link
Contributor Author

hubertp commented Nov 6, 2024

Not supported atm, feels like a feature request.

I don't see it as a feature request. I believe it is one of the most essential scenarios when logging.

Agree to disagree. Neither default SLF4J works that way nor did the old homemade loggers that we replaced.
Please file a proper ticket with a feature request and use-cases if you need this for your development.

@JaroslavTulach
Copy link
Member

JaroslavTulach commented Nov 7, 2024

log an exception stacktrace to the console only if verbosity is increased, but log a message with default log setup
Not supported atm, feels like a feature request.
I don't see it as a feature request. I believe it is one of the most essential scenarios when logging.

Agree to disagree. Neither default SLF4J works that way nor did the old homemade loggers that we replaced. Please file a proper ticket with a feature request and use-cases if you need this for your development.

I would like a guideline how to satisfy...

log an exception stacktrace to the console only if verbosity is increased, but log a message with default log setup

...to be part of the documentation update done in this PR. Of course, if there is no way to print expression stacktrace to console (which I disagree with for reasons related to CI execution), then the only reply could be: it is not possible.

@hubertp hubertp merged commit a5ebdf4 into develop Nov 7, 2024
41 checks passed
@hubertp hubertp deleted the wip/hubert/11265-cli-logging branch November 7, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI does not use Enso's logging infrastructure
3 participants