From b8a7194ac3a021e8ea08bb04a85d08b00555a6b3 Mon Sep 17 00:00:00 2001 From: Gregor Zeitlinger Date: Wed, 23 Nov 2022 12:20:35 +0100 Subject: [PATCH] sort spans by start time (parents before children as tiebreaker) to avoid common causes for flaky tests --- .../opentelemetry-sdk-testing.txt | 4 +- .../sdk/testing/assertj/TracesAssert.java | 39 ++++++++++ .../junit5/OpenTelemetryExtension.java | 14 +--- .../sdk/testing/assertj/TracesAssertTest.java | 75 +++++++++++++++++++ .../junit5/OpenTelemetryExtensionTest.java | 5 +- 5 files changed, 121 insertions(+), 16 deletions(-) create mode 100644 sdk/testing/src/test/java/io/opentelemetry/sdk/testing/assertj/TracesAssertTest.java diff --git a/docs/apidiffs/current_vs_latest/opentelemetry-sdk-testing.txt b/docs/apidiffs/current_vs_latest/opentelemetry-sdk-testing.txt index df26146497b..8fd8bbf5ba6 100644 --- a/docs/apidiffs/current_vs_latest/opentelemetry-sdk-testing.txt +++ b/docs/apidiffs/current_vs_latest/opentelemetry-sdk-testing.txt @@ -1,2 +1,4 @@ Comparing source compatibility of against -No changes. \ No newline at end of file +*** MODIFIED CLASS: PUBLIC FINAL io.opentelemetry.sdk.testing.assertj.TracesAssert (not serializable) + === CLASS FILE FORMAT VERSION: 52.0 <- 52.0 + +++ NEW METHOD: PUBLIC(+) STATIC(+) io.opentelemetry.sdk.testing.assertj.TracesAssert assertThat(java.util.List) diff --git a/sdk/testing/src/main/java/io/opentelemetry/sdk/testing/assertj/TracesAssert.java b/sdk/testing/src/main/java/io/opentelemetry/sdk/testing/assertj/TracesAssert.java index 78da139991a..295db2b7ea2 100644 --- a/sdk/testing/src/main/java/io/opentelemetry/sdk/testing/assertj/TracesAssert.java +++ b/sdk/testing/src/main/java/io/opentelemetry/sdk/testing/assertj/TracesAssert.java @@ -7,12 +7,17 @@ import static java.util.stream.Collectors.toList; +import io.opentelemetry.api.trace.SpanContext; import io.opentelemetry.sdk.trace.data.SpanData; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Comparator; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; import java.util.function.Consumer; +import java.util.stream.Collectors; import java.util.stream.StreamSupport; import org.assertj.core.api.AbstractIterableAssert; @@ -21,6 +26,40 @@ public final class TracesAssert extends AbstractIterableAssert< TracesAssert, List>, List, TraceAssert> { + /** Compare spans by start time, placing parents before their children as a tiebreaker. */ + static final Comparator SPAN_DATA_COMPARATOR = + Comparator.comparing(SpanData::getStartEpochNanos) + .thenComparing( + (span1, span2) -> { + SpanContext parent1 = span1.getParentSpanContext(); + if (parent1.isValid() && parent1.getSpanId().equals(span2.getSpanId())) { + return 1; + } + SpanContext parent2 = span2.getParentSpanContext(); + if (parent2.isValid() && parent2.getSpanId().equals(span1.getSpanId())) { + return -1; + } + return 0; + }); + + /** + * Returns an assertion for a list of traces. The provided spans will be grouped into traces by + * their trace ID. + */ + public static TracesAssert assertThat(List spanData) { + Map> traces = + spanData.stream() + .collect( + Collectors.groupingBy( + SpanData::getTraceId, + LinkedHashMap::new, + Collectors.toCollection(ArrayList::new))); + for (List trace : traces.values()) { + trace.sort(SPAN_DATA_COMPARATOR); + } + return assertThat(traces.values()); + } + /** * Returns an assertion for a list of traces. The traces must already be grouped into {@code * List} where each list has spans with the same trace ID. diff --git a/sdk/testing/src/main/java/io/opentelemetry/sdk/testing/junit5/OpenTelemetryExtension.java b/sdk/testing/src/main/java/io/opentelemetry/sdk/testing/junit5/OpenTelemetryExtension.java index 65083df67d5..c023d299a5f 100644 --- a/sdk/testing/src/main/java/io/opentelemetry/sdk/testing/junit5/OpenTelemetryExtension.java +++ b/sdk/testing/src/main/java/io/opentelemetry/sdk/testing/junit5/OpenTelemetryExtension.java @@ -23,11 +23,7 @@ import io.opentelemetry.sdk.trace.data.SpanData; import io.opentelemetry.sdk.trace.export.SimpleSpanProcessor; import java.util.ArrayList; -import java.util.Comparator; -import java.util.LinkedHashMap; import java.util.List; -import java.util.Map; -import java.util.stream.Collectors; import org.junit.jupiter.api.extension.AfterAllCallback; import org.junit.jupiter.api.extension.BeforeAllCallback; import org.junit.jupiter.api.extension.BeforeEachCallback; @@ -124,15 +120,7 @@ public List getMetrics() { * requires AssertJ to be on the classpath. */ public TracesAssert assertTraces() { - Map> traces = - getSpans().stream() - .collect( - Collectors.groupingBy( - SpanData::getTraceId, LinkedHashMap::new, Collectors.toList())); - for (List trace : traces.values()) { - trace.sort(Comparator.comparing(SpanData::getStartEpochNanos)); - } - return assertThat(traces.values()); + return assertThat(spanExporter.getFinishedSpanItems()); } /** diff --git a/sdk/testing/src/test/java/io/opentelemetry/sdk/testing/assertj/TracesAssertTest.java b/sdk/testing/src/test/java/io/opentelemetry/sdk/testing/assertj/TracesAssertTest.java new file mode 100644 index 00000000000..acc977ebaef --- /dev/null +++ b/sdk/testing/src/test/java/io/opentelemetry/sdk/testing/assertj/TracesAssertTest.java @@ -0,0 +1,75 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.testing.assertj; + +import static org.assertj.core.api.Assertions.assertThat; + +import io.opentelemetry.api.trace.SpanContext; +import io.opentelemetry.api.trace.SpanId; +import io.opentelemetry.api.trace.SpanKind; +import io.opentelemetry.api.trace.TraceFlags; +import io.opentelemetry.api.trace.TraceId; +import io.opentelemetry.api.trace.TraceState; +import io.opentelemetry.sdk.testing.trace.TestSpanData; +import io.opentelemetry.sdk.trace.data.StatusData; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import org.junit.jupiter.api.Test; + +class TracesAssertTest { + @Test + void spanDataComparator() { + TestSpanData before = createBasicSpanBuilder().setStartEpochNanos(9).build(); + + String traceId = TraceId.fromLongs(1, 2); + TestSpanData parent = + createBasicSpanBuilder() + .setName("parent") + .setSpanContext( + SpanContext.create( + traceId, SpanId.fromLong(1), TraceFlags.getDefault(), TraceState.getDefault())) + .setStartEpochNanos(10) + .build(); + TestSpanData child = + createBasicSpanBuilder() + .setName("child") + .setSpanContext( + SpanContext.create( + traceId, SpanId.fromLong(2), TraceFlags.getDefault(), TraceState.getDefault())) + .setStartEpochNanos(10) + .setParentSpanContext(parent.getSpanContext()) + .build(); + + TestSpanData sameTime1 = + createBasicSpanBuilder().setName("sameTime1").setStartEpochNanos(11).build(); + TestSpanData sameTime2 = + createBasicSpanBuilder().setName("sameTime2").setStartEpochNanos(11).build(); + + assertSort(Arrays.asList(child, parent, before), before, parent, child); + assertSort(Arrays.asList(parent, child, before), before, parent, child); + + assertSort(Arrays.asList(sameTime1, sameTime2, before), before, sameTime1, sameTime2); + assertSort(Arrays.asList(sameTime2, sameTime1, before), before, sameTime2, sameTime1); + } + + private static void assertSort(List spanData, TestSpanData... expected) { + ArrayList list = new ArrayList<>(spanData); + list.sort(TracesAssert.SPAN_DATA_COMPARATOR); + assertThat(list).containsExactly(expected); + } + + private static TestSpanData.Builder createBasicSpanBuilder() { + return TestSpanData.builder() + .setHasEnded(true) + .setName("spanName") + .setEndEpochNanos(100) + .setKind(SpanKind.SERVER) + .setStatus(StatusData.ok()) + .setTotalRecordedEvents(0) + .setTotalRecordedLinks(0); + } +} diff --git a/sdk/testing/src/test/java/io/opentelemetry/sdk/testing/junit5/OpenTelemetryExtensionTest.java b/sdk/testing/src/test/java/io/opentelemetry/sdk/testing/junit5/OpenTelemetryExtensionTest.java index 4c579c3bfe8..7e595f0345e 100644 --- a/sdk/testing/src/test/java/io/opentelemetry/sdk/testing/junit5/OpenTelemetryExtensionTest.java +++ b/sdk/testing/src/test/java/io/opentelemetry/sdk/testing/junit5/OpenTelemetryExtensionTest.java @@ -17,6 +17,7 @@ import io.opentelemetry.sdk.trace.data.SpanData; import java.util.Arrays; import java.util.LinkedHashMap; +import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -58,9 +59,9 @@ public void getSpansAgain() { @Test public void assertTraces() { - Span span = tracer.spanBuilder("testa1").startSpan(); + Span span = tracer.spanBuilder("testa1").setStartTimestamp(1000, TimeUnit.SECONDS).startSpan(); try (Scope ignored = span.makeCurrent()) { - tracer.spanBuilder("testa2").startSpan().end(); + tracer.spanBuilder("testa2").setStartTimestamp(1000, TimeUnit.SECONDS).startSpan().end(); } finally { span.end(); }