QOL: Allow finishing spans and transactions with using

[jamescrosswell]

Resolves #4321

• ITransactionTracer should implement IDisposable #4321

[codecov]

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (version6@90a3823). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/Sentry/Internal/NoOpSpan.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             version6    #4627   +/-   ##
===========================================
  Coverage            ?   73.20%           
===========================================
  Files               ?      480           
  Lines               ?    17403           
  Branches            ?     3430           
===========================================
  Hits                ?    12739           
  Misses              ?     3811           
  Partials            ?      853           

☔ 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.

[jamescrosswell]

@Flash0ver I'm trying to force an analyzer warning for the CA2000 (so that I can build and test a way to suppress this). I've added this to the csproj file for samples/Sentry.Samples.Console.Basic/Sentry.Samples.Console.Basic.csproj:

<WarningsAsErrors>CA2000</WarningsAsErrors>

However I can't seem to get it to work:

The warning does appear for the undisposed use of Foo (which I added to test the warning was enabled)... but we're not seeing it for our newly disposable ISpan classes. StartTransaction returns an ITransactionTracer that descends from ISpan, which implements IDisposable.

Any ideas/thoughts?

[Flash0ver]

Oh ... perhaps I totally misunderstood CA2000 😟

DisposeObjectsBeforeLosingScope.cs

Hmmm ... it seems that the CA2000-DiagnosticAnalyzer does some Control-Flow-Analysis, that either tries to "see through" the implementation and does not report when it can't statically figure it out, or just does not report for some usage patterns.

var instance1 = new MyDisposable(); //CA2000
var instance2 = new MyFactory().CreateInstance(); //CA2000
var instance3 = MyFactory.CreateStatic(); //CA2000
var instance4 = MyFactory.Instance.CreateInstance(); // does not report CA2000

public sealed class MyDisposable : IDisposable
{
    public void Dispose()
    {
    }
}
public interface IMyFactory
{
    IDisposable CreateInstance();
}
public sealed class MyFactory : IMyFactory
{
    public static IMyFactory Instance { get; } = new MyFactory();
    public static IDisposable CreateStatic() => new MyDisposable();
    public IDisposable CreateInstance() => new MyDisposable();
}

Not sure if that analysis behavior is intended or not.
Perhaps the Analyzer cannot "see through" our rather complex control flow from SentrySdk, via the IHub, to some extension-methods.

---

Now, as a follow-up, I'm wondering:
We do have 2 public types that inherit ISpan, and with that IDisposable:

var transaction = new TransactionTracer(null!, null!); //CA2000
var span = new SpanTracer(null!, transaction, default, default, null!); //CA2000
span.Finish();
transaction.Finish();

Are there valid/intended usage scenarios to instantiate these types directly from user code, which would report CA2000, even when properly Finished?
Or ... perhaps we do not need a DiagnosticSuppressor for CA2000, since it's not reported for our implementation ... which may change in the future 🤔

[jamescrosswell]

Are there valid/intended usage scenarios to instantiate these types directly from user code, which would report CA2000,

I can't think of any scenario in which people would want to construct those manually... so I suspect not.

Or ... perhaps we do not need a DiagnosticSuppressor for CA2000, since it's not reported for our implementation ...

That's possibly the case yes... and without being able to reproduce the warning that it's supposed to suppress, it's relatively difficult to test. I think this PR might be ready for review then...

[Flash0ver]

question: Timer

Should we, in the case of a Timer still being active, disable/dispose the timer too when the transactions is transitioning from unfinished to finished?

Wait ... I read this wrong ... this is actually what is happening:

• the first Finish will Stop/Dispose the Timer, if active
• subsequent Finishes will early-out, but the Timer, if active, has been stopped/disposed previously

[jamescrosswell]

The timer is only relevant if idle timeouts are enabled. That was added by Sean here:

• Support transaction finishing automatically with 'idle timeout' #2452

I think there were plans to maybe use transactions that timed out automatically to instrument MAUI, but this hasn't yet been done. So, in practice, that code path is never executed.

I'm not sure I understand though - do you think there's a potential problem here? In what scenario?