Tenant idle shutdown trace is double-logged (NEST-490)

[Piedone]

The shutdown trace of IdleShutdownTask is double-logged (or even logged 3, 4, 5 times sometimes), and only with Application Insights. So, this is some bug in this module.

Jira issue

[wAsnk]

The docs say that you have to use Program.cs instead of Startup.cs the following way (almost like NLog):
https://learn.microsoft.com/en-us/azure/azure-monitor/app/asp-net-core?tabs=netcorenew%2Cnetcore6#:~:text=Add%20AddApplicationInsightsTelemetry()%20to%20your%20startup.cs%20or%20program.cs%20class.%20The%20choice%20depends%20on%20your%20.NET%20Core%20version.
Otherwise in our current implementation, we will have multiple providers added to the ILoggerFactory whenever a tenant starts, because ILoggerFactory is a shared service between all tenants. So you'll have multiple client and configuration instances loaded to memory as well.

Currently without the idle tenant shutdown feature it means that each tenant has one added, that could also make sense if we want to provide a way to have different connection strings for each tenant. But looking at ILoggerFactory, that contains all providers for all tenants, I'm not even sure if that's even currently the good implementation for that https://learn.microsoft.com/en-us/azure/architecture/guide/multitenant/service/application-insights.
I didn’t try but thinking logically if ILoggerFactory contains every tenants' provider then every tenant event would be sent to every tenants' AI instance if they are different for each tenant. And they can be if it is configured that way in appsettings, ofc we are not doing that currently. So this looks like a problem currently.

With idle tenant shutdown feature on, the context is released then the site is started again with another logger provider added.

This looks like the following in the memory. These TelemetryConfiguration instances are never garbage collected, so they will be put from Heap 0 to Heap 2 and they will stay there and new ones will do the same. It’s because ILoggerFactory and every ILoggerProvider added to it are never disposed. I suppose that’s because the Default tenant are never shut down and should not be.

Btw I’ve never reproduced this issue because it seems like a simple GET call with HttpClient won’t trigger some logic that a manual page load does (weird, and I don’t know why). So the test controller for idle tenant shutdown didn’t repro this.

Soooo I would go with the solution in the PRs, meaning we won’t support different connection strings for tenants. Technically we are doing it now, but with a bug I explained above.

[Piedone]

Let's switch to application-level AI, then. Should anything else in the module's Startup remain there, though, except for what's related to background tasks and LoggingTestMiddleware? As opposed to putting every other initialization into an AddOrchardCoreApplicationInsightsTelemetry(configuration), to be called in the app's startup.

Also, that logging in IdleShutdownTask should rather be an Information.

[jtkech]

Just for info, this is the same kind of reasons that we needed to implement a TenantHttpClientFactory, also dispose configurations when a shell is disposed, and so on, see OrchardCMS/OrchardCore#14348 and the comments of the related issue.

[Piedone]

Thanks for chiming in, JT!

Yeah, that's similar, we're learning that the hard time now. But your findings and fixes, and Krisztián investigating these issues for months, have us now in a place where we can fix all of them for good :).