Replace a custom logger with off the shelf implementation #7559

hubertp · 2023-08-10T20:15:03Z

Pull Request Description

This change replaces Enso's custom logger with an existing, mostly off the shelf logging implementation. The change attempts to provide a 1:1 replacement for the existing solution while requiring only a minimal logic for the initialization.

Loggers are configured completely via logging-server section in application.conf HOCON file, all initial logback configuration has been removed. This opens up a lot of interesting opportunities because we can benefit from all the well maintained slf4j implementations without being to them in terms of functionality.

Most important differences have been outlined in docs/infrastructure/logging.md.

Important Notes

Addresses:

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,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

JaroslavTulach

It is not easy for me to see what is an API to use and what is an implementation. I'd like to ensure somehow the implementation classes that implement appenders/loghandlers do not appear on compilation path of modules that only use the logging.

I believe we need a guidance for using the logging. Is it sl4j or jul? What is the format of log record messages to use?

engine/runner/src/main/scala/org/enso/runner/DependencyPreinstaller.scala

engine/runtime-language-epb/src/main/java/org/enso/interpreter/epb/EpbContext.java

engine/runtime/src/main/scala/org/enso/compiler/Compiler.scala

build.sbt

JaroslavTulach · 2023-08-16T13:29:30Z

Important note. Currently the slf4j classes are duplicated. They are both in:

enso$ unzip -v runner.jar | grep -i slf4j
enso$ unzip -v runtime.jar | grep -i slf4j

that's wrong. We cannot have two logging API classes and we cannot have two logging implementations!

hubertp · 2023-08-21T07:23:41Z

Important note. Currently the slf4j classes are duplicated. They are both in:
enso$ unzip -v runner.jar | grep -i slf4j
enso$ unzip -v runtime.jar | grep -i slf4j
that's wrong. We cannot have two logging API classes and we cannot have two logging implementations!

