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.

Sign up for GitHub

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

Feat: Perf. for fragments #1528

Merged
merged 35 commits into from
Jun 28, 2021
Merged

Feat: Perf. for fragments #1528

Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
3ce3661
Feat: Perf. for fragments
marandaneto Jun 11, 2021
2ca9823
format
marandaneto Jun 11, 2021
1c7e065
fix
marandaneto Jun 14, 2021
add0350
extra ctor
marandaneto Jun 15, 2021
bd7f30d
Support transaction waiting for children to finish.
maciejwalkowiak Jun 16, 2021
79bd7b9
Changelog.
maciejwalkowiak Jun 16, 2021
5705d0e
Expose waitForChildren with Hub and Sentry APIs.
maciejwalkowiak Jun 16, 2021
b658cdc
Merge branch 'main' into feat/perf-fragments
marandaneto Jun 16, 2021
0962c4f
fix
marandaneto Jun 16, 2021
2e4ad56
Merge remote-tracking branch 'origin/gh-1523' into feat/perf-fragments
marandaneto Jun 16, 2021
705b3de
Merge branch 'main' into feat/perf-fragments
marandaneto Jun 16, 2021
0ccbf03
Merge remote-tracking branch 'origin/gh-1523' into feat/perf-fragments
marandaneto Jun 16, 2021
7c0d868
Fix & Polish.
maciejwalkowiak Jun 16, 2021
eb2c03c
Refactor
maciejwalkowiak Jun 16, 2021
340343f
Merge remote-tracking branch 'origin/gh-1523' into feat/perf-fragments
marandaneto Jun 16, 2021
6c70853
fix
marandaneto Jun 16, 2021
4133da5
Merge branch 'main' into gh-1523
marandaneto Jun 16, 2021
e72175a
Merge remote-tracking branch 'origin/gh-1523' into feat/perf-fragments
marandaneto Jun 16, 2021
c95b0e5
test
marandaneto Jun 16, 2021
12102a0
fix
marandaneto Jun 16, 2021
687d67b
fix merge
marandaneto Jun 23, 2021
a9d6fe9
add overload
marandaneto Jun 23, 2021
e61b20f
fix tests
marandaneto Jun 23, 2021
ad9abc1
fix test
marandaneto Jun 23, 2021
60cb1c5
Merge branch 'main' into feat/perf-fragments
marandaneto Jun 24, 2021
ebae3ab
fix
marandaneto Jun 24, 2021
32cca7e
tests
marandaneto Jun 24, 2021
156c1e6
check
marandaneto Jun 24, 2021
6b7429a
changelog
marandaneto Jun 24, 2021
2db51a4
Merge branch 'main' into feat/perf-fragments
marandaneto Jun 25, 2021
ace275c
Merge branch 'main' into feat/perf-fragments
marandaneto Jun 25, 2021
7e062b7
review
marandaneto Jun 25, 2021
c064ab7
rename span op
marandaneto Jun 25, 2021
bef4872
Merge branch 'main' into feat/perf-fragments
marandaneto Jun 28, 2021
d745940
fix tests
marandaneto Jun 28, 2021
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* Fix: Add data field to SentrySpan (#1555)
* Fix: Clock drift issue when calling DateUtils#getDateTimeWithMillisPrecision (#1557)
* Feat: Set mechanism type in SentryExceptionResolver (#1556)
* Feat: Perf. for fragments (#1528)
* Ref: Prefer snake case for HTTP integration data keys (#1559)

## 5.1.0-beta.1
Expand Down
4 changes: 2 additions & 2 deletions sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,10 @@ private void startTracing(final @NonNull Activity activity) {

// in case appStartTime isn't available, we don't create a span for it.
if (firstActivityCreated || appStartTime == null) {
transaction = hub.startTransaction(activityName, UI_LOAD_OP);
transaction = hub.startTransaction(activityName, UI_LOAD_OP, (Date) null, true);
} else {
// start transaction with app start timestamp
transaction = hub.startTransaction(activityName, UI_LOAD_OP, appStartTime);
transaction = hub.startTransaction(activityName, UI_LOAD_OP, appStartTime, true);
// start specific span for app start

appStartSpan = transaction.startChild(getAppStartOp(), getAppStartDesc(), appStartTime);
Expand Down
23 changes: 12 additions & 11 deletions sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import android.app.Activity
import android.app.Application
import android.os.Bundle
import com.nhaarman.mockitokotlin2.any
import com.nhaarman.mockitokotlin2.anyOrNull
import com.nhaarman.mockitokotlin2.check
import com.nhaarman.mockitokotlin2.eq
import com.nhaarman.mockitokotlin2.mock
Expand Down Expand Up @@ -41,7 +42,7 @@ class ActivityLifecycleIntegrationTest {
fun getSut(apiVersion: Int = 29): ActivityLifecycleIntegration {
whenever(hub.startTransaction(any<String>(), any())).thenReturn(transaction)
whenever(hub.options).thenReturn(options)
whenever(hub.startTransaction(any(), any(), any<Date>())).thenReturn(transaction)
whenever(hub.startTransaction(any(), any(), anyOrNull<Date>(), any())).thenReturn(transaction)
whenever(buildInfo.sdkInfoVersion).thenReturn(apiVersion)
return ActivityLifecycleIntegration(application, buildInfo)
}
Expand Down Expand Up @@ -219,7 +220,7 @@ class ActivityLifecycleIntegrationTest {
sut.onActivityPreCreated(activity, fixture.bundle)

verify(fixture.hub, never()).startTransaction(any<String>(), any())
verify(fixture.hub, never()).startTransaction(any(), any(), any<Date>())
verify(fixture.hub, never()).startTransaction(any(), any(), any<Date>(), any())
}

@Test
Expand All @@ -233,8 +234,8 @@ class ActivityLifecycleIntegrationTest {
sut.onActivityPreCreated(activity, fixture.bundle)

// call only once
verify(fixture.hub).startTransaction(any<String>(), any())
verify(fixture.hub, never()).startTransaction(any(), any(), any<Date>())
// verify(fixture.hub).startTransaction(any<String>(), any())
verify(fixture.hub, never()).startTransaction(any(), any(), any<Date>(), any())
}

@Test
Expand All @@ -250,7 +251,7 @@ class ActivityLifecycleIntegrationTest {

verify(fixture.hub).startTransaction(any(), check {
assertEquals("ui.load", it)
}, any<Date>())
}, any<Date>(), any())
}

@Test
Expand All @@ -266,7 +267,7 @@ class ActivityLifecycleIntegrationTest {

verify(fixture.hub).startTransaction(check {
assertEquals("Activity", it)
}, any(), any<Date>())
}, any(), any<Date>(), any())
}

@Test
Expand Down Expand Up @@ -427,7 +428,7 @@ class ActivityLifecycleIntegrationTest {
sut.onActivityCreated(activity, mock())

verify(fixture.hub, never()).startTransaction(any<String>(), any())
verify(fixture.hub, never()).startTransaction(any(), any(), any<Date>())
verify(fixture.hub, never()).startTransaction(any(), any(), any<Date>(), any())
}

@Test
Expand All @@ -454,7 +455,7 @@ class ActivityLifecycleIntegrationTest {
val activity = mock<Activity>()
sut.onActivityCreated(activity, mock())

verify(fixture.hub).startTransaction(any(), any(), any<Date>())
verify(fixture.hub).startTransaction(any(), any(), any<Date>(), any())
}

@Test
Expand Down Expand Up @@ -538,7 +539,7 @@ class ActivityLifecycleIntegrationTest {
sut.onActivityPreCreated(activity, fixture.bundle)

// call only once
verify(fixture.hub).startTransaction(any(), any(), eq(date))
verify(fixture.hub).startTransaction(any(), any(), eq(date), any())
}

@Test
Expand Down Expand Up @@ -620,14 +621,14 @@ class ActivityLifecycleIntegrationTest {
val activity = mock<Activity>()
sut.onActivityPreCreated(activity, fixture.bundle)

verify(fixture.hub).startTransaction(any(), any(), any<Date>())
verify(fixture.hub).startTransaction(any(), any(), any<Date>(), any())
sut.onActivityCreated(activity, fixture.bundle)
sut.onActivityPostResumed(activity)

val newActivity = mock<Activity>()
sut.onActivityPreCreated(newActivity, fixture.bundle)

verify(fixture.hub).startTransaction(any<String>(), any())
verify(fixture.hub).startTransaction(any(), any(), any<Date>(), any())
}

private fun setAppStartTime(date: Date = Date(0)) {
Expand Down
15 changes: 12 additions & 3 deletions sentry-android-fragment/api/sentry-android-fragment.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ public final class io/sentry/android/fragment/BuildConfig {

public final class io/sentry/android/fragment/FragmentLifecycleIntegration : android/app/Application$ActivityLifecycleCallbacks, io/sentry/Integration, java/io/Closeable {
public fun <init> (Landroid/app/Application;)V
public fun <init> (Landroid/app/Application;ZZ)V
public fun close ()V
public fun onActivityCreated (Landroid/app/Activity;Landroid/os/Bundle;)V
public fun onActivityDestroyed (Landroid/app/Activity;)V
Expand All @@ -20,9 +21,14 @@ public final class io/sentry/android/fragment/FragmentLifecycleIntegration : and
}

public final class io/sentry/android/fragment/SentryFragmentLifecycleCallbacks : androidx/fragment/app/FragmentManager$FragmentLifecycleCallbacks {
public fun <init> ()V
public fun <init> (Lio/sentry/IHub;)V
public synthetic fun <init> (Lio/sentry/IHub;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public static final field Companion Lio/sentry/android/fragment/SentryFragmentLifecycleCallbacks$Companion;
public static final field FRAGMENT_LOAD_OP Ljava/lang/String;
public fun <init> (Lio/sentry/IHub;ZZ)V
public synthetic fun <init> (Lio/sentry/IHub;ZZILkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun <init> (ZZ)V
public synthetic fun <init> (ZZILkotlin/jvm/internal/DefaultConstructorMarker;)V
public final fun getEnableAutoFragmentLifecycleTracing ()Z
public final fun getEnableFragmentLifecycleBreadcrumbs ()Z
public fun onFragmentAttached (Landroidx/fragment/app/FragmentManager;Landroidx/fragment/app/Fragment;Landroid/content/Context;)V
public fun onFragmentCreated (Landroidx/fragment/app/FragmentManager;Landroidx/fragment/app/Fragment;Landroid/os/Bundle;)V
public fun onFragmentDestroyed (Landroidx/fragment/app/FragmentManager;Landroidx/fragment/app/Fragment;)V
Expand All @@ -36,3 +42,6 @@ public final class io/sentry/android/fragment/SentryFragmentLifecycleCallbacks :
public fun onFragmentViewDestroyed (Landroidx/fragment/app/FragmentManager;Landroidx/fragment/app/Fragment;)V
}

public final class io/sentry/android/fragment/SentryFragmentLifecycleCallbacks$Companion {
}

24 changes: 16 additions & 8 deletions ...android-fragment/src/main/java/io/sentry/android/fragment/FragmentLifecycleIntegration.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,40 +6,48 @@ import android.app.Application.ActivityLifecycleCallbacks
import android.os.Bundle
import androidx.fragment.app.FragmentActivity
import io.sentry.IHub
import io.sentry.ILogger
import io.sentry.Integration
import io.sentry.SentryLevel.DEBUG
import io.sentry.SentryOptions
import java.io.Closeable

class FragmentLifecycleIntegration(private val application: Application) :
class FragmentLifecycleIntegration(
private val application: Application,
private val enableFragmentLifecycleBreadcrumbs: Boolean,
private val enableAutoFragmentLifecycleTracing: Boolean
) :
ActivityLifecycleCallbacks,
Integration,
Closeable {

constructor(application: Application) : this(application, true, false)

private lateinit var hub: IHub
private lateinit var logger: ILogger
private lateinit var options: SentryOptions

override fun register(hub: IHub, options: SentryOptions) {
this.hub = hub
this.logger = options.logger
this.options = options

application.registerActivityLifecycleCallbacks(this)
logger.log(DEBUG, "FragmentLifecycleIntegration installed.")
options.logger.log(DEBUG, "FragmentLifecycleIntegration installed.")
}

override fun close() {
application.unregisterActivityLifecycleCallbacks(this)
if (::logger.isInitialized) {
logger.log(DEBUG, "FragmentLifecycleIntegration removed.")
if (::options.isInitialized) {
options.logger.log(DEBUG, "FragmentLifecycleIntegration removed.")
}
}

override fun onActivityCreated(activity: Activity, savedInstanceState: Bundle?) {
(activity as? FragmentActivity)
?.supportFragmentManager
?.registerFragmentLifecycleCallbacks(
SentryFragmentLifecycleCallbacks(hub),
SentryFragmentLifecycleCallbacks(
hub = hub,
enableFragmentLifecycleBreadcrumbs = enableFragmentLifecycleBreadcrumbs,
enableAutoFragmentLifecycleTracing = enableAutoFragmentLifecycleTracing),
true
)
}
Expand Down
69 changes: 68 additions & 1 deletion ...oid-fragment/src/main/java/io/sentry/android/fragment/SentryFragmentLifecycleCallbacks.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,30 @@ import androidx.fragment.app.FragmentManager.FragmentLifecycleCallbacks
import io.sentry.Breadcrumb
import io.sentry.HubAdapter
import io.sentry.IHub
import io.sentry.ISpan
import io.sentry.SentryLevel.INFO
import io.sentry.SpanStatus

@Suppress("TooManyFunctions")
class SentryFragmentLifecycleCallbacks(
private val hub: IHub = HubAdapter.getInstance()
private val hub: IHub = HubAdapter.getInstance(),
val enableFragmentLifecycleBreadcrumbs: Boolean,
val enableAutoFragmentLifecycleTracing: Boolean
) : FragmentLifecycleCallbacks() {

constructor(
enableFragmentLifecycleBreadcrumbs: Boolean = true,
enableAutoFragmentLifecycleTracing: Boolean = false
) : this(
hub = HubAdapter.getInstance(),
enableFragmentLifecycleBreadcrumbs = enableFragmentLifecycleBreadcrumbs,
enableAutoFragmentLifecycleTracing = enableAutoFragmentLifecycleTracing
)

private val isPerformanceEnabled = hub.options.isTracingEnabled && enableAutoFragmentLifecycleTracing
Copy link
Member

Choose a reason for hiding this comment

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

If we use the accessor like I suggested above, it would affect this too given the new hub could have perf enabled but the first not. So technically we'd need to evaluate this on each call hubAcessor().options....

Unless we get a new instance of this class for each Hub (each Sentry.init) in which case it would be fine we bind to the on Hub and disable when the hub closes.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch @bruno-garcia. I think this would to the trick

Suggested change
private val isPerformanceEnabled = hub.options.isTracingEnabled && enableAutoFragmentLifecycleTracing
private val isPerformanceEnabled get() = hub.options.isTracingEnabled && enableAutoFragmentLifecycleTracing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its a new instance per activity, as the callback is bound to the fragment manager, and each activity has its own fragmentmanager, I guess its the same as #1528 (comment)
so in the case that a new activity is setup and isTracingEnabled is disabled, nothing is gonna happen.


private val fragmentsWithOngoingTransactions = mutableMapOf<Fragment, ISpan>()

override fun onFragmentAttached(
fragmentManager: FragmentManager,
fragment: Fragment,
Expand All @@ -38,6 +55,8 @@ class SentryFragmentLifecycleCallbacks(
savedInstanceState: Bundle?
) {
addBreadcrumb(fragment, "created")

startTracing(fragment)
}

override fun onFragmentViewCreated(
Expand All @@ -55,6 +74,8 @@ class SentryFragmentLifecycleCallbacks(

override fun onFragmentResumed(fragmentManager: FragmentManager, fragment: Fragment) {
addBreadcrumb(fragment, "resumed")

stopTracing(fragment)
}

override fun onFragmentPaused(fragmentManager: FragmentManager, fragment: Fragment) {
Expand All @@ -71,13 +92,18 @@ class SentryFragmentLifecycleCallbacks(

override fun onFragmentDestroyed(fragmentManager: FragmentManager, fragment: Fragment) {
addBreadcrumb(fragment, "destroyed")

stopTracing(fragment)
}

override fun onFragmentDetached(fragmentManager: FragmentManager, fragment: Fragment) {
addBreadcrumb(fragment, "detached")
}

private fun addBreadcrumb(fragment: Fragment, state: String) {
if (!enableFragmentLifecycleBreadcrumbs) {
return
}
val breadcrumb = Breadcrumb().apply {
type = "navigation"
setData("state", state)
Expand All @@ -91,4 +117,45 @@ class SentryFragmentLifecycleCallbacks(
private fun getFragmentName(fragment: Fragment): String {
return fragment.javaClass.simpleName
}

private fun isRunningSpan(fragment: Fragment): Boolean =
fragmentsWithOngoingTransactions.containsKey(fragment)

private fun startTracing(fragment: Fragment) {
if (!isPerformanceEnabled || isRunningSpan(fragment)) {
return
}

var transaction: ISpan? = null
hub.configureScope {
transaction = it.transaction
}

val fragmentName = getFragmentName(fragment)
val span = transaction?.startChild(FRAGMENT_LOAD_OP, fragmentName)

span?.let {
fragmentsWithOngoingTransactions[fragment] = it
}
}

private fun stopTracing(fragment: Fragment) {
if (!isPerformanceEnabled || !isRunningSpan(fragment)) {
Copy link
Member

Choose a reason for hiding this comment

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

h: If isPerformanceEnabled is evaluated on each call it could be that it is disabled here and never finish the span. If isPerformanceEnabled is set to false and we have a running span we could just discard the span.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you cannot mutate the isPerformanceEnabled after the SDK is inited, so I don't see the point of taking care of this corner case, the same happens on ActivityLifecycleIntegration

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't you do some crazy stuff like this?

public class MyApplication extends Application {

  private SentryOptions myOptions;

  @Override
  public void onCreate() {
    super.onCreate();
    
    SentryAndroid.init(
        this,
        options -> {
          myOptions = options;
        });

    // some time passes
    myOptions.setTracesSampleRate(null);
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you could, yes, this would be misusing the API hence not really a case that we should take care, if we take this option in consideration, there are N cases that would break the whole SDK, or using reflection and setting non null fields to null etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also init should be called only once, its part of the spec.

return
}

val span = fragmentsWithOngoingTransactions[fragment]
span?.let {
var status = it.status
if (status == null) {
status = SpanStatus.OK
}
it.finish(status)
fragmentsWithOngoingTransactions.remove(fragment)
}
}

companion object {
const val FRAGMENT_LOAD_OP = "fragment.load"
}
}
Loading