diff --git a/gradle/changelog/resilient_trace_exporter.yaml b/gradle/changelog/resilient_trace_exporter.yaml new file mode 100644 index 0000000000..1dc034b6bc --- /dev/null +++ b/gradle/changelog/resilient_trace_exporter.yaml @@ -0,0 +1,2 @@ +- type: fixed + description: Makes the trace exporter resilient against errors in plugins diff --git a/scm-core/src/main/java/sonia/scm/trace/Tracer.java b/scm-core/src/main/java/sonia/scm/trace/Tracer.java index 9b1de6af18..2214f310e4 100644 --- a/scm-core/src/main/java/sonia/scm/trace/Tracer.java +++ b/scm-core/src/main/java/sonia/scm/trace/Tracer.java @@ -25,6 +25,7 @@ package sonia.scm.trace; import jakarta.inject.Inject; +import lombok.extern.slf4j.Slf4j; import java.util.Set; @@ -48,6 +49,7 @@ import java.util.Set; * * @since 2.9.0 */ +@Slf4j public final class Tracer { private final Set exporters; @@ -70,7 +72,11 @@ public final class Tracer { */ void export(SpanContext span) { for (Exporter exporter : exporters) { - exporter.export(span); + try { + exporter.export(span); + } catch (Exception e) { + log.warn("got error from trace exporter", e); + } } } } diff --git a/scm-core/src/test/java/sonia/scm/trace/TracerTest.java b/scm-core/src/test/java/sonia/scm/trace/TracerTest.java index 12985038f8..2d6adac60e 100644 --- a/scm-core/src/test/java/sonia/scm/trace/TracerTest.java +++ b/scm-core/src/test/java/sonia/scm/trace/TracerTest.java @@ -25,79 +25,112 @@ package sonia.scm.trace; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Set; import static org.assertj.core.api.Assertions.assertThat; class TracerTest { - private Tracer tracer; - private CollectingExporter exporter; + @Nested + class WithNormalExporter { + private Tracer tracer; + private CollectingExporter exporter; - @BeforeEach - void setUpTracer() { - exporter = new CollectingExporter(); - tracer = new Tracer(Collections.singleton(exporter)); - } - - @Test - void shouldReturnSpan() { - tracer.span("sample").close(); - - SpanContext span = exporter.spans.get(0); - assertThat(span.getKind()).isEqualTo("sample"); - assertThat(span.getOpened()).isNotNull(); - assertThat(span.getClosed()).isNotNull(); - assertThat(span.isFailed()).isFalse(); - } - - @Test - @SuppressWarnings("java:S2925") // it is ok, to use sleep here - void shouldReturnPositiveDuration() throws InterruptedException { - try (Span span = tracer.span("sample")) { - span.label("l1", "one"); - Thread.sleep(1L); + @BeforeEach + void setUpTracer() { + exporter = new CollectingExporter(); + tracer = new Tracer(Collections.singleton(exporter)); } - SpanContext span = exporter.spans.get(0); - assertThat(span.duration()).isPositive(); + @Test + void shouldReturnSpan() { + tracer.span("sample").close(); + + SpanContext span = exporter.spans.get(0); + assertThat(span.getKind()).isEqualTo("sample"); + assertThat(span.getOpened()).isNotNull(); + assertThat(span.getClosed()).isNotNull(); + assertThat(span.isFailed()).isFalse(); + } + + @Test + @SuppressWarnings("java:S2925") // it is ok, to use sleep here + void shouldReturnPositiveDuration() throws InterruptedException { + try (Span span = tracer.span("sample")) { + span.label("l1", "one"); + Thread.sleep(1L); + } + + SpanContext span = exporter.spans.get(0); + assertThat(span.duration()).isPositive(); + } + + @Test + void shouldConvertLabels() { + try (Span span = tracer.span("sample")) { + span.label("int", 21); + span.label("long", 42L); + span.label("float", 21.0f); + span.label("double", 42.0d); + span.label("boolean", true); + span.label("object", new StringWrapper("value")); + } + + Map labels = exporter.spans.get(0).getLabels(); + assertThat(labels) + .containsEntry("int", "21") + .containsEntry("long", "42") + .containsEntry("float", "21.0") + .containsEntry("double", "42.0") + .containsEntry("boolean", "true") + .containsEntry("object", "value"); + } + + @Test + void shouldReturnFailedSpan() { + try (Span span = tracer.span("failing")) { + span.failed(); + } + + SpanContext span = exporter.spans.get(0); + assertThat(span.getKind()).isEqualTo("failing"); + assertThat(span.isFailed()).isTrue(); + } + + private static class StringWrapper { + + private final String value; + + public StringWrapper(String value) { + this.value = value; + } + + @Override + public String toString() { + return value; + } + } } @Test - void shouldConvertLabels() { - try (Span span = tracer.span("sample")) { - span.label("int", 21); - span.label("long", 42L); - span.label("float", 21.0f); - span.label("double", 42.0d); - span.label("boolean", true); - span.label("object", new StringWrapper("value")); - } + void shouldNotFailIfExporterFails() { + CollectingExporter exporterThatShouldBeCalled = new CollectingExporter(); + Exporter failingExporter = span -> { + throw new RuntimeException("this should not break everything"); + }; - Map labels = exporter.spans.get(0).getLabels(); - assertThat(labels) - .containsEntry("int", "21") - .containsEntry("long", "42") - .containsEntry("float", "21.0") - .containsEntry("double", "42.0") - .containsEntry("boolean", "true") - .containsEntry("object", "value"); - } + Tracer tracer = new Tracer(Set.of(failingExporter, exporterThatShouldBeCalled)); + Span span = tracer.span("test"); + span.close(); - @Test - void shouldReturnFailedSpan() { - try (Span span = tracer.span("failing")) { - span.failed(); - } - - SpanContext span = exporter.spans.get(0); - assertThat(span.getKind()).isEqualTo("failing"); - assertThat(span.isFailed()).isTrue(); + assertThat(exporterThatShouldBeCalled.spans).isNotEmpty(); } public static class CollectingExporter implements Exporter { @@ -109,19 +142,4 @@ class TracerTest { spans.add(spanContext); } } - - private static class StringWrapper { - - private final String value; - - public StringWrapper(String value) { - this.value = value; - } - - @Override - public String toString() { - return value; - } - } - }