HostBuilderExtensions.UseElasticApm: subscribe to subscribers params

[gregkalapos]

Fixes #1059

In Elastic.Apm.Extensions.Hosting.HostBuilderExtensions.UseElasticApm(this IHostBuilder builder, params IDiagnosticsSubscriber[] subscribers) the params IDiagnosticsSubscriber[] subscribers were never activated. This was because of the following wrong check:

if(Agent.IsConfigured && Agent.Config.Enabled)
  if (subscribers != null && subscribers.Any() && Agent.IsConfigured) Agent.Subscribe(subscribers);

Problem here was that Agent.IsConfigured remained false until IApmAgent got resolved first - but that always runs after the check.

Proposed fix: we force the resolution directly within the UseElasticApm method:

var serviceProvider = services.BuildServiceProvider();
var agent = serviceProvider.GetService<IApmAgent>();

And do a similar check after that.

I feel doing "lazy agent loading" here makes no sense, the agent should be immediately enabled, otherwise it would e.g. not collect metrics, plus there is no guarantee that a user forces to resolve IApmAgent.

[apmmachine]

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

• Build Cause: [Pull request #1060 updated]

• Start Time: 2020-11-24T13:56:55.216+0000

• Duration: 58 min 8 sec

Test stats 🧪

Test	Results
Failed	0
Passed	13021
Skipped	0
Total	13021

Steps errors

Expand to view the steps failures

Build

• Took 0 min 24 sec . View more details on here
• Description: .ci/windows/dotnet.bat

[codecov-io]

Codecov Report

Merging #1060 (4e22ddb) into master (5b45e67) will increase coverage by 45.83%.
The diff coverage is 94.73%.

@@             Coverage Diff             @@
##           master    #1060       +/-   ##
===========================================
+ Coverage   33.08%   78.92%   +45.83%     
===========================================
  Files         120      148       +28     
  Lines        6190     7259     +1069     
===========================================
+ Hits         2048     5729     +3681     
+ Misses       4142     1530     -2612     

Impacted Files	Coverage Δ
...ic.Apm.Extensions.Hosting/HostBuilderExtensions.cs	84.28% <84.61%> (ø)
...tic.Apm.SqlClient/SqlClientDiagnosticSubscriber.cs	86.66% <100.00%> (ø)
src/Elastic.Apm/Logging/ConsoleLogger.cs	96.72% <100.00%> (+30.05%)	⬆️
src/Elastic.Apm/Report/PayloadSenderV2.cs	88.62% <100.00%> (+88.62%)	⬆️
...rc/Elastic.Apm.Extensions.Hosting/NetCoreLogger.cs	85.71% <0.00%> (ø)
...Elasticsearch/HttpConnectionDiagnosticsListener.cs	0.00% <0.00%> (ø)
...astic.Apm.SqlClient/SqlClientDiagnosticListener.cs	76.08% <0.00%> (ø)
...icListener/AspNetCoreErrorDiagnosticsSubscriber.cs	90.90% <0.00%> (ø)
...c.Apm.GrpcClient/GrpcClientDiagnosticSubscriber.cs	90.90% <0.00%> (ø)
...DiagnosticListener/AspNetCoreDiagnosticListener.cs	85.71% <0.00%> (ø)
... and 131 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 dbad2d5...4e22ddb. Read the comment docs.

[russcam]

LGTM 👍

[russcam]

can agent components be resolved once, and passed to both agent instance and Agent.Setup(...) i.e.

var components = sp.GetService<AgentComponents>();
var newAgentInstance = new ApmAgent(components );
Agent.Setup(components);					
return newAgentInstance;

Also, does this mean there are potentially two instances of an agent in the application - a singleton instance created by this method, and the lazy instance on Agent that's used internally?

[gregkalapos]

Also, does this mean there are potentially two instances of an agent in the application - a singleton instance created by this method, and the lazy instance on Agent that's used internally?

Hahh - that's a good point - we should not new up an ApmAgent instance here. Technically they were 2 instances, but they were sharing the AgentComponents, but still - best is to just return Agent.Instance. I changed this part.

can agent components be resolved once, and passed to both agent instance and Agent.Setup(...) i.e.

With the previous change this is not needed. Otherwise I'd agree.

[russcam]

Should there also be a test to assert that subscribers are not subscribed when the agent is not enabled?

[gregkalapos]

Agreed - I added a test.