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

Feat: Perf. for fragments #1528

Merged
merged 35 commits into from
Jun 28, 2021
Merged
Show file tree
Hide file tree
Changes from 18 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
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
## 5.1.0-beta.1

* Feat: Measure app start time (#1487)
* Feat: Automatic breadcrumbs logging for fragment lifecycle (#1522)
* Feat: Automatic breadcrumbs logging for fragment lifecycle (#1522)
* Feat: Support transaction waiting for children to finish. (#1535)

## 5.0.1

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import io.sentry.SentryLevel;
import io.sentry.SentryOptions;
import io.sentry.SpanStatus;
import io.sentry.TransactionContext;
import io.sentry.util.Objects;
import java.io.Closeable;
import java.io.IOException;
Expand Down Expand Up @@ -134,10 +135,14 @@ 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(
new TransactionContext(activityName, UI_LOAD_OP), null, false, null, true);
} else {
// start transaction with app start timestamp
transaction = hub.startTransaction(activityName, UI_LOAD_OP, appStartTime);
transaction =
hub.startTransaction(
new TransactionContext(activityName, UI_LOAD_OP), null, false, appStartTime, true);
marandaneto marked this conversation as resolved.
Show resolved Hide resolved
// start specific span for app start

appStartSpan = transaction.startChild(getAppStartOp(), getAppStartDesc(), appStartTime);
Expand Down
10 changes: 8 additions & 2 deletions sentry-android-fragment/api/sentry-android-fragment.api
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,11 @@ public final class io/sentry/android/fragment/FragmentLifecycleIntegration : and
}

public final class io/sentry/android/fragment/SentryFragmentLifecycleCallbacks : androidx/fragment/app/FragmentManager$FragmentLifecycleCallbacks {
public static final field Companion Lio/sentry/android/fragment/SentryFragmentLifecycleCallbacks$Companion;
public fun <init> ()V
public fun <init> (Lio/sentry/IHub;)V
public synthetic fun <init> (Lio/sentry/IHub;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun <init> (Lio/sentry/IHub;ZZZ)V
public synthetic fun <init> (Lio/sentry/IHub;ZZZILkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun <init> (ZZZ)V
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 +39,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 {
}

Original file line number Diff line number Diff line change
Expand Up @@ -6,40 +6,49 @@ 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
marandaneto marked this conversation as resolved.
Show resolved Hide resolved
) :
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,
performanceEnabled = options.isTracingEnabled,
enableAutoFragmentLifecycleTracing = enableAutoFragmentLifecycleTracing),
true
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,33 @@ 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(
marandaneto marked this conversation as resolved.
Show resolved Hide resolved
private val hub: IHub = HubAdapter.getInstance()
private val hub: IHub = HubAdapter.getInstance(),
bruno-garcia marked this conversation as resolved.
Show resolved Hide resolved
private val enableFragmentLifecycleBreadcrumbs: Boolean = true,
performanceEnabled: Boolean = false,
enableAutoFragmentLifecycleTracing: Boolean = false
) : FragmentLifecycleCallbacks() {

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

private val isPerformanceEnabled = performanceEnabled && enableAutoFragmentLifecycleTracing

private val fragmentsWithOngoingTransactions = mutableMapOf<Fragment, ISpan>()
marandaneto marked this conversation as resolved.
Show resolved Hide resolved

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

startTracing(fragment)
bruno-garcia marked this conversation as resolved.
Show resolved Hide resolved
}

override fun onFragmentViewCreated(
Expand All @@ -55,6 +77,9 @@ class SentryFragmentLifecycleCallbacks(

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

// ideally it should be post resumed
stopTracing(fragment)
marandaneto marked this conversation as resolved.
Show resolved Hide resolved
}

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

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

stopTracing(fragment)
marandaneto marked this conversation as resolved.
Show resolved Hide resolved
}

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 +121,44 @@ 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
}

val currentSpan = hub.span

val span: ISpan
val fragmentName = getFragmentName(fragment)

// should be a span of the activity transaction or its own transaction?
span = currentSpan?.startChild(FRAGMENT_LOAD_OP, fragmentName)
?: hub.startTransaction(fragmentName, FRAGMENT_LOAD_OP)

fragmentsWithOngoingTransactions[fragment] = span
}

private fun stopTracing(fragment: Fragment) {
marandaneto marked this conversation as resolved.
Show resolved Hide resolved
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 {
private const val FRAGMENT_LOAD_OP = "fragment.load"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,15 @@ public final class io/sentry/samples/android/SecondActivity : androidx/appcompat
public fun <init> ()V
}

public final class io/sentry/samples/android/ThirdActivityFragment : androidx/appcompat/app/AppCompatActivity {
public fun <init> ()V
}

public final class io/sentry/samples/android/ThirdFragment : androidx/fragment/app/Fragment {
public fun <init> ()V
public fun onViewCreated (Landroid/view/View;Landroid/os/Bundle;)V
}

public final class io/sentry/samples/android/databinding/ActivityMainBinding : androidx/viewbinding/ViewBinding {
public final field addAttachment Landroid/widget/Button;
public final field anr Landroid/widget/Button;
Expand All @@ -68,6 +77,7 @@ public final class io/sentry/samples/android/databinding/ActivityMainBinding : a
public final field nativeCrash Landroid/widget/Button;
public final field openSampleFragment Landroid/widget/Button;
public final field openSecondActivity Landroid/widget/Button;
public final field openThirdFragment Landroid/widget/Button;
public final field sendMessage Landroid/widget/Button;
public final field sendUserFeedback Landroid/widget/Button;
public final field setUser Landroid/widget/Button;
Expand All @@ -92,6 +102,16 @@ public final class io/sentry/samples/android/databinding/ActivitySecondBinding :
public static fun inflate (Landroid/view/LayoutInflater;Landroid/view/ViewGroup;Z)Lio/sentry/samples/android/databinding/ActivitySecondBinding;
}

public final class io/sentry/samples/android/databinding/ActivityThirdFragmentBinding : androidx/viewbinding/ViewBinding {
public final field container Landroid/widget/FrameLayout;
public final field fragmentContainerView Landroidx/fragment/app/FragmentContainerView;
public static fun bind (Landroid/view/View;)Lio/sentry/samples/android/databinding/ActivityThirdFragmentBinding;
public synthetic fun getRoot ()Landroid/view/View;
public fun getRoot ()Landroid/widget/FrameLayout;
public static fun inflate (Landroid/view/LayoutInflater;)Lio/sentry/samples/android/databinding/ActivityThirdFragmentBinding;
public static fun inflate (Landroid/view/LayoutInflater;Landroid/view/ViewGroup;Z)Lio/sentry/samples/android/databinding/ActivityThirdFragmentBinding;
}

public final class io/sentry/samples/android/databinding/FragmentSampleBinding : androidx/viewbinding/ViewBinding {
public final field container Landroid/widget/FrameLayout;
public static fun bind (Landroid/view/View;)Lio/sentry/samples/android/databinding/FragmentSampleBinding;
Expand All @@ -110,3 +130,11 @@ public final class io/sentry/samples/android/databinding/FragmentSampleInnerBind
public static fun inflate (Landroid/view/LayoutInflater;Landroid/view/ViewGroup;Z)Lio/sentry/samples/android/databinding/FragmentSampleInnerBinding;
}

public final class io/sentry/samples/android/databinding/ThirdFragmentBinding : androidx/viewbinding/ViewBinding {
public static fun bind (Landroid/view/View;)Lio/sentry/samples/android/databinding/ThirdFragmentBinding;
public synthetic fun getRoot ()Landroid/view/View;
public fun getRoot ()Landroid/widget/LinearLayout;
public static fun inflate (Landroid/view/LayoutInflater;)Lio/sentry/samples/android/databinding/ThirdFragmentBinding;
public static fun inflate (Landroid/view/LayoutInflater;Landroid/view/ViewGroup;Z)Lio/sentry/samples/android/databinding/ThirdFragmentBinding;
}

1 change: 1 addition & 0 deletions sentry-samples/sentry-samples-android/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ dependencies {
implementation(project(":sentry-android"))
implementation(project(":sentry-android-okhttp"))
implementation(project(":sentry-android-fragment"))
implementation(Config.Libs.fragment)

// how to exclude androidx if release health feature is disabled
// implementation(project(":sentry-android")) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@

<activity android:name=".SecondActivity" />

<activity android:name=".ThirdActivityFragment" />

<!-- NOTE: Replace the test DSN below with YOUR OWN DSN to see the events from this app in your Sentry project/dashboard-->
<meta-data android:name="io.sentry.dsn" android:value="https://1053864c67cc410aa1ffc9701bd6f93d@o447951.ingest.sentry.io/5428559" />

Expand Down Expand Up @@ -79,7 +81,7 @@
<!-- <meta-data android:name="io.sentry.traces.activity.enable" android:value="false" />-->

<!-- how to disable the Activity auto instrumentation automatically finished on onActivityPostPaused-->
<meta-data android:name="io.sentry.traces.activity.auto-finish.enable" android:value="false" />
<!-- <meta-data android:name="io.sentry.traces.activity.auto-finish.enable" android:value="false" />-->
marandaneto marked this conversation as resolved.
Show resolved Hide resolved

<!-- how to enable and set a sampleRate (anything between 0.01 and 1.0), it's disabled by default-->
<!-- <meta-data android:name="io.sentry.sample-rate" android:value="0.5" />-->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@
import android.os.Bundle;
import androidx.appcompat.app.AppCompatActivity;
import io.sentry.Attachment;
import io.sentry.ISpan;
import io.sentry.Sentry;
import io.sentry.SpanStatus;
import io.sentry.UserFeedback;
import io.sentry.protocol.SentryId;
import io.sentry.protocol.User;
Expand Down Expand Up @@ -159,15 +157,11 @@ protected void onCreate(Bundle savedInstanceState) {
binding.openSampleFragment.setOnClickListener(
view -> SampleFragment.newInstance().show(getSupportFragmentManager(), null));

setContentView(binding.getRoot());
}
binding.openThirdFragment.setOnClickListener(
view -> {
startActivity(new Intent(this, ThirdActivityFragment.class));
});

@Override
protected void onResume() {
super.onResume();
final ISpan span = Sentry.getSpan();
if (span != null) {
span.finish(SpanStatus.OK);
}
setContentView(binding.getRoot());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public void onCreate() {
// return event;
// });
// options.setAnrTimeoutIntervalMillis(2000);
options.addIntegration(new FragmentLifecycleIntegration(MyApplication.this));
options.addIntegration(new FragmentLifecycleIntegration(MyApplication.this, true, true));
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,11 +89,6 @@ class SecondActivity : AppCompatActivity() {
Sentry.captureException(t)

showText(true, "error: ${t.message}")

// I opt out enableActivityLifecycleTracingAutoFinish so I know best when to end my transaction
// be sure to finish all your spans before this
val transaction = Sentry.getSpan()
transaction?.finish(SpanStatus.INTERNAL_ERROR)
}

override fun onResponse(call: Call<List<Repo>>, response: Response<List<Repo>>) {
Expand All @@ -102,11 +97,6 @@ class SecondActivity : AppCompatActivity() {
span.finish(SpanStatus.OK)

showText(text = "items: ${repos.size}")

// I opt out enableActivityLifecycleTracingAutoFinish so I know best when to end my transaction
// be sure to finish all your spans before this
val transaction = Sentry.getSpan()
transaction?.finish(SpanStatus.OK)
}
})
}
Expand Down
Loading