feat: UI events transactions by romtsn · Pull Request #1975 · getsentry/sentry-java

romtsn · 2022-04-05T14:11:13Z

📜 Description

Adds idleTimeout to SentryTracer to finish idled transaction
Creates a new transaction whenever a UI action is performed (click, swipe, scroll).

💡 Motivation and Context

Q1 Goal
Closes #1811

💚 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

github-actions · 2022-04-05T14:11:47Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 0161bd7

marandaneto · 2022-04-05T14:15:27Z

@romtsn This should target 6.x.x

sentry/src/main/java/io/sentry/SentryOptions.java

sentry/src/main/java/io/sentry/SentryTracer.java

romtsn · 2022-04-05T14:24:39Z

@romtsn This should target 6.x.x

Why?

marandaneto · 2022-04-06T09:45:48Z

@romtsn This should target 6.x.x

Why?

Hub interface is a public API and it has changed.

romtsn · 2022-04-06T12:12:01Z

@marandaneto mm, but only methods marked as Internal have changed. Only if we care about ABI, then it makes sense.

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

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

...droid-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java

sentry/src/main/java/io/sentry/SentryTracer.java

sentry/src/main/java/io/sentry/config/PropertiesProvider.java

marandaneto · 2022-04-08T11:58:01Z

@marandaneto mm, but only methods marked as Internal have changed. Only if we care about ABI, then it makes sense.

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.
Unless @bruno-garcia says otherwise.

romtsn · 2022-04-11T07:27:22Z

@marandaneto mm, but only methods marked as Internal have changed. Only if we care about ABI, then it makes sense.

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. Unless @bruno-garcia says otherwise.

sounds good, have no problem with targeting 6.x.x actually!

…ng removed

romtsn · 2022-04-28T04:28:25Z

@marandaneto @philipphofmann @adinauer this is ready for review PTAL

philipphofmann

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

CHANGELOG.md

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

sentry/src/test/java/io/sentry/SentryTracerTest.kt

philipphofmann · 2022-05-03T14:29:10Z

sentry/src/test/java/io/sentry/SentryTracerTest.kt

    }
+
+    @Test
+    fun `when initialized without idleTimeout, does not schedule finish timer`() {


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.

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.

Yes, makes sense. Thanks for the clarification.

marandaneto · 2022-05-04T16:39:34Z

@romtsn please update the PR Checklist

CHANGELOG.md

romtsn · 2022-05-05T12:06:07Z

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

Yeah, this is done here ->

sentry-java/sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java

Lines 265 to 276 in f67519e

    
           void stopTracing(final @NotNull SpanStatus status) { 
        
             if (activeTransaction != null) { 
        
               activeTransaction.finish(status); 
        
             } 
        
             hub.configureScope( 
        
                 scope -> { 
        
                   clearScope(scope); 
        
                 }); 
        
             activeTransaction = null; 
        
             activeView.clear(); 
        
             activeEventType = null; 
        
           }

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

codecov-commenter · 2022-05-12T09:33:58Z

Codecov Report

Merging #1975 (bdcd818) into 6.x.x (d1c8c4d) will increase coverage by 0.07%.
The diff coverage is 85.05%.

@@             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

Impacted Files	Coverage Δ
sentry/src/main/java/io/sentry/Hub.java	`75.72% <ø> (ø)`
sentry/src/main/java/io/sentry/HubAdapter.java	`8.19% <ø> (ø)`
sentry/src/main/java/io/sentry/IHub.java	`87.50% <0.00%> (-3.81%)`	⬇️
sentry/src/main/java/io/sentry/NoOpHub.java	`48.78% <ø> (ø)`
...entry/src/main/java/io/sentry/NoOpTransaction.java	`25.80% <0.00%> (-0.87%)`	⬇️
sentry/src/main/java/io/sentry/Sentry.java	`45.06% <ø> (ø)`
sentry/src/main/java/io/sentry/SentryTracer.java	`91.17% <83.01%> (+0.37%)`	⬆️
...main/java/io/sentry/config/PropertiesProvider.java	`90.00% <85.71%> (-10.00%)`	⬇️
...entry/src/main/java/io/sentry/ExternalOptions.java	`100.00% <100.00%> (ø)`
sentry/src/main/java/io/sentry/SentryOptions.java	`81.74% <100.00%> (+0.85%)`	⬆️
... and 4 more

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 d1c8c4d...bdcd818. Read the comment docs.

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

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

...droid-core/src/main/java/io/sentry/android/core/internal/gestures/SentryGestureListener.java

This reverts commit 1f2d580.

romtsn · 2022-05-13T14:11:11Z

Reverted frame metrics as we'll do it as part of #2101

UI events transactions

752c285

Format code

02316ec

romtsn commented Apr 5, 2022

View reviewed changes

sentry/src/main/java/io/sentry/SentryOptions.java Show resolved Hide resolved

romtsn commented Apr 5, 2022

View reviewed changes

sentry/src/main/java/io/sentry/SentryTracer.java Outdated Show resolved Hide resolved