-
-
Notifications
You must be signed in to change notification settings - Fork 442
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: UI events transactions #1975
Conversation
|
@romtsn This should target |
Why? |
|
@marandaneto mm, but only methods marked as |
sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/UserInteractionIntegration.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/UserInteractionIntegration.java
Outdated
Show resolved
Hide resolved
...droid-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java
Outdated
Show resolved
Hide resolved
...droid-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java
Outdated
Show resolved
Hide resolved
Indeed, but I'd rather point to v6 to speed up this major release, by that I mean more people working on the new code base and testing out all the changes. |
sounds good, have no problem with targeting 6.x.x actually! |
@marandaneto @philipphofmann @adinauer this is ready for review PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the PR looks great. Thanks for all the effort. Maybe I missed it, but I'm not sure if the code handles the following scenario:
Scenario: Ongoing UI event transaction
Given an ongoing UI event transaction
When the SDK creates a new screen load transaction
Then the SDK finishes the previous transactions
And removes it from the scope
And sets the status to canceled
And waits for its children to finish
sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java
Show resolved
Hide resolved
@@ -562,4 +566,102 @@ class SentryTracerTest { | |||
anyOrNull() | |||
) | |||
} | |||
|
|||
@Test | |||
fun `when initialized without idleTimeout, does not schedule finish timer`() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m
: Can we also add a test for when the idle timeout expires, but the transaction has still an unfinished child. I think we should then wait for the child to finish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we said we cancel the timer when a new child is added and reset it when the last child is finished, so there's technically no idle timeout while there's an ongoing child span.
Even though, this has been missing in my code, so I'll update that, thanks for the point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, makes sense. Thanks for the clarification.
@romtsn please update the PR Checklist |
Yeah, this is done here -> Lines 265 to 276 in f67519e
With the only difference that we do this when the current screen goes inactive (onPause). This makes sense for us, because we use android's WindowCallback, which will get detached as soon as the activity/screen goes inactive, therefore it wouldn't make sense for us to wait until the new screen transaction, but rather finish it here. |
...droid-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## 6.x.x #1975 +/- ##
============================================
+ Coverage 80.79% 80.86% +0.07%
- Complexity 3146 3169 +23
============================================
Files 228 228
Lines 11645 11714 +69
Branches 1565 1577 +12
============================================
+ Hits 9409 9473 +64
- Misses 1649 1650 +1
- Partials 587 591 +4
Continue to review full report at Codecov.
|
sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java
Outdated
Show resolved
Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/UserInteractionIntegration.java
Show resolved
Hide resolved
...droid-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java
Outdated
Show resolved
Hide resolved
This reverts commit 1f2d580.
Reverted frame metrics as we'll do it as part of #2101 |
📜 Description
idleTimeout
toSentryTracer
to finish idled transaction💡 Motivation and Context
Q1 Goal
Closes #1811
💚 How did you test it?
📝 Checklist
🔮 Next steps