Auto Transactions

[denrase]

📜 Description

Automatically start transactions in navigation observer.

• When a new route is pushed a transaction is started with the name from the RouteSettings.
• The transaction is finished after 3 seconds timeout.
• The current transaction is finished when a new one is pushed.
• The transaction is finished when the route is popped.
• The previous is re-started when a route is popped.

Wait for Children

• Ported implementation from Android where transactions are only finished after children are done.

💡 Motivation and Context

First step to track mobile performance metrics.

❓ Caveats and Open Questions

• The clients need to provide RouteSettings with name parameter.

💚 How did you test it?

• Created unit tests
• Test in a sample app

📝 Checklist

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

🔮 Next steps

• Trimming of time when child spans are added/finished.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.67%. Comparing base (115aef7) to head (4d8e600).
⚠️ Report is 1605 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #643      +/-   ##
==========================================
+ Coverage   90.57%   90.67%   +0.10%     
==========================================
  Files          95       97       +2     
  Lines        3108     3142      +34     
==========================================
+ Hits         2815     2849      +34     
  Misses        293      293              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ueman]

The clients need to provide RouteSettings with name parameter. We fall back to unnamed for now.

Maybe it should just drop unnamed routes? I think it's misleading when all unnamed routes are tracked as the same transaction. You can't do anything with that data either.
If we drop unnamed routes, we could add an option (or do this anyway) which logs/asserts if an unnamed route is pushed.

When a route is popped, there is no more active route, as we don't restart the previous one.

I think I've done it in #611
My reasoning was that you can return values from a navigation. This can lead to work which possibly should be traced.
I've done this a couple times in apps I worked on.

The SentryNavigationObserver sets the routes name on the scope transaction. Is this something we need to consider here?

Isn't this disabled by default? It's at least configureable by a flag. So it could be enabled or disabled depending on wether performance tracing is on.

---

I suppose #611 isn't needed anymore and can be closed?
@denrase thanks for taking over

[denrase]

If we drop unnamed routes, we could add an option (or do this anyway) which logs/asserts if an unnamed route is pushed.

Good suggestion, maybe this is also a good way to surface this to users, as there's some small additional work they need to do for this feature.

My reasoning was that you can return values from a navigation. This can lead to work which possibly should be traced.
I've done this a couple times in apps I worked on.

Ah i see, you start a new trace with the previous route on didPop, i will also add this.

Isn't this disabled by default? It's at least configureable by a flag. So it could be enabled or disabled depending on wether performance tracing is on.

Good point, we'd probably also want a flag for transactions for users who want to opt out.

[marandaneto]

@marandaneto Yes, let me just fix conflict and address the NoOp feedback.

mm the feedback discussed on a call is not implemented yet, e.g. removing the finishAfter but rather getting the callback via ctor

[denrase]

@marandaneto Yes, I left some feedback regarding this. We'd need to provide the param through many methods instead of exposing just one in the span.

[marandaneto]

@marandaneto Yes, I left some feedback regarding this. We'd need to provide the param through many methods instead of exposing just one in the span.

the idea is to pass via ctor, not the methods, so we'd not need to provide the param through all the methods, Java does it.

[marandaneto]

@brustolin or @bruno-garcia would you like to do a final pass? its good from my side

[marandaneto]

@denrase follow up is #667 and #668

[marandaneto]

LGTM
thanks a lot @denrase

[bruno-garcia]

nit otherwise lgtm