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

Don't use static Agent API on APM middleware registration

Open eduardocampano opened this issue 4 years ago • 11 comments

UseElasticApm function relies on static Agent.Setup(config), which throws if registering the middleware was already done. But this is not considering the scenario where more than one IHost or IWebHost instances can be running on the same process.

I think the common practice is registering a new IApmAgent instance as singleton to IServiceCollection and injecting it into the middleware. AutoMapper had the same issue previously and they fixed it following this approach and they just removed the static API for good in version 9.

That said, having IApmAgent registered in DI allows injecting into controllers/services instead of relying on statics as documentation recommends.

Furthermore, current configuration approach only allows configuring the Agent form IConfiguration and would be flexible to have an Options Builder approach like EF Core has.

eduardocampano avatar Aug 13 '19 02:08 eduardocampano

I agree that UseElasticApm shouldn't rely on static fields and static API should be deprecated.

We already have an issue with integration tests in our Web application that uses APM agent. We are using WebApplicationFactory as explained here: https://docs.microsoft.com/en-us/aspnet/core/test/integration-tests?view=aspnetcore-2.2#customize-webapplicationfactory

When we inject сustomized WebApplicationFactory into different test classes (via IClassFixture as explained in article) then multiple instances created and method UseElasticApm called multiple times. So we catch exception: "The singleton APM agent has already been instantiated and can no longer be configured".

We have that problem only with APM agent and other middlewares work just fine.

OlegUfaev avatar Sep 05 '19 11:09 OlegUfaev

What's the progress on this issue? To me this is a pretty big issue, as it bascally means all integration tests fail.

Are there any known work-arounds?

mterwoord avatar Sep 09 '19 15:09 mterwoord

This is a dumb workaround, but it works:

            try
            {
                app.UseAllElasticApm(_configuration);
            }
            catch (Exception ex)
            {
                // ignore this error
            }

You could, of course, make it more sophisticated.

pburrows avatar Sep 11 '19 19:09 pburrows

Thanks. But that only enables it for the first integration test, not the rest. Also it can hide errors occurring in production. What I did now was to run integration tests with Environment=Test, and then use that to exclude registering ElasticApm

mterwoord avatar Sep 12 '19 06:09 mterwoord

@mterwoord, as legitimate workaround for xUnit integration tests is using AssemblyFixture. It doesn't affect you source code of host configuration. However, AssemblyFixture isn't suitable for all. In case of unit tests, you can use some portion of magic instead of static API.

P.S.: I don't justify static api, only suggest workaround to not block using Elastic.Apm as it's awesome product.

vhatsura avatar Sep 13 '19 08:09 vhatsura

Hey :) Any news on that? I'm mostly interested in using non-static API since we have not only http but also async events with rabbitmq and I'm not sure if static API will be working properly in that scenario.

mdrgeniebelt avatar Apr 29 '20 14:04 mdrgeniebelt

@Mpdreamz, do you know if there is any plan to fix the issue?

arxange1 avatar Jun 11 '20 21:06 arxange1

We released the Elastic.Apm.Extensions.Hosting package which I believe will make this better - it's currently beta, in the next release we can remove the beta flag. That package has an extension method for IHostBuilder and you don't need to register a middleware anymore, it basically creates the agent and adds its components to IServiceCollection.

It's also part of the NetCoreAll package that most of the people use.

Just do this in when you create your hostbuilder

public static IHostBuilder CreateHostBuilder(string[] args) =>
	Host.CreateDefaultBuilder(args)
		.ConfigureWebHostDefaults(webBuilder => { webBuilder.UseStartup<Startup>(); })
		.UseAllElasticApm(); //<- this is from `Elastic.Apm.NetCoreAll` and uses the Elastic.Apm.Extensions.Hosting package

Once you do that, you'll be able to dependency inject a tracer - for example:

public class HomeController : Controller
{
	private readonly ITracer _tracer;

	public HomeController(ITracer tracer) => _tracer = tracer; //you get the tracer from DI

	public IActionResult SomeMethod()
	{
		//now you can use the non static API
		var span = _tracer.CurrentSpan?.StartSpan("MySpan", "SampleSpanType");

		try
		{
			//...app code
		}
		finally
		{
			span?.End();
		}

		return View();
	}
}

Sometimes I see people wanting the whole agent instance - I don't understand why people want that (happy to hear what you want to do with it), nevertheless, you can also inject an IApmAgent instance into your class. I believe most people want to manually create spans and transactions and for that I'd suggest injecting an ITracer - the tracer is the one that let's you create those.

Now, in the latest beta release of this package I believe we don't have ASP.NET Core monitoring (which is a huge missing thing), but that's already on master, so in the next release that'll be there and you can try it already on master.

Also, I'd like to point out that there is an Agent.IsConfigured property which lets you avoid the double initialization problem.

On the "static Agent API" part: I understand everyone's concerns, I hope the approach above satisfies peoples needs - about the reasons why we can't drop it I wrote here and maybe this here is also related.

Let me know if this would help with everyone's problem here. Also maybe give a try to this new package.

gregkalapos avatar Oct 07 '20 19:10 gregkalapos

That package has an extension method for IHostBuilder and you don't need to register a middleware anymore

@gregkalapos I just transitioned my current project to it, this resolves any double initialization problems (race conditions.. one in 4 startups is lucky) I've had with multiple IHostedServices next to the ASP.NET Core stack but then no HTTP traces are created.

After playing around, this is the only working single configuration I could come up with that works under all cirumcstances:

    public static class HostBuilderExtensions
    {
        public static IHostBuilder UseApm(this IHostBuilder hostBuilder)
        {
            hostBuilder
                .UseElasticApm(
                    new HttpDiagnosticsSubscriber(),
                    new EfCoreDiagnosticsSubscriber(),
                    new SqlClientDiagnosticSubscriber(),
                    new ElasticsearchDiagnosticsSubscriber(),
                    new AspNetCoreDiagnosticsSubscriber()) // not included when calling hostBuilder.UseAllElasticApm()
                .ConfigureServices(svc =>
                {
                    // custom project-specific helpers and extensions for working with transactions & spans and custom log context enrichment
                    svc.AddSingleton<IApmCorrelationHelper, ApmCorrelationHelper>();
                    svc.AddSingleton<MvcActionObservabilityFilter>();
                    svc.Configure<MvcOptions>(
                        mvcOptions => mvcOptions.Filters.Add(
                            new ServiceFilterAttribute(typeof(MvcActionObservabilityFilter)) { IsReusable = true }));

                    // fix missing HTTP transactions when using Elastic.Apm.Extensions.Hosting APIs
                    svc.AddTransient<IStartupFilter, ApmStartupFilter>();
                    if (Type.GetType("Elastic.Apm.Api.Tracer, Elastic.Apm") is { } tracerType
                        && Type.GetType("Elastic.Apm.ApmAgent, Elastic.Apm") is { } agentType)
                    {
                        svc.AddSingleton(tracerType, sp => sp.GetRequiredService<ITracer>());
                        svc.AddSingleton(agentType, sp => sp.GetRequiredService<IApmAgent>());
                    }
                });
            return hostBuilder;
        }

        private class ApmStartupFilter : IStartupFilter
        {
            public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next)
            {
                return builder =>
                {
                    if (Type.GetType("Elastic.Apm.AspNetCore.ApmMiddleware, Elastic.Apm.AspNetCore") is { } apmMiddleware)
                    {
                        builder.UseMiddleware(apmMiddleware);
                    }

                    next(builder);
                };
            }
        }
    }

Am I missing some configuration option that provides a single configuration and allows for ASP.NET Core + IHostedService usage without startup race issues? (plus I really don't want to miss some transactions that happen at the app start in these IHostedServices)

Since this needs reflection to access internal types, I'd be happy for a version that works with public API only.

dasMulli avatar Oct 14 '20 20:10 dasMulli

With regards to workarounds with Xunit integration tests and double initialization of Agent, one workaround is to ensure that each Xunit project only has a single class that takes a WebApplicationFactory. I had to do this because I'm using a library that consumes APM and I cannot control how it gets bootstrapped.

triple-vee avatar Jun 23 '21 05:06 triple-vee

Sometimes I see people wanting the whole agent instance - I don't understand why people want that (happy to hear what you want to do with it), nevertheless, you can also inject an IApmAgent instance into your class. I believe most people want to manually create spans and transactions and for that I'd suggest injecting an ITracer - the tracer is the one that let's you create those.

One use case where it seems you cannot use ITracer is StackExchengeRedisCache for IDistributedCache.

options.ConnectionMultiplexerFactory = async () =>
{
  var connection = await ConnectionMultiplexer.ConnectAsync(options.ConfigurationOptions.ToString());
  connection.UseElasticApm();
  return connection;
};

You need to pass IApmAgent instance to .UseElasticApm method.

https://discuss.elastic.co/t/how-do-i-apply-elastic-apm-stackexchange-redis-to-microsofts-idistributedcache/292324

OskarsPakers avatar Mar 13 '24 06:03 OskarsPakers