Skip to content

Commit

Permalink
Address review comment
Browse files Browse the repository at this point in the history
Signed-off-by: Gagan Juneja <[email protected]>
  • Loading branch information
Gagan Juneja committed Sep 6, 2023
1 parent bb28422 commit ea0a16e
Show file tree
Hide file tree
Showing 5 changed files with 22 additions and 66 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -1940,19 +1931,6 @@ private synchronized void stopNodesAndClients(Collection<NodeAndClient> nodeAndC
removeExclusions(excludedNodeIds);
}

private void validateTracingTerminalState(Node node) {
if (node != null) {
Optional<Telemetry> telemetry = ((MockNode) node).getTelemetry();
if (isValidMockTracingTelemetry(telemetry)) {
((MockTracingTelemetry) telemetry.get().getTracingTelemetry()).validateTracingStateOnShutdown();
}
}
}

private static boolean isValidMockTracingTelemetry(Optional<Telemetry> telemetry) {
return telemetry.isPresent() && (telemetry.get() instanceof MockTelemetry) && (telemetry.get().getTracingTelemetry() != null);
}

/**
* Restarts a random data node in the cluster
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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> telemetry = ((MockNode) node).getTelemetry();
if (isValidMockTracingTelemetry(telemetry)) {
((MockTracingTelemetry) telemetry.get().getTracingTelemetry()).validateTracingStateOnShutdown();
}
}
}

private static boolean isValidMockTracingTelemetry(Optional<Telemetry> telemetry) {
return telemetry.isPresent() && telemetry.get() instanceof MockTelemetry && telemetry.get().getTracingTelemetry() != null;
}

/**
* This method returns <code>true</code> 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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<MockSpanData> 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;
}
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<MockSpanData> 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;
}
}

}
}

0 comments on commit ea0a16e

Please sign in to comment.