From ea0a16e3c4b894e39a83c85bf5f86e7c869c7aeb Mon Sep 17 00:00:00 2001 From: Gagan Juneja Date: Wed, 6 Sep 2023 20:08:14 +0530 Subject: [PATCH] Address review comment Signed-off-by: Gagan Juneja --- .../opensearch/test/InternalTestCluster.java | 22 ------------------ .../test/OpenSearchSingleNodeTestCase.java | 20 ++-------------- .../opensearch/test/OpenSearchTestCase.java | 2 ++ .../tracing/MockTracingTelemetry.java | 23 ------------------- .../tracing/StrictCheckSpanProcessor.java | 21 ++++++++++++++--- 5 files changed, 22 insertions(+), 66 deletions(-) diff --git a/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java index 1339d8a802043..6ad7fcddb3041 100644 --- a/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/opensearch/test/InternalTestCluster.java @@ -961,25 +961,16 @@ public Client smartClient() { @Override public synchronized void close() throws IOException { - /** - * MockTracingTelemetry:validateTracingStateOnShutdown validates the shared span storage across all the nodes. - * Hence, we just need any one node to ensure the correctness of the system. - */ - Node anyNodeToGetRefOfTelemetry = null; if (this.open.compareAndSet(true, false)) { if (activeDisruptionScheme != null) { activeDisruptionScheme.testClusterClosed(); activeDisruptionScheme = null; } try { - if (nodes.firstEntry() != null && nodes.firstEntry().getValue() != null) { - anyNodeToGetRefOfTelemetry = nodes.firstEntry().getValue().node; - } IOUtils.close(nodes.values()); } finally { nodes = Collections.emptyNavigableMap(); executor.shutdownNow(); - validateTracingTerminalState(anyNodeToGetRefOfTelemetry); } } } @@ -1940,19 +1931,6 @@ private synchronized void stopNodesAndClients(Collection nodeAndC removeExclusions(excludedNodeIds); } - private void validateTracingTerminalState(Node node) { - if (node != null) { - Optional telemetry = ((MockNode) node).getTelemetry(); - if (isValidMockTracingTelemetry(telemetry)) { - ((MockTracingTelemetry) telemetry.get().getTracingTelemetry()).validateTracingStateOnShutdown(); - } - } - } - - private static boolean isValidMockTracingTelemetry(Optional telemetry) { - return telemetry.isPresent() && (telemetry.get() instanceof MockTelemetry) && (telemetry.get().getTracingTelemetry() != null); - } - /** * Restarts a random data node in the cluster */ diff --git a/test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java b/test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java index 072fb14547291..3ee1a45228973 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java @@ -76,6 +76,7 @@ import org.opensearch.test.telemetry.MockTelemetry; import org.opensearch.test.telemetry.MockTelemetryPlugin; import org.opensearch.test.telemetry.tracing.MockTracingTelemetry; +import org.opensearch.test.telemetry.tracing.StrictCheckSpanProcessor; import org.opensearch.transport.TransportSettings; import org.junit.AfterClass; import org.junit.BeforeClass; @@ -103,12 +104,10 @@ public abstract class OpenSearchSingleNodeTestCase extends OpenSearchTestCase { private static Node NODE = null; - private static Node nodeForTracingStrictCheck = null; protected void startNode(long seed) throws Exception { assert NODE == null; NODE = RandomizedContext.current().runWithPrivateRandomness(seed, this::newNode); - nodeForTracingStrictCheck = NODE; // we must wait for the node to actually be up and running. otherwise the node might have started, // elected itself cluster-manager but might not yet have removed the // SERVICE_UNAVAILABLE/1/state not recovered / initialized block @@ -196,23 +195,8 @@ public static void setUpClass() throws Exception { @AfterClass public static void tearDownClass() throws Exception { stopNode(); - validateTracingTerminalState(nodeForTracingStrictCheck); - nodeForTracingStrictCheck = null; + StrictCheckSpanProcessor.validateTracingStateOnShutdown(); } - - private static void validateTracingTerminalState(Node node) { - if (node != null) { - Optional telemetry = ((MockNode) node).getTelemetry(); - if (isValidMockTracingTelemetry(telemetry)) { - ((MockTracingTelemetry) telemetry.get().getTracingTelemetry()).validateTracingStateOnShutdown(); - } - } - } - - private static boolean isValidMockTracingTelemetry(Optional telemetry) { - return telemetry.isPresent() && telemetry.get() instanceof MockTelemetry && telemetry.get().getTracingTelemetry() != null; - } - /** * This method returns true if the node that is used in the background should be reset * after each test. This is useful if the test changes the cluster state metadata etc. The default is diff --git a/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java b/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java index 8490ee4fc39bc..fa8b292324fae 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchTestCase.java @@ -130,6 +130,7 @@ import org.opensearch.search.MockSearchService; import org.opensearch.test.junit.listeners.LoggingListener; import org.opensearch.test.junit.listeners.ReproduceInfoPrinter; +import org.opensearch.test.telemetry.tracing.StrictCheckSpanProcessor; import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.TransportService; import org.opensearch.transport.nio.MockNioTransportPlugin; @@ -651,6 +652,7 @@ protected static void checkStaticState(boolean afterClass) throws Exception { nettyLoggedLeaks.clear(); } } + StrictCheckSpanProcessor.validateTracingStateOnShutdown(); } // this must be a separate method from other ensure checks above so suite scoped integ tests can call...TODO: fix that diff --git a/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/MockTracingTelemetry.java b/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/MockTracingTelemetry.java index 99108d4fa9f8c..bc827f15d18a4 100644 --- a/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/MockTracingTelemetry.java +++ b/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/MockTracingTelemetry.java @@ -12,11 +12,7 @@ import org.opensearch.telemetry.tracing.TracingContextPropagator; import org.opensearch.telemetry.tracing.TracingTelemetry; import org.opensearch.telemetry.tracing.attributes.Attributes; -import org.opensearch.test.telemetry.tracing.validators.AllSpansAreEndedProperly; -import org.opensearch.test.telemetry.tracing.validators.AllSpansHaveUniqueId; -import java.util.Arrays; -import java.util.List; import java.util.concurrent.atomic.AtomicBoolean; /** @@ -50,23 +46,4 @@ public TracingContextPropagator getContextPropagator() { public void close() { shutdown.set(true); } - - /** - * Ensures the strict check succeeds for all the spans. - */ - public void validateTracingStateOnShutdown() { - List spanData = ((StrictCheckSpanProcessor) spanProcessor).getFinishedSpanItems(); - if (spanData.size() != 0) { - TelemetryValidators validators = new TelemetryValidators( - Arrays.asList(new AllSpansAreEndedProperly(), new AllSpansHaveUniqueId()) - ); - try { - validators.validate(spanData, 1); - } catch (Error e) { - ((StrictCheckSpanProcessor) spanProcessor).clear(); - throw e; - } - } - - } } diff --git a/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/StrictCheckSpanProcessor.java b/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/StrictCheckSpanProcessor.java index 203f77f1550c9..c6e57531d23df 100644 --- a/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/StrictCheckSpanProcessor.java +++ b/test/telemetry/src/main/java/org/opensearch/test/telemetry/tracing/StrictCheckSpanProcessor.java @@ -9,8 +9,11 @@ package org.opensearch.test.telemetry.tracing; import org.opensearch.telemetry.tracing.Span; +import org.opensearch.test.telemetry.tracing.validators.AllSpansAreEndedProperly; +import org.opensearch.test.telemetry.tracing.validators.AllSpansHaveUniqueId; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; @@ -63,9 +66,21 @@ private MockSpanData toMockSpanData(Span span) { } /** - * Clears the StrictCheck span storage. + * Ensures the strict check succeeds for all the spans. */ - public void clear() { - spanMap.clear(); + public static void validateTracingStateOnShutdown() { + List spanData = new ArrayList<>(spanMap.values()); + if (spanData.size() != 0) { + TelemetryValidators validators = new TelemetryValidators( + Arrays.asList(new AllSpansAreEndedProperly(), new AllSpansHaveUniqueId()) + ); + try { + validators.validate(spanData, 1); + } catch (Error e) { + spanMap.clear(); + throw e; + } + } + } }