Add analyzer to warn about wrong UseGoogleTrace/UseMvc order #2054

evildour · 2018-04-05T14:37:48Z

Resolves #1982

Recommend to review one commit at a time.

This also adds in support for easily auto-generating more analyzers in the future.
Analyzers are packaged into the nupkg file of the target assembly, but we can change that to make them their own NuGet packages if necessary.
My only question is about how we want to assign rule ids. I was using a "GCPxxxx" format for our internal analyzers, from 0001-0004 currently, but I think that format works nicely for the public analyzers as well, so I assigned this one to use "GCP0005". However, if we want a clean numbering from 1 for all the public analyzers, we can use a different format and/or range for the internal ones. Just let me know and I'll change the ids. I don't feel too strongly about it, but we should figure this out before release because changing it later would be a breaking change (rule severity customizations and in-source error suppressions wouldn't work after a change).

jskeet · 2018-04-05T14:47:28Z

I'm fine for us to mix and match between internal and public, so long as everything is still unique - I don't think users will attach particular meaning.

I probably won't be able to review this until Tuesday, but I'll ask friends in the Roslyn team to have a look in terms of packaging.

evildour · 2018-04-05T14:48:27Z

Great, thanks!

evildour · 2018-04-05T15:01:37Z

Travis failure looks like it may be this: dotnet/msbuild#1957. There is a workaround here: dotnet/msbuild#1957 (comment), which I'll try.

codecov · 2018-04-05T15:10:42Z

Codecov Report

Merging #2054 into master will decrease coverage by 27.89%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2054      +/-   ##
==========================================
- Coverage   55.85%   27.96%   -27.9%     
==========================================
  Files         516       58     -458     
  Lines       15673      937   -14736     
==========================================
- Hits         8754      262    -8492     
+ Misses       6919      675    -6244

Impacted Files	Coverage Δ
...le.Cloud.Diagnostics.Common/Trace/SpanIdFactory.cs	`0% <0%> (-100%)`	⬇️
...d.Diagnostics.Common/Trace/InternalTraceOptions.cs	`0% <0%> (-100%)`	⬇️
...e.Cloud.Diagnostics.Common/Trace/StopwatchTimer.cs	`0% <0%> (-100%)`	⬇️
...d.Diagnostics.Common/Trace/ManagedTracerFactory.cs	`0% <0%> (-100%)`	⬇️
...Cloud.Diagnostics.Common/Trace/StartSpanOptions.cs	`0% <0%> (-100%)`	⬇️
...d.Diagnostics.Common/ErrorReporting/EventTarget.cs	`0% <0%> (-100%)`	⬇️
....Cloud.Diagnostics.Common/FlushableConsumerBase.cs	`0% <0%> (-100%)`	⬇️
....Common/Google.Cloud.Diagnostics.Common/Project.cs	`0% <0%> (-100%)`	⬇️
...ogle.Cloud.Diagnostics.Common/Trace/TraceLabels.cs	`3.12% <0%> (-96.88%)`	⬇️
...Cloud.Diagnostics.Common/SizedBufferingConsumer.cs	`0% <0%> (-94.45%)`	⬇️
... and 476 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb50bf0...c6a3151. Read the comment docs.

iantalarico

Just a first pass, I need to read up a little on some of this and will take another pass at it.

This is pretty cool, thanks for doing this Mike!

Not sure if it's worth doing here or in another PR but we should probably do the same for error reporting bits.

analyzerScripts/install.ps1

@@ -0,0 +1,58 @@
+param($installPath, $toolsPath, $package, $project)


analyzerScripts/install.ps1

@@ -0,0 +1,58 @@
+param($installPath, $toolsPath, $package, $project)


analyzerScripts/install.ps1

@@ -0,0 +1,58 @@
+param($installPath, $toolsPath, $package, $project)


analyzerScripts/install.ps1

@@ -0,0 +1,58 @@
+param($installPath, $toolsPath, $package, $project)


analyzerScripts/uninstall.ps1

+                }
+                catch
+                {
+


tools/Google.Cloud.Tools.Analyzers/MissingCopyrightNoticeAnalyzer.cs

@@ -93,7 +93,8 @@ public class MissingCopyrightNoticeAnalyzer : DiagnosticAnalyzer
                "//------------------------------------------------------------------------------\n// <auto-generated>",
                "//------------------------------------------------------------------------------\r\n// <auto-generated>",
                "// Generated by the protocol buffer compiler.  DO NOT EDIT!",
-                "// Generated by the MSBuild WriteCodeFragment class.");
+                "// Generated by the MSBuild WriteCodeFragment class.",
+                "// This file was automatically created from");


tools/Google.Cloud.Tools.ProjectGenerator/Program.cs

+                var analyzersDirectory = Path.Combine(apiRoot, api.Id + ".Analyzers");
+                if (!Directory.Exists(analyzersDirectory))
+                {
+                    Directory.CreateDirectory(analyzersDirectory);


tools/Google.Cloud.Tools.ProjectGenerator/Program.cs

+
+                AddPackFile(
+                    $@"..\{api.Id}.Analyzers\bin\$(Configuration)\{AnalyzersTargetFramework}\{api.Id}.Analyzers.dll",
+                    "analyzers/dotnet/cs");


...ore/Google.Cloud.Diagnostics.AspNetCore.Analyzers.Tests/GoogleTraceBeforeMvcAnalyzerTests.cs

+            base.FinalizeSolution(projectId, ref solution);
+        }
+    }
+}


...ics.AspNetCore/Google.Cloud.Diagnostics.AspNetCore.Analyzers/GoogleTraceBeforeMvcAnalyzer.cs

+        private const string UseGoogleTrace = "UseGoogleTrace";
+        private const string UseMvc = "UseMvc";
+
+        private static readonly LocalizableString Title =


evildour · 2018-04-06T16:43:12Z

What need's to be done for the error reporting bits? Is it the same concept of an ordering requirement?

iantalarico · 2018-04-06T16:47:41Z

@evildour Yea it's pretty much the same thing it needs to be called before things you want uncaught exceptions logged for MVC is probably where the bulk of this would be.

iantalarico · 2018-04-06T16:48:36Z

@evildour Hit enter too soon. It's probably worth doing error reporting in a separate PR as this is already fairly large.

evildour · 2018-04-06T16:52:16Z

Yeah I agree. I'd like to nail down the packaging strategy first so we can get this in. Then I'll generalize it in another PR.

evildour · 2018-04-24T12:14:07Z

Ok now the analyzers will be their own package. PTAL.

jskeet

I reviewed the tests more than the implementation, but it looks okay. Is there anything specific you want me to pay more attention to?

...ers/Google.Cloud.Diagnostics.AspNetCore.Analyzers.Tests/GoogleTraceBeforeMvcAnalyzerTests.cs

+{
+    public class GoogleTraceBeforeMvcAnalyzerTests : CodeFixVerifier
+    {
+        private static readonly MetadataReference RuntimeReference =


...ers/Google.Cloud.Diagnostics.AspNetCore.Analyzers.Tests/GoogleTraceBeforeMvcAnalyzerTests.cs

+        }
+
+        [Fact]
+        public void SeparateChecksInSeaprateScopes()


...gnostics.AspNetCore.Analyzers/Google.Cloud.Diagnostics.AspNetCore.Analyzers/RoslynHelpers.cs

+    {
+        internal static ITypeSymbol Type(this ISymbol symbol)
+        {
+            switch (symbol.Kind)


evildour · 2018-04-24T13:26:19Z

I guess just the package details because I think we should publish these analyzers after this PR is merged.

Note: I realized I forgot to actually look in the package to make sure things were correct before, and they weren't, so I also made a few fixes there. PTAL at the project generator.

jskeet

Changes seem reasonable. That said, the project generator is definitely growing in a somewhat Vogon-like way... we may want to take a step back and see if the current requirements could be better implemented with an overhaul. I'm slightly surprised we've managed to bend it this far :)

evildour · 2018-04-24T14:46:44Z

Yeah, that might be a good idea, but in a different PR. Are you ok if I publish this right after merging?

@iantalarico are you still taking a look here or are you good?

… can be packaged

evildour · 2018-04-25T13:29:32Z

(rebase)

evildour assigned jskeet and iantalarico Apr 5, 2018

iantalarico reviewed Apr 6, 2018

View reviewed changes

evildour force-pushed the trace-analyzer branch from 998f356 to e33a684 Compare April 24, 2018 12:13

jskeet approved these changes Apr 24, 2018

View reviewed changes

evildour added 13 commits April 25, 2018 09:28

Add analyzer supporting files and testing helper project

eda4ed2

Update ProjectGenerator to support adding analyzers to projects

03dc3dc

Update apis.json for Diagnostics.AspNetCore analyzers

35ea506

Fix incorrect project reference with analyzer test generation

639abcc

Regenerate projects

bb6f2e9

Add analyzer to warn about wrong UseGoogleTrace/UseMvc order

2eb7eb1

Work around issues with project dependencies on Linux

feed274

Address PR comments

dfbd025

Some minor cleanup

ca0513e

Update project generator to create root-level analyzer projects which…

b52c2cb

… can be packaged

Update apis.json to create root level analyzers

28cc6d7

Move analyzer files to new locations

7de54f9

Regenerate projects and clean up old files

8c0167e

Address PR comments and fix some issues with the package contents

c6a3151

evildour force-pushed the trace-analyzer branch from c1172dd to c6a3151 Compare April 25, 2018 13:29

evildour merged commit 0e9dfea into googleapis:master Apr 25, 2018

evildour deleted the trace-analyzer branch April 25, 2018 13:36

		@@ -0,0 +1,58 @@
		param($installPath, $toolsPath, $package, $project)

Add analyzer to warn about wrong UseGoogleTrace/UseMvc order #2054

Add analyzer to warn about wrong UseGoogleTrace/UseMvc order #2054

Conversation

evildour commented Apr 5, 2018

jskeet commented Apr 5, 2018

evildour commented Apr 5, 2018

evildour commented Apr 5, 2018

codecov bot commented Apr 5, 2018 • edited Loading

Codecov Report

iantalarico left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

evildour commented Apr 6, 2018

iantalarico commented Apr 6, 2018

iantalarico commented Apr 6, 2018

evildour commented Apr 6, 2018

evildour commented Apr 24, 2018

jskeet left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

evildour commented Apr 24, 2018

jskeet left a comment

Choose a reason for hiding this comment

evildour commented Apr 24, 2018

evildour commented Apr 25, 2018

codecov bot commented Apr 5, 2018 •

edited

Loading