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

Provide option to re-enable log4j2 JMX support when they disable it by default #40273

Closed
sdavids opened this issue Apr 9, 2024 · 8 comments
Closed
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@sdavids
Copy link

sdavids commented Apr 9, 2024

configurations {
  implementation {
    exclude(module = "spring-boot-starter-logging")
  }
}

dependencies {
  implementation("org.springframework.boot:spring-boot-starter")
  implementation("org.springframework.boot:spring-boot-starter-log4j2")
  testImplementation("org.springframework.boot:spring-boot-starter-test")
}
package com.example.demo;

import org.junit.jupiter.api.Test;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.jmx.support.MBeanServerFactoryBean;

import static org.assertj.core.api.Assertions.assertThat;

@SpringBootTest
class DemoApplicationTests {

  @Test
  void contextLoads() {
    var factory = new MBeanServerFactoryBean();
    factory.setLocateExistingServerIfPossible(true);
    factory.afterPropertiesSet();
    var server = factory.getObject();
    assertThat(server.getDomains()).doesNotContain("org.apache.logging.log4j2"); // fails
  }
}

JMX support is enabled by default.

https://logging.apache.org/log4j/2.x/manual/jmx.html#enabling-jmx

Related

Spring Boot - Disable JMX by default

JUnit 5 - Disable Log4J JMX beans creation in tests


Ideally the auto-configuration would set log4j2.disableJmx=true if spring.jmx.enabled=false thereby no log4j2 JMX beans would be created.

Alternatively, the log4j2.disableJmx property should be mentioned in at least these two places:

@philwebb
Copy link
Member

I think we might want to do something here, but I'm not sure we should tie things with spring.jmx.enabled since that property is really supposed to turn off Spring specific JXM features.

I wonder if we should wait to see how apache/logging-log4j2#1229 plays out. If Log4J flip the default then we might be able to provide a quick way to turn it back on for users that need it.

@philwebb philwebb added status: on-hold We can't start working on this issue yet for: team-meeting An issue we'd like to discuss as a team to make progress type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Apr 13, 2024
@philwebb philwebb added this to the General Backlog milestone Apr 15, 2024
@philwebb philwebb changed the title Do not create log4j2 JMX beans if spring.jmx.enabled=false Provide option to re-enable log4j2 JMX support when they disable it by default Apr 15, 2024
@wilkinsona
Copy link
Member

Log4j2 has flipped the default so we can consider an option to flip it back as and when we upgrade to a version with the change. That said, I learned from apache/logging-log4j2#2468 (review) that Log4j 3.0 won't have any JMX support at all so I wonder if this is really worth it. The log4j2.disableJmx=false system property will be available irrespective of what we do here.

@sdavids
Copy link
Author

sdavids commented Apr 17, 2024

I suggest having no code changes but a documentation improvement.


https://docs.spring.io/spring-boot/reference/actuator/jmx.html

Add something along the lines of:

[NOTE]
====
`spring.jmx.enabled` affects only the management beans provided by Spring.

The enablement of management beans provided by other frameworks, e.g. https://logging.apache.org/log4j/2.x/manual/jmx.html[Log4j2], https://www.quartz-scheduler.org/api/2.3.0/constant-values.html#org.quartz.impl.StdSchedulerFactory.PROP_SCHED_JMX_EXPORT[Quartz], or https://docs.jboss.org/hibernate/orm/5.2/userguide/html_single/appendices/Configurations.html#configurations-jmx[Hibernate], is independent.
====

https://docs.spring.io/spring-boot/docs/current/reference/html/application-properties.html#application-properties.core.spring.jmx.enabled

Expose management beans to the JMX domain.

Expose Spring's management beans to the JMX domain.

@ppkarwasz
Copy link
Contributor

@wilkinsona,

Log4j2 has flipped the default so we can consider an option to flip it back as and when we upgrade to a version with the change. That said, I learned from apache/logging-log4j2#2468 (review) that Log4j 3.0 won't have any JMX support at all so I wonder if this is really worth it. The log4j2.disableJmx=false system property will be available irrespective of what we do here.

I think it would make sense for a Spring Application to use !spring.jmx.enabled as default value for log4j2.disableJmx regardless of the version of Log4j Core used. Adding the appropriate logic to SpringEnvironmentPropertySource should be enough to accomplish this.

There are not many users that use Log4j JMX, there are probably even less users that enable JMX in Log4j, but not in Spring Boot, so it might even be safe to do it in a patch release.

@sdavids
Copy link
Author

sdavids commented Sep 10, 2024

https://github.com/apache/logging-log4j2/releases/tag/rel%2F2.24.0

Starting in version 2.24.0, JMX support is disabled by default [...]

@philwebb philwebb added the for: team-meeting An issue we'd like to discuss as a team to make progress label Sep 10, 2024
@philwebb
Copy link
Member

We discussed this today and given that JMX support will be dropped in Log4j 3.0 we don't think we should add support directly in Spring Boot. We want to keep spring.jmx.enabled specifically for Spring code. If folks want to enable Log4j JMX support they can use the standard Log4j configuration to do so.

@philwebb philwebb closed this as not planned Won't fix, can't repro, duplicate, stale Sep 11, 2024
@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed type: enhancement A general enhancement status: on-hold We can't start working on this issue yet for: team-meeting An issue we'd like to discuss as a team to make progress labels Sep 11, 2024
@philwebb philwebb removed this from the General Backlog milestone Sep 11, 2024
@sdavids
Copy link
Author

sdavids commented Sep 11, 2024

What about the documentation change?

#40273 (comment)

It would underscore your statement:

We want to keep spring.jmx.enabled specifically for Spring code.

@philwebb
Copy link
Member

Thanks @sdavids, I somehow missed that comment. I've opened #42272 to deal with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

5 participants