ITransactionTracer should implement IDisposable

[tjmccurdy-arhaus]

Problem Statement

Transaction scopes should be easier to manage in code by having the ITransactionTracer implement IDisposable.

Solution Brainstorm

**using** var tran = SentrySdk.StartTransaction("Startup", "Init");
/* Do something */
/* Once the code goes out of scope, the transaction should "complete" automatically because of the "Dispose" method getting called explicitly, or implicitly. */

Product Area

Unknown

[getsantry]

Assigning to @getsentry/support for routing ⏲️

[Flash0ver]

For consistency, we may also want to consider SentrySdk.StartSpan(string, string) returning ISpan.

I suppose the implementation(s) of Dispose() would invoke Finish().

Pros

• easy usage via using statement and using declaration
  • guaranteed Finish of SpanStatus.Ok
• Diagnostic CA2000 (if enabled) could flag some unfinished Transactions/Spans

Cons

• Diagnostic CA2000 (if enabled) could report some false positives where
  • Finish is invoked but Dispose is not
  • ownership of the Transaction/Span is transferred between types/members
• Finish() is currently not safe to call multiple times
  • Dispose() must be safe to be called multiple times without throwing exceptions nor causing unwanted side-effects
• just using using statement or using declaration could "hide" the Finish method group (SpanStatus and/or Exception overloads)
• when using any of the Finish overloads, in combination with Diagnostic CA2000 (if enabled), could lead to patterns like

var transaction = SentrySdk.StartTransaction("test", "test");
...
transaction.Finish(exception, spanStatus);
transaction.Dispose();

---

In general, IDisposable and IAsyncDisposable are primarily intended for deterministic cleanup of managed and unmanaged resources. However, the ecosystem (including e.g. Microsoft.Extensions.Logging) often uses them for the semantics of "end/complete/finish/close an operation/scope".
Well ... to be fair ...Sentry.TransactionTracer does own a Timer with Callback, that does benefit from deterministic cleanup.

[tjmccurdy-arhaus]

Question: Do you have a use-case (for learning purposes) for when an ITransactionTracer should not be disposed of once it goes out of scope? I am not sure how that would happen actually since the .NET GC would eventually just keep trying to remove that instance, but keep failing, and then move it off into Gen1, Gen2 GC collection. But I am interested. I would imagine that most developers would manage their own scoped-lifetimes of variables, and IF an ITransactionTracer needed to be "kept alive" longer, one might declare it at a class level, or even higher.

My main use-case here is either Azure Functions or Azure WebJobs, where there might be one very long-lived "transaction" representing the overall run of some "workflow", with smaller short-lived "spans" (yes, it would be nice if those also implemented IDisposable).

Maybe "Finish" becomes internal to the class logic and developers should only use "Dispose()"?

[Flash0ver]

@tjmccurdy-arhaus ... first of all: thank you very much for using Sentry, and opening this issue and discussion to improve sentry-dotnet.

Yeah, I believe you're right. I may have misunderstood Idle-Transactions, that don't seem to be used too widely / at all.

Apart from that, I think that - usually - the component that starts the Transaction is meant/responsible for finishing the Transaction ... which could be a great usage-scenario for a using declaration indeed, where at the beginning of a method a Transaction is started, passed to methods that are called, and then disposed/finished at the end of the method.

From what I can glean is that we don't root Transactions, so when instances go out of scope unfinished, nothing would be sent to Sentry and the GC will eventually clean up all the managed resources nicely. So strictly speaking there is nothing really to Dispose (unless maybe Idle-Transactions).

I have the feeling that we can't make Finish non-public, not only due to the breaking change, but also the three overloads that we currently expose.
Also, we would need to make sure that we don't enqueue multiple requests to Sentry on double-Dispose and/or double-Finish ... e.g.

{
  using ITransactionTracer transaction = SentrySdk.StartTransaction("test", "test");
  ..
  if (condition)
  {
    transaction.Finish(SpanStatus.Cancelled);
  }
} // implicit Dispose/Finish before return

It seems that Sentry is ignoring / deduping the deduplications ... but we should avoid the local resources and HTTP request involved.

Additionally, depending on user scenarios, when creating/passing/finishing Transactions in different components (potentially a bit of a corner case though), or using methods from the Finish group other than the parameter-less overload, CA2000 may report (if enabled by the user) false positives.

@jamescrosswell, what do you think?