Catch exceptions from trace exporters

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<rene.pfeuffer@cloudogu.com>
Co-authored-by: René Pfeuffer<rene.pfeuffer@cloudogu.com>

gradle/changelog/resilient_trace_exporter.yaml

@@ -0,0 +1,2 @@
+- type: fixed
+  description: Makes the trace exporter resilient against errors in plugins

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<Exporter> 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);
+      }
     }
   }
 }

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<String, String> 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<String, String> 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;
-    }
-  }
-
 }