Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various fixes to instrumentations running on the main thread #4039

Closed
wants to merge 11 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ 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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -40,7 +39,7 @@ 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 (view instanceof ViewGroup) {
final ViewGroup viewGroup = (ViewGroup) view;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ class UserInteractionIntegrationTest {
)
)

sut.register(fixture.hub, fixture.options)
sut.onActivityPaused(fixture.activity)

verify(fixture.window).callback = null
Expand All @@ -160,6 +161,7 @@ class UserInteractionIntegrationTest {
)
)

sut.register(fixture.hub, fixture.options)
sut.onActivityPaused(fixture.activity)

verify(fixture.window).callback = delegate
Expand All @@ -170,8 +172,30 @@ class UserInteractionIntegrationTest {
val callback = mock<SentryWindowCallback>()
val sut = fixture.getSut(callback)

sut.register(fixture.hub, fixture.options)
sut.onActivityPaused(fixture.activity)

verify(callback).stopTracking()
}

@Test
fun `does not instrument if the callback is already ours`() {
val delegate = mock<Window.Callback>()
val context = mock<Context>()
val resources = Fixture.mockResources()
whenever(context.resources).thenReturn(resources)
val existingCallback = SentryWindowCallback(
delegate,
context,
mock(),
mock()
)
val sut = fixture.getSut(existingCallback)

sut.register(fixture.hub, fixture.options)
sut.onActivityResumed(fixture.activity)

val argumentCaptor = argumentCaptor<Window.Callback>()
verify(fixture.window, never()).callback = argumentCaptor.capture()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -127,28 +128,42 @@ 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(
final @NotNull FileInputStream delegate,
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();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -134,33 +135,52 @@ 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;
}

private static boolean isTracingEnabled(final @NotNull IScopes scopes) {
final @NotNull SentryOptions options = scopes.getOptions();
return options.isTracingEnabled();
}
}
}
Original file line number Diff line number Diff line change
@@ -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);
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,16 +61,18 @@ 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)
return SentryFileInputStream.Factory.create(
delegate,
tmpFile,
scopes
) as SentryFileInputStream
)
}
}

Expand Down Expand Up @@ -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`() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add the same test in SentryFileOutputStreamTest?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I can add one!

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) {
Expand Down