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

Improve DI friendliness of Logging #4014

Closed
dpk83 opened this issue Dec 15, 2022 · 8 comments · Fixed by #5648
Closed

Improve DI friendliness of Logging #4014

dpk83 opened this issue Dec 15, 2022 · 8 comments · Fixed by #5648
Labels
enhancement New feature or request logs Logging signal related

Comments

@dpk83
Copy link

dpk83 commented Dec 15, 2022

Feature Request

Is your feature request related to a problem?

  • Today Logging extensions provide AddOpenTelemetry extension on ILoggingBuilder, however AddProcessor and SetResourceProvider extensions are exposed on OpenTelemetryLoggerOptions. This is restrictive for services which are primarily using DI. Exposing these extensions on OpenTelemetryLoggerOptions and not on ILoggingBuilder and/or IServiceCollection limits the ability for third party libraries to provide easier capabilities for adding processor for example, or to compose the resource provider from resources exposed by multiple third party libraries etc. With the DI based mechanism a processor can then be added which can get access to other objects via DI injection and making the APIs really DI friendly for users
  • Another ask here is to also expose an override for the existing AddOpenTelemetry extension to take in options via

Describe the solution you'd like:

Expose the extensions SetResourceProvider and AddProcessor on ILoggingBuilder.

public static ILoggingBuilder AddProcessor<T>(this ILoggingBuilder builder)
            where T : BaseProcessor<LogRecord>{}

public static ILoggingBuilder SetResourceBuilder(this ILoggingBuilder loggingBuilder, ResourceBuilder resourceBuilder);

Add another overload for AddOpenTelemetry extension

public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, IConfigurationSection configurationSection) {}

Describe alternatives you've considered.

Currently we had to implement custom LoggerProvider to get the right functionality

Additional Context

We would like to move to directly using OpenTelemetryLogger and extending it as opposed to having our own implementation of LoggerProvider and Logger.

@dpk83 dpk83 added the enhancement New feature or request label Dec 15, 2022
@dpk83
Copy link
Author

dpk83 commented Dec 15, 2022

@reyang Just raising the ask here for tracking purpose. We have the proposed changes that we can discuss in January.

@CodeBlanch
Copy link
Member

A couple thoughts on this...

  • RE: Options, FYI one change that will make it into 1.4.0 is this line which automatically binds logging options to the OTel options class. I updated the example to use this as well. Maybe that will be useful for some of your cases?

  • RE: DI, much effort went into making this possible (eg [Logs] Support dependency injection in logging build-up #3504) but ultimately none of it made it into 1.4.0 😢 The reason for this is OTel spec looks likely to introduce a LoggerProvider and Logger into the API. There is a branch main-logs where I have moved this work. @dpk83 let's chat offline about how we can combine our efforts on this?

@julealgon
Copy link

A couple thoughts on this...

@CodeBlanch that doesn't help with OtlpExporterOptions though... That one is currently manually instantiated by the logger version of AddOtlpExporter, and relies solely on environment variables to create the options instance (that's a really bad practice folks....):

public OtlpExporterOptions()
: this(new ConfigurationBuilder().AddEnvironmentVariables().Build(), new())

This basically means I have to do this nasty workaround here, since I'm not reading OTEL_EXPORTER_OTLP_ENDPOINT from an envvar on this project, but from a different configuration source:

IHost host = Host.CreateDefaultBuilder(args)
    ...
    .ConfigureLogging((context, logging) => 
    {
        // HACK:
        // Force the OTEL endpoint envvar to be populated even if the value is already provided through other
        // configuration means.
        //
        // The OTEL exporter for logs only looks at actual environment variables currently instead of fetching the value
        // from any configuration source like the metrics and traces stacks do.
        // 
        // Since we rely solely on Azure AppConfiguration for settings, we don't provide the 'OTEL_EXPORTER_OTLP_ENDPOINT'
        // as an environment variable, but as a global setting in AppConfiguration. That works as expected for metrics and traces,
        // but is ignored by the logs exporter in its current incarnation.
        // 
        // More information:
        // https://github.com/open-telemetry/opentelemetry-dotnet/issues/4014
        SetOtelExporterEndpointEnvironmentVariable(context);

        logging.AddOpenTelemetry(c => c
            .SetResourceBuilder(...)
            .AddOtlpExporter());
    })

...

static void SetOtelExporterEndpointEnvironmentVariable(HostBuilderContext context)
{
    const string otelExporterEndpointVariableName = "OTEL_EXPORTER_OTLP_ENDPOINT";
    var endpoint = context.Configuration[otelExporterEndpointVariableName];
    Environment.SetEnvironmentVariable(otelExporterEndpointVariableName, endpoint);
}

I'd strongly recommend moving away from this approach of setting the default values like that, and into a proper IConfigureOptions implementation.

It goes without saying that an options model should be a simple POCO object with no logic like that.

@CodeBlanch
Copy link
Member

@julealgon I don't disagree with you at all 😄 I'm hoping to clean up logging in 1.6 so it has the same surface as metrics & logs. Just waiting on the OTel spec for logging to go stable. That process is moving slowly, but moving!

If you want to unblock yourself right now (on 1.4) try something like this...

