Add support for configuring lobgack logging via JMX #4486

vpavic · 2015-11-14T03:06:47Z

It would be nice to have support for modifying logging configuration in runtime using JMX.

This PR enables Logback's JMX Configurator as a part of default logging setup.

I've signed the CLA.

philwebb · 2015-11-14T03:53:24Z

Thanks. Have you signed the CLA?

snicoll · 2015-11-14T07:13:38Z

See also #3819 - I'd rather have an actuator endpoint for that if possible (with something that works regardless of the logging system).

vpavic · 2015-11-14T11:35:41Z

@philwebb, yes, I did sign the CLA :)

@snicoll, thanks for the #3819 reference. I remember seeing that issue a while ago, but couldn't find it probably because I was focused around the default (i.e. Logback) logging setup.

Regarding actuator endpoint, does the suggested LoggingEndpoint imply that all logging implementations would share the same set of operations? This would be somewhat limiting, since the users would not get the enitre set of operations for their logging framework of choice thus again needing to manually enable JMX support (for Logback at least, see below).

Also, concerning the other supported logging frameworks:

JUL already exposes its operations via JMX by virtue of being part of Java SE, regardless of user's choice of logging framework
Log4J2 enables JMX support by default
Log4J does not have production ready JMX support)

HTTP operations are easily enabled using Jolokia.

WDYT?

vpavic · 2015-11-24T22:11:59Z

@philwebb, @snicoll, any thoughts on my previous comment?

philwebb · 2015-11-24T22:19:08Z

@vpavic Not yet. I've been on vacation :)

snicoll · 2015-11-25T08:47:18Z

Thanks for the reminder. I only meant to say that if we had something, we should add something based on the LoggingSystem (i.e. the abstraction). We could also deploy dedicated JMX endpoints when they are available though.

vpavic · 2015-12-14T19:38:57Z

Any chance this makes the 1.3.1.RELEASE?

I'm also willing to work on a separate PR to provide the abstraction based endpoint, but I'm short of time ATM.

snicoll · 2015-12-14T19:50:46Z

I am not a huge fan of merging this one in the 1.3 line. The logging endpoint may actually be a mini-theme for 1.4 and if we merge this one now we'll have to maintain it in its current form. I'd rather give us enough space to improve it first (if necessary).

philwebb · 2015-12-14T19:57:53Z

It feels a little bit risky to add this to a point release. We've had some problems in the past where JMX beans clashed. There are already quite a lot of issues resolved for 1.3.1 so I'd rather defer some of the more risky ones.

vpavic · 2015-12-14T20:45:45Z

@snicoll, @philwebb, note taken - thanks for your feedback!

philwebb · 2016-01-06T16:11:14Z

@vpavic We're thinking of porting the Spring Cloud feature that lets you configure logging level properties via JMX. If we did this, all logging system would get update support, but some of the other features exposed via ch.qos.logback.classic.jmx.JMXConfiguratorMBean wouldn't be available.

What do you currently use JMXConfiguratorMBean for? Is it simply to update logger levels, or do you need the more advanced features such as reloadByURL?

vpavic · 2016-01-06T21:57:52Z

Hi @philwebb - yes, we primarily use it for updating logger levels, however level getters and reloadDefaultConfiguration operations are often used as well.

vpavic · 2016-02-03T07:23:20Z

We're thinking of porting the Spring Cloud feature that lets you configure logging level properties via JMX.

@philwebb could you provide some reference to this feature?

Also having in mind previous comment:

It feels a little bit risky to add this to a point release.

Any chance this PR (or something similar) sees light in 1.4.0? :)

vpavic · 2016-02-26T19:37:13Z

Any feedback on the previous comment?

snicoll · 2016-02-29T08:43:32Z

I am not super keen on deploying a logging-specific endpoint automatically at this point. I'd rather like to see a LoggingEndpoint of some sort with some of the features that are exposed by the one provided by logback. One way to mitigate that if you still want that MBean to be deployed is some hook point that would allow you to easily register it.

vpavic · 2016-02-29T09:31:47Z

@snicoll thanks for your feedback. I'll try to put together a PR for LoggingEndpoint shortly and see where that gets us.

Regarding the hook point you mentioned, I assume this would also be based on the logging abstraction, to provide customization regardless of the logging system used?

snicoll · 2016-02-29T09:38:17Z

I was more thinking of an hookpoint to allow you to register an extra MBean very easily.

vpavic · 2016-03-03T07:27:51Z

Closing in favor of #5321.

philwebb · 2016-03-29T19:58:28Z

Since #5321 is on hold, I wonder if it is worth reconsidering this one. See also #5396

vpavic · 2016-03-29T20:12:27Z

@philwebb thanks for reconsidering this.

FWIW I personally still highly prefer having native Logback JMX operations available, vs logging abstraction endpoint. This is not to say the logging abstraction endpoint has no use but IMO users familiar with Logback would probably also prefer native JMX.

snicoll

Thanks for your patience on this PR @vpavic

