opentelemetry-dotnet icon indicating copy to clipboard operation
opentelemetry-dotnet copied to clipboard

Logs could be exported via ConsoleExporter after LoggerFactory is disposed

Open reyang opened this issue 4 years ago • 2 comments

Bug Report

List of NuGet packages and version that you are using (e.g. OpenTelemetry 0.4.0-beta.2):

  • 1.0.1

Runtime version (e.g. net461, net48, netcoreapp2.1, netcoreapp3.1, etc. You can find this information from the *.csproj file):

  • all the versions

Symptom

What is the expected behavior?

If the logger factory is disposed, the console exporter should stop exporting data. (or we can keep exporting the data but put a special message telling the user that something is wrong?)

What is the actual behavior?

We can still see the log record "This log record is still exported by the ConsoleExporter." from the console.

Reproduce

using System.Collections.Generic;

using Microsoft.Extensions.Logging;
using OpenTelemetry.Logs;

public class Program
{
    static void Main()
    {
        var logger = SetupLogging();

        logger.LogInformation("This log record is still exported by the ConsoleExporter.");
    }

    static ILogger SetupLogging()
    {
        using var loggerFactory = LoggerFactory.Create(builder => builder
            .AddOpenTelemetry(options =>
            {
                options.AddConsoleExporter();
            }));

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

        logger.LogInformation("Logging setup completed successfully!");

        return logger;
    }
}

We will close this issue if:

  • The repro project you share with us is complex. We can't investigate custom projects, so don't point us to such, please.
  • If we can not reproduce the behavior you're reporting.

Additional Context

Add any other context about the problem here.

reyang avatar Feb 24 '21 20:02 reyang

Issue exists outside of OpenTelemetry also, as shown in below code.

static void Main(string[] args)
        {
            var factory = LoggerFactory.Create((builder) => builder.AddConsole());
            var logger = factory.CreateLogger("testCategory");
            logger.LogError("Sample error");
            factory.Dispose();
            logger.LogError("After dispose Sample error"); // this is also displayed in Console.
        }

Some options: Modify ConsoleExporter to not export after Disposed. This only fixes the ConsoleExporter. Modify OpenTelemetryLogger to not invoke processor pipeline if disposed. This will address all exporters including Console.. or see if this is something .NET runtime can fix. (we'd likely still need some interim solution.)

cijothomas avatar Feb 24 '21 21:02 cijothomas

@reyang , I'm new to this but I would like to contribute and work on this, will drop a PR later, try to fix ConsoleExporter first.

jackyshang12321 avatar Aug 08 '22 02:08 jackyshang12321

@reyang , I'm new to this but I would like to contribute and work on this, will drop a PR later, try to fix ConsoleExporter first.

Thanks @jackyshang12321, I've assigned this issue to you.

reyang avatar Aug 08 '22 16:08 reyang

I'm on the fence if we should do anything here. Looking at what @cijothomas posted, the change on #3578 would cause our LoggerProvider to behave differently than the runtime ones. Is there any danger with the current behavior? Seems to make it more forgiving to mistakes without any real consequence.

CodeBlanch avatar Aug 15 '22 16:08 CodeBlanch

What is the expected behavior?

If the logger factory is disposed, the console exporter should stop exporting data. (or we can keep exporting the data but put a special message telling the user that something is wrong?)

I feel having a special message might be a good starting point.

reyang avatar Aug 15 '22 16:08 reyang

Hey @reyang , it makes sense to add a special message to tell the user that something is wrong rather than stop exporting data. Code updated, thank you both

jackyshang12321 avatar Aug 15 '22 23:08 jackyshang12321

Hi @reyang , Could you please help review the updated PR when you get a chance, appreciated if any suggestions or feedback

jackyshang12321 avatar Aug 18 '22 00:08 jackyshang12321

Hi @reyang , Could you please help review the updated PR when you get a chance, appreciated if any suggestions or feedback

I've replied here https://github.com/open-telemetry/opentelemetry-dotnet/pull/3578#pullrequestreview-1076701507.

reyang avatar Aug 18 '22 05:08 reyang