apm-agent-dotnet icon indicating copy to clipboard operation
apm-agent-dotnet copied to clipboard

Remove ASP.NET Core middleware and `ApmMiddlewareExtension`

Open gregkalapos opened this issue 3 years ago • 2 comments

With the Elastic.Apm.Extensions.Hosting package we support IHostBuilder based agent setup.

We also introduced AspNetCoreDiagnosticListener which does the same as the middleware does, but it works based on DiagnosticSource. Reason for introducing it was to 1) support DOTNET_STARTUP_HOOKS based, zero code change agent setup (it'd be very hard to register an ASP.NET Core middleware there) and 2) turn on ASP.NET Core monitoring as well with IHostBuilder.

With this now we created a situation that there are 2 ways to turn on the agent on ASP.NET Core:

This uses AspNetCoreDiagnosticListener:

Program.cs:

public class Program
{
	public static IHostBuilder CreateHostBuilder(string[] args) =>
		Host.CreateDefaultBuilder(args)
			.ConfigureWebHostDefaults(webBuilder => { webBuilder.UseStartup<Startup>(); })
			.UseAllElasticApm();
}

And this uses ApmMiddleware through ApmMiddlewareExtension:

Startup.cs:

public void Configure(IApplicationBuilder app, IHostingEnvironment env)
{
   app.UseAllElasticApm(Configuration);
   //..more code
}

The AspNetCoreDiagnosticListener and ApmMiddleware share all the logic already.

This is a bit confusing and it's a duplication.

Therefore, the idea is to obsolete the IApplicationBuilder setup with the ApmMiddleware and just go with IHostBuilder.

Potential issues:

  • Check if this works and what to do on old 2.x versions.

This would be a breaking change for existing users, so we need to have a grace period for this.

gregkalapos avatar Nov 10 '20 12:11 gregkalapos

@gregkalapos The AspNetCore 2.1 is the LTS version which Microsoft will be supporting until August 2021: https://dotnet.microsoft.com/platform/support/policy/dotnet-core

This means there will be many companies that will continue to have 2.1 applications until at least close to that date - it would be a shame to miss out on any new APM goodness between now and then if 2.1 is no longer supported.

Perhaps you could remove it for the NetCoreApp3.0 target and let us 2.1/NetStandard consumers wallow in confusion a bit longer? ;)

tbutler-qontigo avatar Nov 19 '20 06:11 tbutler-qontigo

I like the IApplicationBuilder setup because we only want to enable APM in some environments and this is the place where we distinguish between them with IWebHostEnvironment.IsProduction() etc. How would this look with IHostBuilder?

lenaschoenburg avatar Jun 02 '21 12:06 lenaschoenburg