Merge branch 'main' into scope-request

CHANGELOG.md

@@ -14,12 +14,15 @@
 * Enchancement: Integration interface better compatibility with Kotlin null-safety
 * Enchancement: Simplify Sentry configuration in Spring integration (#1259)
 * Enchancement: Add Request to the Scope. #1270
+* Fix: Fix SentryTransaction#getStatus NPE (#1273)
+* Enchancement: Optimize SentryTracingFilter when hub is disabled.
+* Fix: SentryTransaction#finish should not clear another transaction from the scope (#1278)
 
 Breaking Changes:
 * Enchancement: SentryExceptionResolver should not send handled errors by default (#1248).
+* Ref: Optimize DuplicateEventDetectionEventProcessor performance (#1247).
 * Enchancement: Add overloads for startTransaction taking op and description (#1244)
 
-
 # 4.1.0
 
 * Improve Kotlin compatibility for SdkVersion (#1213)

sentry-spring/api/sentry-spring.api

@@ -105,6 +105,7 @@ public class io/sentry/spring/tracing/SentryTracingConfiguration {
 public class io/sentry/spring/tracing/SentryTracingFilter : org/springframework/web/filter/OncePerRequestFilter {
 	public fun <init> ()V
 	public fun <init> (Lio/sentry/IHub;Lio/sentry/spring/SentryRequestResolver;)V
+	public fun <init> (Lio/sentry/IHub;Lio/sentry/spring/SentryRequestResolver;Lio/sentry/spring/tracing/TransactionNameProvider;)V
 	protected fun doFilterInternal (Ljavax/servlet/http/HttpServletRequest;Ljavax/servlet/http/HttpServletResponse;Ljavax/servlet/FilterChain;)V
 }
 

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

@@ -36,8 +36,7 @@ public class SentryTracingFilter extends OncePerRequestFilter {
   /** Operation used by {@link SentryTransaction} created in {@link SentryTracingFilter}. */
   private static final String TRANSACTION_OP = "http.server";
 
-  private final @NotNull TransactionNameProvider transactionNameProvider =
-      new TransactionNameProvider();
+  private final @NotNull TransactionNameProvider transactionNameProvider;
   private final @NotNull IHub hub;
   private final @NotNull SentryRequestResolver requestResolver;
 
@@ -47,12 +46,21 @@ public SentryTracingFilter() {
 
   public SentryTracingFilter(
       final @NotNull IHub hub, final @NotNull SentryRequestResolver requestResolver) {
+    this(hub, requestResolver, new TransactionNameProvider());
+  }
+
+  public SentryTracingFilter(
+      final @NotNull IHub hub,
+      final @NotNull SentryRequestResolver requestResolver,
+      final @NotNull TransactionNameProvider transactionNameProvider) {
     this.hub = Objects.requireNonNull(hub, "hub is required");
     this.requestResolver = Objects.requireNonNull(requestResolver, "requestResolver is required");
+    this.transactionNameProvider =
+        Objects.requireNonNull(transactionNameProvider, "transactionNameProvider is required");
   }
 
   SentryTracingFilter(final @NotNull IHub hub) {
-    this(hub, new SentryRequestResolver(hub));
+    this(hub, new SentryRequestResolver(hub), new TransactionNameProvider());
   }
 
   @Override
@@ -62,24 +70,29 @@ protected void doFilterInternal(
       final @NotNull FilterChain filterChain)
       throws ServletException, IOException {
 
-    final String sentryTraceHeader = httpRequest.getHeader(SentryTraceHeader.SENTRY_TRACE_HEADER);
+    if (hub.isEnabled()) {
+      final String sentryTraceHeader = httpRequest.getHeader(SentryTraceHeader.SENTRY_TRACE_HEADER);
 
-    // at this stage we are not able to get real transaction name
-    final ITransaction transaction = startTransaction(httpRequest, sentryTraceHeader);
-    try {
-      filterChain.doFilter(httpRequest, httpResponse);
-    } finally {
-      // after all filters run, templated path pattern is available in request attribute
-      final String transactionName = transactionNameProvider.provideTransactionName(httpRequest);
-      // if transaction name is not resolved, the request has not been processed by a controller and
-      // we should not report it to Sentry
-      if (transactionName != null) {
-        transaction.setName(transactionName);
-        transaction.setOperation(TRANSACTION_OP);
-        transaction.setRequest(requestResolver.resolveSentryRequest(httpRequest));
-        transaction.setStatus(SpanStatus.fromHttpStatusCode(httpResponse.getStatus()));
-        transaction.finish();
+      // at this stage we are not able to get real transaction name
+      final ITransaction transaction = startTransaction(httpRequest, sentryTraceHeader);
+      try {
+        filterChain.doFilter(httpRequest, httpResponse);
+      } finally {
+        // after all filters run, templated path pattern is available in request attribute
+        final String transactionName = transactionNameProvider.provideTransactionName(httpRequest);
+        // if transaction name is not resolved, the request has not been processed by a controller
+        // and
+        // we should not report it to Sentry
+        if (transactionName != null) {
+          transaction.setName(transactionName);
+          transaction.setOperation(TRANSACTION_OP);
+          transaction.setRequest(requestResolver.resolveSentryRequest(httpRequest));
+          transaction.setStatus(SpanStatus.fromHttpStatusCode(httpResponse.getStatus()));
+          transaction.finish();
+        }
       }
+    } else {
+      filterChain.doFilter(httpRequest, httpResponse);
     }
   }
 

sentry-spring/src/test/kotlin/io/sentry/spring/tracing/SentryTracingFilterTest.kt

@@ -4,7 +4,10 @@ import com.nhaarman.mockitokotlin2.any
 import com.nhaarman.mockitokotlin2.check
 import com.nhaarman.mockitokotlin2.eq
 import com.nhaarman.mockitokotlin2.mock
+import com.nhaarman.mockitokotlin2.spy
 import com.nhaarman.mockitokotlin2.verify
+import com.nhaarman.mockitokotlin2.verifyNoMoreInteractions
+import com.nhaarman.mockitokotlin2.verifyZeroInteractions
 import com.nhaarman.mockitokotlin2.whenever
 import io.sentry.IHub
 import io.sentry.SentryOptions
@@ -14,6 +17,7 @@ import io.sentry.SpanId
 import io.sentry.SpanStatus
 import io.sentry.TransactionContext
 import io.sentry.protocol.SentryId
+import io.sentry.spring.SentryRequestResolver
 import javax.servlet.FilterChain
 import javax.servlet.http.HttpServletRequest
 import kotlin.test.Test
@@ -30,12 +34,14 @@ class SentryTracingFilterTest {
         val request = MockHttpServletRequest()
         val response = MockHttpServletResponse()
         val chain = mock<FilterChain>()
+        val sentryRequestResolver = spy(SentryRequestResolver(hub))
+        val transactionNameProvider = spy(TransactionNameProvider())
 
         init {
             whenever(hub.options).thenReturn(SentryOptions())
         }
 
-        fun getSut(sentryTraceHeader: String? = null): SentryTracingFilter {
+        fun getSut(isEnabled: Boolean = true, sentryTraceHeader: String? = null): SentryTracingFilter {
             request.requestURI = "/product/12"
             request.method = "POST"
             request.setAttribute(HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE, "/product/{id}")
@@ -45,7 +51,8 @@ class SentryTracingFilterTest {
             }
             response.status = 200
             whenever(hub.startTransaction(any<String>(), any(), any())).thenAnswer { SentryTransaction(it.arguments[0] as String, SpanContext(it.arguments[1] as String), hub) }
-            return SentryTracingFilter(hub)
+            whenever(hub.isEnabled).thenReturn(isEnabled)
+            return SentryTracingFilter(hub, sentryRequestResolver, transactionNameProvider)
         }
     }
 
@@ -92,4 +99,18 @@ class SentryTracingFilterTest {
             assertThat(it.contexts.trace!!.parentSpanId).isEqualTo(parentSpanId)
         }, eq(null))
     }
+
+    @Test
+    fun `when hub is disabled, components are not invoked`() {
+        val filter = fixture.getSut(isEnabled = false)
+
+        filter.doFilter(fixture.request, fixture.response, fixture.chain)
+
+        verify(fixture.chain).doFilter(fixture.request, fixture.response)
+
+        verify(fixture.hub).isEnabled
+        verifyNoMoreInteractions(fixture.hub)
+        verifyZeroInteractions(fixture.sentryRequestResolver)
+        verifyZeroInteractions(fixture.transactionNameProvider)
+    }
 }

sentry-spring/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker

@@ -0,0 +1 @@
+mock-maker-inline

sentry/api/sentry.api

@@ -752,6 +752,7 @@ public class io/sentry/SentryOptions {
 	public fun isAttachStacktrace ()Z
 	public fun isAttachThreads ()Z
 	public fun isDebug ()Z
+	public fun isEnableDeduplication ()Z
 	public fun isEnableExternalConfiguration ()Z
 	public fun isEnableNdk ()Z
 	public fun isEnableScopeSync ()Z
@@ -771,6 +772,7 @@ public class io/sentry/SentryOptions {
 	public fun setDist (Ljava/lang/String;)V
 	public fun setDistinctId (Ljava/lang/String;)V
 	public fun setDsn (Ljava/lang/String;)V
+	public fun setEnableDeduplication (Ljava/lang/Boolean;)V
 	public fun setEnableExternalConfiguration (Z)V
 	public fun setEnableNdk (Z)V
 	public fun setEnableScopeSync (Z)V

sentry/src/main/java/io/sentry/DuplicateEventDetectionEventProcessor.java

@@ -2,6 +2,7 @@
 
 import io.sentry.util.Objects;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.WeakHashMap;
@@ -10,7 +11,8 @@
 
 /** Deduplicates events containing throwable that has been already processed. */
 public final class DuplicateEventDetectionEventProcessor implements EventProcessor {
-  private final WeakHashMap<Throwable, Object> capturedObjects = new WeakHashMap<>();
+  private final Map<Throwable, Object> capturedObjects =
+      Collections.synchronizedMap(new WeakHashMap<>());
   private final SentryOptions options;
 
   public DuplicateEventDetectionEventProcessor(final @NotNull SentryOptions options) {
@@ -19,20 +21,24 @@ public DuplicateEventDetectionEventProcessor(final @NotNull SentryOptions option
 
   @Override
   public SentryEvent process(final @NotNull SentryEvent event, final @Nullable Object hint) {
-    final Throwable throwable = event.getOriginThrowable();
-    if (throwable != null) {
-      if (capturedObjects.containsKey(throwable)
-          || containsAnyKey(capturedObjects, allCauses(throwable))) {
-        options
-            .getLogger()
-            .log(
-                SentryLevel.DEBUG,
-                "Duplicate Exception detected. Event %s will be discarded.",
-                event.getEventId());
-        return null;
-      } else {
-        capturedObjects.put(throwable, null);
+    if (options.isEnableDeduplication()) {
+      final Throwable throwable = event.getOriginThrowable();
+      if (throwable != null) {
+        if (capturedObjects.containsKey(throwable)
+            || containsAnyKey(capturedObjects, allCauses(throwable))) {
+          options
+              .getLogger()
+              .log(
+                  SentryLevel.DEBUG,
+                  "Duplicate Exception detected. Event %s will be discarded.",
+                  event.getEventId());
+          return null;
+        } else {
+          capturedObjects.put(throwable, null);
+        }
       }
+    } else {
+      options.getLogger().log(SentryLevel.DEBUG, "Event deduplication is disabled.");
     }
     return event;
   }

sentry/src/main/java/io/sentry/Hub.java

@@ -556,7 +556,10 @@ public void flush(long timeoutMillis) {
                   e);
         } finally {
           if (item != null) {
-            item.getScope().clearTransaction();
+            final Scope scope = item.getScope();
+            if (scope.getTransaction() == transaction) {
+              scope.clearTransaction();
+            }
           }
         }
       }

sentry/src/main/java/io/sentry/IHub.java

@@ -292,6 +292,7 @@ default SentryId captureTransaction(ITransaction transaction) {
    * sampling context the decision if transaction is sampled will be taken by {@link TracesSampler}.
    *
    * @param name the transaction name
+   * @param operation the operation
    * @param customSamplingContext the sampling context
    * @return created transaction.
    */
@@ -319,6 +320,7 @@ ITransaction startTransaction(
    * {@link TracesSampler}.
    *
    * @param name the transaction name
+   * @param operation the operation
    * @return created transaction
    */
   default @NotNull ITransaction startTransaction(

sentry/src/main/java/io/sentry/SentryOptions.java

@@ -254,6 +254,13 @@ public class SentryOptions {
   /** max attachment size in bytes. */
   private long maxAttachmentSize = 20 * 1024 * 1024;
 
+  /**
+   * Enables event deduplication with {@link DuplicateEventDetectionEventProcessor}. Event
+   * deduplication prevents from receiving the same exception multiple times when there is more than
+   * one framework active that captures errors, for example Logback and Spring Boot.
+   */
+  private Boolean enableDeduplication = true;
+
   /**
    * Creates {@link SentryOptions} from properties provided by a {@link PropertiesProvider}.
    *
@@ -271,6 +278,7 @@ public class SentryOptions {
         propertiesProvider.getBooleanProperty("uncaught.handler.enabled"));
     options.setTracesSampleRate(propertiesProvider.getDoubleProperty("traces-sample-rate"));
     options.setDebug(propertiesProvider.getBooleanProperty("debug"));
+    options.setEnableDeduplication(propertiesProvider.getBooleanProperty("enable-deduplication"));
     final Map<String, String> tags = propertiesProvider.getMap("tags");
     for (final Map.Entry<String, String> tag : tags.entrySet()) {
       options.setTag(tag.getKey(), tag.getValue());
@@ -1224,6 +1232,33 @@ public void setMaxAttachmentSize(long maxAttachmentSize) {
     this.maxAttachmentSize = maxAttachmentSize;
   }
 
+  /**
+   * Returns if event deduplication is turned on.
+   *
+   * @return if event deduplication is turned on.
+   */
+  public boolean isEnableDeduplication() {
+    return Boolean.TRUE.equals(enableDeduplication);
+  }
+
+  /**
+   * Returns if event deduplication is turned on or of or {@code null} if not specified.
+   *
+   * @return if event deduplication is turned on or of or {@code null} if not specified.
+   */
+  private @Nullable Boolean getEnableDeduplication() {
+    return enableDeduplication;
+  }
+
+  /**
+   * Enables or disables event deduplication.
+   *
+   * @param enableDeduplication true if enabled false otherwise
+   */
+  public void setEnableDeduplication(final @Nullable Boolean enableDeduplication) {
+    this.enableDeduplication = enableDeduplication;
+  }
+
   /** The BeforeSend callback */
   public interface BeforeSendCallback {
 
@@ -1318,6 +1353,9 @@ void merge(final @NotNull SentryOptions options) {
     if (options.getDebug() != null) {
       setDebug(options.getDebug());
     }
+    if (options.getEnableDeduplication() != null) {
+      setEnableDeduplication(options.getEnableDeduplication());
+    }
     final Map<String, String> tags = new HashMap<>(options.getTags());
     for (final Map.Entry<String, String> tag : tags.entrySet()) {
       this.tags.put(tag.getKey(), tag.getValue());

sentry/src/main/java/io/sentry/SentryTransaction.java

@@ -238,7 +238,7 @@ Date getTimestamp() {
   @Override
   @Nullable
   public SpanStatus getStatus() {
-    return this.getContexts().getTrace().getStatus();
+    return context.getStatus();
   }
 
   @Override

sentry/src/main/java/io/sentry/SpanContext.java

@@ -45,7 +45,11 @@ public SpanContext(final @NotNull String operation, final @Nullable Boolean samp
     this(new SentryId(), new SpanId(), operation, null, sampled);
   }
 
-  /** Creates trace context with defered sampling decision. */
+  /**
+   * Creates trace context with defered sampling decision.
+   *
+   * @param operation the operation
+   */
   public SpanContext(final @NotNull String operation) {
     this(new SentryId(), new SpanId(), operation, null, null);
   }