Swallow ObjectDisposedException

[reyang]

Refer to the analysis here #2831 (comment).

I think it is a very rare case that the user might have disposed the BatchExportingProcessor while the exporter worker thread Join() timed out.

Here goes a self-contained repro simulating this situation:

using System;
using System.Collections.Generic;
using System.Text;
using System.Threading;
using Microsoft.Extensions.Logging;
using OpenTelemetry;
using OpenTelemetry.Logs;

public class Program
{
    public static void Main()
    {
        var processor = new BatchLogRecordExportProcessor(
            exporter: new MyExporter("ExporterX"),
            scheduledDelayMilliseconds: 100,
            maxExportBatchSize: 1);
        using var loggerFactory = LoggerFactory.Create(builder =>
            builder.AddOpenTelemetry(options => options.AddProcessor(processor)));

        var logger = loggerFactory.CreateLogger<Program>();

        for (int i = 0; i < 100000; i++)
        {
            logger.LogInformation("Hello, World!");
        }

        processor.Dispose();
    }

    internal class MyExporter : BaseExporter<LogRecord>
    {
        private readonly string name;

        public MyExporter(string name = "MyExporter")
        {
            this.name = name;
        }

        public override ExportResult Export(in Batch<LogRecord> batch)
        {
            // SuppressInstrumentationScope should be used to prevent exporter
            // code from generating telemetry and causing live-loop.
            using var scope = SuppressInstrumentationScope.Begin();

            Console.WriteLine($"{this.name}.Export(...)");
            Thread.Sleep(1);
            return ExportResult.Success;
        }

        protected override bool OnShutdown(int timeoutMilliseconds)
        {
            Console.WriteLine($"{this.name}.OnShutdown(timeoutMilliseconds={timeoutMilliseconds})");
            return true;
        }

        protected override void Dispose(bool disposing)
        {
            Console.WriteLine($"{this.name}.Dispose({disposing})");
        }
    }
}

[codecov]

Codecov Report

Merging #2844 (d8d4b83) into main (102ac27) will decrease coverage by 0.00%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main    #2844      +/-   ##
==========================================
- Coverage   83.74%   83.73%   -0.01%     
==========================================
  Files         251      251              
  Lines        8863     8866       +3     
==========================================
+ Hits         7422     7424       +2     
- Misses       1441     1442       +1     

Impacted Files	Coverage Δ
src/OpenTelemetry/BatchExportProcessor.cs	87.36% <60.00%> (-1.77%)	⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	82.35% <0.00%> (+2.94%)	⬆️

[reyang]

I think we need to discuss about the design principles, in general the telemetry SDK should "be nice to the user, after the SDK is initialized, we should not punish the user even if they did something wrong". And we have spec covering this.

What's your take on the following scenario:

1. the user wrote a multi-threaded application which calls exporter.ForceFlush.
2. the user disposed the exporter explicitly or implicitly
3. the user threads which are calling exporter.ForceFlush now could run into exception (depending on how it is implemented)

Next question: do you expect all the telemetry APIs (including the SDK components) that we exposed from this project to be bullet proof? e.g. even if things got disposed, member functions can still be called without exception.

More readings https://dev2u.net/2021/08/20/9-common-design-patterns-framework-design-guidelines-conventions-idioms-and-patterns-for-reusable-net-libraries-3rd-edition/: AVOID allowing an object to regain meaningful state after a call to Dispose().