From bf798518cfc10c36a1581a195ea09a2ef346f5a5 Mon Sep 17 00:00:00 2001 From: Harsh Garg Date: Thu, 7 Mar 2024 02:41:34 +0530 Subject: [PATCH] Refactoring UTs to assert counters Signed-off-by: Harsh Garg --- .../coordination/FollowersChecker.java | 2 +- .../coordination/FollowersCheckerTests.java | 31 +++++++++------ .../coordination/LeaderCheckerTests.java | 39 ++++++++++++++++--- .../cluster/coordination/NodeJoinTests.java | 4 +- .../discovery/DiscoveryModuleTests.java | 6 +-- .../snapshots/SnapshotResiliencyTests.java | 3 +- .../AbstractCoordinatorTestCase.java | 6 +-- 7 files changed, 61 insertions(+), 30 deletions(-) diff --git a/server/src/main/java/org/opensearch/cluster/coordination/FollowersChecker.java b/server/src/main/java/org/opensearch/cluster/coordination/FollowersChecker.java index 210a5028dfd4d..c50f50c06370f 100644 --- a/server/src/main/java/org/opensearch/cluster/coordination/FollowersChecker.java +++ b/server/src/main/java/org/opensearch/cluster/coordination/FollowersChecker.java @@ -172,7 +172,7 @@ public void onNodeDisconnected(DiscoveryNode node, Transport.Connection connecti private void initializeMetrics(MetricsRegistry metricsRegistry) { this.followerChecksFailureCounter = metricsRegistry.createCounter( - "follower.checker.failure.count", + "followers.checker.failure.count", "Counter for number of failed follower checks", UNIT ); diff --git a/server/src/test/java/org/opensearch/cluster/coordination/FollowersCheckerTests.java b/server/src/test/java/org/opensearch/cluster/coordination/FollowersCheckerTests.java index d456231c30e4e..fda7660cd3121 100644 --- a/server/src/test/java/org/opensearch/cluster/coordination/FollowersCheckerTests.java +++ b/server/src/test/java/org/opensearch/cluster/coordination/FollowersCheckerTests.java @@ -47,6 +47,7 @@ import org.opensearch.core.transport.TransportResponse.Empty; import org.opensearch.monitor.NodeHealthService; import org.opensearch.monitor.StatusInfo; +import org.opensearch.telemetry.metrics.Counter; import org.opensearch.telemetry.metrics.MetricsRegistry; import org.opensearch.telemetry.metrics.noop.NoopMetricsRegistry; import org.opensearch.telemetry.tracing.noop.NoopTracer; @@ -77,8 +78,6 @@ import java.util.stream.Collectors; import java.util.stream.Stream; -import org.mockito.Mockito; - import static java.util.Collections.emptySet; import static org.opensearch.cluster.coordination.FollowersChecker.FOLLOWER_CHECK_ACTION_NAME; import static org.opensearch.cluster.coordination.FollowersChecker.FOLLOWER_CHECK_INTERVAL_SETTING; @@ -95,6 +94,12 @@ import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.core.IsInstanceOf.instanceOf; +import static org.mockito.ArgumentMatchers.anyDouble; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; public class FollowersCheckerTests extends OpenSearchTestCase { @@ -144,7 +149,7 @@ protected void onSendRequest(long requestId, String action, TransportRequest req assert false : node; }, () -> new StatusInfo(StatusInfo.Status.HEALTHY, "healthy-info"), - Mockito.mock(MetricsRegistry.class) + NoopMetricsRegistry.INSTANCE ); followersChecker.setCurrentNodes(discoveryNodesHolder[0]); @@ -313,7 +318,7 @@ public String toString() { assertThat(reason, equalTo("disconnected")); }, () -> new StatusInfo(HEALTHY, "healthy-info"), - Mockito.mock(MetricsRegistry.class) + NoopMetricsRegistry.INSTANCE ); DiscoveryNodes discoveryNodes = DiscoveryNodes.builder().add(localNode).add(otherNode).localNodeId(localNode.getId()).build(); @@ -393,6 +398,10 @@ public String toString() { final AtomicBoolean nodeFailed = new AtomicBoolean(); + final MetricsRegistry metricsRegistry = mock(MetricsRegistry.class); + final Counter counter = mock(Counter.class); + when(metricsRegistry.createCounter(anyString(), anyString(), anyString())).thenReturn(counter); + final FollowersChecker followersChecker = new FollowersChecker( settings, clusterSettings, @@ -403,7 +412,7 @@ public String toString() { assertThat(reason, equalTo(failureReason)); }, nodeHealthService, - NoopMetricsRegistry.INSTANCE + metricsRegistry ); DiscoveryNodes discoveryNodes = DiscoveryNodes.builder().add(localNode).add(otherNode).localNodeId(localNode.getId()).build(); @@ -449,6 +458,8 @@ public String toString() { deterministicTaskQueue.runAllTasksInTimeOrder(); assertTrue(nodeFailed.get()); assertThat(followersChecker.getFaultyNodes(), contains(otherNode)); + + verify(counter, atLeastOnce()).add(anyDouble()); } public void testFollowerCheckRequestEqualsHashCodeSerialization() { @@ -508,11 +519,7 @@ protected void onSendRequest(long requestId, String action, TransportRequest req if (exception != null) { throw exception; } - }, - (node, reason) -> { assert false : node; }, - () -> new StatusInfo(UNHEALTHY, "unhealthy-info"), - Mockito.mock(MetricsRegistry.class) - ); + }, (node, reason) -> { assert false : node; }, () -> new StatusInfo(UNHEALTHY, "unhealthy-info"), NoopMetricsRegistry.INSTANCE); final long leaderTerm = randomLongBetween(2, Long.MAX_VALUE); final long followerTerm = randomLongBetween(1, leaderTerm - 1); @@ -585,7 +592,7 @@ protected void onSendRequest(long requestId, String action, TransportRequest req if (exception != null) { throw exception; } - }, (node, reason) -> { assert false : node; }, () -> new StatusInfo(HEALTHY, "healthy-info"), Mockito.mock(MetricsRegistry.class)); + }, (node, reason) -> { assert false : node; }, () -> new StatusInfo(HEALTHY, "healthy-info"), NoopMetricsRegistry.INSTANCE); { // Does not call into the coordinator in the normal case @@ -732,7 +739,7 @@ public void testPreferClusterManagerNodes() { ); final FollowersChecker followersChecker = new FollowersChecker(Settings.EMPTY, clusterSettings, transportService, fcr -> { assert false : fcr; - }, (node, reason) -> { assert false : node; }, () -> new StatusInfo(HEALTHY, "healthy-info"), Mockito.mock(MetricsRegistry.class)); + }, (node, reason) -> { assert false : node; }, () -> new StatusInfo(HEALTHY, "healthy-info"), NoopMetricsRegistry.INSTANCE); followersChecker.setCurrentNodes(discoveryNodes); List followerTargets = Stream.of(capturingTransport.getCapturedRequestsAndClear()) .map(cr -> cr.node) diff --git a/server/src/test/java/org/opensearch/cluster/coordination/LeaderCheckerTests.java b/server/src/test/java/org/opensearch/cluster/coordination/LeaderCheckerTests.java index 7194d1009f49f..8029fb3aba7ea 100644 --- a/server/src/test/java/org/opensearch/cluster/coordination/LeaderCheckerTests.java +++ b/server/src/test/java/org/opensearch/cluster/coordination/LeaderCheckerTests.java @@ -44,7 +44,8 @@ import org.opensearch.core.transport.TransportResponse; import org.opensearch.core.transport.TransportResponse.Empty; import org.opensearch.monitor.StatusInfo; -import org.opensearch.telemetry.metrics.noop.NoopMetricsRegistry; +import org.opensearch.telemetry.metrics.Counter; +import org.opensearch.telemetry.metrics.MetricsRegistry; import org.opensearch.telemetry.tracing.noop.NoopTracer; import org.opensearch.test.EqualsHashCodeTestUtils; import org.opensearch.test.EqualsHashCodeTestUtils.CopyFunction; @@ -79,6 +80,13 @@ import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.hamcrest.Matchers.matchesRegex; import static org.hamcrest.Matchers.nullValue; +import static org.mockito.ArgumentMatchers.anyDouble; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; public class LeaderCheckerTests extends OpenSearchTestCase { @@ -176,11 +184,14 @@ public String toString() { transportService.acceptIncomingRequests(); final AtomicBoolean leaderFailed = new AtomicBoolean(); + final MetricsRegistry metricsRegistry = mock(MetricsRegistry.class); + final Counter leaderCheckFailedCounter = mock(Counter.class); + when(metricsRegistry.createCounter(anyString(), anyString(), anyString())).thenReturn(leaderCheckFailedCounter); final LeaderChecker leaderChecker = new LeaderChecker(settings, clusterSettings, transportService, e -> { assertThat(e.getMessage(), matchesRegex("node \\[.*\\] failed \\[[1-9][0-9]*\\] consecutive checks")); assertTrue(leaderFailed.compareAndSet(false, true)); - }, () -> new StatusInfo(StatusInfo.Status.HEALTHY, "healthy-info"), NoopMetricsRegistry.INSTANCE); + }, () -> new StatusInfo(StatusInfo.Status.HEALTHY, "healthy-info"), metricsRegistry); logger.info("--> creating first checker"); leaderChecker.updateLeader(leader1); @@ -230,6 +241,8 @@ public String toString() { ); } leaderChecker.updateLeader(null); + verify(metricsRegistry, times(1)).createCounter(anyString(), anyString(), anyString()); + verify(leaderCheckFailedCounter, times(1)).add(anyDouble()); } enum Response { @@ -294,10 +307,14 @@ public String toString() { transportService.acceptIncomingRequests(); final AtomicBoolean leaderFailed = new AtomicBoolean(); + final MetricsRegistry metricsRegistry = mock(MetricsRegistry.class); + final Counter leaderCheckFailedCounter = mock(Counter.class); + when(metricsRegistry.createCounter(anyString(), anyString(), anyString())).thenReturn(leaderCheckFailedCounter); + final LeaderChecker leaderChecker = new LeaderChecker(settings, clusterSettings, transportService, e -> { assertThat(e.getMessage(), anyOf(endsWith("disconnected"), endsWith("disconnected during check"))); assertTrue(leaderFailed.compareAndSet(false, true)); - }, () -> new StatusInfo(StatusInfo.Status.HEALTHY, "healthy-info"), NoopMetricsRegistry.INSTANCE); + }, () -> new StatusInfo(StatusInfo.Status.HEALTHY, "healthy-info"), metricsRegistry); leaderChecker.updateLeader(leader); { @@ -352,6 +369,8 @@ public String toString() { deterministicTaskQueue.runAllRunnableTasks(); assertTrue(leaderFailed.get()); } + verify(metricsRegistry, times(1)).createCounter(anyString(), anyString(), anyString()); + verify(leaderCheckFailedCounter, times(2)).add(anyDouble()); } public void testFollowerFailsImmediatelyOnHealthCheckFailure() { @@ -408,10 +427,13 @@ public String toString() { transportService.acceptIncomingRequests(); final AtomicBoolean leaderFailed = new AtomicBoolean(); + final MetricsRegistry metricsRegistry = mock(MetricsRegistry.class); + final Counter leaderChecksFailedCounter = mock(Counter.class); + when(metricsRegistry.createCounter(anyString(), anyString(), anyString())).thenReturn(leaderChecksFailedCounter); final LeaderChecker leaderChecker = new LeaderChecker(settings, clusterSettings, transportService, e -> { assertThat(e.getMessage(), endsWith("failed health checks")); assertTrue(leaderFailed.compareAndSet(false, true)); - }, () -> new StatusInfo(StatusInfo.Status.HEALTHY, "healthy-info"), NoopMetricsRegistry.INSTANCE); + }, () -> new StatusInfo(StatusInfo.Status.HEALTHY, "healthy-info"), metricsRegistry); leaderChecker.updateLeader(leader); @@ -431,6 +453,8 @@ public String toString() { assertTrue(leaderFailed.get()); } + verify(metricsRegistry, times(1)).createCounter(anyString(), anyString(), anyString()); + verify(leaderChecksFailedCounter, times(1)).add(anyDouble()); } public void testLeaderBehaviour() { @@ -454,13 +478,16 @@ public void testLeaderBehaviour() { transportService.start(); transportService.acceptIncomingRequests(); + final MetricsRegistry metricsRegistry = mock(MetricsRegistry.class); + final Counter leaderChecksFailedCounter = mock(Counter.class); + when(metricsRegistry.createCounter(anyString(), anyString(), anyString())).thenReturn(leaderChecksFailedCounter); final LeaderChecker leaderChecker = new LeaderChecker( settings, clusterSettings, transportService, e -> fail("shouldn't be checking anything"), () -> nodeHealthServiceStatus.get(), - NoopMetricsRegistry.INSTANCE + metricsRegistry ); final DiscoveryNodes discoveryNodes = DiscoveryNodes.builder() @@ -525,6 +552,8 @@ public void testLeaderBehaviour() { equalTo("rejecting leader check from [" + otherNode + "] sent to a node that is no longer the cluster-manager") ); } + verify(metricsRegistry, times(1)).createCounter(anyString(), anyString(), anyString()); + verifyNoInteractions(leaderChecksFailedCounter); } private class CapturingTransportResponseHandler implements TransportResponseHandler { diff --git a/server/src/test/java/org/opensearch/cluster/coordination/NodeJoinTests.java b/server/src/test/java/org/opensearch/cluster/coordination/NodeJoinTests.java index da7a2e4510166..6c44c000205f7 100644 --- a/server/src/test/java/org/opensearch/cluster/coordination/NodeJoinTests.java +++ b/server/src/test/java/org/opensearch/cluster/coordination/NodeJoinTests.java @@ -61,7 +61,7 @@ import org.opensearch.monitor.StatusInfo; import org.opensearch.node.Node; import org.opensearch.node.remotestore.RemoteStoreNodeService; -import org.opensearch.telemetry.metrics.MetricsRegistry; +import org.opensearch.telemetry.metrics.noop.NoopMetricsRegistry; import org.opensearch.telemetry.tracing.noop.NoopTracer; import org.opensearch.test.ClusterServiceUtils; import org.opensearch.test.OpenSearchTestCase; @@ -272,7 +272,7 @@ protected void onSendRequest( nodeHealthService, persistedStateRegistry, Mockito.mock(RemoteStoreNodeService.class), - Mockito.mock(MetricsRegistry.class) + NoopMetricsRegistry.INSTANCE ); transportService.start(); transportService.acceptIncomingRequests(); diff --git a/server/src/test/java/org/opensearch/discovery/DiscoveryModuleTests.java b/server/src/test/java/org/opensearch/discovery/DiscoveryModuleTests.java index 80830f04ceb3d..44e037e5a9845 100644 --- a/server/src/test/java/org/opensearch/discovery/DiscoveryModuleTests.java +++ b/server/src/test/java/org/opensearch/discovery/DiscoveryModuleTests.java @@ -48,7 +48,7 @@ import org.opensearch.gateway.GatewayMetaState; import org.opensearch.node.remotestore.RemoteStoreNodeService; import org.opensearch.plugins.DiscoveryPlugin; -import org.opensearch.telemetry.metrics.MetricsRegistry; +import org.opensearch.telemetry.metrics.noop.NoopMetricsRegistry; import org.opensearch.telemetry.tracing.noop.NoopTracer; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.transport.MockTransportService; @@ -67,8 +67,6 @@ import java.util.function.BiConsumer; import java.util.function.Supplier; -import org.mockito.Mockito; - import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -132,7 +130,7 @@ private DiscoveryModule newModule(Settings settings, List plugi null, new PersistedStateRegistry(), remoteStoreNodeService, - Mockito.mock(MetricsRegistry.class) + NoopMetricsRegistry.INSTANCE ); } diff --git a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java index 665ce1d3325ab..735f578137762 100644 --- a/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/opensearch/snapshots/SnapshotResiliencyTests.java @@ -223,7 +223,6 @@ import org.opensearch.search.query.QueryPhase; import org.opensearch.snapshots.mockstore.MockEventuallyConsistentRepository; import org.opensearch.tasks.TaskResourceTrackingService; -import org.opensearch.telemetry.metrics.MetricsRegistry; import org.opensearch.telemetry.metrics.noop.NoopMetricsRegistry; import org.opensearch.telemetry.tracing.noop.NoopTracer; import org.opensearch.test.OpenSearchTestCase; @@ -2533,7 +2532,7 @@ public void start(ClusterState initialState) { () -> new StatusInfo(HEALTHY, "healthy-info"), persistedStateRegistry, remoteStoreNodeService, - Mockito.mock(MetricsRegistry.class) + NoopMetricsRegistry.INSTANCE ); clusterManagerService.setClusterStatePublisher(coordinator); coordinator.start(); diff --git a/test/framework/src/main/java/org/opensearch/cluster/coordination/AbstractCoordinatorTestCase.java b/test/framework/src/main/java/org/opensearch/cluster/coordination/AbstractCoordinatorTestCase.java index 6f7bda49803a3..8635974abe115 100644 --- a/test/framework/src/main/java/org/opensearch/cluster/coordination/AbstractCoordinatorTestCase.java +++ b/test/framework/src/main/java/org/opensearch/cluster/coordination/AbstractCoordinatorTestCase.java @@ -88,7 +88,7 @@ import org.opensearch.monitor.StatusInfo; import org.opensearch.node.remotestore.RemoteStoreNodeService; import org.opensearch.repositories.RepositoriesService; -import org.opensearch.telemetry.metrics.MetricsRegistry; +import org.opensearch.telemetry.metrics.noop.NoopMetricsRegistry; import org.opensearch.telemetry.tracing.noop.NoopTracer; import org.opensearch.test.OpenSearchTestCase; import org.opensearch.test.disruption.DisruptableMockTransport; @@ -127,8 +127,6 @@ import java.util.stream.Collectors; import java.util.stream.Stream; -import org.mockito.Mockito; - import static java.util.Collections.emptyList; import static java.util.Collections.emptySet; import static java.util.Collections.singleton; @@ -1177,7 +1175,7 @@ protected Optional getDisruptableMockTransport(Transpo nodeHealthService, persistedStateRegistry, remoteStoreNodeService, - Mockito.mock(MetricsRegistry.class) + NoopMetricsRegistry.INSTANCE ); clusterManagerService.setClusterStatePublisher(coordinator); final GatewayService gatewayService = new GatewayService(