feat(metrics): Trace-connected Metrics (Analyzers)#4840
feat(metrics): Trace-connected Metrics (Analyzers)#4840
Conversation
Flash0ver
added
Roslyn
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4840 +/- ##
==========================================
+ Coverage 73.87% 73.90% +0.02%
==========================================
Files 496 498 +2
Lines 17951 18002 +51
Branches 3516 3527 +11
==========================================
+ Hits 13262 13304 +42
Misses 3831 3831
- Partials 858 867 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Provides hierarchical constants for metric units supported by Sentry Relay, organized into Duration, Information, and Fraction categories. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…-metrics-analyzers
…-metrics-analyzers
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog. Features ✨
Fixes 🐛
Dependencies ⬆️Deps
Other
🤖 This preview updates automatically when you update the PR. |
|
|
||
| Rule ID | Category | Severity | Notes | ||
| --------|----------|----------|------- | ||
| SENTRY1001 | Support | Warning | TraceConnectedMetricsAnalyzer No newline at end of file |
There was a problem hiding this comment.
question: ID
I'm a a bit certain about the ID.
Also, I'm not sure if it should be two separate IDs, one for the Emit* methods, and one for SentryMetric.TryGetValue<TValue>(out TValue value) method.
| https://github.com/SimonCropp/Polyfill | ||
| --> | ||
| <ItemGroup> | ||
| <PackageReference Include="Polyfill" Version="9.8.1" PrivateAssets="all" /> |
| defaultSeverity: DiagnosticSeverity.Warning, | ||
| isEnabledByDefault: true, | ||
| description: Description, | ||
| helpLinkUri: null |
There was a problem hiding this comment.
question: docs
Should we document the Analyzer?
If so, we could provide a link to the documentation page here.
There was a problem hiding this comment.
Will do after the merge / after releasing it via #4962.
| private static readonly string Description = "Integers should be a 64-bit signed integer, while doubles should be a 64-bit floating point number."; | ||
|
|
||
| private static readonly DiagnosticDescriptor Rule = new( | ||
| id: DiagnosticIds.Sentry1001, |
There was a problem hiding this comment.
question: CHANGELOG
Should we mention this Analyzer, and Analyzer in general, in the CHANGELOG?
If so, in the Features category, or a separate category?
There was a problem hiding this comment.
I think the features section... they're basically there to improve the developer experience for SDK users right?
| /// Guide consumers to use the public API of <see href="https://develop.sentry.dev/sdk/telemetry/metrics/">Sentry Trace-connected Metrics</see> correctly. | ||
| /// </summary> | ||
| [DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
| public sealed class TraceConnectedMetricsAnalyzer : DiagnosticAnalyzer |
There was a problem hiding this comment.
question: do we want to do this in general?
This started off as a weekend-project for me.
Do we want to provide this Analyzer in the first place?
There was a problem hiding this comment.
To avoid this explosion of overloads per method group,
and be similar to the implementation of System.Diagnostics.Metrics,
we are not compile-time constraining unsupported types,
but are instead run-time constraining unsupported types (no-op and Debug-Diagnostic-Logging).
Maybe not... we're trying to create a compile time constraint here (using analysers) because we don't want to create a compile time constraint using the typing system???
It might be a better experience for SDK users and less effort for us to maintain, simply to add some overloads that convert the other numeric types automatically to supported types.
There was a problem hiding this comment.
We drafted an alternative API shape, with explicit overloads.
But the devil is in the "API explosion":
-public void EmitCounter<T>(string name, T value) where T : struct
+public void EmitCounter(string name, int value)
+public void EmitCounter(string name, long value)
+// for all EmitCounter overloads
+// for EmitGauge and EmitDistribution method groupsAnd a follow-up uncertainty is about the Before-Send-Callback:
options.Experimental.SetBeforeSendMetric(static metric =>
{
if (metric.TryGetValue(out int integer)) // if not emitted as `Int32`, do we want to cast to `Int32`?
if (metric.TryGetInt32(out int integer)) // similar "explosion" of methods
return metric;
});So we decided to keep the API shape as is and actually do go with the Analyzer.
Should we have made a mistake, we can change both the API and the Analyzer for the next major version.
|
|
||
| internal static class DiagnosticCategories | ||
| { | ||
| internal const string Sentry = nameof(Sentry); |
There was a problem hiding this comment.
question: Category
I'm a bit uncertain about the Category.
The Category can be used to e.g. configure all Diagnostic of a category, rather than or in addition to configuring specific diagnostics per ID,
e.g. via an .globalconfig file
is_global = true
# Configure this new rule only
dotnet_diagnostic.SENTRY1001.severity = none
# Configure all rules within the "Sentry" Category
dotnet_analyzer_diagnostic.category-Sentry.severity = none
The ideas / variants I am having are
- a single Category for all of Sentry's Diagnostic
- have a Category per Feature set
- e.g.
- Traces
- Logs
- Metrics
- e.g.
- re-use the same Categories that the Analyzers of the .NET SDK include
3 participants
based on
My little weekend-project:
Add a Diagnostic-Analyzer to our Compiler-Extensions, that warn users when a metric would not be emitted due to an unsupported numeric value type.
Context:
For the numeric type of a Metric, we currently allow 64-bit sized integral (signed) and floating-point numbers.
That means that e.g.
ulong,System.Int128, ordecimalare currently not supported by Sentry.The compile-time constraint of
Sentry.SentryMetric<T>only constrains to non-nullable value types.Alternatively, we could have implemented respectively types overloads for the method groups:
To avoid this explosion of overloads per method group,
and be similar to the implementation of System.Diagnostics.Metrics,
we are not compile-time constraining unsupported types,
but are instead run-time constraining unsupported types (no-op and Debug-Diagnostic-Logging).
To still warn users unfamiliar with the System.Diagnostics.Metrics.Meter-based APIs,
I built an Analyzer over some weekends to guide new users.