Unfortunately, I am not keen to merge this PR in its current state, see my comments for more details.

Also, there is nothing that unregisters that MBean on shutdown of the context as far as I can see, which means the MBean would be hanging around if the process does not stop.

What happens if several applications run in the same VM? As far as I can see the first one to start would "win". If Logback is ready to make that kind of compromise if a different context name isn't provided, that is fine by me (and probably ok) but it certainly isn't for something that is done automatically.

I'd rather much logback do that stuff automatically and be accountable for rather than us doing it.

Thoughts?

snicoll · 2018-01-09T11:09:45Z

...spring-boot/src/main/java/org/springframework/boot/logging/logback/LogbackLoggingSystem.java

+	private void registerJmxConfigurator(LoggerContext loggerContext,
+			LoggingInitializationContext initializationContext, String configLocation,
+			LogFile logFile) {
+		String objectNameAsStr = MBeanUtil.getObjectNameFor(


This is duplicating code from LogBack which enables the MBean with a single line in its XML configuration. Shouldn't we try to reduce the amount of code on our side for this?

snicoll · 2018-01-09T11:10:47Z

...g-boot/src/test/java/org/springframework/boot/logging/logback/LogbackLoggingSystemTests.java

 	public void testBasicConfigLocation() throws Exception {
 		this.loggingSystem.beforeInitialize();
 		assertThat(getConsoleAppender()).isNotNull();
+		assertThat(ManagementFactory.getPlatformMBeanServer().queryMBeans(


Please test this in a dedicated test. I'd like also to see a test that checks what happens if the MBean has already been registered (using the <jmxConfigurator /> option).

vpavic · 2018-01-12T07:51:45Z

Thanks for the feedback @snicoll, I've updated the PR to address your test related concerns.

This is duplicating code from LogBack which enables the MBean with a single line in its XML configuration. Shouldn't we try to reduce the amount of code on our side for this?

I'm not sure what exactly we can do about that since the code in subject, from ch.qos.logback.classic.gaffer.ConfigurationDelegate, isn't quite structured in a reusable manner. Did you have something specific in mind? My original idea was to keep the registration code as close as possible to Logback's, since that way we detect if JMXConfigurator was already registered via XML.

Also, there is nothing that unregisters that MBean on shutdown of the context as far as I can see, which means the MBean would be hanging around if the process does not stop.

What happens if several applications run in the same VM? As far as I can see the first one to start would "win". If Logback is ready to make that kind of compromise if a different context name isn't provided, that is fine by me (and probably ok) but it certainly isn't for something that is done automatically.

I'd rather much logback do that stuff automatically and be accountable for rather than us doing it.

Not sure I follow this part. Logback's original JMXConfigurator already implements LoggerContextListener and registers itself with the LoggerContext, thus handling the lifecycle events and unregistration. By extending the JMXConfigurator with our specific implementation we do the same.

snicoll · 2018-01-12T09:34:16Z

I'm not sure what exactly we can do about that

Ask Logback to provide a public API that does the same thing as this XML would be a good start I think. It may not work but at least they'll be aware we have this need.

Not sure I follow this part.

Sorry I had overlooked the listener stuff. This PR has missing tests then. If we do something automatically, we need to test what happens on shutdown, what happens when two apps are started in the same process, etc.

Does that make sense?

vpavic · 2018-01-15T07:24:49Z

I've opened LOGBACK-1371 and added a couple of test cases related to lifecycle related aspects.

snicoll

Thanks for the feedback @vpavic!

I am still not confident to merge this change so I've flagged it to see what the rest of the team thinks. I've also added a comment to the issue you've created to check if the Logback team would consider flipping the default.

snicoll · 2018-01-15T08:04:02Z

...g-boot/src/test/java/org/springframework/boot/logging/logback/LogbackLoggingSystemTests.java

+	}
+
+	@Test
+	public void testTwoLoggingSystemsYieldSingleJmxConfigurator() throws Exception {


I don't consider this to be a good thing actually. Which logging configuration are you effectively managing with such setup? Is there a way to provide another name than default so that we get one MBean per app instead?

snicoll · 2018-01-15T08:04:49Z

spring-boot-project/spring-boot/src/test/resources/logback-jmxconfigurator.xml

+<configuration>
+	<include resource="org/springframework/boot/logging/logback/defaults.xml"/>
+	<include resource="org/springframework/boot/logging/logback/console-appender.xml"/>
+	<jmxConfigurator/>


I'd put this one first so that it is more explicit

wilkinsona · 2018-01-17T10:02:11Z

FWIW, I share @snicoll's reservations. In particular, getting a single JXM configurator when there are multiple logging systems doesn't sound right to me.

snicoll · 2018-05-18T09:56:36Z

@vpavic given the discussion above, I don't think we can merge this and I haven't seen any progress on the logback issue you've created. If that changes, I am sure another PR will have to crafted anyway and I am more than happy to review it.

vpavic · 2018-05-21T06:52:31Z

Just to clarify - you're saying LOGBACK-1371 is a prerequisite for anything to be done on Spring Boot side?

snicoll · 2018-05-21T07:36:44Z

Yes. We could consider a new proposal that doesn't exhibit the problems we've raised in the last comment but I'd really prefer if Logback was providing a way to do this without us having to copy/paste code. Also, this is a problem because they don't do it by default and I'd rather have them take that responsibility for their own namespace than us.

vpavic · 2018-05-21T07:40:38Z

Also, this is a problem because they don't do it by default and I'd rather have them take that responsibility for their own namespace than us.

I agree that would be preferred - thanks for the feedback @snicoll.

philwebb added the for: team-attention An issue we'd like other members of the team to review label Nov 24, 2015

wilkinsona added team-meeting and removed for: team-attention An issue we'd like other members of the team to review labels Nov 25, 2015

philwebb removed the team-meeting label Jan 7, 2016

snicoll changed the title ~~Add support for configuring logging via JMX~~ Add support for configuring lobgack logging via JMX Feb 3, 2016

vpavic mentioned this pull request Mar 3, 2016

Provide LoggingEndpoint #5321

Closed

vpavic closed this Mar 3, 2016

philwebb reopened this Mar 29, 2016

philwebb added type: enhancement A general enhancement for: team-attention An issue we'd like other members of the team to review labels Mar 29, 2016

vpavic force-pushed the logging-jmx-support branch from d1272f0 to 59437df Compare November 16, 2017 20:51

philwebb added this to the 2.0.0.RC1 milestone Dec 8, 2017

vpavic force-pushed the logging-jmx-support branch from 59437df to 885f9a3 Compare December 11, 2017 11:06

snicoll self-assigned this Jan 8, 2018

snicoll requested changes Jan 9, 2018

View reviewed changes

snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jan 9, 2018

vpavic force-pushed the logging-jmx-support branch from 885f9a3 to 7cd0103 Compare January 12, 2018 07:43

Add support for configuring logging via JMX

de996f8

vpavic force-pushed the logging-jmx-support branch from 7cd0103 to de996f8 Compare January 15, 2018 07:24

snicoll added for: team-attention An issue we'd like other members of the team to review and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 15, 2018

snicoll reviewed Jan 15, 2018

View reviewed changes

snicoll removed their assignment Jan 16, 2018

snicoll modified the milestones: 2.0.0.RC1, 2.1.0 Jan 17, 2018

snicoll removed the for: team-attention An issue we'd like other members of the team to review label Jan 17, 2018

philwebb removed the priority: normal label Mar 21, 2018

philwebb force-pushed the master branch from 86d82f9 to 7b120c1 Compare May 4, 2018 23:38

snicoll closed this May 18, 2018

snicoll removed this from the Icebox milestone May 18, 2018

snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed type: enhancement A general enhancement labels May 18, 2018

Add support for configuring lobgack logging via JMX #4486

Add support for configuring lobgack logging via JMX #4486

Uh oh!

Conversation

vpavic commented Nov 14, 2015

Uh oh!

philwebb commented Nov 14, 2015

Uh oh!

snicoll commented Nov 14, 2015

Uh oh!

vpavic commented Nov 14, 2015

Uh oh!

vpavic commented Nov 24, 2015

Uh oh!

philwebb commented Nov 24, 2015

Uh oh!

snicoll commented Nov 25, 2015

Uh oh!

vpavic commented Dec 14, 2015

Uh oh!

snicoll commented Dec 14, 2015

Uh oh!

philwebb commented Dec 14, 2015

Uh oh!

vpavic commented Dec 14, 2015

Uh oh!

philwebb commented Jan 6, 2016

Uh oh!

vpavic commented Jan 6, 2016

Uh oh!

vpavic commented Feb 3, 2016

Uh oh!

vpavic commented Feb 26, 2016

Uh oh!

snicoll commented Feb 29, 2016

Uh oh!

vpavic commented Feb 29, 2016

Uh oh!

snicoll commented Feb 29, 2016

Uh oh!

vpavic commented Mar 3, 2016

Uh oh!

philwebb commented Mar 29, 2016

Uh oh!

vpavic commented Mar 29, 2016

Uh oh!

snicoll left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snicoll Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

snicoll Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

vpavic commented Jan 12, 2018

Uh oh!

snicoll commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vpavic commented Jan 15, 2018

Uh oh!

snicoll left a comment

Choose a reason for hiding this comment

Uh oh!

snicoll Jan 15, 2018

Choose a reason for hiding this comment

Uh oh!

snicoll Jan 15, 2018

Choose a reason for hiding this comment

Uh oh!

wilkinsona commented Jan 17, 2018

Uh oh!

snicoll commented May 18, 2018

Uh oh!

vpavic commented May 21, 2018

Uh oh!

snicoll commented May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

snicoll left a comment •

edited

Loading

snicoll commented Jan 12, 2018 •

edited

Loading

snicoll commented May 21, 2018 •

edited

Loading