Added StartTimer extension method to IMetricAggregator

[jamescrosswell]

Resolves #3074

The API for timings has changed from :

        using (new Timing("much.work", MeasurementUnit.Duration.Millisecond))
        {
            // Do some number crunching
        }

... to:

        using (SentrySdk.Metrics.StartTimer("much.work", MeasurementUnit.Duration.Millisecond))
        {
            // Do some number crunching
        }

Unfortunately that trivial change to the API interface required quite a bit of refactoring under the hood. The Timing class needs an IHub to create a transaction/span for the timing so we had to move the main MetricAggregator from ISentryClient to IHub and inject IHub as a dependency (to enable the creation of transactions).

Previously, when it was on SentryClient, the CaptureMetrics and CaptureCodeLocations methods (also on SentryClient) could be internal... because they were only ever referenced from the Client.

Now, with everything on the Hub, the Hub needs to call those methods so they have to be on the ISentryClient interface, which means they have to be public... and so do all of the types used as parameters to those methods.

We have been able to make the Timing class internal now though, at least. 😜

[codecov]

Codecov Report

Attention: 48 lines in your changes are missing coverage. Please review.

Comparison is base (b72b9e7) 76.42% compared to head (fcb1fb4) 76.41%.
Report is 10 commits behind head on main.

Files	Patch %	Lines
src/Sentry/Internal/Hub.cs	6.45%	28 Missing and 1 partial ⚠️
src/Sentry/Timing.cs	0.00%	9 Missing ⚠️
src/Sentry/DisabledMetricAggregator.cs	0.00%	4 Missing ⚠️
src/Sentry/MetricAggregator.cs	66.66%	2 Missing ⚠️
src/Sentry/Extensibility/DisabledHub.cs	0.00%	1 Missing ⚠️
src/Sentry/Extensibility/HubAdapter.cs	0.00%	1 Missing ⚠️
src/Sentry/SentryClient.cs	50.00%	1 Missing ⚠️
src/Sentry/SentrySdk.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3075      +/-   ##
==========================================
- Coverage   76.42%   76.41%   -0.01%     
==========================================
  Files         351      351              
  Lines       13263    13275      +12     
  Branches     2646     2646              
==========================================
+ Hits        10136    10144       +8     
- Misses       2450     2452       +2     
- Partials      677      679       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bitsandfoxes]

That looks much nicer to use! Thanks!

[bruno-garcia]

One thing that bugs me a bit is that, unlike other instruments, Timing creates a span/transaction... and to do that it needs an IHub. The MetricAggregator can't supply that dependency so under the hood this extension method uses SentrySdk.CurrentHub to create spans/transactions for Timings.

Remind me again why isn't Metrics on the Hub instead? That's where we keep all state last I saw

We've discussed previously the possibility of putting the MetricAggregator on IHub rather than on ISentryClient. I think that would make the code significantly more complex. It would couple the MetricAggregator with the Hub (where currently those things are decoupled) and add another layer of indirection for simple things like capturing metric events and transmitting them to Sentry. So I'm not a fan of that solution.

That's where the scope lives and where other state lives too like session state I believe, no?

In any event, this solution enables the API interface we discussed. The only other thing we might want to do is make the Timing class internal so that people can't instanciate that directly (they have to use the IMetricAggregator.StartTimer() syntax).

Makes sense to be internal

[jamescrosswell]

Remind me again why isn't Metrics on the Hub instead? That's where we keep all state last I saw

I can't find the old conversation (too many threads on the original PR and it's really hard to search in resolved conversations).

However, basically it's on the SentryClient as that's where the BackgroundWorker lives... and the aggregator is very similar to the BackgroundWorker. It also doesn't have any dependencies on the Hub - all it needs is a way to send metrics and a way to send code locations (which are two methods on ISentryClient). It would be quite a bit of work, at this stage, to move it over to the Hub, so would need a fairly compelling reason.

That's where the scope lives and where other state lives too like session state I believe, no?

Kind of... it's a bit of a mixed bag.

The Scope really lives in the ScopeManager which holds the Scope alongside a SentryClient.

sentry-dotnet/src/Sentry/Internal/SentryScopeManager.cs

Lines 12 to 16 in 941d9dc

	private KeyValuePair<Scope, ISentryClient>[] ScopeAndClientStack
	{
	get => ScopeStackContainer.Stack ??= NewStack();
	set => ScopeStackContainer.Stack = value;
	}

So the Scope state is loosely and indirectly associated with the SentryClient... although normally as an AsyncLocal - unless we're in Global mode - so arguably it's tied to the Thread rather than either the Hub or the Client. The ConfigureScope API that is used to read/mutate this lives on the Hub though... so in that respect it feels like a Hub thing.

For Session state, both the Hub and the SentryClient hold references to this:

sentry-dotnet/src/Sentry/Internal/Hub.cs

Line 50 in 2a41c48

_sessionManager = sessionManager ?? new GlobalSessionManager(options);

sentry-dotnet/src/Sentry/SentryClient.cs

Line 54 in 2a41c48

_sessionManager = sessionManager ?? new GlobalSessionManager(options);

Although when using a Hub, the Hub._ownedClient will have it's session manager passed in by the Hub. The SentryClient also doesn't provide any methods for working with the session, whereas the Hub has Start/Pause/Resume/End session methods so again, in terms of how you access it, it seems to be via the Hub.

With the current design, you can also do everything you want to for Metrics via the Hub as well:

sentry-dotnet/src/Sentry/Internal/Hub.cs

Line 24 in 2a41c48

public IMetricAggregator Metrics { get; }

That's because all of the functionality related to metrics is pretty much encapsulated in the MetricAggregator... the only exception being the functionality to send those metrics to Sentry (which obviously should be a SentryClient thing).

We could have the MetricAggregator take a hard dependency on IHub (so that you couldn't send metrics without a Hub)... At the moment, you could technically use Metrics and SentryClient independently (without any Hub)... although with this PR the StartTimer method introduces a dependency on the SentrySdk.CurrentHub so we're already starting to break that I guess.

This has already been addressed now.