Skip to content

Conversation

@vpavic
Copy link
Contributor

@vpavic vpavic commented Jul 7, 2016

As described in #5590, FixedWindowRollingPolicy with size limit of 10 MB is used in the default file appender.

This PR changes the default file appender to use SizeAndTimeBasedRollingPolicy and introduces two new properties to improve log file configuration capabilities - logging.file.max-history to limit the number of archive log files to keep and logging.file.max-size to limit the log file size.

Previous size limit of 10 MB is retained as the default and daily rollover is used for time limit.

  • I have signed the CLA

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 7, 2016
@philwebb philwebb added for: team-attention An issue we'd like other members of the team to review and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 7, 2016
@vpavic
Copy link
Contributor Author

vpavic commented Jul 9, 2016

This PR also addresses #4379.

Nice to see this was taken into consideration and labeled as for-team-discussion. I'd also like to comment here and elaborate a bit more my motivation for this and several other logging related PRs, as well as some general thoughts on logging configuration in Boot.

With console output being advocated as preferred logging option feeling is that Boot is (perhaps understandably) tilted towards cloud environments but IMO does not offer the greatest support for running as Linux service.

Well behaved and system friendly Linux services should actually log minimal output to console which is related to the state of the service itself (starting, started, stopping... etc) since that output is generally redirected to system log. Talking in terms of systemd, which is AFAIK the most popular init service in current version of most major Linux distros ATM, console output automatically ends up in journal logging system and the latest entries can be observer upon inspecting service status using systemctl status <service_name> command.

Actual logs regarding the domain of the service in subject should actually end up in different place, which is generally /var/log/<app_name>. These logs should be of course rolled, but not in the manner the current Boot's file appender does, dropping archived logs after just dozens MB of logs have been written. Users should have an option to easily decide size of their log files, and the amount of history kept which is what this PR addresses.

Regarding the separation/filtering of console and file logs, I've also created #6366 which addresses that part and should perhaps be considered together with this change.

Since I've already gone at length about what I'd expect to be able to easily configure regarding logging, I'd also like to reiterate some thoughts about logging system management already expressed in this comment and addressed by #4486.

@philwebb philwebb added this to the 1.4.0 milestone Jul 13, 2016
@philwebb philwebb self-assigned this Jul 13, 2016
@snicoll snicoll removed the for: team-attention An issue we'd like other members of the team to review label Jul 13, 2016
@philwebb philwebb removed this from the 1.4.0 milestone Jul 13, 2016
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 13, 2016
@philwebb philwebb removed the status: waiting-for-triage An issue we've not yet triaged label Jul 13, 2016
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 14, 2016
@philwebb
Copy link
Member

I was looking to merge this in 1.4 but I'm getting worried that we're starting to add complexity to the logging system and ultimately duplicating the logback.xml file. I think we should consider all these logging refinements in 1.5 where we'll have more time to think about the impact.

