From 1c57342cd9b2c41d4208a84d1cc710318ffbea22 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Wed, 3 Jul 2024 10:59:00 +0200 Subject: [PATCH] Catch exceptions from trace exporters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This prevents errors in exporters (like the trace plugin) to escalate. In a specific case it could happen, that due to the cas plugin and the trace plugin the login failed when the trace file for cas was corrupt, because the trace monitor filed to log the cas request and threw an exception due to which the whole login process through cas was blocked. Pushed-by: Rene Pfeuffer Co-authored-by: René Pfeuffer --- .../changelog/resilient_trace_exporter.yaml | 2 + .../src/main/java/sonia/scm/trace/Tracer.java | 8 +- .../test/java/sonia/scm/trace/TracerTest.java | 156 ++++++++++-------- 3 files changed, 96 insertions(+), 70 deletions(-) create mode 100644 gradle/changelog/resilient_trace_exporter.yaml 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; - } - } - }