Summary of changes

• Bump the timeout in BatchingSink.AfterMultipleFailures_SinkIsPermanentlyDisabled()
• Delete unused code
• Update StatsTests so it correctly shuts down all background processes
• Add extra test for DiscoveryService to confirm that we don't check in a tight loop

Reason for change

Bump the timeout in BatchingSink.AfterMultipleFailures_SinkIsPermanentlyDisabled()

I have seen this fail a couple of times in seemingly unrelated PRs. Not sure exactly why, probably nothing, but given it's failing on a mutex timeout, the easy fix is to bump that up.

Update StatsTests so it correctly shuts down all background processes

When investigating a failing integration test, I noticed that the log file was flooded with warnings like these (130,000 lines of warnings!)

2022-09-15 14:00:21.852 +00:00 [WRN] Error discovering available agent services System.Net.WebException: Connection refused Connection refused
 ---> System.Net.Http.HttpRequestException: Connection refused
 ---> System.Net.Sockets.SocketException (111): Connection refused
   at System.Net.Http.ConnectHelper.ConnectAsync(String host, Int32 port, CancellationToken cancellationToken)
   --- End of inner exception stack trace ---
   at System.Net.Http.ConnectHelper.ConnectAsync(String host, Int32 port, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.ConnectAsync(HttpRequestMessage request, Boolean allowHttp2, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.CreateHttp11ConnectionAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.GetHttpConnectionAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.SendWithRetryAsync(HttpRequestMessage request, Boolean doRequestAuth, CancellationToken cancellationToken)
   at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.FinishSendAsyncUnbuffered(Task`1 sendTask, HttpRequestMessage request, CancellationTokenSource cts, Boolean disposeCts)
   at System.Net.HttpWebRequest.SendRequest()
   at System.Net.HttpWebRequest.EndGetResponse(IAsyncResult asyncResult)
   --- End of inner exception stack trace ---
   at System.Net.HttpWebRequest.EndGetResponse(IAsyncResult asyncResult)
   at System.Threading.Tasks.TaskFactory`1.FromAsyncCoreLogic(IAsyncResult iar, Func`2 endFunction, Action`1 endAction, Task`1 promise, Boolean requiresSynchronization)
--- End of stack trace from previous location where exception was thrown ---
   at Datadog.Trace.Agent.Transports.ApiWebRequest.GetAsync() in /project/tracer/src/Datadog.Trace/Agent/Transports/ApiWebRequest.cs:line 52
   at Datadog.Trace.Agent.DiscoveryService.DiscoveryService.FetchConfigurationLoopAsync() in /project/tracer/src/Datadog.Trace/Agent/DiscoveryService/DiscoveryService.cs:line 154
 { MachineName: ".", Process: "[142 dotnet]", AppDomain: "[1 testhost]", TracerVersion: "2.15.0.0" }

I eventually tracked this down to the StatsTests, but it was only reproduced when running the whole suite. I eventually traced it down to the fact that the test creates a TracerManager implicitly (by creating a Tracer, but it doesn't dispose the TracerManager - it was closing the AgentWriter explicitly but that meant the discovery service (and other services if they were enabled) were still running. After the mock agent shut down, these would start throwing logs.

Add extra test for DiscoveryService to confirm that we don't check in a tight loop

The aforementioned logs were written in a very tight loop (every 1ms!), so I was concerned that we had an error in our retry logic. I wrote this test to reproduce it, but it seemed fine, so may be a different issue. Keeping the test for now anyway.

Implementation details

Update StatsTests so it correctly shuts down all background processes

Exposed an internal ShutdownAsync method on the TracerManager (for testing only) that can be used to shutdown all the running services. Removed the (now obsolete) FlushAndShutdownAsync on the Tracer.

Test coverage

Covered by existing tests or added new ones. Checking the integration tests logs showed the problem has gone now

Other details

• We will likely have similar issues with the discovery service in other places. I'll check the logs to see if there's any more evidence of it. We may want to consider a NullDiscoveryService implementation that we use by default in the tests (as we do with NullTelemetryService). The changes here are still required however, as StatsTests explicitly requires a functioning DiscoveryService
• @pierotibou suggested we update our logs to add a sanity check for the number of logs written. I agree, but haven't done it here, as want to make sure we get the discovery service stuff under control first