Things that are of particular concern:

  • How much of logback.xml should we cover in application.properties
  • What key names should we use (e.g. logging.file and logging.file.max-size overlap, should we use logging.file.pattern rather than logging.pattern.file)
  • How many of these properties are logback specific (e.g. does logging.file.max-history make sense for anything other than logback).
  • How much can we define in our shipped *.xml files. (e.g It looks like Add option to configure logging filters #6366 can only be applied via the DefaultLogbackConfiguration class).

Thanks for all your efforts here. I'm sure that we'll be able to do something for 1.5, I just don't want to rush to add something in 1.4 that we might later prefer was fixed in a different way.

@philwebb philwebb removed the status: waiting-for-triage An issue we've not yet triaged label Jul 14, 2016
@philwebb philwebb added this to the 1.5.0 milestone Jul 14, 2016
@vpavic
Copy link
Contributor Author

vpavic commented Jul 14, 2016

Thanks for your feedback.

When creating this PR and #6366 I thought the target release would be 1.5 since they both introduce new configuration properties so 👍 from me on not rushing things.

Since Boot offers a very nice support for installing apps as Linux services with minimal effort, IMO it makes sense to provide adequate logging support for such use-case too rather than having to resort to manual logging configuration. So I primarily see this as providing support for this fairly common use-case rather than logback.xml duplication. You can do so much with logback.xml, of course most of it shouldn't be covered by configuration properties.

should we use logging.file.pattern rather than logging.pattern.file

Personally I'm in favor of grouping thing like logging.console.* and logging.file.*.

does logging.file.max-history make sense for anything other than logback

I think not. OTOH logging.file.max-size can easily be supported with other logging systems.

It looks like #6366 can only be applied via the DefaultLogbackConfiguration class

This is correct.

@philwebb philwebb modified the milestones: 1.5.0, 1.5.0 M1 Aug 30, 2016
@vpavic vpavic force-pushed the gh-5590 branch 2 times, most recently from 9cdead7 to a2301db Compare September 2, 2016 20:01
@vpavic
Copy link
Contributor Author

vpavic commented Sep 7, 2016

@philwebb Could you provide an update on status of this (and related issues), considering it was recently removed from the 1.5 milestone?

@philwebb
Copy link
Member

philwebb commented Sep 8, 2016

Sorry @vpavic, I should have added a comment when I removed the target milestone.

We did some planning for the next Spring Boot versions and realized that it was quite critical to try and get Boot 2.0 out not long after Spring 5 so that people can try the reactive web framework. In order to give ourselves as much time as possible we decided to make Spring Boot 1.5 as light as possible (it will mainly be version updates and few minor improvements).

Our plan is now to branch the code after we release 1.4.1. Master will be for 2.0 work, and we'll create a 1.5.xbranch for the next version and a 1.4.x maintenance branch as well. This should allow us to get cracking with the most risky 2.0 changes immediately and work on the small 1.5 updates.

Once v2.0.0.M1 is released (with all the breaking changes we want to make), we'll go back and sweep the issue tracker for pull-requests and additional issues that seem feasible.

I'm really sorry to leave this one hanging again. Once all the high risk 2.0 work is out the way, we should have a bit more time.

@vpavic
Copy link
Contributor Author

vpavic commented Sep 8, 2016

@philwebb Thank you for the detailed insight into plans for the upcoming Spring Boot versions. It's very much appreciated! This information is valuable to me since we usually align our projects against a certain version of Spring Boot, taking into account our target delivery dates and expected GA date for a given Spring Boot version. I wonder if this kind of info could be made available on project's wiki, or perhaps blog, to supplement the projections from the milestones page?

I guess your comment here also covers some of the other PR's I've proposed, whose status I had planned to inquire about as well. Thanks again.

@jorgheymans
Copy link

👍 for logging.file.max-size

@philwebb philwebb added priority: normal type: enhancement A general enhancement and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged labels Feb 2, 2017
@philwebb philwebb added this to the 2.0.0.M4 milestone Feb 2, 2017
@iNikem
Copy link

iNikem commented Mar 8, 2017

@vpavic Do you care to solve the merging conflict that this PR seems to have?

@vpavic
Copy link
Contributor Author

vpavic commented Mar 8, 2017

@iNikem Since this PR is targeted at 2.0.0.M4 milestone, which is scheduled for August 7th ATM, there's plenty of time to resolve the conflicts.

Previously, `FixedWindowRollingPolicy` with size limit of 10 MB was used as default file appender.

This commit changes the default file appender to use `SizeAndTimeBasedRollingPolicy` and introduces two new properties to improve log file configuration capabilities - `logging.file.max-history` to limit the number of archive log files to keep and `logging.file.max-size` to limit the log file size.
@philwebb
Copy link
Member

philwebb commented Nov 2, 2017

Thanks once again @vpavic !

philwebb pushed a commit that referenced this pull request Nov 2, 2017
Update the logback file appender to use `SizeAndTimeBasedRollingPolicy`
rather than `FixedWindowRollingPolicy`.

Add two new properties to improve log file configuration capabilities:

 - `logging.file.max-history` to limit the number of archive log files
    to keep.
 - `logging.file.max-size` to limit the log file size.

See gh-6352
@philwebb philwebb closed this in f0327fb Nov 2, 2017
philwebb added a commit that referenced this pull request Nov 2, 2017
* pr/6352:
  Polish SizeAndTimeBasedRollingPolicy changes
  Use SizeAndTimeBasedRollingPolicy file appender
@vpavic vpavic deleted the gh-5590 branch November 3, 2017 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: enhancement A general enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants