Capture built in metrics from System.Diagnostics.Metrics API #3052
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3052 +/- ##
==========================================
+ Coverage 75.39% 76.43% +1.04%
==========================================
Files 351 355 +4
Lines 13175 13381 +206
Branches 2613 2656 +43
==========================================
+ Hits 9933 10228 +295
+ Misses 2572 2474 -98
- Partials 670 679 +9 ☔ View full report in Codecov by Sentry. |
jamescrosswell
changed the title
jamescrosswell
changed the title
…stered.Net4_8.Net.verified.txt
…into default-metrics
We have already a package, that's only used for older frameworks: Note that this can be confusing sometimes as a setup so docs here needs to be excellent.
Folks have been confused with this: https://docs.sentry.io/platforms/dotnet/performance/instrumentation/automatic-instrumentation/#diagnosticsource-integration On newer frameworks we include the source in the core Sentry package: sentry-dotnet/src/Sentry/Sentry.csproj Lines 52 to 60 in feabf09 We could bump the min version it depends on: Seems to work on older frameworks but they do a trick of copying older DLLs for old targets sometimes to the new package: So worth testing it out. |
| where T: struct | ||
| { | ||
| var unit = MeasurementUnit.Parse(instrument.Unit); | ||
| var tagDict = tags.ToImmutableArray().ToImmutableDictionary( |
There was a problem hiding this comment.
👁️ it's almost midnight I need to get back to this and do a proper review 😅
There was a problem hiding this comment.
Worth profiling this to see the impact. This being ReadOnlySpan and us allocating immutableArray then dictionary from it seems like could show up depending how often this is called.
There was a problem hiding this comment.
I've added a separate issue to track this:
| /// See https://learn.microsoft.com/en-us/dotnet/core/diagnostics/built-in-metrics for more information. | ||
| /// </para> | ||
| /// </summary> | ||
| public IList<SubstringOrRegexPattern> CaptureSystemDiagnosticsInstruments |
There was a problem hiding this comment.
In terms of defaults, we could capture none/some/all of these build in metrics.
Are there quotas that we need to be mindful of (so are we potentially going to chew through someone's entire Sentry Quota if we start collecting routing match attempt metrics?
If not, all of those built in metrics are potentially useful/interesting to people so we could enable these.
There was a problem hiding this comment.
I've updated this PR to include all of the built in metrics by default.
There was a problem hiding this comment.
Since this is experimental I think it's fine to include it by default but definitely worth keeping an eye on impact.
Cost is usually driven by cardinality so if the metrics have no tags it could be OK. cc @bitsandfoxes to own this conversation internally
There was a problem hiding this comment.
The metrics definitely have tags. For example, http.server.request.duration has 7 attributes, which add dimensionality represented as tags in Sentry. In most situations, that will result in 1 different metric per route configured in the ASP.NET Core application... but that's just one example.
Basically anything that's documented as an Attribute in the built in metrics documentation turns up as a tag in Sentry.
Maybe we don't need some of the built in metrics as the insights are already available via our Tracing instrumentation for many of them?
OK, interesting. So we have: Sentry DiagnosticSource integration
System DiagnosticsMetrics integration
Assuming we can bump the I am wondering whether that might create a clash however. If someone was running Seems like actually these would have to exist as separate packages then... So we'd need a new Sentry.DiagnosticSource.Metrics package that people could add if they're on net6.0 or below, and we just do the same trick for that that we've done already for Sentry.DiagnosticSource. |
I've added a separate issue to track that work: |
| /// See https://learn.microsoft.com/en-us/dotnet/core/diagnostics/built-in-metrics for more information. | ||
| /// </para> | ||
| /// </summary> | ||
| public IList<SubstringOrRegexPattern> CaptureSystemDiagnosticsInstruments |
There was a problem hiding this comment.
Since this is experimental I think it's fine to include it by default but definitely worth keeping an eye on impact.
Cost is usually driven by cardinality so if the metrics have no tags it could be OK. cc @bitsandfoxes to own this conversation internally
| where T: struct | ||
| { | ||
| var unit = MeasurementUnit.Parse(instrument.Unit); | ||
| var tagDict = tags.ToImmutableArray().ToImmutableDictionary( |
There was a problem hiding this comment.
Worth profiling this to see the impact. This being ReadOnlySpan and us allocating immutableArray then dictionary from it seems like could show up depending how often this is called.
The PR makes a start on implementing:
For this first integration we'd like to make it easy for Sentry SDK Users to be able to capture any of the Built In Metrics that are accessible via the System.Diagnostics.Metrics API.
SDK users can use SentryOptions.ExperimentalMetrics.CaptureInstruments to configure one or more
System.Diagnostics.Metrics.Instrumentthat they are interested in. TheSystemDiagnosticsMetricsIntegration(if enabled) will then capture any metrics for those instruments and send them to Sentry.Currently the integration supports the following Instruments:
Dependencies
This integration needs
System.Diagnostics.DiagnosticSource(version 8 I think) to work... Applications that target .NET 8+ include this reference by default so, for the time being, this integration is only available for .NET 8 or greater. I'm not sure if there's a way to enable the integration on previous TFMs if people are OK to addSystem.Diagnostics.DiagnosticSource... I was thinking maybe a separate NuGet package (with that as a dependency) for older TFMs but have it built in for .NET 8 or greater. Two things:At the very least, I think let's not do it in this PR (we can create a separate issue and PR for this if/when we decide to do it).