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 could be exported via ConsoleExporter after LoggerFactory is disposed #1848

Closed
reyang opened this issue Feb 24, 2021 · 8 comments · Fixed by #3578
Closed

Logs could be exported via ConsoleExporter after LoggerFactory is disposed #1848

reyang opened this issue Feb 24, 2021 · 8 comments · Fixed by #3578
Assignees
Labels
bug Something isn't working good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package

Comments

@reyang
Copy link
Member

reyang commented Feb 24, 2021

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 reyang added bug Something isn't working area:exporter labels Feb 24, 2021
@cijothomas cijothomas changed the title Logs should be exported via ConsoleExporter after LoggerFactory is disposed Logs could be exported via ConsoleExporter after LoggerFactory is disposed Feb 24, 2021
@cijothomas
Copy link
Member

cijothomas commented Feb 24, 2021

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 cijothomas added the pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package label Feb 24, 2021
@reyang reyang added help wanted Good for taking. Extra help will be provided by maintainers good first issue Good for newcomers labels Apr 15, 2022
@jackyshang12321
Copy link
Contributor

jackyshang12321 commented Aug 8, 2022

@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.

@reyang
Copy link
Member Author

reyang commented Aug 8, 2022

@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.

@CodeBlanch
Copy link
Member

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.

@reyang
Copy link
Member Author

reyang commented Aug 15, 2022

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.

@jackyshang12321
Copy link
Contributor

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
Copy link
Contributor

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

@reyang
Copy link
Member Author

reyang commented Aug 18, 2022

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 #3578 (review).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
4 participants