diff --git a/CHANGELOG.md b/CHANGELOG.md index 6807041b529..7826d37d156 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +### Fixes + +- Do not instrument File I/O operations if tracing is disabled ([#4039](https://github.com/getsentry/sentry-java/pull/4039)) +- Do not instrument User Interaction multiple times ([#4039](https://github.com/getsentry/sentry-java/pull/4039)) +- Speed up view traversal to find touched target in `UserInteractionIntegration` ([#4039](https://github.com/getsentry/sentry-java/pull/4039)) + ### Internal - Make `SentryClient` constructor public ([#4045](https://github.com/getsentry/sentry-java/pull/4045)) diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/UserInteractionIntegration.java b/sentry-android-core/src/main/java/io/sentry/android/core/UserInteractionIntegration.java index 46275504cbe..653cfd2ee12 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/UserInteractionIntegration.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/UserInteractionIntegration.java @@ -50,6 +50,11 @@ private void startTracking(final @NotNull Activity activity) { delegate = new NoOpWindowCallback(); } + if (delegate instanceof SentryWindowCallback) { + // already instrumented + return; + } + final SentryGestureListener gestureListener = new SentryGestureListener(activity, scopes, options); window.setCallback(new SentryWindowCallback(delegate, activity, gestureListener, options)); diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/AndroidViewGestureTargetLocator.java b/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/AndroidViewGestureTargetLocator.java index 945ebeef649..f271c3da9e3 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/AndroidViewGestureTargetLocator.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/AndroidViewGestureTargetLocator.java @@ -18,7 +18,6 @@ public final class AndroidViewGestureTargetLocator implements GestureTargetLocat private static final String ORIGIN = "old_view_system"; private final boolean isAndroidXAvailable; - private final int[] coordinates = new int[2]; public AndroidViewGestureTargetLocator(final boolean isAndroidXAvailable) { this.isAndroidXAvailable = isAndroidXAvailable; @@ -26,18 +25,16 @@ public AndroidViewGestureTargetLocator(final boolean isAndroidXAvailable) { @Override public @Nullable UiElement locate( - @NotNull Object root, float x, float y, UiElement.Type targetType) { + @Nullable Object root, float x, float y, UiElement.Type targetType) { if (!(root instanceof View)) { return null; } final View view = (View) root; - if (touchWithinBounds(view, x, y)) { - if (targetType == UiElement.Type.CLICKABLE && isViewTappable(view)) { - return createUiElement(view); - } else if (targetType == UiElement.Type.SCROLLABLE - && isViewScrollable(view, isAndroidXAvailable)) { - return createUiElement(view); - } + if (targetType == UiElement.Type.CLICKABLE && isViewTappable(view)) { + return createUiElement(view); + } else if (targetType == UiElement.Type.SCROLLABLE + && isViewScrollable(view, isAndroidXAvailable)) { + return createUiElement(view); } return null; } @@ -52,17 +49,6 @@ private UiElement createUiElement(final @NotNull View targetView) { } } - private boolean touchWithinBounds(final @NotNull View view, final float x, final float y) { - view.getLocationOnScreen(coordinates); - int vx = coordinates[0]; - int vy = coordinates[1]; - - int w = view.getWidth(); - int h = view.getHeight(); - - return !(x < vx || x > vx + w || y < vy || y > vy + h); - } - private static boolean isViewTappable(final @NotNull View view) { return view.isClickable() && view.getVisibility() == View.VISIBLE; } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/ViewUtils.java b/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/ViewUtils.java index 6e7dab2ef5a..8aa8e89d694 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/ViewUtils.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/ViewUtils.java @@ -7,7 +7,6 @@ import io.sentry.android.core.SentryAndroidOptions; import io.sentry.internal.gestures.GestureTargetLocator; import io.sentry.internal.gestures.UiElement; -import io.sentry.util.Objects; import java.util.LinkedList; import java.util.Queue; import org.jetbrains.annotations.ApiStatus; @@ -17,6 +16,32 @@ @ApiStatus.Internal public final class ViewUtils { + private static final int[] coordinates = new int[2]; + + /** + * Verifies if the given touch coordinates are within the bounds of the given view. + * + * @param view the view to check if the touch coordinates are within its bounds + * @param x - the x coordinate of a {@link MotionEvent} + * @param y - the y coordinate of {@link MotionEvent} + * @return true if the touch coordinates are within the bounds of the view, false otherwise + */ + private static boolean touchWithinBounds( + final @Nullable View view, final float x, final float y) { + if (view == null) { + return false; + } + + view.getLocationOnScreen(coordinates); + int vx = coordinates[0]; + int vy = coordinates[1]; + + int w = view.getWidth(); + int h = view.getHeight(); + + return !(x < vx || x > vx + w || y < vy || y > vy + h); + } + /** * Finds a target view, that has been selected/clicked by the given coordinates x and y and the * given {@code viewTargetSelector}. @@ -40,7 +65,12 @@ public final class ViewUtils { @Nullable UiElement target = null; while (queue.size() > 0) { - final View view = Objects.requireNonNull(queue.poll(), "view is required"); + final View view = queue.poll(); + + if (!touchWithinBounds(view, x, y)) { + // if the touch is not hitting the view, skip traversal of its children + continue; + } if (view instanceof ViewGroup) { final ViewGroup viewGroup = (ViewGroup) view; @@ -54,7 +84,7 @@ public final class ViewUtils { if (newTarget != null) { if (targetType == UiElement.Type.CLICKABLE) { target = newTarget; - } else { + } else if (targetType == UiElement.Type.SCROLLABLE) { return newTarget; } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/UserInteractionIntegrationTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/UserInteractionIntegrationTest.kt index 379e3db3532..97e4d46845a 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/UserInteractionIntegrationTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/UserInteractionIntegrationTest.kt @@ -140,6 +140,7 @@ class UserInteractionIntegrationTest { ) ) + sut.register(fixture.scopes, fixture.options) sut.onActivityPaused(fixture.activity) verify(fixture.window).callback = null @@ -160,6 +161,7 @@ class UserInteractionIntegrationTest { ) ) + sut.register(fixture.scopes, fixture.options) sut.onActivityPaused(fixture.activity) verify(fixture.window).callback = delegate @@ -170,8 +172,30 @@ class UserInteractionIntegrationTest { val callback = mock() val sut = fixture.getSut(callback) + sut.register(fixture.scopes, fixture.options) sut.onActivityPaused(fixture.activity) verify(callback).stopTracking() } + + @Test + fun `does not instrument if the callback is already ours`() { + val delegate = mock() + val context = mock() + val resources = Fixture.mockResources() + whenever(context.resources).thenReturn(resources) + val existingCallback = SentryWindowCallback( + delegate, + context, + mock(), + mock() + ) + val sut = fixture.getSut(existingCallback) + + sut.register(fixture.scopes, fixture.options) + sut.onActivityResumed(fixture.activity) + + val argumentCaptor = argumentCaptor() + verify(fixture.window, never()).callback = argumentCaptor.capture() + } } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/internal/gestures/SentryGestureListenerClickTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/internal/gestures/SentryGestureListenerClickTest.kt index 74edfb43025..6c20e9eb042 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/internal/gestures/SentryGestureListenerClickTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/internal/gestures/SentryGestureListenerClickTest.kt @@ -185,7 +185,7 @@ class SentryGestureListenerClickTest { sut.onSingleTapUp(event) - verify(fixture.scopes, never()).addBreadcrumb(any()) + verify(fixture.scopes, never()).addBreadcrumb(any(), anyOrNull()) } @Test @@ -214,7 +214,7 @@ class SentryGestureListenerClickTest { sut.onSingleTapUp(event) - verify(fixture.scopes, never()).addBreadcrumb(any()) + verify(fixture.scopes, never()).addBreadcrumb(any(), anyOrNull()) } @Test @@ -223,7 +223,7 @@ class SentryGestureListenerClickTest { val event = mock() val sut = fixture.getSut(event, attachViewsToRoot = false) - fixture.window.mockDecorView(event = event, touchWithinBounds = false) { + fixture.window.mockDecorView(event = event, touchWithinBounds = true) { whenever(it.childCount).thenReturn(1) whenever(it.getChildAt(0)).thenReturn(fixture.target) } @@ -244,7 +244,7 @@ class SentryGestureListenerClickTest { val event = mock() val sut = fixture.getSut(event, attachViewsToRoot = false) - fixture.window.mockDecorView(event = event, touchWithinBounds = false) { + fixture.window.mockDecorView(event = event, touchWithinBounds = true) { whenever(it.childCount).thenReturn(1) whenever(it.getChildAt(0)).thenReturn(fixture.target) } @@ -253,4 +253,18 @@ class SentryGestureListenerClickTest { verify(fixture.scope).propagationContext = any() } + + @Test + fun `if touch is not within view group bounds does not traverse its children`() { + val event = mock() + val sut = fixture.getSut(event, attachViewsToRoot = false) + fixture.window.mockDecorView(event = event, touchWithinBounds = false) { + whenever(it.childCount).thenReturn(1) + whenever(it.getChildAt(0)).thenReturn(fixture.target) + } + + sut.onSingleTapUp(event) + + verify(fixture.scopes, never()).addBreadcrumb(any(), anyOrNull()) + } } diff --git a/sentry-compose-helper/src/jvmMain/java/io/sentry/compose/gestures/ComposeGestureTargetLocator.java b/sentry-compose-helper/src/jvmMain/java/io/sentry/compose/gestures/ComposeGestureTargetLocator.java index bd056aeb919..68e32269a1d 100644 --- a/sentry-compose-helper/src/jvmMain/java/io/sentry/compose/gestures/ComposeGestureTargetLocator.java +++ b/sentry-compose-helper/src/jvmMain/java/io/sentry/compose/gestures/ComposeGestureTargetLocator.java @@ -42,7 +42,7 @@ public ComposeGestureTargetLocator(final @NotNull ILogger logger) { @Override public @Nullable UiElement locate( - @NotNull Object root, float x, float y, UiElement.Type targetType) { + @Nullable Object root, float x, float y, UiElement.Type targetType) { // lazy init composeHelper as it's using some reflection under the hood if (composeHelper == null) { diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index a0d559f12f5..3303818dd6e 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -4147,6 +4147,7 @@ public final class io/sentry/instrumentation/file/SentryFileOutputStream : java/ public final class io/sentry/instrumentation/file/SentryFileOutputStream$Factory { public fun ()V public static fun create (Ljava/io/FileOutputStream;Ljava/io/File;)Ljava/io/FileOutputStream; + public static fun create (Ljava/io/FileOutputStream;Ljava/io/File;Lio/sentry/IScopes;)Ljava/io/FileOutputStream; public static fun create (Ljava/io/FileOutputStream;Ljava/io/File;Z)Ljava/io/FileOutputStream; public static fun create (Ljava/io/FileOutputStream;Ljava/io/FileDescriptor;)Ljava/io/FileOutputStream; public static fun create (Ljava/io/FileOutputStream;Ljava/lang/String;)Ljava/io/FileOutputStream; diff --git a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java index ea7d7f09a54..0ee5df6d799 100644 --- a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java +++ b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java @@ -3,6 +3,7 @@ import io.sentry.IScopes; import io.sentry.ISpan; import io.sentry.ScopesAdapter; +import io.sentry.SentryOptions; import java.io.File; import java.io.FileDescriptor; import java.io.FileInputStream; @@ -127,20 +128,27 @@ public static final class Factory { public static FileInputStream create( final @NotNull FileInputStream delegate, final @Nullable String name) throws FileNotFoundException { - return new SentryFileInputStream( - init(name != null ? new File(name) : null, delegate, ScopesAdapter.getInstance())); + final @NotNull IScopes scopes = ScopesAdapter.getInstance(); + return isTracingEnabled(scopes) + ? new SentryFileInputStream(init(name != null ? new File(name) : null, delegate, scopes)) + : delegate; } public static FileInputStream create( final @NotNull FileInputStream delegate, final @Nullable File file) throws FileNotFoundException { - return new SentryFileInputStream(init(file, delegate, ScopesAdapter.getInstance())); + final @NotNull IScopes scopes = ScopesAdapter.getInstance(); + return isTracingEnabled(scopes) + ? new SentryFileInputStream(init(file, delegate, scopes)) + : delegate; } public static FileInputStream create( final @NotNull FileInputStream delegate, final @NotNull FileDescriptor descriptor) { - return new SentryFileInputStream( - init(descriptor, delegate, ScopesAdapter.getInstance()), descriptor); + final @NotNull IScopes scopes = ScopesAdapter.getInstance(); + return isTracingEnabled(scopes) + ? new SentryFileInputStream(init(descriptor, delegate, scopes), descriptor) + : delegate; } static FileInputStream create( @@ -148,7 +156,14 @@ static FileInputStream create( final @Nullable File file, final @NotNull IScopes scopes) throws FileNotFoundException { - return new SentryFileInputStream(init(file, delegate, scopes)); + return isTracingEnabled(scopes) + ? new SentryFileInputStream(init(file, delegate, scopes)) + : delegate; + } + + private static boolean isTracingEnabled(final @NotNull IScopes scopes) { + final @NotNull SentryOptions options = scopes.getOptions(); + return options.isTracingEnabled(); } } } diff --git a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java index 4ef5022e1c9..21694308ba6 100644 --- a/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java +++ b/sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java @@ -3,6 +3,7 @@ import io.sentry.IScopes; import io.sentry.ISpan; import io.sentry.ScopesAdapter; +import io.sentry.SentryOptions; import java.io.File; import java.io.FileDescriptor; import java.io.FileNotFoundException; @@ -134,33 +135,62 @@ public static final class Factory { public static FileOutputStream create( final @NotNull FileOutputStream delegate, final @Nullable String name) throws FileNotFoundException { - return new SentryFileOutputStream( - init(name != null ? new File(name) : null, false, delegate, ScopesAdapter.getInstance())); + final @NotNull IScopes scopes = ScopesAdapter.getInstance(); + return isTracingEnabled(scopes) + ? new SentryFileOutputStream( + init(name != null ? new File(name) : null, false, delegate, scopes)) + : delegate; } public static FileOutputStream create( final @NotNull FileOutputStream delegate, final @Nullable String name, final boolean append) throws FileNotFoundException { - return new SentryFileOutputStream( - init( - name != null ? new File(name) : null, append, delegate, ScopesAdapter.getInstance())); + final @NotNull IScopes scopes = ScopesAdapter.getInstance(); + return isTracingEnabled(scopes) + ? new SentryFileOutputStream( + init(name != null ? new File(name) : null, append, delegate, scopes)) + : delegate; } public static FileOutputStream create( final @NotNull FileOutputStream delegate, final @Nullable File file) throws FileNotFoundException { - return new SentryFileOutputStream(init(file, false, delegate, ScopesAdapter.getInstance())); + final @NotNull IScopes scopes = ScopesAdapter.getInstance(); + return isTracingEnabled(scopes) + ? new SentryFileOutputStream(init(file, false, delegate, scopes)) + : delegate; } public static FileOutputStream create( final @NotNull FileOutputStream delegate, final @Nullable File file, final boolean append) throws FileNotFoundException { - return new SentryFileOutputStream(init(file, append, delegate, ScopesAdapter.getInstance())); + final @NotNull IScopes scopes = ScopesAdapter.getInstance(); + return isTracingEnabled(scopes) + ? new SentryFileOutputStream(init(file, append, delegate, scopes)) + : delegate; } public static FileOutputStream create( final @NotNull FileOutputStream delegate, final @NotNull FileDescriptor fdObj) { - return new SentryFileOutputStream(init(fdObj, delegate, ScopesAdapter.getInstance()), fdObj); + final @NotNull IScopes scopes = ScopesAdapter.getInstance(); + return isTracingEnabled(scopes) + ? new SentryFileOutputStream(init(fdObj, delegate, scopes), fdObj) + : delegate; + } + + public static FileOutputStream create( + final @NotNull FileOutputStream delegate, + final @Nullable File file, + final @NotNull IScopes scopes) + throws FileNotFoundException { + return isTracingEnabled(scopes) + ? new SentryFileOutputStream(init(file, false, delegate, scopes)) + : delegate; + } + + private static boolean isTracingEnabled(final @NotNull IScopes scopes) { + final @NotNull SentryOptions options = scopes.getOptions(); + return options.isTracingEnabled(); } } } diff --git a/sentry/src/main/java/io/sentry/internal/gestures/GestureTargetLocator.java b/sentry/src/main/java/io/sentry/internal/gestures/GestureTargetLocator.java index 79109f70ff6..3fbfa1ab980 100644 --- a/sentry/src/main/java/io/sentry/internal/gestures/GestureTargetLocator.java +++ b/sentry/src/main/java/io/sentry/internal/gestures/GestureTargetLocator.java @@ -1,11 +1,10 @@ package io.sentry.internal.gestures; -import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; public interface GestureTargetLocator { @Nullable UiElement locate( - final @NotNull Object root, final float x, final float y, final UiElement.Type targetType); + final @Nullable Object root, final float x, final float y, final UiElement.Type targetType); } diff --git a/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileInputStreamTest.kt b/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileInputStreamTest.kt index def4e8c5559..70d9bef24a4 100644 --- a/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileInputStreamTest.kt +++ b/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileInputStreamTest.kt @@ -61,8 +61,10 @@ class SentryFileInputStreamTest { internal fun getSut( tmpFile: File? = null, - delegate: FileInputStream - ): SentryFileInputStream { + delegate: FileInputStream, + tracesSampleRate: Double? = 1.0 + ): FileInputStream { + options.tracesSampleRate = tracesSampleRate whenever(scopes.options).thenReturn(options) sentryTracer = SentryTracer(TransactionContext("name", "op"), scopes) whenever(scopes.span).thenReturn(sentryTracer) @@ -70,7 +72,7 @@ class SentryFileInputStreamTest { delegate, tmpFile, scopes - ) as SentryFileInputStream + ) } } @@ -274,6 +276,15 @@ class SentryFileInputStreamTest { assertEquals(false, fileIOSpan.data[SpanDataConvention.BLOCKED_MAIN_THREAD_KEY]) assertNull(fileIOSpan.data[SpanDataConvention.CALL_STACK_KEY]) } + + @Test + fun `when tracing is disabled does not instrument the stream`() { + val file = tmpFile + val delegate = ThrowingFileInputStream(file) + val stream = fixture.getSut(file, delegate = delegate, tracesSampleRate = null) + + assertTrue { stream is ThrowingFileInputStream } + } } class ThrowingFileInputStream(file: File) : FileInputStream(file) { diff --git a/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileOutputStreamTest.kt b/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileOutputStreamTest.kt index f1826e5544b..fd70b36c71a 100644 --- a/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileOutputStreamTest.kt +++ b/sentry/src/test/java/io/sentry/instrumentation/file/SentryFileOutputStreamTest.kt @@ -14,6 +14,8 @@ import org.junit.rules.TemporaryFolder import org.mockito.kotlin.mock import org.mockito.kotlin.whenever import java.io.File +import java.io.FileOutputStream +import java.io.IOException import java.util.concurrent.atomic.AtomicBoolean import kotlin.concurrent.thread import kotlin.test.Test @@ -26,6 +28,7 @@ import kotlin.test.assertTrue class SentryFileOutputStreamTest { class Fixture { val scopes = mock() + val options = SentryOptions() lateinit var sentryTracer: SentryTracer internal fun getSut( @@ -34,7 +37,7 @@ class SentryFileOutputStreamTest { append: Boolean = false, optionsConfiguration: (SentryOptions) -> Unit = {} ): SentryFileOutputStream { - val options = SentryOptions().apply { + options.run { threadChecker = ThreadChecker.getInstance() addInAppInclude("org.junit") optionsConfiguration(this) @@ -46,6 +49,22 @@ class SentryFileOutputStreamTest { } return SentryFileOutputStream(tmpFile, append, scopes) } + + internal fun getSut( + tmpFile: File? = null, + delegate: FileOutputStream, + tracesSampleRate: Double? = 1.0 + ): FileOutputStream { + options.tracesSampleRate = tracesSampleRate + whenever(scopes.options).thenReturn(options) + sentryTracer = SentryTracer(TransactionContext("name", "op"), scopes) + whenever(scopes.span).thenReturn(sentryTracer) + return SentryFileOutputStream.Factory.create( + delegate, + tmpFile, + scopes + ) + } } @get:Rule @@ -197,4 +216,25 @@ class SentryFileOutputStreamTest { assertEquals(false, fileIOSpan.data[SpanDataConvention.BLOCKED_MAIN_THREAD_KEY]) assertNull(fileIOSpan.data[SpanDataConvention.CALL_STACK_KEY]) } + + @Test + fun `when tracing is disabled does not instrument the stream`() { + val file = tmpFile + val delegate = ThrowingFileOutputStream(file) + val stream = fixture.getSut(file, delegate = delegate, tracesSampleRate = null) + + assertTrue { stream is ThrowingFileOutputStream } + } +} + +class ThrowingFileOutputStream(file: File) : FileOutputStream(file) { + val throwable = IOException("Oops!") + + override fun write(b: Int) { + throw throwable + } + + override fun close() { + throw throwable + } }