-
-
Notifications
You must be signed in to change notification settings - Fork 229
feat(metrics): Trace-connected Metrics (Analyzers) #4840
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 19 commits
6be1e01
e462457
6c5ca23
9485ca4
aa46acb
e9c349e
484f17f
e65db5f
de065c7
5228fbe
32caf06
98991a3
d730432
e7bb36f
d2a02e1
b87bc4d
4b4bc03
68a2c7a
9855d05
70a58d0
b90d71e
f168d5c
8af37bb
5d4128d
eb69de6
e8b9376
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ; Shipped analyzer releases | ||
| ; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| ; Unshipped analyzer release | ||
| ; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md | ||
|
|
||
| ### New Rules | ||
|
|
||
| Rule ID | Category | Severity | Notes | ||
| --------|----------|----------|------- | ||
| SENTRY1001 | Support | Warning | TraceConnectedMetricsAnalyzer | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| using Microsoft.CodeAnalysis; | ||
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using Microsoft.CodeAnalysis.Operations; | ||
|
|
||
| namespace Sentry.Compiler.Extensions.Analyzers; | ||
|
|
||
| /// <summary> | ||
| /// 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 | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: do we want to do this in general? This started off as a weekend-project for me.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| { | ||
| private static readonly string Title = "Unsupported numeric type of Metric"; | ||
| private static readonly string MessageFormat = "{0} is unsupported type for Sentry Metrics. The only supported types are byte, short, int, long, float, and double."; | ||
| 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, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: CHANGELOG Should we mention this Analyzer, and Analyzer in general, in the CHANGELOG?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the features section... they're basically there to improve the developer experience for SDK users right?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added CHANGELOG entry via 6c5ca23 |
||
| title: Title, | ||
| messageFormat: MessageFormat, | ||
| category: DiagnosticCategories.Support, | ||
| defaultSeverity: DiagnosticSeverity.Warning, | ||
| isEnabledByDefault: true, | ||
| description: Description, | ||
| helpLinkUri: null | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: docs Should we document the Analyzer?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do after the merge / after releasing it via #4962. |
||
| ); | ||
|
|
||
| /// <inheritdoc /> | ||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); | ||
|
|
||
| /// <inheritdoc /> | ||
| public override void Initialize(AnalysisContext context) | ||
| { | ||
| context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); | ||
| context.EnableConcurrentExecution(); | ||
|
|
||
| context.RegisterOperationAction(Execute, OperationKind.Invocation); | ||
| } | ||
|
|
||
| private static void Execute(OperationAnalysisContext context) | ||
| { | ||
| Debug.Assert(context.Operation.Language == LanguageNames.CSharp); | ||
| Debug.Assert(context.Operation.Kind is OperationKind.Invocation); | ||
|
|
||
| context.CancellationToken.ThrowIfCancellationRequested(); | ||
|
|
||
| if (context.Operation is not IInvocationOperation invocation) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var method = invocation.TargetMethod; | ||
| if (method.DeclaredAccessibility != Accessibility.Public || method.IsStatic || method.Parameters.Length == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (!method.IsGenericMethod || method.Arity != 1 || method.TypeArguments.Length != 1) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (method.ContainingAssembly is null || method.ContainingAssembly.Name != "Sentry") | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (method.ContainingNamespace is null || method.ContainingNamespace.Name != "Sentry") | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| string fullyQualifiedMetadataName; | ||
| if (method.Name is "EmitCounter" or "EmitGauge" or "EmitDistribution") | ||
| { | ||
| fullyQualifiedMetadataName = "Sentry.SentryMetricEmitter"; | ||
| } | ||
| else if (method.Name is "TryGetValue") | ||
| { | ||
| fullyQualifiedMetadataName = "Sentry.SentryMetric"; | ||
| } | ||
| else | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var typeArgument = method.TypeArguments[0]; | ||
| if (typeArgument.SpecialType is SpecialType.System_Byte or SpecialType.System_Int16 or SpecialType.System_Int32 or SpecialType.System_Int64 or SpecialType.System_Single or SpecialType.System_Double) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (typeArgument is ITypeParameterSymbol) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var sentryType = context.Compilation.GetTypeByMetadataName(fullyQualifiedMetadataName); | ||
| if (sentryType is null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (!SymbolEqualityComparer.Default.Equals(method.ContainingType, sentryType)) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var location = invocation.Syntax.GetLocation(); | ||
| var diagnostic = Diagnostic.Create(Rule, location, typeArgument.ToDisplayString(SymbolDisplayFormats.FullNameFormat)); | ||
| context.ReportDiagnostic(diagnostic); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| namespace Sentry.Compiler.Extensions; | ||
|
|
||
| internal static class DiagnosticCategories | ||
| { | ||
| internal const string Support = nameof(Support); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| namespace Sentry.Compiler.Extensions; | ||
|
|
||
| internal static class DiagnosticIds | ||
| { | ||
| internal const string Sentry1001 = "SENTRY1001"; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| using Microsoft.CodeAnalysis; | ||
|
|
||
| namespace Sentry.Compiler.Extensions; | ||
|
|
||
| internal static class SymbolDisplayFormats | ||
| { | ||
| internal static SymbolDisplayFormat FullNameFormat { get; } = new SymbolDisplayFormat( | ||
| globalNamespaceStyle: SymbolDisplayGlobalNamespaceStyle.Omitted, | ||
| typeQualificationStyle: SymbolDisplayTypeQualificationStyle.NameAndContainingTypesAndNamespaces, | ||
| genericsOptions: SymbolDisplayGenericsOptions.IncludeTypeParameters | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 forSentryMetric.TryGetValue<TValue>(out TValue value)method.