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

log4j2: Use size based triggering instead of time based #9849

Closed
wants to merge 1 commit into from

Conversation

vishesh92
Copy link
Member

Description

This PR changes the triggering policy for log4j from time based to size based. This helps prevents issues where disk gets full due to logs on the disk and the application becomes unavailable.

I have set the max file size to 100 MB and the maximum number of old logs files to be retained to 10. At max the storage required for a single log file will be 100MB * (10 old log files + 1 current log file) = 1100MB. The actual size would be less since the old log files are gzipped.

Added these 2 variables which are configured in replace.properties:

LOGMAXINDEX - Maximum number of old files to retain.
LOGMAXFILESIZE - Maximum size of the log file before rotation

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI
  • test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

How Has This Been Tested?

How did you try to break this feature and the system with this change?

@vishesh92
Copy link
Member Author

@blueorangutan package

@blueorangutan
Copy link

@vishesh92 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

@weizhouapache
Copy link
Member

@vishesh92
what's the purpose of this change ? anyone complained about it ?

the new file names are not user-friendly. It will be difficult to find the logs of a specific date.

Copy link

codecov bot commented Oct 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 15.78%. Comparing base (019f2c6) to head (df70c16).

Additional details and impacted files
@@            Coverage Diff             @@
##               main    #9849    +/-   ##
==========================================
  Coverage     15.78%   15.78%            
- Complexity    12564    12565     +1     
==========================================
  Files          5627     5627            
  Lines        492250   492250            
  Branches      61405    61913   +508     
==========================================
  Hits          77710    77710            
  Misses       406066   406066            
  Partials       8474     8474            
Flag Coverage Δ
uitests 4.04% <ø> (ø)
unittests 16.60% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@rohityadavcloud rohityadavcloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vishesh92 maybe we can keep the default triggering policy to be time based but document how size based policy could be used? So the PR is essentially changes in replace.properties & a separate doc PR to given examples on configuration of logging.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11426

@weizhouapache
Copy link
Member

@vishesh92 maybe we can keep the default triggering policy to be time based but document how size based policy could be used? So the PR is essentially changes in replace.properties & a separate doc PR to given examples on configuration of logging.

actually we could add both policies in the packages and install on mgmt server and agent, and let user to choose.
The old time-based policy should be default. as @rohityadavcloud and @harikrishna-patnala said

I also agree with @NuxRo that it would be better to add the timestamp to the filename, instead of `%i%.
@vishesh92 is it possible ?

LOGMAXINDEX=10 is too small in my opinion. the compression size of a 100MB log could be smaller than 10MB.

Copy link
Member

@rohityadavcloud rohityadavcloud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove size based trigger policy from the PR - but instead create a doc PR for users who would want it, on how to do that.

@vishesh92
Copy link
Member Author

Let's remove size based trigger policy from the PR - but instead create a doc PR for users who would want it, on how to do that.

Created a doc PR for this here: apache/cloudstack-documentation#450

Closing this PR.

@vishesh92 vishesh92 closed this Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants