TRUNK-6392: Improvements to the OpenMRS Logging plugin#6104
Open
ibacher wants to merge 10 commits into
Open
Conversation
OpenMRS leverages Log4J2's plugin system to provide a compatibility layer with older versions of OpenMRS's ability to configure the logging system. This commit addresses a number of small issues in the initial implementation of this plugin that do not work as expected in production conditions. This includes: - Proper handling of looking up custom logger properties during application start-up - Incorrect synchronization of the MemoryAppender, which could cause message losses between reconfigurations - Not acquiring correct privileges to read global properties - applyLogLevels could apply to the wrong logger, especially when being used to configure loggers that aren't part of the default configuration - Composite logger configurations were not correctly handled It also adds regression testing for most of the features in the logging plugin.
There was a problem hiding this comment.
Pull request overview
This pull request improves the OpenMRS Log4J2 logging plugin compatibility layer to behave more reliably under real startup and reconfiguration conditions, and adds regression tests around property lookup, configuration discovery, log-level application, and global property listener behavior.
Changes:
- Add proxy-privilege handling when reading global properties for logging configuration/lookup.
- Improve logger level application to correctly target non-default/custom logger names and handle composite configurations.
- Refactor in-memory log buffering (MemoryAppender) and add comprehensive unit/integration-style tests for the logging utilities and plugin behaviors.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| api/src/main/java/org/openmrs/logging/OpenmrsConfigurationFactory.java | Improves config discovery and multi-config composition; applies log levels with privilege elevation. |
| api/src/main/java/org/openmrs/logging/OpenmrsLoggingUtil.java | Fixes log appender lookup, adds privilege elevation for GP reads, and improves logger config targeting. |
| api/src/main/java/org/openmrs/logging/OpenmrsPropertyLookup.java | Adds startup-safe defaults and privilege elevation for GP lookups used in Log4J2 configs. |
| api/src/main/java/org/openmrs/logging/MemoryAppender.java | Refactors buffer lifecycle/sharing across reconfigurations and adds queue-capacity awareness. |
| api/src/main/java/org/openmrs/logging/LoggingConfigurationGlobalPropertyListener.java | Adjusts deletion handling to avoid unnecessary reloads for log.level and fixes switch flow. |
| api/src/main/java/org/openmrs/util/ThreadSafeCircularFifoQueue.java | Adds capacity() accessor used by MemoryAppender refactor. |
| api/src/main/java/org/openmrs/util/MemoryAppender.java | Updates deprecated adapter to match new MemoryAppender constructor signature. |
| api/src/test/java/org/openmrs/logging/OpenmrsPropertyLookupTest.java | New tests for property lookup behavior across startup vs normal operation, including privilege handling. |
| api/src/test/java/org/openmrs/logging/OpenmrsLoggingUtilTest.java | New tests for logger level application, log location lookup, and reload behavior. |
| api/src/test/java/org/openmrs/logging/OpenmrsConfigurationFactoryTest.java | New tests for discovering/sorting/ignoring config files and extension constants. |
| api/src/test/java/org/openmrs/logging/MemoryAppenderTest.java | Expanded tests for buffer sizing/defaults, ordering, overwrites, plugin factory behavior, and layout validation. |
| api/src/test/java/org/openmrs/logging/LoggingConfigurationGlobalPropertyListenerTest.java | New tests ensuring correct reload/apply behavior and caching semantics on GP updates/deletes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
api/src/main/java/org/openmrs/logging/LoggingConfigurationGlobalPropertyListener.java:89
- In globalPropertyDeleted(), the GLOBAL_PROPERTY_LOG_LEVEL case falls through to the unconditional reloadLoggingConfiguration() call, so deleting log.level will still trigger a full logging reconfiguration. This contradicts the class javadoc and the listener’s globalPropertyChanged() behavior (which returns early for log.level). Consider returning immediately for GLOBAL_PROPERTY_LOG_LEVEL so deletion does not reload the whole configuration.
public void globalPropertyDeleted(String propertyName) {
switch (propertyName) {
case OpenmrsConstants.GP_LOG_LAYOUT:
logLayout = null;
break;
case OpenmrsConstants.GP_LOG_LOCATION:
logLocation = null;
break;
case OpenmrsConstants.GLOBAL_PROPERTY_LOG_LEVEL:
break;
default:
return;
}
OpenmrsLoggingUtil.reloadLoggingConfiguration();
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
api/src/main/java/org/openmrs/logging/OpenmrsLoggingUtil.java:219
- The
@NonNullannotation onlogLevelconflicts with the explicitlogLevel == nullhandling (and corresponding tests that pass null). To avoid misleading nullness tooling, either change the parameter to@Nullable(and keep the guard) or remove the null guard and enforce non-null at call sites.
public static Level stringToLevel(@NonNull String logLevel, @Nullable String logClass) {
Level level;
if (logLevel == null) {
return Level.WARN;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
api/src/test/java/org/openmrs/logging/MemoryAppenderTest.java:346
- In this test,
appender1/appender2are started but never stopped. That can leave background logging components registered and make the test suite order-dependent. Stop both appenders in thefinallyblock to ensure proper cleanup.
// All events should be preserved
List<String> logLines = appender2.getLogLines();
assertThat(logLines, contains("A", "B", "C"));
assertThat(appender2.getBufferSize(), equalTo(10));
} finally {
migrationLogger.removeAppender(appender1);
migrationLogger.setLevel(null);
((Logger) LogManager.getRootLogger()).getContext().updateLoggers();
}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6104 +/- ##
============================================
+ Coverage 59.04% 59.22% +0.17%
- Complexity 9237 9296 +59
============================================
Files 693 693
Lines 37257 37306 +49
Branches 5485 5495 +10
============================================
+ Hits 21999 22094 +95
+ Misses 13287 13230 -57
- Partials 1971 1982 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Also moves GP check to cases where we actually have a session to use
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of what I changed
OpenMRS leverages Log4J2's plugin system to provide a compatibility layer with older versions of OpenMRS's ability to configure the logging system.
This commit addresses a number of small issues in the initial implementation of this plugin that do not work as expected in production conditions. This includes:
It also adds regression testing for most of the features in the logging plugin.
Issue I worked on
see https://issues.openmrs.org/browse/TRUNK-6392
Note that this goes a bit beyond the scope of that ticket, but does address the concern as well.
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream masterSummary by CodeRabbit
Bug Fixes
Tests