apm-agent-dotnet
apm-agent-dotnet copied to clipboard
elastic
[BUG] UseElasticApm calls BuildServiceProvider which may cause multiple instantiations of Singletons
APM Agent version
1.13.0
Environment
Operating system and version: Windows 10
.NET Framework/Core name and version (e.g. .NET 4.6.1, NET Core 3.1.100) : .NET 5
Application Target Framework(s) (e.g. net461, netcoreapp3.1): .NET 5
Describe the bug
I'm not sure this qualifies as a bug per se, but I experienced some unwanted interactions between the Serilog Elastic sink's durable mode and the Elastic APM Agent.
Due to a call to BuildServiceProvider (here https://github.com/elastic/apm-agent-dotnet/blob/a6554a298ab9fe5f350a20fa32827c842ba396d7/src/Elastic.Apm.Extensions.Hosting/HostBuilderExtensions.cs#L70), some of my singleton services were instantiated twice (once by .UseAllElasticApm and a second time by my own HostBuilder.Build() call). This becomes a problem when using the "durable mode" with the Elastic sink, since it creates and holds file handles for a long time, which causes a sort of deadlock when there's two of them running at the same time.
Code example (this doesn't log anything to Elastic due to aforementioned "deadlock"):
Host.CreateDefaultBuilder(args)
.ConfigureServices(services =>
{
services.AddHostedService<SampleHostedService>();
})
.UseSerilog((context, loggerConfiguration) =>
{
loggerConfiguration.WriteTo.Elastic(new ElasticsearchSinkOptions(new Uri("http://localhost:9200"))
{
BufferBaseFilename = "test"
});
})
.UseAllElasticApm()
.Build()
It's generally bad form to call BuildServiceProvider since it creates these (not always) subtle issues.
- https://stackoverflow.com/a/56058498
- https://github.com/dotnet/aspnetcore/issues/14587#issuecomment-545723876
- https://github.com/Xabaril/AspNetCore.Diagnostics.HealthChecks/issues/351
To Reproduce
See the code example above.
Expected behavior
Logging isn't instantiated twice.
Actual behavior
Logging was instantiated twice.
We are affected by this as well. As mentioned, calling BuildServiceProvider manually is really a hack that should not be done in a library like this.
After spending quite some time with this problem, it turns out this is more problematic than I initially expected.
As @cvium already noted in the initial issue description, the root cause of this problem is the call to BuildServiceProvider() in the UseElasticApm extension method. This materializes all dependencies along the way and violates e.g. the singleton expectation for the logger instance as described.
The current expectation as documented by the unit test method HostBuilderExtensionTests.IsAgentInitializedAfterUseElasticApm is that after the call to UseElasticApm (or is it actually supposed to be the call to Build 🤔 ?), the Agent is instantiated and configured.
If we want to keep this behavior, we need to make sure to instantiate the agent without calling BuildServiceProvider.
This however creates following dependency problem: Agent -> AgentComponents -> IPayloadSender.
The only way to retrieve an IPayloadSender instance is via calling GetService which requires an IServiceProvider. So this is a dead end (especially for testing scenarios where a non-default IPayloadSender should be easily injectable). If this is not a concern for actual production scenarios, we might find a workaround though. However, I definitely need your input on this @gregkalapos.
Alternatively, we could delay the instantiation of Agent to a later point in time (when called via GetService). However, this would be a breaking change.
I'm hitting this as well.
It looks like calling UseElasticApm very early in the IHostBuilder chain may work around most of the problems, since then the BuildServiceProvider call materializes fewer dependencies. But developers typically need to set up configuration and logging before UseElasticApm (since you want APM to use what you've set up for those), and if either of those are sensitive to the BuildServiceProvider call you're out of luck. (As cvium noted, some uses of Serilog are that way.)
