sentry-dotnet
sentry-dotnet copied to clipboard
Closing Unfinished Transactions
Consider:
public void Main()
{
SentrySdk.Init(...);
var transaction = SentrySdk.StartTransaction("foo", "bar");
DoSomething();
transaction.Finish();
}
If DoSomething throws an unhandled exception, the transaction is never finished and thus not sent to Sentry. We should consider whether we should catch this in and finish the transactions with an error status, so that information is not lost.
We could just do that in our unhandled exception handlers, or when capturing an exception that is marked unhandled (so it works with all integrations). However, that doesn't take care of this possibility:
public void Main()
{
SentrySdk.Init(...);
try
{
var transaction = SentrySdk.StartTransaction("foo", "bar");
DoSomething();
transaction.Finish();
}
catch (Exception e)
{
SentrySdk.CaptureException(e);
}
}
Of course, that should probably be written as:
public void Main()
{
SentrySdk.Init(...);
try
{
var transaction = SentrySdk.StartTransaction("foo", "bar");
DoSomething();
transaction.Finish();
}
catch (Exception e)
{
transaction.Finish(SpanStatus.UnknownError);
SentrySdk.CaptureException(e);
}
}
But we could also do that if we made it IDisposable, such as:
public void Main()
{
SentrySdk.Init(...);
try
{
using var transaction = SentrySdk.StartTransaction("foo", "bar");
DoSomething();
transaction.Finish();
}
catch (Exception e)
{
SentrySdk.CaptureException(e);
}
}
... which then simplifies the original problem to:
public void Main()
{
SentrySdk.Init(...);
using var transaction = SentrySdk.StartTransaction("foo", "bar");
DoSomething();
transaction.Finish();
}
Unfortunately, that would be a breaking change - and one that is easily missed.
An alternative API that we could add without breaking anything would be:
public void Main()
{
SentrySdk.Init(...);
SentrySdk.WithTransaction("foo", "bar", transaction => {
DoSomething();
transaction.Finish();
});
}
Some previous discussion on this here: https://github.com/getsentry/develop/issues/443
Also relates to #1844
My inclination is to do two things:
- In
Hub.CaptureEventwe already have some logic for marking sessions as crashed for unhandled exceptions. We could also finish open transactions there with an error status. - Add the
SentrySdk.WithTransactionAPI described above to provide a better pit of success. MarkStartTransactionobsolete in favor ofWithTransaction.
We may also need to consider if we need to finish unfinished spans in the same way or not.
For now, let's forgo the API change and just focus on closing the transaction with an error status (See 1 above).
Probably should return Aborted as a status? https://github.com/getsentry/sentry-dotnet/blob/19da8b8885e4b98b650714754b919d758df0784b/src/Sentry/SpanStatus.cs#L56