-
-
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 all 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 | Sentry | 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 | ||
| { | ||
| private const string Title = "Unsupported numeric type of Metric"; | ||
| private const string MessageFormat = "{0} is unsupported type for Sentry Metrics. The only supported types are byte, short, int, long, float, and double."; | ||
| private const 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.Sentry, | ||
| 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 Sentry = nameof(Sentry); | ||
|
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: 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, The ideas / variants I am having are
|
||
| } | ||
| 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 |
|---|---|---|
|
|
@@ -15,11 +15,36 @@ | |
| <PrivateAssets>all</PrivateAssets> | ||
| <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets> | ||
| </PackageReference> | ||
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.3.1" PrivateAssets="all"/> | ||
| <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.3.1" PrivateAssets="all" /> | ||
| </ItemGroup> | ||
|
|
||
| <!-- | ||
| We use Simon Cropp's Polyfill source-only package to access APIs in lower targets. | ||
| https://github.com/SimonCropp/Polyfill | ||
| --> | ||
| <ItemGroup> | ||
| <PackageReference Include="Polyfill" Version="9.8.1" PrivateAssets="all" /> | ||
|
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. note: see also #4879 |
||
| </ItemGroup> | ||
| <!-- We currently don't require Polyfills for System.Memory. Ensure the feature is disabled and suppress the MSBuild Warning from Polyfill. --> | ||
| <Target Name="BeforePreparePolyfill" BeforeTargets="PreparePolyfill"> | ||
| <PropertyGroup Condition="'$(TargetFramework)' == 'netstandard2.0'"> | ||
| <PolyfillNoWarnIncorrectVersion>true</PolyfillNoWarnIncorrectVersion> | ||
| </PropertyGroup> | ||
| </Target> | ||
| <Target Name="AfterPreparePolyfill" AfterTargets="PreparePolyfill" DependsOnTargets="PreparePolyfill"> | ||
| <PropertyGroup Condition="'$(TargetFramework)' == 'netstandard2.0'"> | ||
| <DefineConstants>$([System.String]::Copy('$(DefineConstants)').Replace('FeatureMemory','').Replace(';;',';'))</DefineConstants> | ||
| </PropertyGroup> | ||
| </Target> | ||
|
|
||
| <ItemGroup> | ||
| <Using Remove="System.Text.Json" /> | ||
| <Using Remove="System.Text.Json.Serialization" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <AdditionalFiles Remove="AnalyzerReleases.Shipped.md" /> | ||
| <AdditionalFiles Remove="AnalyzerReleases.Unshipped.md" /> | ||
| </ItemGroup> | ||
|
|
||
| </Project> | ||
| 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( | ||
| 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: 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.
Choose a reason for hiding this comment
The 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.
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.
We drafted an alternative API shape, with explicit overloads.
But the devil is in the "API explosion":
And a follow-up uncertainty is about the Before-Send-Callback:
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.