Changed the usage from slf4j-jboss-logging to slf4j-jboss-logmanager. #12692

SlyngDK · 2020-10-13T19:35:16Z

Is related with #12615
To be able to support forwarding slf4j logging parameters to LogManager without formatting more than once.
The change from slf4j-jboss-logging to slf4j-jboss-logmanager.

A new release is still required of slf4j-jboss-logmanager, to include the parameter forwarding.

jamezp · 2020-10-13T21:14:36Z

@SlyngDK I've released 1.1.0.Final of the org.jboss.slf4j:slf4j-jboss-logmanager.

build-parent/pom.xml

independent-projects/arc/pom.xml

independent-projects/bootstrap/pom.xml

independent-projects/bootstrap/maven-resolver/pom.xml

SlyngDK · 2020-10-14T13:41:32Z

@dmlloyd I started to see the Maximum number of attachments exceeded again when running tests local.

[INFO] Running io.quarkus.vertx.core.deployment.VerticleWithClassNameDeploymentTest
Exception in thread "Thread-59" java.lang.ExceptionInInitializerError
        at io.netty.channel.DefaultChannelId.<clinit>(DefaultChannelId.java:60)
        at io.quarkus.netty.runtime.NettyRecorder$1.run(NettyRecorder.java:28)
        at java.base/java.lang.Thread.run(Thread.java:832)
Caused by: java.lang.IllegalStateException: Maximum number of attachments exceeded
        at org.jboss.logmanager.LoggerNode.attachIfAbsent(LoggerNode.java:471)
        at org.jboss.logmanager.Logger.attachIfAbsent(Logger.java:194)
        at org.slf4j.impl.Slf4jLoggerFactory$1.run(Slf4jLoggerFactory.java:42)
        at org.slf4j.impl.Slf4jLoggerFactory$1.run(Slf4jLoggerFactory.java:39)
        at java.base/java.security.AccessController.doPrivileged(AccessController.java:312)
        at org.slf4j.impl.Slf4jLoggerFactory.getLogger(Slf4jLoggerFactory.java:39)
        at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:363)
        at io.netty.util.internal.logging.Slf4JLoggerFactory.newInstance(Slf4JLoggerFactory.java:49)
        at io.netty.util.internal.logging.InternalLoggerFactory.getInstance(InternalLoggerFactory.java:92)
        at io.netty.util.internal.logging.InternalLoggerFactory.getInstance(InternalLoggerFactory.java:85)
        at io.netty.util.internal.SystemPropertyUtil.<clinit>(SystemPropertyUtil.java:29)
        ... 3 more
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.375 s <<< FAILURE! - in io.quarkus.vertx.core.deployment.VerticleWithClassNameDeploymentTest

dmlloyd · 2020-10-14T14:12:24Z

@dmlloyd I started to see the Maximum number of attachments exceeded again when running tests local.

[INFO] Running io.quarkus.vertx.core.deployment.VerticleWithClassNameDeploymentTest
Exception in thread "Thread-59" java.lang.ExceptionInInitializerError
        at io.netty.channel.DefaultChannelId.<clinit>(DefaultChannelId.java:60)
        at io.quarkus.netty.runtime.NettyRecorder$1.run(NettyRecorder.java:28)
        at java.base/java.lang.Thread.run(Thread.java:832)
Caused by: java.lang.IllegalStateException: Maximum number of attachments exceeded
        at org.jboss.logmanager.LoggerNode.attachIfAbsent(LoggerNode.java:471)
        at org.jboss.logmanager.Logger.attachIfAbsent(Logger.java:194)
        at org.slf4j.impl.Slf4jLoggerFactory$1.run(Slf4jLoggerFactory.java:42)
        at org.slf4j.impl.Slf4jLoggerFactory$1.run(Slf4jLoggerFactory.java:39)
        at java.base/java.security.AccessController.doPrivileged(AccessController.java:312)
        at org.slf4j.impl.Slf4jLoggerFactory.getLogger(Slf4jLoggerFactory.java:39)
        at org.slf4j.LoggerFactory.getLogger(LoggerFactory.java:363)
        at io.netty.util.internal.logging.Slf4JLoggerFactory.newInstance(Slf4JLoggerFactory.java:49)
        at io.netty.util.internal.logging.InternalLoggerFactory.getInstance(InternalLoggerFactory.java:92)
        at io.netty.util.internal.logging.InternalLoggerFactory.getInstance(InternalLoggerFactory.java:85)
        at io.netty.util.internal.SystemPropertyUtil.<clinit>(SystemPropertyUtil.java:29)
        ... 3 more
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.375 s <<< FAILURE! - in io.quarkus.vertx.core.deployment.VerticleWithClassNameDeploymentTest

Are you able to attach a debugger to this, to check that all three fields are non-null at this point (thus showing that all three attachments are in fact populated and there isn't some bug I'm not seeing)? If so I guess I'll add one or two more slots, and release again.

SlyngDK · 2020-10-14T18:19:47Z

Are you able to attach a debugger to this, to check that all three fields are non-null at this point (thus showing that all three attachments are in fact populated and there isn't some bug I'm not seeing)? If so I guess I'll add one or two more slots, and release again.

