feat: Auto-create traces for MAUI navigation events#5111
feat: Auto-create traces for MAUI navigation events#5111jamescrosswell wants to merge 19 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 ✨
🤖 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% 74.00% +0.01%
==========================================
Files 499 500 +1
Lines 18067 18181 +114
Branches 3520 3555 +35
==========================================
+ Hits 13368 13455 +87
- Misses 3839 3859 +20
- Partials 860 867 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…inished transactions
|
|
||
| var transaction = _hub is IHubInternal internalHub | ||
| ? internalHub.StartTransaction(context, _options.NavigationTransactionIdleTimeout) | ||
| : _hub.StartTransaction(context); |
There was a problem hiding this comment.
In practice, everything that implements IHub also implements IHubInternal and vice versa... so this code path will never execute.
Potentially we could replace it with an UnreachableException... that seems more dangerous (albeit more explicit/readable).
Maybe rather than using an UnreachableException then, we just add code comment here to explain.
@Flash0ver thoughts?
| /// <summary> | ||
| /// Resets the idle timeout for auto-finishing transactions. No-op for transactions without an idle timeout. | ||
| /// </summary> | ||
| public void ResetIdleTimeout(); |
There was a problem hiding this comment.
Technically this would be a breaking change as well... we could just put this (as an internal) on the concrete class rather than the interface.
@Flash0ver what do you think?
| { | ||
| hub.GetSentryOptions()?.LogError("Attempting to initianise the SentrySdk with a HubAdapter can lead to infinite recursion. Initialisation cancelled."); | ||
| return DisabledHub.Instance; | ||
| } |
There was a problem hiding this comment.
I added this to (try to) stop Warden from complaining about a potential infinite recursion... since HubAdapter forwards all calls to SentrySdk.CurrentHub.
We could throw an exception here instead of logging and returning a DisabledHub but in practice, HubAdapter would never be assigned to SentrySdk.CurrentHub so I think what we have here is already more than enough (arguably unecessary).
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Missing
ObjectDisposedExceptionhandling inChildSpanFinished- Wrapped the idle timer restart in
ChildSpanFinishedwith anObjectDisposedExceptioncatch to handle concurrentFinish()disposal safely.
- Wrapped the idle timer restart in
- ✅ Fixed: Discarded transaction appears active, blocking new transaction creation
- Set
EndTimestampin the idle-timeout discard path so discarded transactions reportIsFinished == trueand are not reused.
- Set
Or push these changes by commenting:
@cursor push 8bb9e11346
Preview (8bb9e11346)
diff --git a/src/Sentry/TransactionTracer.cs b/src/Sentry/TransactionTracer.cs
--- a/src/Sentry/TransactionTracer.cs
+++ b/src/Sentry/TransactionTracer.cs
@@ -287,6 +287,7 @@
if (shouldDiscard)
{
_options?.LogDebug("Idle transaction '{0}' has no child spans. Discarding.", SpanId);
+ EndTimestamp ??= _stopwatch.CurrentDateTimeOffset;
_idleTimer?.Dispose();
_hub.ConfigureScope(static (scope, tracer) => scope.ResetTransaction(tracer), this);
return;
@@ -378,7 +379,15 @@
// Only restart the idle timer when there are no more active (unfinished) child spans
if (_activeSpanTracker.PeekActive() == null)
{
- _idleTimer?.Start(_idleTimeout.Value);
+ try
+ {
+ _idleTimer?.Start(_idleTimeout.Value);
+ }
+ catch (ObjectDisposedException)
+ {
+ // Finish() may dispose the timer concurrently between the _hasFinished check and Start().
+ // Swallow the exception — the transaction is already finishing.
+ }
}
}
diff --git a/test/Sentry.Tests/TransactionTracerTests.cs b/test/Sentry.Tests/TransactionTracerTests.cs
--- a/test/Sentry.Tests/TransactionTracerTests.cs
+++ b/test/Sentry.Tests/TransactionTracerTests.cs
@@ -6,6 +6,29 @@
{
private static readonly TimeSpan AnyTimeout = TimeSpan.FromSeconds(30);
+ private sealed class ThrowOnDisposedTimer : ISentryTimer
+ {
+ private bool _isDisposed;
+
+ public ThrowOnDisposedTimer(Action _)
+ {
+ }
+
+ public void Start(TimeSpan timeout)
+ {
+ if (_isDisposed)
+ {
+ throw new ObjectDisposedException(nameof(ThrowOnDisposedTimer));
+ }
+ }
+
+ public void Cancel()
+ {
+ }
+
+ public void Dispose() => _isDisposed = true;
+ }
+
private static (TransactionTracer transaction, MockTimer timer) CreateIdleTransaction(
IHub hub, string name = "name", string op = "op")
{
@@ -122,16 +145,42 @@
{
// Given an auto-generated UI event transaction with no child spans
var hub = Substitute.For<IHub>();
- var (_, timer) = CreateIdleTransaction(hub);
+ var (transaction, timer) = CreateIdleTransaction(hub);
// When the idleTimeout fires
timer.Fire();
// Then the SDK discards the transaction (does not capture it)
hub.DidNotReceive().CaptureTransaction(Arg.Any<SentryTransaction>());
+ transaction.IsFinished.Should().BeTrue();
+ transaction.EndTimestamp.Should().NotBeNull();
}
[Fact]
+ public void ChildSpanFinished_DisposedIdleTimer_DoesNotThrow()
+ {
+ // Given an idle transaction whose timer has been disposed after the finished-state check
+ var hub = Substitute.For<IHub>();
+ ThrowOnDisposedTimer? disposedTimer = null;
+ var transaction = new TransactionTracer(
+ hub,
+ new TransactionContext("name", "op"),
+ idleTimeout: AnyTimeout,
+ timerFactory: callback =>
+ {
+ disposedTimer = new ThrowOnDisposedTimer(callback);
+ return disposedTimer;
+ });
+ disposedTimer!.Dispose();
+
+ // When ChildSpanFinished attempts to restart the disposed timer
+ var action = () => transaction.ChildSpanFinished();
+
+ // Then no ObjectDisposedException escapes
+ action.Should().NotThrow();
+ }
+
+ [Fact]
public void IdleTimeout_WithFinishedChildSpan_TrimsEndTimestampToLatestSpan()
{
// Given an auto-generated UI event transaction with one finished child spanThis Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b9e4ff1. Configure here.
|
|
||
| private InterlockedBoolean _cancelIdleTimeout; | ||
| private bool _hasFinished; | ||
| private readonly ReaderWriterLockSlim _finishLock = new(); |
There was a problem hiding this comment.
ReaderWriterLockSlim _finishLock is never disposed
The _finishLock field of type ReaderWriterLockSlim is created but never disposed. ReaderWriterLockSlim implements IDisposable and holds native resources that should be released. The Dispose() method only calls Finish() which disposes _idleTimer but not _finishLock, leading to potential resource leaks when many transactions are created.
Verification
Read the full TransactionTracer.cs file. Verified that _finishLock is a ReaderWriterLockSlim (line 21). Checked ISentryTimer interface (extends IDisposable). Examined Dispose() method (lines 568-571) which only calls Finish(). Examined TryFinishOnce() (lines 451-469) which disposes _idleTimer but not _finishLock. Confirmed ReaderWriterLockSlim implements IDisposable.
Identified by Warden code-review · 9TQ-89C
2 participants
Resolves: #5109
Implementation
Tracing for UI applications is a little bit complex.
In principle, we start a transaction when a navigation event is initiated and finish it when the navigation completes... sounds easy.
In practice, very little work will be done on the main UI thread as we don't want to freeze/block this. So a navigation event might start, a new view loads, a spinner icon animation kicks off and, technically, at that point, the navigation is completed. In reality the app may be implementing some kind of progressive display and loading of portions of that display is still taking place in the background (getting some stuff from a database to load up in the main panel, pulling access control credentials from a remote source to determine what actions should be available on the menu etc.).
The solution we have then is to start a new transaction with a idle timer on it:
This won't be accurate 100% of the time, but it should be a pretty good approximation most of the time.
Out of scope
The Android and Cocoa SDKs instrument traces for navigation, gesture and scroll events. This PR does so only for navigation events.
We plan to implement automated traces for gesture events in a followup PR: