Feature Request: Warn the user if they call app.UseGoogleTrace() after app.UseMvc()

[SurferJeffAtGoogle]

If they call app.UseGoogleTrace() after app.UseMvc() they'll see no errors and no warnings and no traces, and have no clue why.

If there is a way to detect and warn users in this scenario, that would save them a lot of time.

[iantalarico]

Looking at the IApplicationBuilder interface it doesn't provide a way for us to do this.

[evildour]

I wonder if this is something a Roslyn analyzer can help with. I believe you can distribute analyzers with the nuget package (Roslyn itself does this).

I can try to look into the feasibility of the analyzer if we want to do something like that.

[iantalarico]

@evildour If you don't mind taking a look into this that would be great

[evildour]

I don't mind at all. This is the fun stuff :) Ok, so I'll re-open this then.

[iantalarico]

Perfect, thanks Mike!

[evildour]

After some prototyping, it seems easy to do the analysis for chained calls, like app.UseMvc().UseGoogleTrace(), even if there are other builder calls in between the two. I'm not sure yet about something like this though:

app.UseMvc();
app.UseGoogleTrace();

Also, it seems relatively easy to get the analyzer used for users of the library: as long as the library includes the analyzer as one of it's own nuget packages (and not a normal reference), anything referencing the Google.Cloud.Diagnostics.AspNetCore library (via nuget or any kind of reference) will have the analyzers run as well.

I'm a little ignorant about the problem here and how it might come up. Can you provide a bit more info so I can try to catch as many issues as possible and provide a helpful message?

• What happens when things are in the wrong order?
• Will the problem occur with any of the UseMvc... extensions, like UseMvcWithDefaultRoute?
• Are there any other methods (extension or otherwise) available on IApplicationBuilder which would have the same issue?
• Is it normal to use chained calls always, or will some users prefer to use separate statements, as in the code snippet above?
• If possible, should we include a code fix provider to automatically move UseGoogleTrace before UseMvc?

[iantalarico]

The ordering of these depends on how they are called so for example if we have:

app.UseMvc();
app.UseGoogleTrace();

It's calls will end up like this:

// state mvc request
// start trace
// end trace
// end mvc request.

Where if we do the opposite we have:

app.UseGoogleTrace();
app.UseMvc();

Which leads to:

// start trace
// state mvc request
// end mvc request.
// end trace

Responses to your questions:

1. In the first case we literately trace nothing. While in the second we trace the mvc request. Ideally we would like the UseGoogleTrace before most things so we can trace the full request.

2. (and 3) Yes this could happen with any extension. The goal with this is to catch some cases which are probably and accident. (unlikely a user wants to use trace and not capture mvc requests). We can always add more checks as we see issues.

3. It can be used either way.

4. No I don't think we should ever fix it as there could be reasons for this just a warning so if they do not see traces and look at logs they will see the message.

[evildour]

Ok thanks. I have an idea for how to analyze the separate statements approach. It isn't pretty, but might work. I'll try to prototype this later today or next week.

As for the code fix provider, these are opt-in: the user has to choose to apply them, so it won't automatically fix intentional code which violates the analyzer's checks.

[henkmollema]

I like this feature, but I'd like to point out that using the new UseGoogleDiagnostics extension method will eliminate this problem for most applications.

[evildour]

Sorry @henkmollema, I just saw your comment now. I'll publish the analyzer for now and discuss with @iantalarico about what we can do regarding the new method. Is it equivalent? Would it make sense for the analyzer to simply instruct people to use that instead?

[henkmollema]

It is the equivalent of configured Trace, Logging and Error Reporting Middleware. The startup filter makes sure it is configured before any user middleware is added:

https://github.com/GoogleCloudPlatform/google-cloud-dotnet/blob/0e9dfea904ea70b9a1dab3712f23acff041d987a/apis/Google.Cloud.Diagnostics.AspNetCore/Google.Cloud.Diagnostics.AspNetCore/GoogleDiagnosticsStartupFilter.cs#L53-L63

[evildour]

I don't think there's anything to change here. I think if people want to use UseGoogleTrace on their own, the analyzer will continue to provide the guardrails. It shouldn't be the responsibility to the anlayzer to direct people to use UseGoogleDiagnostics instead since it does more than UseGoogleTrace on its own and users might not realize that.

Do you agree @iantalarico?

[iantalarico]

@evildour Yes I agree. I'm sure many people will use UseGoogleDiagnostics but for those who don't this will help them

[henkmollema]

Agreed 😄