Look like the Slf4jLogger is added multiple times.

jamezp · 2020-10-14T18:38:27Z

That's odd. The keys are different, but the values are the same. The other odd part is two of them have a null name.

dmlloyd · 2020-10-14T18:55:02Z

The values aren't identical - I bet we're seeing the logger be added from two different class loaders. Looking at att2.getClass().getClassLoader versus att3.getClass().getClassLoader should determine if this hypothesis is correct.

jamezp · 2020-10-14T18:55:49Z

I bet it's a test class loader issue. If I run the test by itself it passes. It's the third test that fails too and there are 3 attachments allowed.

jamezp · 2020-10-14T18:58:40Z

That is it. If I used:

<plugin>
    <artifactId>maven-surefire-plugin</artifactId>
    <configuration>
        <reuseForks>false</reuseForks>
    </configuration>
</plugin>

I don't see the error message.

SlyngDK · 2020-10-15T10:03:28Z

That is it. If I used:

<plugin>
    <artifactId>maven-surefire-plugin</artifactId>
    <configuration>
        <reuseForks>false</reuseForks>
    </configuration>
</plugin>

I don't see the error message.

In which pom.xml, should that be added?
That will slow down running the tests, will that be accepted?

SlyngDK · 2020-10-15T12:16:19Z

I found another possible solution.
Added

<parentFirstArtifact>org.slf4j:slf4j-api</parentFirstArtifact>
<parentFirstArtifact>org.jboss.slf4j:slf4j-jboss-logmanager</parentFirstArtifact>

to core/runtime/pom.xml
I think this is better, than disabling reuseForks.

jamezp · 2020-10-15T15:11:32Z

@SlyngDK I set it in the vert.x core tests since it was the first one that was failing. I really just did that to confirm my theory because as you said it slows tests down quite a bit :)

SlyngDK · 2020-10-16T06:33:48Z

The build was successful and ready to review.

gsmet · 2020-10-19T09:27:09Z

I squashed the commits.

gsmet

Questions:

should we do the same for the others? commons-logging for instance?
should we update the documentation we have here: https://quarkus.io/guides/logging#logging-adapters ?
should we also add exclusions for the other -jboss-logging artifacts (we already recommend using the logmanager for Log4j and Log4j2)?
can we check that the new components used are Apache licensed? If not, can we release them under the Apache License.

I'm done with my questions :). I know some of these questions are broader than the current PR but let's get it right for all the adapters.

gsmet · 2020-10-19T09:34:00Z

@vsevel could you test the failure you reported with this PR? Thanks!

SlyngDK · 2020-10-19T14:49:40Z

Questions:

* should we do the same for the others? `commons-logging` for instance?

commons-logging don't have parameters, and therefore not same issue when forwarding parameters to logmanager.
@dmlloyd or @jamezp will be better to answers this than me.

* should we update the documentation we have here: https://quarkus.io/guides/logging#logging-adapters ?

The docs in docs/src/main/asciidoc/logging.adoc is updated with the changed dependency.
I don't know if anything else is required to be updated.

* should we also add exclusions for the other `-jboss-logging` artifacts (we already recommend using the logmanager for Log4j and Log4j2)?

I can add them if you want it.

* can we check that the new components used are Apache licensed? If not, can we release them under the Apache License.

slf4j-jboss-logmanager is under Apache 2.0 License.

jamezp · 2020-10-19T15:42:24Z

I can confirm that commons-logging does not use format parameters so it should not be an issue.

gsmet · 2020-10-19T16:38:17Z

My question is not if it's not an issue to use the logmanager artifact or not but more: is it the right thing to do?

From what I can see, it looks like the right thing to do so I think we should be consistent. Except if I'm missing something?

jamezp · 2020-10-19T18:12:00Z

My apologies @gsmet. Yes I think using the slf4j-jboss-logmanager is the right thing to do. It takes a layer of abstraction way from logging with slf4j which IMO is a good thing. You also get the benefit of having the parameters passed in stored in the log record for availability in structured log formatters.

gsmet · 2020-10-19T18:25:53Z

@jamezp no need to apologize, I'm just trying to understand the right thing to do :).

So IIUC, we should use the logmanager artifacts for all the logging components, right?

jamezp · 2020-10-19T18:42:41Z

@gsmet Looking at the bom, hopefully that's the correct place, it looks like once the slf4j-jboss-logging dependency is replaced with the slf4j-jboss-logmanager version then the only left would be org.jboss.logging:commons-logging-jboss-logging. This one is likely okay to leave as delegating to jboss-logging. We do this in WildFly for the reason listed in https://issues.redhat.com/browse/WFCORE-3656. However for Quarkus that might not be an issue as I think the jboss-logmanager-embedded is always used.

vsevel · 2020-10-19T18:46:51Z

@gsmet I confirm that the issue in 1.9.0.CR1 described in #12634 does not appear with this PR or 1.9.0.Final

gsmet · 2020-10-20T11:46:21Z

@dmlloyd could you shed some light on James' last comment in the context of Quarkus?

