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 10 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.
Jump to
Jump to file
Failed to load files.
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 debouncing mechanism and before-capture callbacks for screenshots and view hierarchies ([#2773](https://github.com/getsentry/sentry-java/pull/2773))
- 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))
adinauer marked this conversation as resolved.
Show resolved Hide resolved

## 6.23.0

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import io.sentry.protocol.MeasurementValue;
import io.sentry.protocol.TransactionNameSource;
import io.sentry.util.Objects;
import io.sentry.util.TracingUtils;
import java.io.Closeable;
import java.io.IOException;
import java.lang.ref.WeakReference;
Expand Down Expand Up @@ -122,11 +123,9 @@ public void register(final @NotNull IHub hub, final @NotNull SentryOptions optio
fullyDisplayedReporter = this.options.getFullyDisplayedReporter();
timeToFullDisplaySpanEnabled = this.options.isEnableTimeToFullDisplayTracing();

if (this.options.isEnableActivityLifecycleBreadcrumbs() || performanceEnabled) {
application.registerActivityLifecycleCallbacks(this);
this.options.getLogger().log(SentryLevel.DEBUG, "ActivityLifecycleIntegration installed.");
addIntegrationToSdkVersion();
}
application.registerActivityLifecycleCallbacks(this);
this.options.getLogger().log(SentryLevel.DEBUG, "ActivityLifecycleIntegration installed.");
addIntegrationToSdkVersion();
}

private boolean isPerformanceEnabled(final @NotNull SentryAndroidOptions options) {
Expand Down Expand Up @@ -176,7 +175,9 @@ private void stopPreviousTransactions() {

private void startTracing(final @NotNull Activity activity) {
WeakReference<Activity> weakActivity = new WeakReference<>(activity);
if (performanceEnabled && !isRunningTransaction(activity) && hub != null) {
if (!performanceEnabled && hub != null) {
romtsn marked this conversation as resolved.
Show resolved Hide resolved
TracingUtils.startNewTrace(hub);
} else if (performanceEnabled && !isRunningTransaction(activity) && hub != null) {
// as we allow a single transaction running on the bound Scope, we finish the previous ones
stopPreviousTransactions();

Expand Down Expand Up @@ -367,44 +368,48 @@ public synchronized void onActivityCreated(

@Override
public synchronized void onActivityStarted(final @NotNull Activity activity) {
// The docs on the screen rendering performance tracing
// (https://firebase.google.com/docs/perf-mon/screen-traces?platform=android#definition),
// state that the tracing starts for every Activity class when the app calls .onActivityStarted.
// Adding an Activity in onActivityCreated leads to Window.FEATURE_NO_TITLE not
// working. Moving this to onActivityStarted fixes the problem.
activityFramesTracker.addActivity(activity);
if (performanceEnabled || options.isEnableActivityLifecycleBreadcrumbs()) {
// The docs on the screen rendering performance tracing
// (https://firebase.google.com/docs/perf-mon/screen-traces?platform=android#definition),
// state that the tracing starts for every Activity class when the app calls
// .onActivityStarted.
// Adding an Activity in onActivityCreated leads to Window.FEATURE_NO_TITLE not
// working. Moving this to onActivityStarted fixes the problem.
activityFramesTracker.addActivity(activity);
}

addBreadcrumb(activity, "started");
adinauer marked this conversation as resolved.
Show resolved Hide resolved
}

@SuppressLint("NewApi")
@Override
public synchronized void onActivityResumed(final @NotNull Activity activity) {

// app start span
@Nullable final SentryDate appStartStartTime = AppStartState.getInstance().getAppStartTime();
@Nullable final SentryDate appStartEndTime = AppStartState.getInstance().getAppStartEndTime();
// in case the SentryPerformanceProvider is disabled it does not set the app start times,
// and we need to set the end time manually here,
// the start time gets set manually in SentryAndroid.init()
if (appStartStartTime != null && appStartEndTime == null) {
AppStartState.getInstance().setAppStartEnd();
}
finishAppStartSpan();

final @Nullable ISpan ttidSpan = ttidSpanMap.get(activity);
final @Nullable ISpan ttfdSpan = ttfdSpanMap.get(activity);
final View rootView = activity.findViewById(android.R.id.content);
if (buildInfoProvider.getSdkInfoVersion() >= Build.VERSION_CODES.JELLY_BEAN
&& rootView != null) {
FirstDrawDoneListener.registerForNextDraw(
rootView, () -> onFirstFrameDrawn(ttfdSpan, ttidSpan), buildInfoProvider);
} else {
// Posting a task to the main thread's handler will make it executed after it finished
// its current job. That is, right after the activity draws the layout.
mainHandler.post(() -> onFirstFrameDrawn(ttfdSpan, ttidSpan));
if (performanceEnabled || options.isEnableActivityLifecycleBreadcrumbs()) {
adinauer marked this conversation as resolved.
Show resolved Hide resolved
// app start span
@Nullable final SentryDate appStartStartTime = AppStartState.getInstance().getAppStartTime();
@Nullable final SentryDate appStartEndTime = AppStartState.getInstance().getAppStartEndTime();
// in case the SentryPerformanceProvider is disabled it does not set the app start times,
// and we need to set the end time manually here,
// the start time gets set manually in SentryAndroid.init()
if (appStartStartTime != null && appStartEndTime == null) {
AppStartState.getInstance().setAppStartEnd();
}
finishAppStartSpan();

final @Nullable ISpan ttidSpan = ttidSpanMap.get(activity);
final @Nullable ISpan ttfdSpan = ttfdSpanMap.get(activity);
final View rootView = activity.findViewById(android.R.id.content);
if (buildInfoProvider.getSdkInfoVersion() >= Build.VERSION_CODES.JELLY_BEAN
&& rootView != null) {
FirstDrawDoneListener.registerForNextDraw(
rootView, () -> onFirstFrameDrawn(ttfdSpan, ttidSpan), buildInfoProvider);
} else {
// Posting a task to the main thread's handler will make it executed after it finished
// its current job. That is, right after the activity draws the layout.
mainHandler.post(() -> onFirstFrameDrawn(ttfdSpan, ttidSpan));
}
addBreadcrumb(activity, "resumed");
adinauer marked this conversation as resolved.
Show resolved Hide resolved
}
adinauer marked this conversation as resolved.
Show resolved Hide resolved
addBreadcrumb(activity, "resumed");
}

@Override
Expand Down Expand Up @@ -450,35 +455,38 @@ public synchronized void onActivitySaveInstanceState(

@Override
public synchronized void onActivityDestroyed(final @NotNull Activity activity) {
addBreadcrumb(activity, "destroyed");

// in case the appStartSpan isn't completed yet, we finish it as cancelled to avoid
// memory leak
finishSpan(appStartSpan, SpanStatus.CANCELLED);
if (performanceEnabled || options.isEnableActivityLifecycleBreadcrumbs()) {
addBreadcrumb(activity, "destroyed");

// we finish the ttidSpan as cancelled in case it isn't completed yet
final ISpan ttidSpan = ttidSpanMap.get(activity);
final ISpan ttfdSpan = ttfdSpanMap.get(activity);
finishSpan(ttidSpan, SpanStatus.DEADLINE_EXCEEDED);

// we finish the ttfdSpan as deadline_exceeded in case it isn't completed yet
finishExceededTtfdSpan(ttfdSpan, ttidSpan);
cancelTtfdAutoClose();
// in case the appStartSpan isn't completed yet, we finish it as cancelled to avoid
// memory leak
finishSpan(appStartSpan, SpanStatus.CANCELLED);

// in case people opt-out enableActivityLifecycleTracingAutoFinish and forgot to finish it,
// we make sure to finish it when the activity gets destroyed.
stopTracing(activity, true);
// we finish the ttidSpan as cancelled in case it isn't completed yet
final ISpan ttidSpan = ttidSpanMap.get(activity);
final ISpan ttfdSpan = ttfdSpanMap.get(activity);
finishSpan(ttidSpan, SpanStatus.DEADLINE_EXCEEDED);

// set it to null in case its been just finished as cancelled
appStartSpan = null;
ttidSpanMap.remove(activity);
ttfdSpanMap.remove(activity);
// we finish the ttfdSpan as deadline_exceeded in case it isn't completed yet
finishExceededTtfdSpan(ttfdSpan, ttidSpan);
cancelTtfdAutoClose();

// clear it up, so we don't start again for the same activity if the activity is in the activity
// stack still.
// if the activity is opened again and not in memory, transactions will be created normally.
if (performanceEnabled) {
activitiesWithOngoingTransactions.remove(activity);
// in case people opt-out enableActivityLifecycleTracingAutoFinish and forgot to finish it,
// we make sure to finish it when the activity gets destroyed.
stopTracing(activity, true);

// set it to null in case its been just finished as cancelled
appStartSpan = null;
ttidSpanMap.remove(activity);
ttfdSpanMap.remove(activity);

// clear it up, so we don't start again for the same activity if the activity is in the
// activity
// stack still.
// if the activity is opened again and not in memory, transactions will be created normally.
if (performanceEnabled) {
activitiesWithOngoingTransactions.remove(activity);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ private void addBreadcrumb(
hint);
}

// TODO should we start a new trace here if performance is disabled?
adinauer marked this conversation as resolved.
Show resolved Hide resolved
private void startTracing(final @NotNull UiElement target, final @NotNull String eventType) {
if (!(options.isTracingEnabled() && options.isEnableUserInteractionTracing())) {
return;
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,6 +33,7 @@ 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
Expand All @@ -54,6 +56,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 +144,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 +165,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 +199,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 +208,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 +1387,26 @@ 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)
}

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 @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import io.sentry.TransactionContext
import io.sentry.TransactionOptions
import io.sentry.protocol.TransactionNameSource
import org.junit.runner.RunWith
import org.mockito.ArgumentCaptor
import org.mockito.kotlin.any
import org.mockito.kotlin.argumentCaptor
import org.mockito.kotlin.check
Expand All @@ -29,6 +30,7 @@ import org.mockito.kotlin.whenever
import org.robolectric.annotation.Config
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertNotSame
import kotlin.test.assertNull

@RunWith(AndroidJUnit4::class)
Expand All @@ -43,6 +45,7 @@ class SentryNavigationListenerTest {
val context = mock<Context>()
val resources = mock<Resources>()
val scope = mock<Scope>()
lateinit var options: SentryOptions

lateinit var transaction: SentryTracer

Expand All @@ -56,14 +59,13 @@ class SentryNavigationListenerTest {
hasViewIdInRes: Boolean = true,
transaction: SentryTracer? = null
): SentryNavigationListener {
whenever(hub.options).thenReturn(
SentryOptions().apply {
dsn = "http://key@localhost/proj"
setTracesSampleRate(
tracesSampleRate
)
}
)
options = SentryOptions().apply {
dsn = "http://key@localhost/proj"
setTracesSampleRate(
tracesSampleRate
)
}
whenever(hub.options).thenReturn(options)

this.transaction = transaction ?: SentryTracer(
TransactionContext(
Expand Down Expand Up @@ -347,4 +349,21 @@ class SentryNavigationListenerTest {
captor.thirdValue.accept(fixture.transaction)
verify(fixture.scope).clearTransaction()
}

@Test
fun `starts new trace if performance is disabled`() {
val sut = fixture.getSut(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.onDestinationChanged(fixture.navController, fixture.destination, null)

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