feat: Auto-create traces for MAUI navigation events#5111
feat: Auto-create traces for MAUI navigation events#5111jamescrosswell wants to merge 15 commits intomainfrom
Conversation
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Features ✨
Fixes 🐛
Dependencies ⬆️Deps
Other
🤖 This preview updates automatically when you update the PR. |
| /// Starts a transaction that will automatically finish after <paramref name="idleTimeout"/> if not | ||
| /// finished explicitly first. | ||
| /// </summary> | ||
| public ITransactionTracer StartTransaction(ITransactionContext context, TimeSpan? idleTimeout); |
There was a problem hiding this comment.
Technically it'd be a breaking change if we added this method to the public IHub interface. Potentially in v7 we could consider making this public and getting rid of the IHubInternal interface.
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5111 +/- ##
==========================================
- Coverage 73.99% 73.97% -0.03%
==========================================
Files 499 500 +1
Lines 18067 18149 +82
Branches 3520 3550 +30
==========================================
+ Hits 13368 13425 +57
- Misses 3839 3858 +19
- Partials 860 866 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ITransactionTracer IHubInternal.StartTransaction(ITransactionContext context, TimeSpan? idleTimeout) | ||
| => SentrySdk.StartTransaction(context, idleTimeout); |
There was a problem hiding this comment.
Infinite recursion in HubAdapter.StartTransaction causes StackOverflowException
The new IHubInternal.StartTransaction(context, idleTimeout) implementation in HubAdapter forwards to SentrySdk.StartTransaction(context, idleTimeout). However, SentrySdk.StartTransaction checks if CurrentHub is IHubInternal and calls internalHub.StartTransaction(context, idleTimeout). Since HubAdapter implements IHubInternal and is commonly set as CurrentHub, this creates an infinite recursion loop that will result in a StackOverflowException.
Verification
Read HubAdapter.cs lines 129-130 confirming it calls SentrySdk.StartTransaction. Read SentrySdk.cs lines 671-674 confirming the IHubInternal dispatch pattern. HubAdapter implements IHubInternal (line 16), so SentrySdk will call back into HubAdapter, creating infinite recursion.
Identified by Warden find-bugs · N5D-RKE
There was a problem hiding this comment.
HubAdapter is never set to SentrySdk.CurrentHub... the whole point of HubAdapter is to allow us to mock the otherwise static/unmockable SentrySdk class in unit tests.
I guess we could enforce this explicitly. Addressed in: 0418c99
There was a problem hiding this comment.
Fix attempt detected (commit 0418c99)
HubAdapter now implements IHubInternal and added the StartTransaction(context, idleTimeout) method that calls SentrySdk.StartTransaction, which will dispatch back to the same method via the IHubInternal check in SentrySdk, perpetuating the infinite recursion loop.
The original issue appears unresolved. Please review and try again.
Evaluated by Warden
| var latestSpanEnd = _spans | ||
| .Where(s => s.IsFinished) | ||
| .Select(s => s.EndTimestamp) | ||
| .Max(); |
There was a problem hiding this comment.
Max() on empty sequence throws InvalidOperationException when idle transaction has no finished spans
When Finish() is called directly on an idle transaction (_idleTimeout.HasValue is true) that has child spans but none are finished, the LINQ expression _spans.Where(s => s.IsFinished).Select(s => s.EndTimestamp).Max() operates on an empty sequence. LINQ's Max() throws InvalidOperationException ("Sequence contains no elements") when the input sequence is empty. This causes a runtime crash when manually finishing an idle transaction before any child span completes.
Verification
Read TransactionTracer.cs lines 430-442 to understand the code flow. Confirmed _spans is ConcurrentBag<ISpan> (line 169/171), and the Where clause filters for IsFinished which checks EndTimestamp is not null (SpanTracer.cs line 41). Read tests confirming idle transactions can have unfinished spans. LINQ Max() on empty IEnumerable<T> throws InvalidOperationException per .NET documentation.
Identified by Warden code-review · JKQ-D53
2 participants
Resolves: #5109
Out of scope
The Android and Cocoa SDKs instrument traces for navigation, gesture and scroll events. This PR does so only for navigation events.
See #5116 for details: