Feat: Perf. for fragments by marandaneto · Pull Request #1528 · getsentry/sentry-java

marandaneto · 2021-06-11T12:38:22Z

📜 Description

Feat: Perf. for fragments

💡 Motivation and Context

Able to start spans out of fragments

💚 How did you test it?

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
No breaking changes

🔮 Next steps

...ples/sentry-samples-android/src/main/java/io/sentry/samples/android/ThirdActivityFragment.kt

...ndroid-fragment/src/main/java/io/sentry/android/fragment/SentryFragmentLifecycleCallbacks.kt

codecov-commenter · 2021-06-14T12:30:20Z

Codecov Report

Merging #1528 (d745940) into main (e7fb5c3) will not change coverage.
The diff coverage is 66.66%.

@@            Coverage Diff            @@
##               main    #1528   +/-   ##
=========================================
  Coverage     76.03%   76.03%           
  Complexity     1955     1955           
=========================================
  Files           192      192           
  Lines          6765     6765           
  Branches        675      675           
=========================================
  Hits           5144     5144           
  Misses         1294     1294           
  Partials        327      327

Impacted Files	Coverage Δ
sentry/src/main/java/io/sentry/IHub.java	`90.47% <66.66%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e7fb5c3...d745940. Read the comment docs.

Fixes #1523

sentry-samples/sentry-samples-android/src/main/AndroidManifest.xml

sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java

...ndroid-fragment/src/main/java/io/sentry/android/fragment/SentryFragmentLifecycleCallbacks.kt

bruno-garcia · 2021-06-24T14:59:17Z

...ndroid-fragment/src/main/java/io/sentry/android/fragment/SentryFragmentLifecycleCallbacks.kt

+        enableAutoFragmentLifecycleTracing = enableAutoFragmentLifecycleTracing
+    )
+
+    private val isPerformanceEnabled = hub.options.isTracingEnabled && enableAutoFragmentLifecycleTracing


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.

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

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.

...ndroid-fragment/src/main/java/io/sentry/android/fragment/SentryFragmentLifecycleCallbacks.kt

philipphofmann

On Cocoa, we only track spans until onResume is called because otherwise, the duration of a transaction would vary vastly. We could also create spans for onCreate, onStart and onResume. Then you get more insight into how long each of the lifecycle methods took.

sentry-android-core/src/test/java/io/sentry/android/core/ActivityLifecycleIntegrationTest.kt

sentry-samples/sentry-samples-android/api/sentry-samples-android.api

...ry-android-fragment/src/main/java/io/sentry/android/fragment/FragmentLifecycleIntegration.kt

...ndroid-fragment/src/main/java/io/sentry/android/fragment/SentryFragmentLifecycleCallbacks.kt

philipphofmann · 2021-06-25T07:02:41Z

...ndroid-fragment/src/main/java/io/sentry/android/fragment/SentryFragmentLifecycleCallbacks.kt

+        enableAutoFragmentLifecycleTracing = enableAutoFragmentLifecycleTracing
+    )
+
+    private val isPerformanceEnabled = hub.options.isTracingEnabled && enableAutoFragmentLifecycleTracing


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

...ndroid-fragment/src/main/java/io/sentry/android/fragment/SentryFragmentLifecycleCallbacks.kt

philipphofmann · 2021-06-25T07:13:44Z

...ndroid-fragment/src/main/java/io/sentry/android/fragment/SentryFragmentLifecycleCallbacks.kt

+    }
+
+    private fun stopTracing(fragment: Fragment) {
+        if (!isPerformanceEnabled || !isRunningSpan(fragment)) {


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.

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

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); } }

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

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

...ndroid-fragment/src/main/java/io/sentry/android/fragment/SentryFragmentLifecycleCallbacks.kt

marandaneto · 2021-06-25T08:18:54Z

On Cocoa, we only track spans until onResume is called because otherwise, the duration of a transaction would vary vastly. We could also create spans for onCreate, onStart and onResume. Then you get more insight into how long each of the lifecycle methods took.

the idea is that first, we measure the impact of adding spans for every single lifecycle method and later expand upon feedback, fragment has like 12 lifecycle methods, it's too noisy

philipphofmann · 2021-06-25T10:43:01Z

the idea is that first, we measure the impact of adding spans for every single lifecycle method and later expand upon feedback, fragment has like 12 lifecycle methods, it's too noisy

Didn't know that. We currently don't have much "noise" on Android, so adding some makes sense to me as performance is about getting more insight. But I'm fine with the let's see how it goes approach.

marandaneto · 2021-06-25T11:28:09Z

the idea is that first, we measure the impact of adding spans for every single lifecycle method and later expand upon feedback, fragment has like 12 lifecycle methods, it's too noisy

Didn't know that. We currently don't have much "noise" on Android, so adding some makes sense to me as performance is about getting more insight. But I'm fine with the let's see how it goes approach.

we've discussed this for Activity instrumentation, I'm just taking the same approach.
the difference is also that on Activitys and Fragments lifecycle, most of the work, if not only, is in the onCreate method, nothing else.

bruno-garcia

...ndroid-fragment/src/main/java/io/sentry/android/fragment/SentryFragmentLifecycleCallbacks.kt

Feat: Perf. for fragments

3ce3661

marandaneto commented Jun 11, 2021

View reviewed changes

...ples/sentry-samples-android/src/main/java/io/sentry/samples/android/ThirdActivityFragment.kt Outdated Show resolved Hide resolved

format

2ca9823

bruno-garcia reviewed Jun 11, 2021

View reviewed changes

fix

1c7e065

marandaneto and others added 13 commits June 15, 2021 13:27

extra ctor

add0350

Support transaction waiting for children to finish.

bd7f30d

Fixes #1523

Changelog.

79bd7b9

Expose waitForChildren with Hub and Sentry APIs.

5705d0e

Merge branch 'main' into feat/perf-fragments

b658cdc

fix

0962c4f

Merge remote-tracking branch 'origin/gh-1523' into feat/perf-fragments

2e4ad56

Merge branch 'main' into feat/perf-fragments

705b3de

Merge remote-tracking branch 'origin/gh-1523' into feat/perf-fragments

0ccbf03

Fix & Polish.

7c0d868

Refactor

eb2c03c

Merge remote-tracking branch 'origin/gh-1523' into feat/perf-fragments

340343f

fix

6c70853

marandaneto changed the base branch from main to gh-1523 June 16, 2021 12:19

marandaneto added 3 commits June 16, 2021 14:20

Merge branch 'main' into gh-1523

4133da5

Merge remote-tracking branch 'origin/gh-1523' into feat/perf-fragments

e72175a

test

c95b0e5

marandaneto commented Jun 16, 2021

View reviewed changes

sentry-samples/sentry-samples-android/src/main/AndroidManifest.xml Outdated Show resolved Hide resolved

fix

12102a0

marandaneto commented Jun 16, 2021

View reviewed changes

sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java Outdated Show resolved Hide resolved

marandaneto mentioned this pull request Jun 16, 2021

Support transaction finishing automatically with 'idle timeout' #1523

Closed

philipphofmann assigned marandaneto Jun 21, 2021

marandaneto mentioned this pull request Jun 23, 2021

Support transaction waiting for children to finish. #1535

Merged

4 tasks

Base automatically changed from gh-1523 to main June 23, 2021 12:48

marandaneto added 8 commits June 23, 2021 14:56

fix merge

687d67b

add overload

a9d6fe9

fix tests

e61b20f

fix test

ad9abc1

Merge branch 'main' into feat/perf-fragments

60cb1c5

fix

ebae3ab

tests

32cca7e

check

156c1e6

marandaneto marked this pull request as ready for review June 24, 2021 14:23

marandaneto requested review from bruno-garcia and philipphofmann June 24, 2021 14:24

changelog

6b7429a

bruno-garcia reviewed Jun 24, 2021

View reviewed changes

Merge branch 'main' into feat/perf-fragments

2db51a4

philipphofmann reviewed Jun 25, 2021

View reviewed changes

Merge branch 'main' into feat/perf-fragments

ace275c

review

7e062b7

rename span op

c064ab7

bruno-garcia approved these changes Jun 25, 2021

View reviewed changes

...ndroid-fragment/src/main/java/io/sentry/android/fragment/SentryFragmentLifecycleCallbacks.kt Show resolved Hide resolved

marandaneto added 2 commits June 28, 2021 09:14

Merge branch 'main' into feat/perf-fragments

bef4872

fix tests

d745940

marandaneto merged commit a533114 into main Jun 28, 2021

marandaneto deleted the feat/perf-fragments branch June 28, 2021 08:51

marandaneto removed their assignment Nov 24, 2021

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

Uh oh!

Comments

Conversation

marandaneto commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

philipphofmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marandaneto commented Jun 25, 2021

Uh oh!

philipphofmann commented Jun 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marandaneto commented Jun 25, 2021

Uh oh!

bruno-garcia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marandaneto commented Jun 11, 2021 •

edited

Loading

codecov-commenter commented Jun 14, 2021 •

edited

Loading

philipphofmann commented Jun 25, 2021 •

edited

Loading