slf4j comes not only from direct libs dependencies that we control but also from indirect dependencies. The assembled jar will have slf4j API classes, no matter whether I turn everything in the repo to jul (I don't plan to) or not.
Runtime has no slf4j implementation, runner does. The former uses jul, the latter does not, that's all intended and works just fine.

enso-bot · 2023-08-23T05:41:31Z

Jaroslav Tulach reports a new STANDUP for yesterday (2023-08-22):

Progress: - argument info PR: #7629

fixing Python interop: Special support for Python Date/Time/Zone interop #7617
debugging Mac failure: Special support for Python Date/Time/Zone interop #7617 (comment)
test discussion: https://discord.com/channels/@me/948519891961544714/1143413076037734441
review project renamed: Notification about the project rename action #7613 (comment)
notifying about it at GraalVM chat: https://graalvm.slack.com/archives/CNA7PDH2N/p1692688663290359?thread_ts=1692688611.878139&cid=CNA7PDH2N
thinking about members: Interop Atom's members test should return inherited members as well #6303 (comment)
reporting Accurate tzinfo emulation based on ZoneRules #7626
planning next iteration
reviews and discussions, standup It should be finished by 2023-08-25.

Next Day: Python Interop on Mac CI + better error messages

Discord
Discord - A New Way to Chat with Friends & Communities
Discord is the easiest way to communicate over voice, video, and text. Chat, hang out, and stay close with your friends and communities.

engine/language-server/src/main/resources/application.conf

4e6 · 2023-08-23T08:42:27Z

engine/launcher/src/main/scala/org/enso/launcher/Launcher.scala

@@ -207,7 +208,7 @@ case class Launcher(cliOptions: GlobalCLIOptions) {
  def runRepl(
    projectPath: Option[Path],
    versionOverride: Option[SemVer],
-    logLevel: LogLevel,
+    logLevel: Level,


I think having our own logging abstractions has benefits, i.e. whenever we want to change the implementation, the interface will stay the same. Do we really want to swap it with slf4j?

I didn't see any benefit of having a yet another layer of abstraction and slf4j appears to be a the most commonly used one.
The implementation can be easily added (see e.g. logging-truffle-connection) for the existing example.

lib/scala/logging-config/src/main/java/org/enso/logger/config/LoggerSetup.java

engine/launcher/src/main/scala/org/enso/launcher/cli/LauncherLogging.scala

This change disables an Enso's custom logger with an existing, off the shelf logging implementation with some custom setup and appenders. The change attempts to provide a 1:1 replacement for the existing solution. Currently configuration is a mixture of `application.conf` HOCON file with logback's configuration and programatically changed logger's contexts.

wip

Conversion of raw messages to slf4j's style is now done in the handler.

Still not behaving as expected when setting up loggers for launcher.

When started, launcher should already make use of the logging's configuration.

Cleaned up implementation for all appenders. All logback configs are gone, everything is done via our application configs. Still missing some documentation but no more features are planned.

Ensured that imported subprojects don't add logback in transitive dependencies and one has to explicitly enable it.

Logback implementation is now completely separate from the API used to setup logging in different services. Loading is done once, by reading implementation classes from the configuration file and intializing via reflection. Also discovered and fixed various problems with native image compilation. More documentation that hopefully helps to clarify the usage.

JaroslavTulach

I left few comments, but I'd like us to also move forward. Merge soon!

docs/infrastructure/logging.md

JaroslavTulach · 2023-08-28T09:33:40Z

docs/infrastructure/logging.md

-that corresponds to the indicated stack location, for example `Main.java:123`.
-Native methods may be handled differently, as well as code from different
-languages, for example Enso also includes the columns - `Test.enso:4:3-19`.
+Any custom log level is therefore defined with `-Dx.y.Z.Logger.level` where `x`,


lib/scala/logging-jutil/src/main/java/org/enso/logger/JulHandler.java

lib/scala/logging-config/src/main/java/org/enso/logger/LoggerSetup.java

JaroslavTulach · 2023-08-28T10:58:24Z

lib/scala/logging-config/src/main/java/org/enso/logger/LoggerSetup.java

+import org.slf4j.event.Level;
+
+/** Base class to be implemented by the underlying logging implementation. */
+public abstract class LoggerSetup {


LoggerSetup is used by "client code" - how can it be in the same project as implementation classes like SocketAppender that shall not be accessed directly?

There should be a module (maybe logging-jutil) that contains just the API to use by "client code". Appender class doesn't need to be visible by "client code", right?

lib/scala/logging-utils/src/main/java/org/enso/logger/LoggerUtils.java

...manager/src/main/resources/META-INF/native-image/org/enso/projectmanager/reflect-config.json

engine/launcher/src/main/scala/org/enso/launcher/cli/GlobalCLIOptions.scala

4e6

I left a bunch of comments but overall the PR looks good to me

build.sbt

engine/language-server/src/main/scala/org/enso/languageserver/util/UnhandledLogging.scala

4e6 · 2023-08-28T13:34:04Z

engine/runtime/src/test/java/org/enso/interpreter/test/DebuggingEnsoTest.java

@@ -55,6 +55,10 @@ public void initContext() {
            RuntimeOptions.LANGUAGE_HOME_OVERRIDE,
            Paths.get("../../distribution/component").toFile().getAbsolutePath()
        )
+        .option(
+                RuntimeOptions.LOG_LEVEL,
+                "FINEST"


I wonder how did it pass the formatted check

lib/scala/logging-config/src/main/java/org/enso/logger/config/LoggersLevels.java

4e6 · 2023-08-28T13:46:14Z

lib/scala/logging-jutil/src/main/java/org/enso/logger/Converter.java

+      case TRACE:
+        return java.util.logging.Level.FINEST;
+      default:
+        return java.util.logging.Level.ALL;


The match is already exhaustive. It should be a dead code, right?

We are dealing with Java ;)

Write the code like:

return switch (l) { case DEBUG -> -1; case ERROR -> -3; case INFO -> 5; case WARN -> 10; case TRACE -> 1; };

and then you will not need to complain.

...ala/org/enso/projectmanager/infrastructure/languageserver/LanguageServerSupervisorSpec.scala

docs/infrastructure/logging.md

lib/scala/logging-config/src/main/java/org/enso/logger/config/Appender.java

lib/scala/logging-utils-akka/src/main/java/org/enso/logger/akka/AkkaConverter.java

radeusgd

As far as I have looked (did not go too deep), it looks good to me.

Just a few small suggestions from me

Co-authored-by: Jaroslav Tulach <[email protected]> Co-authored-by: Radosław Waśko <[email protected]>

hubertp changed the title ~~Replace a custom logger with sth off the shelf~~ Replace a custom logger with off the shelf implementation Aug 11, 2023

Akirathan assigned hubertp Aug 14, 2023

hubertp force-pushed the wip/hubert/7253-replace-custom-logger branch from c30d431 to d844b56 Compare August 14, 2023 16:15

JaroslavTulach reviewed Aug 15, 2023

View reviewed changes

enso-bot bot mentioned this pull request Aug 16, 2023

State accepts only Type as key - explain in docs #7572

Closed

4e6 reviewed Aug 23, 2023

View reviewed changes

JaroslavTulach reviewed Aug 23, 2023

View reviewed changes

lib/scala/logging-config/src/main/java/org/enso/logger/config/LoggerSetup.java Outdated Show resolved Hide resolved

JaroslavTulach reviewed Aug 23, 2023

View reviewed changes

engine/launcher/src/main/scala/org/enso/launcher/cli/LauncherLogging.scala Outdated Show resolved Hide resolved

enso-bot bot mentioned this pull request Aug 25, 2023

op1._ yields: Compiler Internal Error: Qualified names should not be present at codegen time #7582

Closed

2 tasks

hubertp added 18 commits August 25, 2023 17:44

format sbt

0bf5abc

Fix native image build, many tests

1591ff6

wip

update changelog

0d55893

minor formatting

51b87b3

fix test

407476f

Bring back jul's formatting to runtime

d597380

Conversion of raw messages to slf4j's style is now done in the handler.

Fix some tests

7b0afd4

Still not behaving as expected when setting up loggers for launcher.

Use test logger when collecting logs of a specific test

372af9e

Bandaid to make sure launcher's logging works

f03d263

When started, launcher should already make use of the logging's configuration.

Significantly simplifying logger's setup

80f0f12

More simplifications, documentation

57ff421

Functionality-wise - done.

bece946

Cleaned up implementation for all appenders. All logback configs are gone, everything is done via our application configs. Still missing some documentation but no more features are planned.

logging-service - bye!

d0a784e

Ensure logging impl is not accidentally used

3faffe3

Ensured that imported subprojects don't add logback in transitive dependencies and one has to explicitly enable it.

Scala -> Java

e87b8a8

Minor simplifications

b142d40

hubertp requested review from radeusgd and Akirathan as code owners August 25, 2023 21:44

hubertp added 2 commits August 27, 2023 15:40

User ServiceLoader to load implementation

d328f6b

nits

d97d39c

JaroslavTulach approved these changes Aug 28, 2023

View reviewed changes