If possible, I would like us to get it right and be as consistent as possible. So if it's a possibility we use the -logmanager for all the logging libraries, that would have my preference. If not, we will live with it I suppose.

dmlloyd · 2020-10-20T11:50:07Z

Sure, it can be done either way. If we want to test the other library though, let's do it now so I know if I need to make any other fixes.

SlyngDK · 2020-10-22T14:28:31Z

@gsmet What work need to be done on this PR, before it can be merged? Is there something I can do?

gsmet · 2020-10-27T18:12:45Z

Merged, thanks!

SlyngDK mentioned this pull request Oct 13, 2020

LogManager FORMAT_FAILURE on application startup #12615

Closed

dmlloyd reviewed Oct 14, 2020

View reviewed changes

build-parent/pom.xml Show resolved Hide resolved

dmlloyd reviewed Oct 14, 2020

View reviewed changes

independent-projects/arc/pom.xml Show resolved Hide resolved

dmlloyd reviewed Oct 14, 2020

View reviewed changes

independent-projects/bootstrap/pom.xml Show resolved Hide resolved

dmlloyd reviewed Oct 14, 2020

View reviewed changes

independent-projects/bootstrap/maven-resolver/pom.xml Show resolved Hide resolved

SlyngDK marked this pull request as ready for review October 14, 2020 14:00

SlyngDK mentioned this pull request Oct 15, 2020

Structured arguments not working with SLF4J quarkiverse/quarkus-logging-json#34

Closed

dmlloyd approved these changes Oct 16, 2020

View reviewed changes

Changed the usage from slf4j-jboss-logging to slf4j-jboss-logmanager.

1552fe8

gsmet added release/noteworthy-feature release/breaking-change labels Oct 19, 2020

gsmet requested changes Oct 19, 2020

View reviewed changes

gsmet approved these changes Oct 27, 2020

View reviewed changes

gsmet merged commit 6ab1dba into quarkusio:master Oct 27, 2020

gsmet added this to the 1.10 - master milestone Oct 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed the usage from slf4j-jboss-logging to slf4j-jboss-logmanager. #12692

Changed the usage from slf4j-jboss-logging to slf4j-jboss-logmanager. #12692

SlyngDK commented Oct 13, 2020

jamezp commented Oct 13, 2020

SlyngDK commented Oct 14, 2020

dmlloyd commented Oct 14, 2020

SlyngDK commented Oct 14, 2020

jamezp commented Oct 14, 2020

dmlloyd commented Oct 14, 2020 •

edited

Loading

jamezp commented Oct 14, 2020 •

edited

Loading

jamezp commented Oct 14, 2020

SlyngDK commented Oct 15, 2020

SlyngDK commented Oct 15, 2020

jamezp commented Oct 15, 2020

SlyngDK commented Oct 16, 2020

gsmet commented Oct 19, 2020

gsmet left a comment

gsmet commented Oct 19, 2020

SlyngDK commented Oct 19, 2020

jamezp commented Oct 19, 2020

gsmet commented Oct 19, 2020

jamezp commented Oct 19, 2020

gsmet commented Oct 19, 2020

jamezp commented Oct 19, 2020

vsevel commented Oct 19, 2020 •

edited

Loading

gsmet commented Oct 20, 2020 •

edited

Loading

dmlloyd commented Oct 20, 2020

SlyngDK commented Oct 22, 2020

gsmet commented Oct 27, 2020

Changed the usage from slf4j-jboss-logging to slf4j-jboss-logmanager. #12692

Changed the usage from slf4j-jboss-logging to slf4j-jboss-logmanager. #12692

Conversation

SlyngDK commented Oct 13, 2020

jamezp commented Oct 13, 2020

SlyngDK commented Oct 14, 2020

dmlloyd commented Oct 14, 2020

SlyngDK commented Oct 14, 2020

jamezp commented Oct 14, 2020

dmlloyd commented Oct 14, 2020 • edited Loading

jamezp commented Oct 14, 2020 • edited Loading

jamezp commented Oct 14, 2020

SlyngDK commented Oct 15, 2020

SlyngDK commented Oct 15, 2020

jamezp commented Oct 15, 2020

SlyngDK commented Oct 16, 2020

gsmet commented Oct 19, 2020

gsmet left a comment

Choose a reason for hiding this comment

gsmet commented Oct 19, 2020

SlyngDK commented Oct 19, 2020

jamezp commented Oct 19, 2020

gsmet commented Oct 19, 2020

jamezp commented Oct 19, 2020

gsmet commented Oct 19, 2020

jamezp commented Oct 19, 2020

vsevel commented Oct 19, 2020 • edited Loading

gsmet commented Oct 20, 2020 • edited Loading

dmlloyd commented Oct 20, 2020

SlyngDK commented Oct 22, 2020

gsmet commented Oct 27, 2020

dmlloyd commented Oct 14, 2020 •

edited

Loading

jamezp commented Oct 14, 2020 •

edited

Loading

vsevel commented Oct 19, 2020 •

edited

Loading

gsmet commented Oct 20, 2020 •

edited

Loading