Use try-with-resources with MockLogAppender (#1595)

I previously added a helper that started a MockLogAppender to ensure it
was never added to a Logger before it was started. I subsequently found
the opposite case in RolloverIT.java where the appender was stopped
before it was closed, therefore creating a race where a concurrently
running test in the same JVM could cause a logging failure. This seems
like a really easy mistake to make when writing a test or introduce when
refactoring a test. I've made a change to use try-with-resources to
ensure that proper setup and teardown is done. This should make it much
harder to introduce this particular test bug in the future.
Unfortunately, it did involve touching a lot of files. The changes here
are purely structural to leverage try-with-resources; no testing logic
has been changed.

Signed-off-by: Andrew Ross <andrross@amazon.com>

modules/transport-netty4/src/internalClusterTest/java/org/opensearch/transport/netty4/OpenSearchLoggingHandlerIT.java

@@ -36,7 +36,6 @@ import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
 import org.opensearch.OpenSearchNetty4IntegTestCase;
 import org.opensearch.action.admin.cluster.node.hotthreads.NodesHotThreadsRequest;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.test.OpenSearchIntegTestCase;
 import org.opensearch.test.InternalTestCluster;
 import org.opensearch.test.MockLogAppender;
@@ -53,17 +52,15 @@ public class OpenSearchLoggingHandlerIT extends OpenSearchNetty4IntegTestCase {
 
     public void setUp() throws Exception {
         super.setUp();
-        appender = MockLogAppender.createStarted();
+        appender = MockLogAppender.createForLoggers(
-        Loggers.addAppender(LogManager.getLogger(OpenSearchLoggingHandler.class), appender);
+            LogManager.getLogger(OpenSearchLoggingHandler.class),
-        Loggers.addAppender(LogManager.getLogger(TransportLogger.class), appender);
+            LogManager.getLogger(TransportLogger.class),
-        Loggers.addAppender(LogManager.getLogger(TcpTransport.class), appender);
+            LogManager.getLogger(TcpTransport.class)
+        );
     }
 
     public void tearDown() throws Exception {
-        Loggers.removeAppender(LogManager.getLogger(OpenSearchLoggingHandler.class), appender);
+        appender.close();
-        Loggers.removeAppender(LogManager.getLogger(TransportLogger.class), appender);
-        Loggers.removeAppender(LogManager.getLogger(TcpTransport.class), appender);
-        appender.stop();
         super.tearDown();
     }
 

plugins/transport-nio/src/internalClusterTest/java/org/opensearch/transport/nio/NioTransportLoggingIT.java

@@ -37,7 +37,6 @@ import org.apache.logging.log4j.LogManager;
 
 import org.opensearch.NioIntegTestCase;
 import org.opensearch.action.admin.cluster.node.hotthreads.NodesHotThreadsRequest;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.test.OpenSearchIntegTestCase;
 import org.opensearch.test.InternalTestCluster;
 import org.opensearch.test.MockLogAppender;
@@ -54,15 +53,11 @@ public class NioTransportLoggingIT extends NioIntegTestCase {
 
     public void setUp() throws Exception {
         super.setUp();
-        appender = MockLogAppender.createStarted();
+        appender = MockLogAppender.createForLoggers(LogManager.getLogger(TransportLogger.class), LogManager.getLogger(TcpTransport.class));
-        Loggers.addAppender(LogManager.getLogger(TransportLogger.class), appender);
-        Loggers.addAppender(LogManager.getLogger(TcpTransport.class), appender);
     }
 
     public void tearDown() throws Exception {
-        Loggers.removeAppender(LogManager.getLogger(TransportLogger.class), appender);
+        appender.close();
-        Loggers.removeAppender(LogManager.getLogger(TcpTransport.class), appender);
-        appender.stop();
         super.tearDown();
     }
 

server/src/internalClusterTest/java/org/opensearch/action/admin/indices/rollover/RolloverIT.java

@@ -44,7 +44,6 @@ import org.opensearch.cluster.ClusterState;
 import org.opensearch.cluster.metadata.AutoExpandReplicas;
 import org.opensearch.cluster.metadata.IndexMetadata;
 import org.opensearch.cluster.routing.allocation.AllocationService;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.time.DateFormatter;
 import org.opensearch.common.unit.ByteSizeUnit;
@@ -263,8 +262,8 @@ public class RolloverIT extends OpenSearchIntegTestCase {
         ensureGreen();
         Logger allocationServiceLogger = LogManager.getLogger(AllocationService.class);
 
-        MockLogAppender appender = new MockLogAppender();
+        final RolloverResponse response;
-        appender.start();
+        try (MockLogAppender appender = MockLogAppender.createForLoggers(allocationServiceLogger)) {
             appender.addExpectation(
                 new MockLogAppender.UnseenEventExpectation(
                     "no related message logged on dry run",
@@ -273,13 +272,11 @@ public class RolloverIT extends OpenSearchIntegTestCase {
                     "*test_index*"
                 )
             );
-        Loggers.addAppender(allocationServiceLogger, appender);
 
-        final RolloverResponse response = client().admin().indices().prepareRolloverIndex("test_alias").dryRun(true).get();
+            response = client().admin().indices().prepareRolloverIndex("test_alias").dryRun(true).get();
 
             appender.assertAllExpectationsMatched();
-        appender.stop();
+        }
-        Loggers.removeAppender(allocationServiceLogger, appender);
 
         assertThat(response.getOldIndex(), equalTo("test_index-1"));
         assertThat(response.getNewIndex(), equalTo("test_index-000002"));

server/src/internalClusterTest/java/org/opensearch/cluster/allocation/ClusterRerouteIT.java

@@ -59,7 +59,6 @@ import org.opensearch.cluster.routing.allocation.decider.EnableAllocationDecider
 import org.opensearch.cluster.routing.allocation.decider.ThrottlingAllocationDecider;
 import org.opensearch.common.Priority;
 import org.opensearch.common.io.FileSystemUtils;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.unit.TimeValue;
 import org.opensearch.core.internal.io.IOUtils;
@@ -458,8 +457,7 @@ public class ClusterRerouteIT extends OpenSearchIntegTestCase {
 
         Logger actionLogger = LogManager.getLogger(TransportClusterRerouteAction.class);
 
-        MockLogAppender dryRunMockLog = new MockLogAppender();
+        try (MockLogAppender dryRunMockLog = MockLogAppender.createForLoggers(actionLogger)) {
-        dryRunMockLog.start();
             dryRunMockLog.addExpectation(
                 new MockLogAppender.UnseenEventExpectation(
                     "no completed message logged on dry run",
@@ -468,7 +466,6 @@ public class ClusterRerouteIT extends OpenSearchIntegTestCase {
                     "allocated an empty primary*"
                 )
             );
-        Loggers.addAppender(actionLogger, dryRunMockLog);
 
             AllocationCommand dryRunAllocation = new AllocateEmptyPrimaryAllocationCommand(indexName, 0, nodeName1, true);
             ClusterRerouteResponse dryRunResponse = client().admin()
@@ -485,11 +482,9 @@ public class ClusterRerouteIT extends OpenSearchIntegTestCase {
             assertThat(dryRunResponse.getExplanations().getYesDecisionMessages().get(0), containsString("allocated an empty primary"));
 
             dryRunMockLog.assertAllExpectationsMatched();
-        dryRunMockLog.stop();
+        }
-        Loggers.removeAppender(actionLogger, dryRunMockLog);
 
-        MockLogAppender allocateMockLog = new MockLogAppender();
+        try (MockLogAppender allocateMockLog = MockLogAppender.createForLoggers(actionLogger)) {
-        allocateMockLog.start();
             allocateMockLog.addExpectation(
                 new MockLogAppender.SeenEventExpectation(
                     "message for first allocate empty primary",
@@ -506,7 +501,6 @@ public class ClusterRerouteIT extends OpenSearchIntegTestCase {
                     "allocated an empty primary*" + nodeName2 + "*"
                 )
             );
-        Loggers.addAppender(actionLogger, allocateMockLog);
 
             AllocationCommand yesDecisionAllocation = new AllocateEmptyPrimaryAllocationCommand(indexName, 0, nodeName1, true);
             AllocationCommand noDecisionAllocation = new AllocateEmptyPrimaryAllocationCommand("noexist", 1, nodeName2, true);
@@ -524,8 +518,7 @@ public class ClusterRerouteIT extends OpenSearchIntegTestCase {
             assertThat(response.getExplanations().getYesDecisionMessages().get(0), containsString(nodeName1));
 
             allocateMockLog.assertAllExpectationsMatched();
-        allocateMockLog.stop();
+        }
-        Loggers.removeAppender(actionLogger, allocateMockLog);
     }
 
     public void testClusterRerouteWithBlocks() {

server/src/internalClusterTest/java/org/opensearch/discovery/single/SingleNodeDiscoveryIT.java

@@ -39,7 +39,6 @@ import org.apache.logging.log4j.core.LogEvent;
 import org.opensearch.cluster.ClusterState;
 import org.opensearch.cluster.coordination.JoinHelper;
 import org.opensearch.cluster.service.ClusterService;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.node.Node.DiscoverySettings;
 import org.opensearch.test.OpenSearchIntegTestCase;
@@ -119,8 +118,8 @@ public class SingleNodeDiscoveryIT extends OpenSearchIntegTestCase {
     }
 
     public void testCannotJoinNodeWithSingleNodeDiscovery() throws Exception {
-        MockLogAppender mockAppender = new MockLogAppender();
+        Logger clusterLogger = LogManager.getLogger(JoinHelper.class);
-        mockAppender.start();
+        try (MockLogAppender mockAppender = MockLogAppender.createForLoggers(clusterLogger)) {
             mockAppender.addExpectation(
                 new MockLogAppender.SeenEventExpectation("test", JoinHelper.class.getCanonicalName(), Level.INFO, "failed to join") {
 
@@ -175,17 +174,10 @@ public class SingleNodeDiscoveryIT extends OpenSearchIntegTestCase {
                     Function.identity()
                 )
             ) {
-
-            Logger clusterLogger = LogManager.getLogger(JoinHelper.class);
-            Loggers.addAppender(clusterLogger, mockAppender);
-            try {
                 other.beforeTest(random(), 0);
                 final ClusterState first = internalCluster().getInstance(ClusterService.class).state();
                 assertThat(first.nodes().getSize(), equalTo(1));
                 assertBusy(() -> mockAppender.assertAllExpectationsMatched());
-            } finally {
-                Loggers.removeAppender(clusterLogger, mockAppender);
-                mockAppender.stop();
             }
         }
     }

server/src/test/java/org/opensearch/bootstrap/MaxMapCountCheckTests.java

@@ -40,7 +40,6 @@ import org.apache.logging.log4j.message.ParameterizedMessage;
 import org.apache.lucene.util.Constants;
 import org.opensearch.cluster.metadata.Metadata;
 import org.opensearch.common.io.PathUtils;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.test.AbstractBootstrapCheckTestCase;
 import org.opensearch.test.MockLogAppender;
@@ -157,8 +156,7 @@ public class MaxMapCountCheckTests extends AbstractBootstrapCheckTestCase {
             final IOException ioException = new IOException("fatal");
             when(reader.readLine()).thenThrow(ioException);
             final Logger logger = LogManager.getLogger("testGetMaxMapCountIOException");
-            final MockLogAppender appender = new MockLogAppender();
+            try (MockLogAppender appender = MockLogAppender.createForLoggers(logger)) {
-            appender.start();
                 appender.addExpectation(
                     new ParameterizedMessageLoggingExpectation(
                         "expected logged I/O exception",
@@ -169,20 +167,17 @@ public class MaxMapCountCheckTests extends AbstractBootstrapCheckTestCase {
                         e -> ioException == e
                     )
                 );
-            Loggers.addAppender(logger, appender);
                 assertThat(check.getMaxMapCount(logger), equalTo(-1L));
                 appender.assertAllExpectationsMatched();
                 verify(reader).close();
-            Loggers.removeAppender(logger, appender);
+            }
-            appender.stop();
         }
 
         {
             reset(reader);
             when(reader.readLine()).thenReturn("eof");
             final Logger logger = LogManager.getLogger("testGetMaxMapCountNumberFormatException");
-            final MockLogAppender appender = new MockLogAppender();
+            try (MockLogAppender appender = MockLogAppender.createForLoggers(logger)) {
-            appender.start();
                 appender.addExpectation(
                     new ParameterizedMessageLoggingExpectation(
                         "expected logged number format exception",
@@ -193,12 +188,10 @@ public class MaxMapCountCheckTests extends AbstractBootstrapCheckTestCase {
                         e -> e instanceof NumberFormatException && e.getMessage().equals("For input string: \"eof\"")
                     )
                 );
-            Loggers.addAppender(logger, appender);
                 assertThat(check.getMaxMapCount(logger), equalTo(-1L));
                 appender.assertAllExpectationsMatched();
                 verify(reader).close();
-            Loggers.removeAppender(logger, appender);
+            }
-            appender.stop();
         }
 
     }

server/src/test/java/org/opensearch/cluster/NodeConnectionsServiceTests.java

@@ -34,6 +34,7 @@ package org.opensearch.cluster;
 
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.opensearch.OpenSearchTimeoutException;
 import org.opensearch.Version;
 import org.opensearch.action.ActionListener;
@@ -46,7 +47,6 @@ import org.opensearch.common.CheckedRunnable;
 import org.opensearch.common.UUIDs;
 import org.opensearch.common.component.Lifecycle;
 import org.opensearch.common.component.LifecycleListener;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.transport.BoundTransportAddress;
 import org.opensearch.common.transport.TransportAddress;
@@ -352,10 +352,8 @@ public class NodeConnectionsServiceTests extends OpenSearchTestCase {
         for (DiscoveryNode disconnectedNode : disconnectedNodes) {
             transportService.disconnectFromNode(disconnectedNode);
         }
-        MockLogAppender appender = new MockLogAppender();
+        final Logger logger = LogManager.getLogger("org.opensearch.cluster.NodeConnectionsService");
-        try {
+        try (MockLogAppender appender = MockLogAppender.createForLoggers(logger)) {
-            appender.start();
-            Loggers.addAppender(LogManager.getLogger("org.opensearch.cluster.NodeConnectionsService"), appender);
             for (DiscoveryNode targetNode : targetNodes) {
                 if (disconnectedNodes.contains(targetNode)) {
                     appender.addExpectation(
@@ -396,9 +394,6 @@ public class NodeConnectionsServiceTests extends OpenSearchTestCase {
 
             runTasksUntil(deterministicTaskQueue, CLUSTER_NODE_RECONNECT_INTERVAL_SETTING.get(Settings.EMPTY).millis());
             appender.assertAllExpectationsMatched();
-        } finally {
-            Loggers.removeAppender(LogManager.getLogger("org.opensearch.cluster.NodeConnectionsService"), appender);
-            appender.stop();
         }
         for (DiscoveryNode disconnectedNode : disconnectedNodes) {
             transportService.disconnectFromNode(disconnectedNode);
@@ -409,10 +404,8 @@ public class NodeConnectionsServiceTests extends OpenSearchTestCase {
         for (DiscoveryNode disconnectedNode : disconnectedNodes) {
             transportService.disconnectFromNode(disconnectedNode);
         }
-        appender = new MockLogAppender();
+
-        try {
+        try (MockLogAppender appender = MockLogAppender.createForLoggers(logger)) {
-            appender.start();
-            Loggers.addAppender(LogManager.getLogger("org.opensearch.cluster.NodeConnectionsService"), appender);
             for (DiscoveryNode targetNode : targetNodes) {
                 if (disconnectedNodes.contains(targetNode) && newTargetNodes.get(targetNode.getId()) != null) {
                     appender.addExpectation(
@@ -493,9 +486,6 @@ public class NodeConnectionsServiceTests extends OpenSearchTestCase {
             service.connectToNodes(newTargetNodes, () -> {});
             deterministicTaskQueue.runAllRunnableTasks();
             appender.assertAllExpectationsMatched();
-        } finally {
-            Loggers.removeAppender(LogManager.getLogger("org.opensearch.cluster.NodeConnectionsService"), appender);
-            appender.stop();
         }
     }
 

server/src/test/java/org/opensearch/cluster/coordination/CoordinatorTests.java

@@ -33,7 +33,6 @@ package org.opensearch.cluster.coordination;
 
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.core.LogEvent;
 import org.opensearch.OpenSearchException;
 import org.opensearch.Version;
@@ -48,7 +47,6 @@ import org.opensearch.cluster.node.DiscoveryNode;
 import org.opensearch.cluster.node.DiscoveryNodeRole;
 import org.opensearch.cluster.node.DiscoveryNodes;
 import org.opensearch.common.io.stream.StreamOutput;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.regex.Regex;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.settings.Settings.Builder;
@@ -1265,19 +1263,12 @@ public class CoordinatorTests extends AbstractCoordinatorTestCase {
 
             cluster1.clusterNodes.add(newNode);
 
-            MockLogAppender mockAppender = new MockLogAppender();
+            try (MockLogAppender mockAppender = MockLogAppender.createForLoggers(LogManager.getLogger(JoinHelper.class))) {
-            mockAppender.start();
                 mockAppender.addExpectation(
                     new MockLogAppender.SeenEventExpectation("test1", JoinHelper.class.getCanonicalName(), Level.INFO, "*failed to join*")
                 );
-            Logger joinLogger = LogManager.getLogger(JoinHelper.class);
-            Loggers.addAppender(joinLogger, mockAppender);
                 cluster1.runFor(DEFAULT_STABILISATION_TIME, "failing join validation");
-            try {
                 mockAppender.assertAllExpectationsMatched();
-            } finally {
-                Loggers.removeAppender(joinLogger, mockAppender);
-                mockAppender.stop();
             }
             assertEquals(0, newNode.getLastAppliedClusterState().version());
 
@@ -1554,10 +1545,11 @@ public class CoordinatorTests extends AbstractCoordinatorTestCase {
             }
 
             for (int i = scaledRandomIntBetween(1, 10); i >= 0; i--) {
-                final MockLogAppender mockLogAppender = new MockLogAppender();
+                try (
-                try {
+                    MockLogAppender mockLogAppender = MockLogAppender.createForLoggers(
-                    mockLogAppender.start();
+                        LogManager.getLogger(ClusterFormationFailureHelper.class)
-                    Loggers.addAppender(LogManager.getLogger(ClusterFormationFailureHelper.class), mockLogAppender);
+                    )
+                ) {
                     mockLogAppender.addExpectation(new MockLogAppender.LoggingExpectation() {
                         final Set<DiscoveryNode> nodesLogged = new HashSet<>();
 
@@ -1599,9 +1591,6 @@ public class CoordinatorTests extends AbstractCoordinatorTestCase {
                     });
                     cluster.runFor(warningDelayMillis + DEFAULT_DELAY_VARIABILITY, "waiting for warning to be emitted");
                     mockLogAppender.assertAllExpectationsMatched();
-                } finally {
-                    Loggers.removeAppender(LogManager.getLogger(ClusterFormationFailureHelper.class), mockLogAppender);
-                    mockLogAppender.stop();
                 }
             }
         }
@@ -1613,11 +1602,12 @@ public class CoordinatorTests extends AbstractCoordinatorTestCase {
             cluster.stabilise();
             final ClusterNode brokenNode = cluster.getAnyNodeExcept(cluster.getAnyLeader());
 
-            final MockLogAppender mockLogAppender = new MockLogAppender();
+            try (
-            try {
+                MockLogAppender mockLogAppender = MockLogAppender.createForLoggers(
-                mockLogAppender.start();
+                    LogManager.getLogger(Coordinator.CoordinatorPublication.class),
-                Loggers.addAppender(LogManager.getLogger(Coordinator.CoordinatorPublication.class), mockLogAppender);
+                    LogManager.getLogger(LagDetector.class)
-                Loggers.addAppender(LogManager.getLogger(LagDetector.class), mockLogAppender);
+                )
+            ) {
 
                 mockLogAppender.addExpectation(
                     new MockLogAppender.SeenEventExpectation(
@@ -1683,10 +1673,6 @@ public class CoordinatorTests extends AbstractCoordinatorTestCase {
                 );
 
                 mockLogAppender.assertAllExpectationsMatched();
-            } finally {
-                Loggers.removeAppender(LogManager.getLogger(Coordinator.CoordinatorPublication.class), mockLogAppender);
-                Loggers.removeAppender(LogManager.getLogger(LagDetector.class), mockLogAppender);
-                mockLogAppender.stop();
             }
         }
     }

server/src/test/java/org/opensearch/cluster/routing/allocation/DiskThresholdMonitorTests.java

@@ -33,7 +33,6 @@ package org.opensearch.cluster.routing.allocation;
 
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 import org.opensearch.Version;
 import org.opensearch.action.ActionListener;
 import org.opensearch.cluster.ClusterInfo;
@@ -51,7 +50,6 @@ import org.opensearch.cluster.routing.RoutingTable;
 import org.opensearch.cluster.routing.ShardRoutingState;
 import org.opensearch.common.Priority;
 import org.opensearch.common.collect.ImmutableOpenMap;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.ClusterSettings;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.index.shard.ShardId;
@@ -659,25 +657,30 @@ public class DiskThresholdMonitorTests extends OpenSearchAllocationTestCase {
 
     private void assertNoLogging(DiskThresholdMonitor monitor, ImmutableOpenMap<String, DiskUsage> diskUsages)
         throws IllegalAccessException {
-        MockLogAppender mockAppender = new MockLogAppender();
+        try (MockLogAppender mockAppender = MockLogAppender.createForLoggers(LogManager.getLogger(DiskThresholdMonitor.class))) {
-        mockAppender.start();
             mockAppender.addExpectation(
-            new MockLogAppender.UnseenEventExpectation("any INFO message", DiskThresholdMonitor.class.getCanonicalName(), Level.INFO, "*")
+                new MockLogAppender.UnseenEventExpectation(
+                    "any INFO message",
+                    DiskThresholdMonitor.class.getCanonicalName(),
+                    Level.INFO,
+                    "*"
+                )
             );
             mockAppender.addExpectation(
-            new MockLogAppender.UnseenEventExpectation("any WARN message", DiskThresholdMonitor.class.getCanonicalName(), Level.WARN, "*")
+                new MockLogAppender.UnseenEventExpectation(
+                    "any WARN message",
+                    DiskThresholdMonitor.class.getCanonicalName(),
+                    Level.WARN,
+                    "*"
+                )
             );
 
-        Logger diskThresholdMonitorLogger = LogManager.getLogger(DiskThresholdMonitor.class);
-        Loggers.addAppender(diskThresholdMonitorLogger, mockAppender);
-
             for (int i = between(1, 3); i >= 0; i--) {
                 monitor.onNewInfo(clusterInfo(diskUsages));
             }
 
             mockAppender.assertAllExpectationsMatched();
-        Loggers.removeAppender(diskThresholdMonitorLogger, mockAppender);
+        }
-        mockAppender.stop();
     }
 
     private void assertRepeatedWarningMessages(DiskThresholdMonitor monitor, ImmutableOpenMap<String, DiskUsage> diskUsages, String message)
@@ -701,7 +704,7 @@ public class DiskThresholdMonitorTests extends OpenSearchAllocationTestCase {
 
     private void assertLogging(DiskThresholdMonitor monitor, ImmutableOpenMap<String, DiskUsage> diskUsages, Level level, String message)
         throws IllegalAccessException {
-        MockLogAppender mockAppender = new MockLogAppender();
+        try (MockLogAppender mockAppender = MockLogAppender.createForLoggers(LogManager.getLogger(DiskThresholdMonitor.class))) {
             mockAppender.start();
             mockAppender.addExpectation(
                 new MockLogAppender.SeenEventExpectation("expected message", DiskThresholdMonitor.class.getCanonicalName(), level, message)
@@ -715,14 +718,10 @@ public class DiskThresholdMonitorTests extends OpenSearchAllocationTestCase {
                 )
             );
 
-        Logger diskThresholdMonitorLogger = LogManager.getLogger(DiskThresholdMonitor.class);
-        Loggers.addAppender(diskThresholdMonitorLogger, mockAppender);
-
             monitor.onNewInfo(clusterInfo(diskUsages));
 
             mockAppender.assertAllExpectationsMatched();
-        Loggers.removeAppender(diskThresholdMonitorLogger, mockAppender);
+        }
-        mockAppender.stop();
     }
 
     private static ClusterInfo clusterInfo(ImmutableOpenMap<String, DiskUsage> diskUsages) {

server/src/test/java/org/opensearch/cluster/service/ClusterApplierServiceTests.java

@@ -34,7 +34,6 @@ package org.opensearch.cluster.service;
 
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 import org.opensearch.Version;
 import org.opensearch.cluster.ClusterName;
 import org.opensearch.cluster.ClusterState;
@@ -47,7 +46,6 @@ import org.opensearch.cluster.node.DiscoveryNode;
 import org.opensearch.cluster.node.DiscoveryNodes;
 import org.opensearch.cluster.routing.allocation.decider.EnableAllocationDecider;
 import org.opensearch.cluster.service.ClusterApplier.ClusterApplyListener;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.ClusterSettings;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.unit.TimeValue;
@@ -131,8 +129,7 @@ public class ClusterApplierServiceTests extends OpenSearchTestCase {
 
     @TestLogging(value = "org.opensearch.cluster.service:TRACE", reason = "to ensure that we log cluster state events on TRACE level")
     public void testClusterStateUpdateLogging() throws Exception {
-        MockLogAppender mockAppender = new MockLogAppender();
+        try (MockLogAppender mockAppender = MockLogAppender.createForLoggers(LogManager.getLogger(ClusterApplierService.class))) {
-        mockAppender.start();
             mockAppender.addExpectation(
                 new MockLogAppender.SeenEventExpectation(
                     "test1",
@@ -158,9 +155,6 @@ public class ClusterApplierServiceTests extends OpenSearchTestCase {
                 )
             );
 
-        Logger clusterLogger = LogManager.getLogger(ClusterApplierService.class);
-        Loggers.addAppender(clusterLogger, mockAppender);
-        try {
             clusterApplierService.currentTimeOverride = threadPool.relativeTimeInMillis();
             clusterApplierService.runOnApplierThread(
                 "test1",
@@ -198,16 +192,12 @@ public class ClusterApplierServiceTests extends OpenSearchTestCase {
                 }
             });
             assertBusy(mockAppender::assertAllExpectationsMatched);
-        } finally {
-            Loggers.removeAppender(clusterLogger, mockAppender);
-            mockAppender.stop();
         }
     }
 
     @TestLogging(value = "org.opensearch.cluster.service:WARN", reason = "to ensure that we log cluster state events on WARN level")
     public void testLongClusterStateUpdateLogging() throws Exception {
-        MockLogAppender mockAppender = new MockLogAppender();
+        try (MockLogAppender mockAppender = MockLogAppender.createForLoggers(LogManager.getLogger(ClusterApplierService.class))) {
-        mockAppender.start();
             mockAppender.addExpectation(
                 new MockLogAppender.UnseenEventExpectation(
                     "test1 shouldn't see because setting is too low",
@@ -235,9 +225,6 @@ public class ClusterApplierServiceTests extends OpenSearchTestCase {
                 )
             );
 
-        Logger clusterLogger = LogManager.getLogger(ClusterApplierService.class);
-        Loggers.addAppender(clusterLogger, mockAppender);
-        try {
             final CountDownLatch latch = new CountDownLatch(4);
             final CountDownLatch processedFirstTask = new CountDownLatch(1);
             clusterApplierService.currentTimeOverride = threadPool.relativeTimeInMillis();
@@ -301,12 +288,9 @@ public class ClusterApplierServiceTests extends OpenSearchTestCase {
                 }
             });
             latch.await();
-        } finally {
-            Loggers.removeAppender(clusterLogger, mockAppender);
-            mockAppender.stop();
-        }
             mockAppender.assertAllExpectationsMatched();
         }
+    }
 
     public void testLocalNodeMasterListenerCallbacks() {
         TimedClusterApplierService timedClusterApplierService = createTimedClusterService(false);

server/src/test/java/org/opensearch/cluster/service/MasterServiceTests.java

@@ -34,7 +34,6 @@ package org.opensearch.cluster.service;
 
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 import org.opensearch.OpenSearchException;
 import org.opensearch.Version;
 import org.opensearch.cluster.AckedClusterStateUpdateTask;
@@ -54,7 +53,6 @@ import org.opensearch.cluster.node.DiscoveryNodes;
 import org.opensearch.common.Nullable;
 import org.opensearch.common.Priority;
 import org.opensearch.common.collect.Tuple;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.ClusterSettings;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.unit.TimeValue;
@@ -327,8 +325,7 @@ public class MasterServiceTests extends OpenSearchTestCase {
 
     @TestLogging(value = "org.opensearch.cluster.service:TRACE", reason = "to ensure that we log cluster state events on TRACE level")
     public void testClusterStateUpdateLogging() throws Exception {
-        MockLogAppender mockAppender = new MockLogAppender();
+        try (MockLogAppender mockAppender = MockLogAppender.createForLoggers(LogManager.getLogger(MasterService.class))) {
-        mockAppender.start();
             mockAppender.addExpectation(
                 new MockLogAppender.SeenEventExpectation(
                     "test1 start",
@@ -421,8 +418,6 @@ public class MasterServiceTests extends OpenSearchTestCase {
                 )
             );
 
-        Logger clusterLogger = LogManager.getLogger(MasterService.class);
-        Loggers.addAppender(clusterLogger, mockAppender);
             try (MasterService masterService = createMasterService(true)) {
                 masterService.submitStateUpdateTask("test1", new ClusterStateUpdateTask() {
                     @Override
@@ -486,9 +481,7 @@ public class MasterServiceTests extends OpenSearchTestCase {
                     }
                 });
                 assertBusy(mockAppender::assertAllExpectationsMatched);
-        } finally {
+            }
-            Loggers.removeAppender(clusterLogger, mockAppender);
-            mockAppender.stop();
         }
     }
 
@@ -741,8 +734,7 @@ public class MasterServiceTests extends OpenSearchTestCase {
 
     @TestLogging(value = "org.opensearch.cluster.service:WARN", reason = "to ensure that we log cluster state events on WARN level")
     public void testLongClusterStateUpdateLogging() throws Exception {
-        MockLogAppender mockAppender = new MockLogAppender();
+        try (MockLogAppender mockAppender = MockLogAppender.createForLoggers(LogManager.getLogger(MasterService.class))) {
-        mockAppender.start();
             mockAppender.addExpectation(
                 new MockLogAppender.UnseenEventExpectation(
                     "test1 shouldn't log because it was fast enough",
@@ -792,8 +784,6 @@ public class MasterServiceTests extends OpenSearchTestCase {
                 )
             );
 
-        Logger clusterLogger = LogManager.getLogger(MasterService.class);
-        Loggers.addAppender(clusterLogger, mockAppender);
             try (
                 MasterService masterService = new MasterService(
                     Settings.builder()
@@ -819,12 +809,12 @@ public class MasterServiceTests extends OpenSearchTestCase {
                 final AtomicReference<ClusterState> clusterStateRef = new AtomicReference<>(initialClusterState);
                 masterService.setClusterStatePublisher((event, publishListener, ackListener) -> {
                     if (event.source().contains("test5")) {
-                    relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY).millis()
+                        relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY)
-                        + randomLongBetween(1, 1000000);
+                            .millis() + randomLongBetween(1, 1000000);
                     }
                     if (event.source().contains("test6")) {
-                    relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY).millis()
+                        relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY)
-                        + randomLongBetween(1, 1000000);
+                            .millis() + randomLongBetween(1, 1000000);
                         throw new OpenSearchException("simulated error during slow publication which should trigger logging");
                     }
                     clusterStateRef.set(event.state());
@@ -861,8 +851,8 @@ public class MasterServiceTests extends OpenSearchTestCase {
                 masterService.submitStateUpdateTask("test2", new ClusterStateUpdateTask() {
                     @Override
                     public ClusterState execute(ClusterState currentState) {
-                    relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY).millis()
+                        relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY)
-                        + randomLongBetween(1, 1000000);
+                            .millis() + randomLongBetween(1, 1000000);
                         throw new IllegalArgumentException("Testing handling of exceptions in the cluster state task");
                     }
 
@@ -879,8 +869,8 @@ public class MasterServiceTests extends OpenSearchTestCase {
                 masterService.submitStateUpdateTask("test3", new ClusterStateUpdateTask() {
                     @Override
                     public ClusterState execute(ClusterState currentState) {
-                    relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY).millis()
+                        relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY)
-                        + randomLongBetween(1, 1000000);
+                            .millis() + randomLongBetween(1, 1000000);
                         return ClusterState.builder(currentState).incrementVersion().build();
                     }
 
@@ -897,8 +887,8 @@ public class MasterServiceTests extends OpenSearchTestCase {
                 masterService.submitStateUpdateTask("test4", new ClusterStateUpdateTask() {
                     @Override
                     public ClusterState execute(ClusterState currentState) {
-                    relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY).millis()
+                        relativeTimeInMillis += MasterService.MASTER_SERVICE_SLOW_TASK_LOGGING_THRESHOLD_SETTING.get(Settings.EMPTY)
-                        + randomLongBetween(1, 1000000);
+                            .millis() + randomLongBetween(1, 1000000);
                         return currentState;
                     }
 
@@ -963,12 +953,10 @@ public class MasterServiceTests extends OpenSearchTestCase {
                     }
                 });
                 latch.await();
-        } finally {
-            Loggers.removeAppender(clusterLogger, mockAppender);
-            mockAppender.stop();
             }
             mockAppender.assertAllExpectationsMatched();
         }
+    }
 
     public void testAcking() throws InterruptedException {
         final DiscoveryNode node1 = new DiscoveryNode("node1", buildNewFakeTransportAddress(), emptyMap(), emptySet(), Version.CURRENT);

server/src/test/java/org/opensearch/common/settings/SettingTests.java

@@ -37,7 +37,6 @@ import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.core.LogEvent;
 import org.opensearch.cluster.metadata.IndexMetadata;
 import org.opensearch.common.collect.Tuple;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.AbstractScopedSettings.SettingUpdater;
 import org.opensearch.common.settings.Setting.Property;
 import org.opensearch.common.unit.ByteSizeUnit;
@@ -1479,7 +1478,8 @@ public class SettingTests extends OpenSearchTestCase {
         );
         final IndexSettings settings = new IndexSettings(metadata, Settings.EMPTY);
 
-        final MockLogAppender mockLogAppender = new MockLogAppender();
+        final Logger logger = LogManager.getLogger(IndexScopedSettings.class);
+        try (MockLogAppender mockLogAppender = MockLogAppender.createForLoggers(logger)) {
             mockLogAppender.addExpectation(
                 new MockLogAppender.SeenEventExpectation(
                     "message",
@@ -1493,18 +1493,11 @@ public class SettingTests extends OpenSearchTestCase {
                     }
                 }
             );
-        mockLogAppender.start();
-        final Logger logger = LogManager.getLogger(IndexScopedSettings.class);
-        try {
-            Loggers.addAppender(logger, mockLogAppender);
             settings.updateIndexMetadata(
                 newIndexMeta("index1", Settings.builder().put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), "10s").build())
             );
 
             mockLogAppender.assertAllExpectationsMatched();
-        } finally {
-            Loggers.removeAppender(logger, mockLogAppender);
-            mockLogAppender.stop();
         }
     }
 }

server/src/test/java/org/opensearch/common/settings/SettingsFilterTests.java

@@ -35,7 +35,6 @@ import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
 import org.opensearch.common.Strings;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.Setting.Property;
 import org.opensearch.common.xcontent.XContentBuilder;
 import org.opensearch.common.xcontent.json.JsonXContent;
@@ -143,14 +142,10 @@ public class SettingsFilterTests extends OpenSearchTestCase {
     private void assertExpectedLogMessages(Consumer<Logger> consumer, MockLogAppender.LoggingExpectation... expectations)
         throws IllegalAccessException {
         Logger testLogger = LogManager.getLogger("org.opensearch.test");
-        MockLogAppender appender = MockLogAppender.createStarted();
+        try (MockLogAppender appender = MockLogAppender.createForLoggers(testLogger)) {
-        Loggers.addAppender(testLogger, appender);
-        try {
             Arrays.stream(expectations).forEach(appender::addExpectation);
             consumer.accept(testLogger);
             appender.assertAllExpectationsMatched();
-        } finally {
-            Loggers.removeAppender(testLogger, appender);
         }
     }
 

server/src/test/java/org/opensearch/discovery/HandshakingTransportAddressConnectorTests.java

@@ -42,7 +42,6 @@ import org.opensearch.action.ActionListener;
 import org.opensearch.cluster.ClusterName;
 import org.opensearch.cluster.node.DiscoveryNode;
 import org.opensearch.common.Nullable;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.transport.TransportAddress;
 import org.opensearch.test.OpenSearchTestCase;
@@ -174,8 +173,8 @@ public class HandshakingTransportAddressConnectorTests extends OpenSearchTestCas
 
         FailureListener failureListener = new FailureListener();
 
-        MockLogAppender mockAppender = new MockLogAppender();
+        Logger targetLogger = LogManager.getLogger(HandshakingTransportAddressConnector.class);
-        mockAppender.start();
+        try (MockLogAppender mockAppender = MockLogAppender.createForLoggers(targetLogger)) {
             mockAppender.addExpectation(
                 new MockLogAppender.SeenEventExpectation(
                     "message",
@@ -184,16 +183,10 @@ public class HandshakingTransportAddressConnectorTests extends OpenSearchTestCas
                     "*completed handshake with [*] but followup connection failed*"
                 )
             );
-        Logger targetLogger = LogManager.getLogger(HandshakingTransportAddressConnector.class);
-        Loggers.addAppender(targetLogger, mockAppender);
 
-        try {
             handshakingTransportAddressConnector.connectToRemoteMasterNode(discoveryAddress, failureListener);
             failureListener.assertFailure();
             mockAppender.assertAllExpectationsMatched();
-        } finally {
-            Loggers.removeAppender(targetLogger, mockAppender);
-            mockAppender.stop();
         }
     }
 

server/src/test/java/org/opensearch/gateway/IncrementalClusterStateWriterTests.java

@@ -51,7 +51,6 @@ import org.opensearch.cluster.routing.RoutingTable;
 import org.opensearch.cluster.routing.allocation.AllocationService;
 import org.opensearch.cluster.routing.allocation.decider.ClusterRebalanceAllocationDecider;
 import org.opensearch.common.collect.Tuple;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.ClusterSettings;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.xcontent.NamedXContentRegistry;
@@ -568,18 +567,11 @@ public class IncrementalClusterStateWriterTests extends OpenSearchAllocationTest
         IncrementalClusterStateWriter incrementalClusterStateWriter,
         MockLogAppender.LoggingExpectation expectation
     ) throws IllegalAccessException, WriteStateException {
-        MockLogAppender mockAppender = new MockLogAppender();
-        mockAppender.start();
-        mockAppender.addExpectation(expectation);
         Logger classLogger = LogManager.getLogger(IncrementalClusterStateWriter.class);
-        Loggers.addAppender(classLogger, mockAppender);
+        try (MockLogAppender mockAppender = MockLogAppender.createForLoggers(classLogger)) {
-
+            mockAppender.addExpectation(expectation);
-        try {
             incrementalClusterStateWriter.updateClusterState(clusterState);
-        } finally {
-            Loggers.removeAppender(classLogger, mockAppender);
-            mockAppender.stop();
-        }
             mockAppender.assertAllExpectationsMatched();
         }
     }
+}

server/src/test/java/org/opensearch/gateway/PersistedClusterStateServiceTests.java

@@ -51,7 +51,6 @@ import org.opensearch.cluster.metadata.Metadata;
 import org.opensearch.cluster.node.DiscoveryNode;
 import org.opensearch.cluster.node.DiscoveryNodes;
 import org.opensearch.common.UUIDs;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.ClusterSettings;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.util.BigArrays;
@@ -1178,24 +1177,17 @@ public class PersistedClusterStateServiceTests extends OpenSearchTestCase {
         PersistedClusterStateService.Writer writer,
         MockLogAppender.LoggingExpectation expectation
     ) throws IllegalAccessException, IOException {
-        MockLogAppender mockAppender = new MockLogAppender();
-        mockAppender.start();
-        mockAppender.addExpectation(expectation);
         Logger classLogger = LogManager.getLogger(PersistedClusterStateService.class);
-        Loggers.addAppender(classLogger, mockAppender);
+        try (MockLogAppender mockAppender = MockLogAppender.createForLoggers(classLogger)) {
-
+            mockAppender.addExpectation(expectation);
-        try {
             if (previousState == null) {
                 writer.writeFullStateAndCommit(currentTerm, clusterState);
             } else {
                 writer.writeIncrementalStateAndCommit(currentTerm, previousState, clusterState);
             }
-        } finally {
-            Loggers.removeAppender(classLogger, mockAppender);
-            mockAppender.stop();
-        }
             mockAppender.assertAllExpectationsMatched();
         }
+    }
 
     @Override
     public Settings buildEnvSettings(Settings settings) {

server/src/test/java/org/opensearch/http/AbstractHttpServerTransportTests.java

@@ -37,7 +37,6 @@ import org.apache.logging.log4j.LogManager;
 import org.opensearch.common.UUIDs;
 import org.opensearch.common.bytes.BytesArray;
 import org.opensearch.common.bytes.BytesReference;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.network.NetworkService;
 import org.opensearch.common.network.NetworkUtils;
 import org.opensearch.common.settings.ClusterSettings;
@@ -268,12 +267,8 @@ public class AbstractHttpServerTransportTests extends OpenSearchTestCase {
                     .put(HttpTransportSettings.SETTING_HTTP_TRACE_LOG_EXCLUDE.getKey(), excludeSettings)
                     .build()
             );
-            MockLogAppender appender = new MockLogAppender();
             final String traceLoggerName = "org.opensearch.http.HttpTracer";
-            try {
+            try (MockLogAppender appender = MockLogAppender.createForLoggers(LogManager.getLogger(traceLoggerName))) {
-                appender.start();
-                Loggers.addAppender(LogManager.getLogger(traceLoggerName), appender);
-
                 final String opaqueId = UUIDs.randomBase64UUID(random());
                 appender.addExpectation(
                     new MockLogAppender.PatternSeenEventExpectation(
@@ -342,9 +337,6 @@ public class AbstractHttpServerTransportTests extends OpenSearchTestCase {
 
                 transport.incomingRequest(fakeRestRequestExcludedPath.getHttpRequest(), fakeRestRequestExcludedPath.getHttpChannel());
                 appender.assertAllExpectationsMatched();
-            } finally {
-                Loggers.removeAppender(LogManager.getLogger(traceLoggerName), appender);
-                appender.stop();
             }
         }
     }

server/src/test/java/org/opensearch/ingest/IngestServiceTests.java

@@ -34,7 +34,6 @@ package org.opensearch.ingest;
 
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 import org.apache.lucene.util.SetOnce;
 import org.opensearch.OpenSearchParseException;
 import org.opensearch.ResourceNotFoundException;
@@ -59,7 +58,6 @@ import org.opensearch.cluster.metadata.Metadata;
 import org.opensearch.cluster.node.DiscoveryNode;
 import org.opensearch.cluster.service.ClusterService;
 import org.opensearch.common.bytes.BytesArray;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.util.concurrent.OpenSearchExecutors;
 import org.opensearch.common.xcontent.XContentBuilder;
@@ -544,8 +542,7 @@ public class IngestServiceTests extends OpenSearchTestCase {
         );
         ClusterState previousClusterState = clusterState;
         clusterState = IngestService.innerPut(putRequest, clusterState);
-        MockLogAppender mockAppender = new MockLogAppender();
+        try (MockLogAppender mockAppender = MockLogAppender.createForLoggers(LogManager.getLogger(IngestService.class))) {
-        mockAppender.start();
             mockAppender.addExpectation(
                 new MockLogAppender.SeenEventExpectation(
                     "test1",
@@ -554,14 +551,8 @@ public class IngestServiceTests extends OpenSearchTestCase {
                     "failed to update ingest pipelines"
                 )
             );
-        Logger ingestLogger = LogManager.getLogger(IngestService.class);
-        Loggers.addAppender(ingestLogger, mockAppender);
-        try {
             ingestService.applyClusterState(new ClusterChangedEvent("", clusterState, previousClusterState));
             mockAppender.assertAllExpectationsMatched();
-        } finally {
-            Loggers.removeAppender(ingestLogger, mockAppender);
-            mockAppender.stop();
         }
         pipeline = ingestService.getPipeline(id);
         assertNotNull(pipeline);

server/src/test/java/org/opensearch/monitor/fs/FsHealthServiceTests.java

@@ -34,13 +34,11 @@ package org.opensearch.monitor.fs;
 
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 import org.apache.lucene.mockfile.FilterFileChannel;
 import org.apache.lucene.mockfile.FilterFileSystemProvider;
 import org.opensearch.cluster.coordination.DeterministicTaskQueue;
 import org.opensearch.common.io.PathUtils;
 import org.opensearch.common.io.PathUtilsForTesting;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.ClusterSettings;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.env.NodeEnvironment;
@@ -158,12 +156,10 @@ public class FsHealthServiceTests extends OpenSearchTestCase {
         PathUtilsForTesting.installMock(fileSystem);
         final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
 
-        MockLogAppender mockAppender = new MockLogAppender();
+        try (
-        mockAppender.start();
+            MockLogAppender mockAppender = MockLogAppender.createForLoggers(LogManager.getLogger(FsHealthService.class));
-
+            NodeEnvironment env = newNodeEnvironment()
-        Logger logger = LogManager.getLogger(FsHealthService.class);
+        ) {
-        Loggers.addAppender(logger, mockAppender);
-        try (NodeEnvironment env = newNodeEnvironment()) {
             FsHealthService fsHealthService = new FsHealthService(settings, clusterSettings, testThreadPool, env);
             int counter = 0;
             for (Path path : env.nodeDataPaths()) {
@@ -183,8 +179,6 @@ public class FsHealthServiceTests extends OpenSearchTestCase {
             assertEquals(env.nodeDataPaths().length, disruptFileSystemProvider.getInjectedPathCount());
             assertBusy(mockAppender::assertAllExpectationsMatched);
         } finally {
-            Loggers.removeAppender(logger, mockAppender);
-            mockAppender.stop();
             PathUtilsForTesting.teardown();
             ThreadPool.terminate(testThreadPool, 500, TimeUnit.MILLISECONDS);
         }

server/src/test/java/org/opensearch/plugins/PluginsServiceTests.java

@@ -32,9 +32,7 @@
 
 package org.opensearch.plugins;
 
-import org.opensearch.common.logging.Loggers;
 import org.apache.logging.log4j.Level;
-import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
 import org.apache.lucene.util.Constants;
 import org.apache.lucene.util.LuceneTestCase;
@@ -737,8 +735,7 @@ public class PluginsServiceTests extends OpenSearchTestCase {
     public void testFindPluginDirs() throws Exception {
         final Path plugins = createTempDir();
 
-        final MockLogAppender mockLogAppender = new MockLogAppender();
+        try (MockLogAppender mockLogAppender = MockLogAppender.createForLoggers(LogManager.getLogger(PluginsService.class))) {
-        mockLogAppender.start();
             mockLogAppender.addExpectation(
                 new MockLogAppender.SeenEventExpectation(
                     "[.test] warning",
@@ -747,8 +744,6 @@ public class PluginsServiceTests extends OpenSearchTestCase {
                     "Non-plugin file located in the plugins folder with the following name: [.DS_Store]"
                 )
             );
-        final Logger testLogger = LogManager.getLogger(PluginsService.class);
-        Loggers.addAppender(testLogger, mockLogAppender);
 
             final Path fake = plugins.resolve("fake");
             Path testFile = plugins.resolve(".DS_Store");
@@ -777,6 +772,7 @@ public class PluginsServiceTests extends OpenSearchTestCase {
             assertThat(PluginsService.findPluginDirs(plugins), containsInAnyOrder(fake));
             mockLogAppender.assertAllExpectationsMatched();
         }
+    }
 
     public void testExistingMandatoryClasspathPlugin() {
         final Settings settings = Settings.builder()

server/src/test/java/org/opensearch/transport/InboundHandlerTests.java

@@ -34,7 +34,6 @@ package org.opensearch.transport;
 
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 import org.opensearch.OpenSearchException;
 import org.opensearch.Version;
 import org.opensearch.action.ActionListener;
@@ -46,7 +45,6 @@ import org.opensearch.common.io.stream.BytesStreamOutput;
 import org.opensearch.common.io.stream.InputStreamStreamInput;
 import org.opensearch.common.io.stream.NamedWriteableRegistry;
 import org.opensearch.common.io.stream.StreamInput;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.unit.TimeValue;
 import org.opensearch.common.util.BigArrays;
@@ -260,8 +258,7 @@ public class InboundHandlerTests extends OpenSearchTestCase {
         // response so we must just close the connection on an error. To avoid the failure disappearing into a black hole we at least log
         // it.
 
-        final MockLogAppender mockAppender = new MockLogAppender();
+        try (MockLogAppender mockAppender = MockLogAppender.createForLoggers(LogManager.getLogger(InboundHandler.class))) {
-        mockAppender.start();
             mockAppender.addExpectation(
                 new MockLogAppender.SeenEventExpectation(
                     "expected message",
@@ -270,10 +267,7 @@ public class InboundHandlerTests extends OpenSearchTestCase {
                     "could not send error response to handshake"
                 )
             );
-        final Logger inboundHandlerLogger = LogManager.getLogger(InboundHandler.class);
-        Loggers.addAppender(inboundHandlerLogger, mockAppender);
 
-        try {
             final AtomicBoolean isClosed = new AtomicBoolean();
             channel.addCloseListener(ActionListener.wrap(() -> assertTrue(isClosed.compareAndSet(false, true))));
 
@@ -293,15 +287,11 @@ public class InboundHandlerTests extends OpenSearchTestCase {
             assertTrue(isClosed.get());
             assertNull(channel.getMessageCaptor().get());
             mockAppender.assertAllExpectationsMatched();
-        } finally {
-            Loggers.removeAppender(inboundHandlerLogger, mockAppender);
-            mockAppender.stop();
         }
     }
 
     public void testLogsSlowInboundProcessing() throws Exception {
-        final MockLogAppender mockAppender = new MockLogAppender();
+        try (MockLogAppender mockAppender = MockLogAppender.createForLoggers(LogManager.getLogger(InboundHandler.class))) {
-        mockAppender.start();
             mockAppender.addExpectation(
                 new MockLogAppender.SeenEventExpectation(
                     "expected message",
@@ -310,11 +300,8 @@ public class InboundHandlerTests extends OpenSearchTestCase {
                     "handling inbound transport message "
                 )
             );
-        final Logger inboundHandlerLogger = LogManager.getLogger(InboundHandler.class);
-        Loggers.addAppender(inboundHandlerLogger, mockAppender);
 
             handler.setSlowLogThreshold(TimeValue.timeValueMillis(5L));
-        try {
             final Version remoteVersion = Version.CURRENT;
             final long requestId = randomNonNegativeLong();
             final Header requestHeader = new Header(
@@ -336,9 +323,6 @@ public class InboundHandlerTests extends OpenSearchTestCase {
             handler.inboundMessage(channel, requestMessage);
             assertNotNull(channel.getMessageCaptor().get());
             mockAppender.assertAllExpectationsMatched();
-        } finally {
-            Loggers.removeAppender(inboundHandlerLogger, mockAppender);
-            mockAppender.stop();
         }
     }
 

server/src/test/java/org/opensearch/transport/TcpTransportTests.java

@@ -40,7 +40,6 @@ import org.opensearch.action.support.PlainActionFuture;
 import org.opensearch.cluster.node.DiscoveryNode;
 import org.opensearch.common.component.Lifecycle;
 import org.opensearch.common.io.stream.BytesStreamOutput;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.network.NetworkService;
 import org.opensearch.common.network.NetworkUtils;
 import org.opensearch.common.settings.Settings;
@@ -524,12 +523,7 @@ public class TcpTransportTests extends OpenSearchTestCase {
         MockLogAppender.LoggingExpectation... expectations
     ) throws IllegalAccessException {
         final TestThreadPool testThreadPool = new TestThreadPool("test");
-        MockLogAppender appender = new MockLogAppender();
+        try (MockLogAppender appender = MockLogAppender.createForLoggers(LogManager.getLogger(TcpTransport.class))) {
-
-        try {
-            appender.start();
-
-            Loggers.addAppender(LogManager.getLogger(TcpTransport.class), appender);
             for (MockLogAppender.LoggingExpectation expectation : expectations) {
                 appender.addExpectation(expectation);
             }
@@ -568,8 +562,6 @@ public class TcpTransportTests extends OpenSearchTestCase {
             appender.assertAllExpectationsMatched();
 
         } finally {
-            Loggers.removeAppender(LogManager.getLogger(TcpTransport.class), appender);
-            appender.stop();
             ThreadPool.terminate(testThreadPool, 30, TimeUnit.SECONDS);
         }
     }

server/src/test/java/org/opensearch/transport/TransportLoggerTests.java

@@ -38,7 +38,6 @@ import org.opensearch.action.admin.cluster.stats.ClusterStatsAction;
 import org.opensearch.action.admin.cluster.stats.ClusterStatsRequest;
 import org.opensearch.common.bytes.BytesReference;
 import org.opensearch.common.io.stream.BytesStreamOutput;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.util.concurrent.ThreadContext;
 import org.opensearch.test.OpenSearchTestCase;
@@ -51,22 +50,8 @@ import static org.mockito.Mockito.mock;
 
 @TestLogging(value = "org.opensearch.transport.TransportLogger:trace", reason = "to ensure we log network events on TRACE level")
 public class TransportLoggerTests extends OpenSearchTestCase {
-
+    public void testLoggingHandler() throws Exception {
-    private MockLogAppender appender;
+        try (MockLogAppender appender = MockLogAppender.createForLoggers(LogManager.getLogger(TransportLogger.class))) {
-
-    public void setUp() throws Exception {
-        super.setUp();
-        appender = MockLogAppender.createStarted();
-        Loggers.addAppender(LogManager.getLogger(TransportLogger.class), appender);
-    }
-
-    public void tearDown() throws Exception {
-        Loggers.removeAppender(LogManager.getLogger(TransportLogger.class), appender);
-        appender.stop();
-        super.tearDown();
-    }
-
-    public void testLoggingHandler() throws IOException {
             final String writePattern = ".*\\[length: \\d+"
                 + ", request id: \\d+"
                 + ", type: request"
@@ -103,6 +88,7 @@ public class TransportLoggerTests extends OpenSearchTestCase {
             TransportLogger.logOutboundMessage(mock(TcpChannel.class), bytesReference);
             appender.assertAllExpectationsMatched();
         }
+    }
 
     private BytesReference buildRequest() throws IOException {
         boolean compress = randomBoolean();

test/framework/src/main/java/org/opensearch/test/MockLogAppender.java

@@ -32,11 +32,15 @@
 package org.opensearch.test;
 
 import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.core.LogEvent;
 import org.apache.logging.log4j.core.appender.AbstractAppender;
 import org.apache.logging.log4j.core.filter.RegexFilter;
+import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.regex.Regex;
 
+import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.regex.Pattern;
@@ -47,32 +51,43 @@ import static org.hamcrest.MatcherAssert.assertThat;
 /**
  * Test appender that can be used to verify that certain events were logged correctly
  */
-public class MockLogAppender extends AbstractAppender {
+public class MockLogAppender extends AbstractAppender implements AutoCloseable {
 
     private static final String COMMON_PREFIX = System.getProperty("opensearch.logger.prefix", "org.opensearch.");
 
     private final List<LoggingExpectation> expectations;
+    private final List<Logger> loggers;
 
     /**
-     * Creates and starts a MockLogAppender. Generally preferred over using the constructor
+     * Creates an instance and adds it as an appender to the given Loggers. Upon
-     * directly because adding an unstarted appender to the static logging context can cause
+     * closure, this instance will then remove itself from the Loggers it was added
-     * difficult-to-identify errors in the tests and this method makes it impossible to do
+     * to. It is strongly recommended to use this class in a try-with-resources block
-     * that.
+     * to guarantee that it is properly removed from all Loggers. Since the logging
+     * state is static and therefore global within a JVM, it can cause unrelated
+     * tests to fail if, for example, they trigger a logging statement that tried to
+     * write to a closed MockLogAppender instance.
      */
-    public static MockLogAppender createStarted() throws IllegalAccessException {
+    public static MockLogAppender createForLoggers(Logger... loggers) throws IllegalAccessException {
-        final MockLogAppender appender = new MockLogAppender();
+        final MockLogAppender appender = new MockLogAppender(
+            RegexFilter.createFilter(".*(\n.*)*", new String[0], false, null, null),
+            Collections.unmodifiableList(Arrays.asList(loggers))
+        );
         appender.start();
+        for (Logger logger : loggers) {
+            Loggers.addAppender(logger, appender);
+        }
         return appender;
     }
 
-    public MockLogAppender() throws IllegalAccessException {
+    private MockLogAppender(RegexFilter filter, List<Logger> loggers) {
-        super("mock", RegexFilter.createFilter(".*(\n.*)*", new String[0], false, null, null), null);
+        super("mock", filter, null);
         /*
          * We use a copy-on-write array list since log messages could be appended while we are setting up expectations. When that occurs,
          * we would run into a concurrent modification exception from the iteration over the expectations in #append, concurrent with a
          * modification from #addExpectation.
          */
-        expectations = new CopyOnWriteArrayList<>();
+        this.expectations = new CopyOnWriteArrayList<>();
+        this.loggers = loggers;
     }
 
     public void addExpectation(LoggingExpectation expectation) {
@@ -92,6 +107,14 @@ public class MockLogAppender extends AbstractAppender {
         }
     }
 
+    @Override
+    public void close() {
+        for (Logger logger : loggers) {
+            Loggers.removeAppender(logger, this);
+        }
+        this.stop();
+    }
+
     public interface LoggingExpectation {
         void match(LogEvent event);
 

test/framework/src/main/java/org/opensearch/transport/AbstractSimpleTransportTestCase.java

@@ -34,6 +34,7 @@ package org.opensearch.transport;
 
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.message.ParameterizedMessage;
 import org.apache.logging.log4j.util.Supplier;
 import org.apache.lucene.util.CollectionUtil;
@@ -51,7 +52,6 @@ import org.opensearch.common.SuppressForbidden;
 import org.opensearch.common.io.stream.BytesStreamOutput;
 import org.opensearch.common.io.stream.StreamInput;
 import org.opensearch.common.io.stream.StreamOutput;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.network.CloseableChannel;
 import org.opensearch.common.network.NetworkAddress;
 import org.opensearch.common.network.NetworkUtils;
@@ -1202,10 +1202,8 @@ public abstract class AbstractSimpleTransportTestCase extends OpenSearchTestCase
                 .build()
         );
 
-        MockLogAppender appender = new MockLogAppender();
+        final Logger logger = LogManager.getLogger("org.opensearch.transport.TransportService.tracer");
-        try {
+        try (MockLogAppender appender = MockLogAppender.createForLoggers(logger)) {
-            appender.start();
-            Loggers.addAppender(LogManager.getLogger("org.opensearch.transport.TransportService.tracer"), appender);
             final String requestSent = ".*\\[internal:test].*sent to.*\\{TS_B}.*";
             final MockLogAppender.LoggingExpectation requestSentExpectation = new MockLogAppender.PatternSeenEventExpectation(
                 "sent request",
@@ -1291,9 +1289,6 @@ public abstract class AbstractSimpleTransportTestCase extends OpenSearchTestCase
             future.txGet();
 
             assertBusy(appender::assertAllExpectationsMatched);
-        } finally {
-            Loggers.removeAppender(LogManager.getLogger("org.opensearch.transport.TransportService.tracer"), appender);
-            appender.stop();
         }
     }
 

test/framework/src/test/java/org/opensearch/transport/nio/TestEventHandlerTests.java

@@ -35,7 +35,6 @@ package org.opensearch.transport.nio;
 import org.apache.logging.log4j.Level;
 import org.apache.logging.log4j.LogManager;
 import org.opensearch.common.CheckedRunnable;
-import org.opensearch.common.logging.Loggers;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.nio.ServerChannelContext;
 import org.opensearch.nio.SocketChannelContext;
@@ -54,22 +53,8 @@ import static org.mockito.Mockito.mock;
 
 public class TestEventHandlerTests extends OpenSearchTestCase {
 
-    private MockLogAppender appender;
-
-    public void setUp() throws Exception {
-        super.setUp();
-        appender = new MockLogAppender();
-        Loggers.addAppender(LogManager.getLogger(MockNioTransport.class), appender);
-        appender.start();
-    }
-
-    public void tearDown() throws Exception {
-        Loggers.removeAppender(LogManager.getLogger(MockNioTransport.class), appender);
-        appender.stop();
-        super.tearDown();
-    }
-
     public void testLogOnElapsedTime() throws Exception {
+        try (MockLogAppender appender = MockLogAppender.createForLoggers(LogManager.getLogger(MockNioTransport.class))) {
             long start = System.nanoTime();
             long end = start + TimeUnit.MILLISECONDS.toNanos(400);
             AtomicBoolean isStart = new AtomicBoolean(true);
@@ -124,3 +109,4 @@ public class TestEventHandlerTests extends OpenSearchTestCase {
             }
         }
     }
+}