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

feat(logging): create global log4j config (#23631) #29694

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

spbolton
Copy link
Contributor

@spbolton spbolton commented Aug 21, 2024

Proposed Changes

Issue #23631

  • Add global log4j2 configuration
  • default log4j2.configurationFile set in tomcat setenv.sh
  • default Log4jContextSelector=org.apache.logging.log4j.core.async.BasicAsyncLoggerContextSelector
    • This provides a single context and is not based on the classloader
  • created lib folder for logging jars and added to tomcat classpath
  • moved redis session manager jars into their own folder
  • Normalized maven dependencies to help with debugging and versions centrally specified in bom
  • Removed log4j reloading functionality

As well as using a common configuration for tomcat and webapp logs. This also resolves an inconsistency when The logging class is referenced in code before the ContextLifecycleListener is called. Previously the wrong configuration contained in the resources classpath of the ant-tooling jar was picked up initially.

Tests

  • Test tomcat log messages are output to the same location
  • Test any issues with logging from plugins
  • Test ability to override log4j config and change log levels for core tomcat classes, webapp classes and plugin classes. currently can map to /srv/OVERRIDE/WEB-INF/log4j in container or update CATALINA_OPTS to include -Dlog4j2.configurationFile={path to file}
  • Test session manager functionality is not broken
  • Will need a review of the logging behavior from cloud engineering when this image is applied into our cloud environment including any log-level changes they may want to have by default

Further changes

There is follow up work to this PR that improves the way that the config can be specified and overridden. This change maintains the existing config file behavior but just applies it across the entire application.

Checklist

  • Tests
  • Translations
  • Security Implications Contemplated (add notes if applicable)

Additional Info

This PR resolves #23631 (create global log4j config).

Screenshots

Original Updated
** original screenshot ** ** updated screenshot **

This PR fixes: #23631

@spbolton spbolton linked an issue Aug 21, 2024 that may be closed by this pull request
@spbolton spbolton changed the title feat(): create global log4j config (#23631) feat(logging): create global log4j config (#23631) Aug 21, 2024
@spbolton spbolton force-pushed the issue-23631-initial-changes branch 3 times, most recently from f7791d0 to bb53cf5 Compare August 21, 2024 20:58
@spbolton spbolton added this pull request to the merge queue Aug 22, 2024
Merged via the queue into master with commit 46b0a3d Aug 22, 2024
31 checks passed
@spbolton spbolton deleted the issue-23631-initial-changes branch August 22, 2024 13:40
github-merge-queue bot pushed a commit that referenced this pull request Aug 24, 2024
… (#29746)

### Proposed Changes
* Fix classloader issue with session manager introduced in PR #29694
* Use CATALINA_HOME in setenv scripts instead of TOMCAT_HOME. The first
is always available, the second is only set within the docker startup
scripts

Instructions for Log4j require the classes to be added to the system
classloader when tomcat loads by adding to the main java CLASSPATH.
These logging jars do not need to reference any of the tomcat jars
the session manager jars do need to reference the tomcat jars e.g.
catalina.jar in the main lib folder. This is what is called the
common.loader and is defined in catalina.properties file.
the session manager jars either need to be in a folder referenced in
this common.loader property or as before dropped into the existing lib
folder. There is no need to change the old behaviour at this time so we
can just change the install location back.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create global log4j config
4 participants