Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[logs] Reloading application configuration may lead to unnecessary thread creation #5513

Closed
CodeBlanch opened this issue Apr 5, 2024 · 0 comments · Fixed by #5514
Closed
Labels
bug Something isn't working logs Logging signal related pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Milestone

Comments

@CodeBlanch
Copy link
Member

Overview

The delegate passed to OpenTelemetryLoggingExtensions.AddOpenTelemetry is registered as an options configuration (IConfigureOptions) and will be replayed on a fresh instance of OpenTelemetryLoggerOptions anytime the application IConfiguration is reloaded. If that delegate performs openTelemetryLoggerOptions.AddProcessor(new BatchLogRecordExportProcessor(...)) a background thread is created which is never signaled to shutdown. In processes which reload configuration periodically this will lead to an ever-increasing number of threads being created. Note: The additional threads are never assigned logs to export/process they will just sit asleep and periodically wake up to check if there is work to do (which there never is).

Reproduction \ example

var root = new ConfigurationBuilder().Build();

var services = new ServiceCollection();

services.AddSingleton<IConfiguration>(root);

services.AddLogging(logging => logging
    .AddConfiguration(root.GetSection("logging")) // Note: This is performed automatically when using a host
    .AddOpenTelemetry(options =>
    {
        options.AddProcessor(new BatchLogRecordExportProcessor(new OtlpLogExporter(new())));
    }));

using var serviceProvider = services.BuildServiceProvider();

var loggerFactory = serviceProvider.GetRequiredService<ILoggerFactory>();

for (int i = 0; i < 100; i++)
{
    root.Reload();
}

// 101 threads created here

image

Not impacted

  • Delegates using the factory form of AddProcessor which accepts the sp parameter (options.AddProcessor(sp => BuildProcessor(sp)) are NOT impacted. The factory delegate is never invoked for configuration reloads. The AddOtlpExporter extension uses the factory form so it is NOT impacted.

  • The experimental LoggerProviderBuilder and WithLogging APIs are NOT impacted.

     var root = new ConfigurationBuilder().Build();
    
     var services = new ServiceCollection();
    
     services.AddSingleton<IConfiguration>(root);
    
     services.AddLogging(logging => logging
         .AddConfiguration(root.GetSection("logging"))); // Note: This is performed automatically when using a host
    
     services.AddOpenTelemetry()
         .WithLogging(builder =>
         {
             builder.AddProcessor(new BatchLogRecordExportProcessor(new OtlpLogExporter(new())));
         });
    
     using var serviceProvider = services.BuildServiceProvider();
    
     var loggerFactory = serviceProvider.GetRequiredService<ILoggerFactory>();
    
     for (int i = 0; i < 100; i++)
     {
         root.Reload();
     }
    
     // Only 1 thread created here
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working logs Logging signal related pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant