Merge branch 'main' into gh-1365

CHANGELOG.md

@@ -1,6 +1,8 @@
 # Unreleased
 
 * Feat: OkHttp callback for Customising the Span (#1478)
+* Feat: Add breadcrumb in Spring RestTemplate integration (#1481)
+* Fix: Cloning Stack (#1483)
 
 # 5.0.0-beta.4
 

sentry-spring-boot-starter/src/test/kotlin/io/sentry/spring/boot/SentrySpanRestTemplateCustomizerTest.kt

@@ -1,7 +1,10 @@
 package io.sentry.spring.boot
 
+import com.nhaarman.mockitokotlin2.check
 import com.nhaarman.mockitokotlin2.mock
+import com.nhaarman.mockitokotlin2.verify
 import com.nhaarman.mockitokotlin2.whenever
+import io.sentry.Breadcrumb
 import io.sentry.IHub
 import io.sentry.Scope
 import io.sentry.SentryOptions
@@ -10,8 +13,8 @@ import io.sentry.SpanStatus
 import io.sentry.TransactionContext
 import java.io.IOException
 import kotlin.test.Test
+import kotlin.test.assertEquals
 import org.assertj.core.api.Assertions.assertThat
-import org.springframework.http.HttpMethod
 import org.springframework.http.HttpStatus
 import org.springframework.http.MediaType
 import org.springframework.test.web.client.MockRestServiceServer
@@ -41,7 +44,6 @@ class SentrySpanRestTemplateCustomizerTest {
                 whenever(hub.span).thenReturn(transaction)
 
                 val scenario = mockServer.expect(MockRestRequestMatchers.requestTo("/test/123"))
-                    .andExpect(MockRestRequestMatchers.method(HttpMethod.GET))
                     .andExpect {
                         // must have trace id from the parent transaction and must not contain spanId from the parent transaction
                         assertThat(it.headers["sentry-trace"]!!.first()).startsWith(transaction.spanContext.traceId.toString())
@@ -57,7 +59,6 @@ class SentrySpanRestTemplateCustomizerTest {
                 }
             } else {
                 mockServer.expect(MockRestRequestMatchers.requestTo("/test/123"))
-                    .andExpect(MockRestRequestMatchers.method(HttpMethod.GET))
                     .andRespond(MockRestResponseCreators.withStatus(status).body("OK").contentType(MediaType.APPLICATION_JSON))
             }
 
@@ -126,4 +127,54 @@ class SentrySpanRestTemplateCustomizerTest {
         fixture.customizer.customize(restTemplate)
         assertThat(restTemplate.interceptors).hasSize(1)
     }
+
+    @Test
+    fun `when transaction is active adds breadcrumb when http calls succeeds`() {
+        fixture.getSut(isTransactionActive = true).postForObject("/test/{id}", "content", String::class.java, 123)
+        verify(fixture.hub).addBreadcrumb(check<Breadcrumb> {
+            assertEquals("http", it.type)
+            assertEquals("/test/123", it.data["url"])
+            assertEquals("POST", it.data["method"])
+            assertEquals(7, it.data["requestBodySize"])
+        })
+    }
+
+    @SuppressWarnings("SwallowedException")
+    @Test
+    fun `when transaction is active adds breadcrumb when http calls results in exception`() {
+        try {
+            fixture.getSut(isTransactionActive = true, status = HttpStatus.INTERNAL_SERVER_ERROR).getForObject("/test/{id}", String::class.java, 123)
+        } catch (e: Throwable) {
+        }
+        verify(fixture.hub).addBreadcrumb(check<Breadcrumb> {
+            assertEquals("http", it.type)
+            assertEquals("/test/123", it.data["url"])
+            assertEquals("GET", it.data["method"])
+        })
+    }
+
+    @Test
+    fun `when transaction is not active adds breadcrumb when http calls succeeds`() {
+        fixture.getSut(isTransactionActive = false).postForObject("/test/{id}", "content", String::class.java, 123)
+        verify(fixture.hub).addBreadcrumb(check<Breadcrumb> {
+            assertEquals("http", it.type)
+            assertEquals("/test/123", it.data["url"])
+            assertEquals("POST", it.data["method"])
+            assertEquals(7, it.data["requestBodySize"])
+        })
+    }
+
+    @SuppressWarnings("SwallowedException")
+    @Test
+    fun `when transaction is not active adds breadcrumb when http calls results in exception`() {
+        try {
+            fixture.getSut(isTransactionActive = false, status = HttpStatus.INTERNAL_SERVER_ERROR).getForObject("/test/{id}", String::class.java, 123)
+        } catch (e: Throwable) {
+        }
+        verify(fixture.hub).addBreadcrumb(check<Breadcrumb> {
+            assertEquals("http", it.type)
+            assertEquals("/test/123", it.data["url"])
+            assertEquals("GET", it.data["method"])
+        })
+    }
 }

sentry-spring/src/main/java/io/sentry/spring/tracing/SentrySpanClientHttpRequestInterceptor.java

@@ -1,13 +1,15 @@
 package io.sentry.spring.tracing;
 
 import com.jakewharton.nopen.annotation.Open;
+import io.sentry.Breadcrumb;
 import io.sentry.IHub;
 import io.sentry.ISpan;
 import io.sentry.SentryTraceHeader;
 import io.sentry.SpanStatus;
 import io.sentry.util.Objects;
 import java.io.IOException;
 import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
 import org.springframework.http.HttpRequest;
 import org.springframework.http.client.ClientHttpRequestExecution;
 import org.springframework.http.client.ClientHttpRequestInterceptor;
@@ -27,28 +29,44 @@ public SentrySpanClientHttpRequestInterceptor(final @NotNull IHub hub) {
       @NotNull byte[] body,
       @NotNull ClientHttpRequestExecution execution)
       throws IOException {
-    final ISpan activeSpan = hub.getSpan();
-    if (activeSpan == null) {
-      return execution.execute(request, body);
-    }
+    Integer responseStatusCode = null;
+    try {
+      final ISpan activeSpan = hub.getSpan();
+      if (activeSpan == null) {
+        return execution.execute(request, body);
+      }
 
-    final ISpan span = activeSpan.startChild("http.client");
-    span.setDescription(request.getMethodValue() + " " + request.getURI());
+      final ISpan span = activeSpan.startChild("http.client");
+      span.setDescription(request.getMethodValue() + " " + request.getURI());
 
-    final SentryTraceHeader sentryTraceHeader = span.toSentryTrace();
-    request.getHeaders().add(sentryTraceHeader.getName(), sentryTraceHeader.getValue());
-    try {
-      final ClientHttpResponse response = execution.execute(request, body);
-      // handles both success and error responses
-      span.setStatus(SpanStatus.fromHttpStatusCode(response.getRawStatusCode()));
-      return response;
-    } catch (Exception e) {
-      // handles cases like connection errors
-      span.setThrowable(e);
-      span.setStatus(SpanStatus.INTERNAL_ERROR);
-      throw e;
+      final SentryTraceHeader sentryTraceHeader = span.toSentryTrace();
+      request.getHeaders().add(sentryTraceHeader.getName(), sentryTraceHeader.getValue());
+      try {
+        final ClientHttpResponse response = execution.execute(request, body);
+        // handles both success and error responses
+        span.setStatus(SpanStatus.fromHttpStatusCode(response.getRawStatusCode()));
+        responseStatusCode = response.getRawStatusCode();
+        return response;
+      } catch (Exception e) {
+        // handles cases like connection errors
+        span.setThrowable(e);
+        span.setStatus(SpanStatus.INTERNAL_ERROR);
+        throw e;
+      } finally {
+        span.finish();
+      }
     } finally {
-      span.finish();
+      addBreadcrumb(request, body, responseStatusCode);
     }
   }
+
+  private void addBreadcrumb(
+      final @NotNull HttpRequest request,
+      final @NotNull byte[] body,
+      final @Nullable Integer responseStatusCode) {
+    final Breadcrumb breadcrumb =
+        Breadcrumb.http(request.getURI().toString(), request.getMethodValue(), responseStatusCode);
+    breadcrumb.setData("requestBodySize", body.length);
+    hub.addBreadcrumb(breadcrumb);
+  }
 }

sentry/src/main/java/io/sentry/Stack.java

@@ -2,6 +2,7 @@
 
 import io.sentry.util.Objects;
 import java.util.Deque;
+import java.util.Iterator;
 import java.util.concurrent.LinkedBlockingDeque;
 import org.jetbrains.annotations.NotNull;
 
@@ -60,9 +61,14 @@ public Stack(final @NotNull ILogger logger, final @NotNull StackItem rootStackIt
   }
 
   public Stack(final @NotNull Stack stack) {
-    this(stack.logger, stack.items.getFirst());
-    for (final StackItem item : stack.items) {
-      push(new StackItem(item));
+    this(stack.logger, new StackItem(stack.items.getFirst()));
+    final Iterator<StackItem> iterator = stack.items.iterator();
+    // skip first item (root item)
+    if (iterator.hasNext()) {
+      iterator.next();
+    }
+    while (iterator.hasNext()) {
+      push(new StackItem(iterator.next()));
     }
   }
 

sentry/src/test/java/io/sentry/StackTest.kt

@@ -61,12 +61,21 @@ class StackTest {
     @Test
     fun `cloning stack clones stack items`() {
         val stack = fixture.getSut()
+        stack.push(StackItem(fixture.options, fixture.client, fixture.scope))
         val clone = Stack(stack)
 
-        val stackRootItem = stack.peek()
-        val cloneRootItem = clone.peek()
-        assertNotEquals(stackRootItem, cloneRootItem)
-        assertNotEquals(stackRootItem.scope, cloneRootItem.scope)
-        assertEquals(stackRootItem.client, cloneRootItem.client)
+        assertEquals(stack.size(), clone.size())
+        // assert first stack item
+        assertStackItems(stack.peek(), clone.peek())
+        stack.pop()
+        clone.pop()
+        // assert root item
+        assertStackItems(stack.peek(), clone.peek())
+    }
+
+    private fun assertStackItems(item1: StackItem, item2: StackItem) {
+        assertNotEquals(item1, item2)
+        assertNotEquals(item1.scope, item2.scope)
+        assertEquals(item1.client, item2.client)
     }
 }