// Register OpenTelemetryLoggerProvider
appBuilder.Logging.AddOpenTelemetry(options => options.AddOpenTelemetry(c => c.SetResourceBuilder(...));

// Add OtlpExporter once ServiceProvider is availalble
appBuilder.Services.AddOptions<OpenTelemetryLoggerOptions>().Configure<IServiceProvider>((options, sp) =>
{
    var config = sp.GetRequiredService<IConfiguration>();
    options.AddOtlpExporter(otlpExporterOptions =>
    {
        var endpoint = config.GetValue<string?>("OpenTelemetry:Endpoint", null);
        if (endpoint != null)
        {
            otlpExporterOptions.Endpoint = new Uri(endpoint);
        }
    });
});

@cijothomas cijothomas added the logs Logging signal related label Mar 1, 2023
@julealgon
Copy link

@julealgon I don't disagree with you at all 😄 I'm hoping to clean up logging in 1.6 so it has the same surface as metrics & logs. Just waiting on the OTel spec for logging to go stable. That process is moving slowly, but moving!

Any estimate on how long that could take? I'm asking because this particular setup hack here is fairly nasty and I have a few projects that will be potentially affected by it coming up as we want to move away from environment variables and into Azure AppConfiguration for these types of global settings.

Would be nice if there was an overload of AddOpenTelemetry on the logger in the meantime that took an IConfiguration for example, as that could completely resolve this by just passing that IConfiguration along into the new OtlpExporterOptions constructor. I can't do that from user code otherwise I would.

With such overload, my code would become this:

IHost host = Host.CreateDefaultBuilder(args)
    ...
    .ConfigureLogging((context, logging) => 
    {
        logging.AddOpenTelemetry(context.Configuration, c => c
            .SetResourceBuilder(...)
            .AddOtlpExporter());
    })

...

Or maybe this if you opted into passing it into the AddOtlpExporter method itself:

IHost host = Host.CreateDefaultBuilder(args)
    ...
    .ConfigureLogging((context, logging) => 
    {
        logging.AddOpenTelemetry(c => c
            .SetResourceBuilder(...)
            .AddOtlpExporter(context.Configuration));
    })

...

I imagine none of those options needs the log spec to stabilize and could be done temporarily? It is not perfect but it is vastly superior than what I have now.

If you want to unblock yourself right now (on 1.4) try something like this...

// Register OpenTelemetryLoggerProvider
appBuilder.Logging.AddOpenTelemetry(options => options.AddOpenTelemetry(c => c.SetResourceBuilder(...));

// Add OtlpExporter once ServiceProvider is availalble
appBuilder.Services.AddOptions<OpenTelemetryLoggerOptions>().Configure<IServiceProvider>((options, sp) =>
{
    var config = sp.GetRequiredService<IConfiguration>();
    options.AddOtlpExporter(otlpExporterOptions =>
    {
        var endpoint = config.GetValue<string?>("OpenTelemetry:Endpoint", null);
        if (endpoint != null)
        {
            otlpExporterOptions.Endpoint = new Uri(endpoint);
        }
    });
});

Well yeah, that would also work. But I almost feel like what I have there ends up being simpler and also works.

Also, you don't need to have the indirection with IServiceProvider there. The idea behind the DI-enabled overload of Configure is precisely to avoid needing to manually grab instances yourself. This works just fine for example:

appBuilder.Services.AddOptions<OpenTelemetryLoggerOptions>().Configure<IConfiguration>((options, config) =>
{
    options.AddOtlpExporter(otlpExporterOptions =>
    {
        var endpoint = config.GetValue<string?>("OpenTelemetry:Endpoint", null);
        if (endpoint != null)
        {
            otlpExporterOptions.Endpoint = new Uri(endpoint);
        }
    });
});

@CodeBlanch
Copy link
Member

Any estimate on how long that could take?

Rought estimate at the moment is 1.6 around 11/2023.

Also, you don't need to have the indirection with IServiceProvider there. The idea behind the DI-enabled overload of Configure is precisely to avoid needing to manually grab instances yourself.

Sure but this issue is about general DI support in logging so I decided to show a more general case 😄

Would be nice if there was an overload of AddOpenTelemetry on the logger in the meantime that took an IConfiguration for example

I don't see a lot of value in this, TBH. OpenTelemetryLoggerOptions are today automatically bound to the Logging:OpenTelemetry section. That is done here.

Or maybe this if you opted into passing it into the AddOtlpExporter method itself:

I think adding overloads like this inside of OtlpExporter...

        public static OpenTelemetryLoggerOptions AddOtlpExporter(
            this OpenTelemetryLoggerOptions loggerOptions,
            IConfiguration configuration) {}

        public static OpenTelemetryLoggerOptions AddOtlpExporter(
            this OpenTelemetryLoggerOptions loggerOptions,
            Action<OtlpExporterOptions> configure) {}

...would totally be doable in 1.5. I'm doing similar over here. BUT would you open a dedicated issue for that?

@julealgon
Copy link

Rought estimate at the moment is 1.6 around 11/2023.

Oof.. .that's a long ways off... waiting until .NET 8 would be less than ideal for us.

Sure but this issue is about general DI support in logging so I decided to show a more general case 😄

Fair enough.

I think adding overloads like this inside of OtlpExporter...

        public static OpenTelemetryLoggerOptions AddOtlpExporter(
            this OpenTelemetryLoggerOptions loggerOptions,
            IConfiguration configuration) {}

        public static OpenTelemetryLoggerOptions AddOtlpExporter(
            this OpenTelemetryLoggerOptions loggerOptions,
            Action<OtlpExporterOptions> configure) {}

...would totally be doable in 1.5. I'm doing similar over here.

As long as the overload respects the global variable identifiers by passing the IConfiguration into the OtlpExporterOptions model (or equivalent logic in case refactoring is done), I'm totally ok with this.

Also, if you don't mind me asking, what is the timeline for 1.5?

BUT would you open a dedicated issue for that?

I can definitely create a dedicated issue for this problem. Look for it by EOD today. Will link with this one for traceability.

@CodeBlanch
Copy link
Member

@julealgon We have these milestones defined you can watch but the idea with 1.5 is to get it out in the summer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request logs Logging signal related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants