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

Tracing without Performance #2788

Merged
merged 30 commits into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
fdf5953
TwP
adinauer Jun 14, 2023
6eba5bc
fix tests
adinauer Jun 15, 2023
537b3fb
also add headers for okhttp if no span is active; add tests; fix exis…
adinauer Jun 19, 2023
686eb4e
Change trace and traceIfAllowed to return instead of taking a callback
adinauer Jun 19, 2023
62a3b39
Merge branch 'main' into feat/tracing-without-performance
adinauer Jun 19, 2023
4e850af
Start new trace in ActivityLifecycleIntegratin
adinauer Jun 19, 2023
15ad792
Also start a new trace in SentryNavigationListener
adinauer Jun 19, 2023
3cb799c
Add tests for continueTrace in spring filters
adinauer Jun 20, 2023
5feaac9
Add changelog
adinauer Jun 20, 2023
2bf3e3e
Merge branch 'main' into feat/tracing-without-performance
adinauer Jun 20, 2023
1b1f062
Add test for TracingUtils
adinauer Jun 20, 2023
66e4bdb
More tests
adinauer Jun 20, 2023
b6bfe7e
Switch from AtomicReference to holder class
adinauer Jun 22, 2023
104ac37
Format code
getsentry-bot Jun 22, 2023
2266d58
Changes according to reviews
adinauer Jun 22, 2023
13a6a38
Merge branch 'main' into feat/tracing-without-performance
adinauer Jun 22, 2023
0bb6886
Start new trace in SentryGestureListener
adinauer Jun 22, 2023
49b1699
More tests
adinauer Jun 23, 2023
e7e50bd
Synchronize on scope for trace creation and usage from scope
adinauer Jun 26, 2023
4e5f6aa
Add withPropagationContext to Scope and synchronize using a private lock
adinauer Jun 26, 2023
1ec1b5d
Default to false for sampled decision of incoming trace without expli…
adinauer Jun 26, 2023
ad3d2a4
Extra variable
adinauer Jun 26, 2023
7db620b
Apply suggestions from code review
adinauer Jun 27, 2023
38956f1
Mark PropagationContext internal
adinauer Jun 27, 2023
ceb2132
Merge branch 'main' into feat/tracing-without-performance
adinauer Jun 29, 2023
ef08924
move changelog
adinauer Jun 29, 2023
747ae3b
Update sentry-android-core/src/main/java/io/sentry/android/core/Activ…
adinauer Jun 29, 2023
2eb2140
Format code
getsentry-bot Jun 29, 2023
23dcd1a
refrain from starting a new trace in some cases
adinauer Jun 29, 2023
a360064
Apply suggestions from code review
adinauer Jun 30, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Features

- Add manifest `AutoInit` to integrations list ([#2795](https://github.com/getsentry/sentry-java/pull/2795))
- Tracing headers (`sentry-trace` and `baggage`) are now attached and passed through even if performance is disabled ([#2788](https://github.com/getsentry/sentry-java/pull/2788))

### Dependencies

Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import io.sentry.android.core.SentryAndroidOptions;
import io.sentry.internal.gestures.UiElement;
import io.sentry.protocol.TransactionNameSource;
import io.sentry.util.TracingUtils;
import java.lang.ref.WeakReference;
import java.util.Collections;
import java.util.Map;
Expand Down Expand Up @@ -185,7 +186,13 @@ private void addBreadcrumb(
}

private void startTracing(final @NotNull UiElement target, final @NotNull String eventType) {
final UiElement uiElement = activeUiElement;
if (!(options.isTracingEnabled() && options.isEnableUserInteractionTracing())) {
if (!(target.equals(uiElement) && eventType.equals(activeEventType))) {
TracingUtils.startNewTrace(hub);
activeUiElement = target;
activeEventType = eventType;
}
return;
}

Expand All @@ -196,7 +203,6 @@ private void startTracing(final @NotNull UiElement target, final @NotNull String
}

final @Nullable String viewIdentifier = target.getIdentifier();
final UiElement uiElement = activeUiElement;

if (activeTransaction != null) {
if (target.equals(uiElement)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import io.sentry.FullyDisplayedReporter
import io.sentry.Hub
import io.sentry.ISentryExecutorService
import io.sentry.Scope
import io.sentry.ScopeCallback
import io.sentry.Sentry
import io.sentry.SentryDate
import io.sentry.SentryLevel
Expand All @@ -32,10 +33,12 @@ import io.sentry.protocol.MeasurementValue
import io.sentry.protocol.TransactionNameSource
import io.sentry.test.getProperty
import org.junit.runner.RunWith
import org.mockito.ArgumentCaptor
import org.mockito.kotlin.any
import org.mockito.kotlin.anyOrNull
import org.mockito.kotlin.argumentCaptor
import org.mockito.kotlin.check
import org.mockito.kotlin.clearInvocations
import org.mockito.kotlin.eq
import org.mockito.kotlin.mock
import org.mockito.kotlin.never
Expand All @@ -54,6 +57,7 @@ import kotlin.test.assertEquals
import kotlin.test.assertFalse
import kotlin.test.assertNotEquals
import kotlin.test.assertNotNull
import kotlin.test.assertNotSame
import kotlin.test.assertNull
import kotlin.test.assertSame
import kotlin.test.assertTrue
Expand Down Expand Up @@ -141,13 +145,13 @@ class ActivityLifecycleIntegrationTest {
}

@Test
fun `When activity lifecycle breadcrumb and tracing are disabled, it doesn't register callback`() {
fun `When activity lifecycle breadcrumb and tracing are disabled, it still registers callback`() {
val sut = fixture.getSut()
fixture.options.isEnableActivityLifecycleBreadcrumbs = false

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

verify(fixture.application, never()).registerActivityLifecycleCallbacks(any())
verify(fixture.application).registerActivityLifecycleCallbacks(any())
}

@Test
Expand All @@ -162,15 +166,15 @@ class ActivityLifecycleIntegrationTest {
}

@Test
fun `When activity lifecycle breadcrumb is disabled and tracesSampleRate is set but tracing is disabled, it does not register callback`() {
fun `When activity lifecycle breadcrumb is disabled and tracesSampleRate is set but tracing is disabled, it still registers callback`() {
val sut = fixture.getSut()
fixture.options.isEnableActivityLifecycleBreadcrumbs = false
fixture.options.tracesSampleRate = 1.0
fixture.options.enableTracing = false

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

verify(fixture.application, never()).registerActivityLifecycleCallbacks(any())
verify(fixture.application).registerActivityLifecycleCallbacks(any())
}

@Test
Expand All @@ -196,7 +200,7 @@ class ActivityLifecycleIntegrationTest {
}

@Test
fun `When activity lifecycle breadcrumb and tracing activity flag are disabled, it doesn't register callback`() {
fun `When activity lifecycle breadcrumb and tracing activity flag are disabled, its still registers callback`() {
val sut = fixture.getSut()
fixture.options.isEnableActivityLifecycleBreadcrumbs = false
fixture.options.tracesSampleRate = 1.0
Expand All @@ -205,7 +209,7 @@ class ActivityLifecycleIntegrationTest {

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

verify(fixture.application, never()).registerActivityLifecycleCallbacks(any())
verify(fixture.application).registerActivityLifecycleCallbacks(any())
}

@Test
Expand Down Expand Up @@ -1384,6 +1388,60 @@ class ActivityLifecycleIntegrationTest {
assertFalse(ttfdSpan2.isFinished)
}

@Test
fun `starts new trace if performance is disabled`() {
val sut = fixture.getSut()
val activity = mock<Activity>()
fixture.options.enableTracing = false

val argumentCaptor: ArgumentCaptor<ScopeCallback> = ArgumentCaptor.forClass(ScopeCallback::class.java)
val scope = Scope(fixture.options)
val propagationContextAtStart = scope.propagationContext
whenever(fixture.hub.configureScope(argumentCaptor.capture())).thenAnswer {
argumentCaptor.value.run(scope)
}

sut.register(fixture.hub, fixture.options)
sut.onActivityCreated(activity, fixture.bundle)

verify(fixture.hub).configureScope(any())
assertNotSame(propagationContextAtStart, scope.propagationContext)
}

@Test
fun `does not start another new trace if one has already been started but does after activity was destroyed`() {
val sut = fixture.getSut()
val activity = mock<Activity>()
fixture.options.enableTracing = false

val argumentCaptor: ArgumentCaptor<ScopeCallback> = ArgumentCaptor.forClass(ScopeCallback::class.java)
val scope = Scope(fixture.options)
val propagationContextAtStart = scope.propagationContext
whenever(fixture.hub.configureScope(argumentCaptor.capture())).thenAnswer {
argumentCaptor.value.run(scope)
}

sut.register(fixture.hub, fixture.options)
sut.onActivityCreated(activity, fixture.bundle)

verify(fixture.hub).configureScope(any())
val propagationContextAfterNewTrace = scope.propagationContext
assertNotSame(propagationContextAtStart, propagationContextAfterNewTrace)

clearInvocations(fixture.hub)
sut.onActivityCreated(activity, fixture.bundle)

verify(fixture.hub, never()).configureScope(any())
assertSame(propagationContextAfterNewTrace, scope.propagationContext)

sut.onActivityDestroyed(activity)

clearInvocations(fixture.hub)
sut.onActivityCreated(activity, fixture.bundle)
verify(fixture.hub).configureScope(any())
assertNotSame(propagationContextAfterNewTrace, scope.propagationContext)
}

private fun runFirstDraw(view: View) {
// Removes OnDrawListener in the next OnGlobalLayout after onDraw
view.viewTreeObserver.dispatchOnDraw()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,16 @@ import android.widget.CheckBox
import android.widget.RadioButton
import io.sentry.Breadcrumb
import io.sentry.IHub
import io.sentry.PropagationContext
import io.sentry.Scope
import io.sentry.Scope.IWithPropagationContext
import io.sentry.ScopeCallback
import io.sentry.SentryLevel.INFO
import io.sentry.android.core.SentryAndroidOptions
import org.mockito.kotlin.any
import org.mockito.kotlin.anyOrNull
import org.mockito.kotlin.check
import org.mockito.kotlin.doAnswer
import org.mockito.kotlin.mock
import org.mockito.kotlin.never
import org.mockito.kotlin.verify
Expand All @@ -36,6 +41,8 @@ class SentryGestureListenerClickTest {
dsn = "https://key@sentry.io/proj"
}
val hub = mock<IHub>()
val scope = mock<Scope>()
val propagationContext = PropagationContext()
lateinit var target: View
lateinit var invalidTarget: View

Expand Down Expand Up @@ -79,6 +86,8 @@ class SentryGestureListenerClickTest {
whenever(context.resources).thenReturn(resources)
whenever(this.target.context).thenReturn(context)
whenever(activity.window).thenReturn(window)
doAnswer { (it.arguments[0] as ScopeCallback).run(scope) }.whenever(hub).configureScope(any())
doAnswer { (it.arguments[0] as IWithPropagationContext).accept(propagationContext); propagationContext; }.whenever(scope).withPropagationContext(any())
return SentryGestureListener(
activity,
hub,
Expand Down Expand Up @@ -228,4 +237,20 @@ class SentryGestureListenerClickTest {
anyOrNull()
)
}

@Test
fun `creates new trace on click`() {
class LocalView(context: Context) : View(context)

val event = mock<MotionEvent>()
val sut = fixture.getSut<LocalView>(event, attachViewsToRoot = false)
fixture.window.mockDecorView<ViewGroup>(event = event, touchWithinBounds = false) {
whenever(it.childCount).thenReturn(1)
whenever(it.getChildAt(0)).thenReturn(fixture.target)
}

sut.onSingleTapUp(event)

verify(fixture.scope).propagationContext = any()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,20 @@ import android.widget.ListAdapter
import androidx.core.view.ScrollingView
import io.sentry.Breadcrumb
import io.sentry.IHub
import io.sentry.PropagationContext
import io.sentry.Scope
import io.sentry.ScopeCallback
import io.sentry.SentryLevel.INFO
import io.sentry.android.core.SentryAndroidOptions
import org.mockito.kotlin.any
import org.mockito.kotlin.anyOrNull
import org.mockito.kotlin.check
import org.mockito.kotlin.clearInvocations
import org.mockito.kotlin.doAnswer
import org.mockito.kotlin.inOrder
import org.mockito.kotlin.mock
import org.mockito.kotlin.never
import org.mockito.kotlin.times
import org.mockito.kotlin.verify
import org.mockito.kotlin.verifyNoMoreInteractions
import org.mockito.kotlin.whenever
Expand All @@ -39,6 +45,8 @@ class SentryGestureListenerScrollTest {
gestureTargetLocators = listOf(AndroidViewGestureTargetLocator(true))
}
val hub = mock<IHub>()
val scope = mock<Scope>()
val propagationContext = PropagationContext()

val firstEvent = mock<MotionEvent>()
val eventsInBetween = listOf(mock<MotionEvent>(), mock(), mock())
Expand Down Expand Up @@ -69,6 +77,8 @@ class SentryGestureListenerScrollTest {
endEvent.mockDirection(firstEvent, direction)
}
whenever(activity.window).thenReturn(window)
doAnswer { (it.arguments[0] as ScopeCallback).run(scope) }.whenever(hub).configureScope(any())
doAnswer { (it.arguments[0] as Scope.IWithPropagationContext).accept(propagationContext); propagationContext }.whenever(scope).withPropagationContext(any())
return SentryGestureListener(
activity,
hub,
Expand Down Expand Up @@ -145,6 +155,7 @@ class SentryGestureListenerScrollTest {
},
anyOrNull()
)
verify(fixture.hub).configureScope(anyOrNull())
verify(fixture.hub).addBreadcrumb(
check<Breadcrumb> {
assertEquals("ui.swipe", it.category)
Expand Down Expand Up @@ -182,6 +193,50 @@ class SentryGestureListenerScrollTest {
verify(fixture.hub, never()).addBreadcrumb(any<Breadcrumb>())
}

@Test
fun `starts a new trace on scroll`() {
val sut = fixture.getSut<ScrollableListView>(direction = "left")

sut.onDown(fixture.firstEvent)
fixture.eventsInBetween.forEach {
sut.onScroll(fixture.firstEvent, it, 10.0f, 0f)
}
sut.onUp(fixture.endEvent)

verify(fixture.scope).propagationContext = any()
}

@Test
fun `does not start a new trace on repeated scroll but does for a different event`() {
val sut = fixture.getSut<ScrollableListView>(direction = "left")

sut.onDown(fixture.firstEvent)
fixture.eventsInBetween.forEach {
sut.onScroll(fixture.firstEvent, it, 10.0f, 0f)
}
sut.onUp(fixture.endEvent)

verify(fixture.scope).propagationContext = any()

clearInvocations(fixture.scope)

sut.onDown(fixture.firstEvent)
fixture.eventsInBetween.forEach {
sut.onScroll(fixture.firstEvent, it, 10.0f, 0f)
}
sut.onUp(fixture.endEvent)
verify(fixture.scope, never()).propagationContext = any()

clearInvocations(fixture.scope)

sut.onDown(fixture.firstEvent)
fixture.eventsInBetween.forEach { sut.onScroll(fixture.firstEvent, it, 0f, 30.0f) }
sut.onFling(fixture.firstEvent, fixture.endEvent, 1.0f, 1.0f)
sut.onUp(fixture.endEvent)

verify(fixture.scope).propagationContext = any()
}

internal class ScrollableView : View(mock()), ScrollingView {
override fun computeVerticalScrollOffset(): Int = 0
override fun computeVerticalScrollExtent(): Int = 0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import android.widget.AbsListView
import android.widget.ListAdapter
import io.sentry.IHub
import io.sentry.Scope
import io.sentry.ScopeCallback
import io.sentry.SentryTracer
import io.sentry.SpanStatus
import io.sentry.TransactionContext
Expand All @@ -20,6 +21,7 @@ import io.sentry.protocol.TransactionNameSource
import org.mockito.kotlin.any
import org.mockito.kotlin.check
import org.mockito.kotlin.clearInvocations
import org.mockito.kotlin.doAnswer
import org.mockito.kotlin.mock
import org.mockito.kotlin.never
import org.mockito.kotlin.verify
Expand All @@ -40,6 +42,7 @@ class SentryGestureListenerTracingTest {
}
val hub = mock<IHub>()
val event = mock<MotionEvent>()
val scope = mock<Scope>()
lateinit var target: View
lateinit var transaction: SentryTracer

Expand Down Expand Up @@ -79,6 +82,7 @@ class SentryGestureListenerTracingTest {

whenever(hub.startTransaction(any(), any<TransactionOptions>()))
.thenReturn(this.transaction)
doAnswer { (it.arguments[0] as ScopeCallback).run(scope) }.whenever(hub).configureScope(any())

return SentryGestureListener(
activity,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import io.sentry.TransactionContext
import io.sentry.TransactionOptions
import io.sentry.TypeCheckHint
import io.sentry.protocol.TransactionNameSource
import io.sentry.util.TracingUtils
import java.lang.ref.WeakReference

/**
Expand Down Expand Up @@ -96,6 +97,7 @@ class SentryNavigationListener @JvmOverloads constructor(
arguments: Map<String, Any?>
) {
if (!isPerformanceEnabled) {
TracingUtils.startNewTrace(hub)
return
}

Expand Down
Loading