serilog-contrib / serilog-sinks-applicationinsights

A Serilog sink that writes events to Microsoft Azure Application Insights
Apache License 2.0
220 stars 71 forks source link

Use sink via JSON configuration without relying on `TelemetryConfiguration.Active` #156

Open simbaja opened 3 years ago

simbaja commented 3 years ago

Per MSFT Documentation, in non-ASP.NET Core related Application Insights packages, TelemetryConfiguration.Active support is not available.

See: https://docs.microsoft.com/en-us/azure/azure-monitor/app/worker-service#configure-the-application-insights-sdk

Looking through the source code, you can see this difference:

ASP.NET Core version: here vs. ASP.NET Worker Service version: here

In WorkerService, which is the recommended solution for non-HTTP based applications, TelemetryConfiguration.Active is never setup, and therefore not fully working for the Serilog sink which relies on TelemetryConfiguration.Active, unless you pass the configuration explicitly in code. Even if you were to add the InstrumentationKey to TelemetryConfiguration.Active before Serilog setup, it will still not use the same configuration, which will cause correlation issues. Also, even for ASP.NET, they are just copying the configuration options, not the actual instance of the configuration. So, you will likely have some correlation issues there too because the default modules and initializers will vary between what DI configures and the default setup of the Active configuration.

Currently, as far as I see, there's no supported way to get the Serilog sink to work through configuration (i.e. an appSettings.json file). The only way I've found to get it to work is to do something like this. As I mentioned, the only reason it works (for the most part) in ASP.NET Core now is because they are doing that extra bit of initialization - but since it's deprecated, I'm not sure we can guarantee that in the future, and it's not perfectly setup even in it's current state. As they state in documentation, it shouldn't be used as it's not guaranteed to be initialized, and even when initialized it's not guaranteed to be consistent with DI configuration.

I know issue #121 was closed, but is there an alternative that I missed for .NET Core support for App Insights in Serilog outside of specifying in code? Would prefer not to specify in code as that means I can't change the configuration of Serilog as easily (for example, I have applications that go in Azure Gov vs. Azure Commercial). This still applies to ASP.NET Core as well due to the issues outlined above.

manu-vilachan commented 3 years ago

Hi,

I've struggled with this problem just these days, and finally, I found a solution to still use the standard appsettings.json configuration. Despite Telemetryconfiguration.Active is deprecated, the .net team creates a configurable property to still maintain its usage in case your code needed, as it's the case. (Issue #1953) What I've added in the Startup class is a line like this:

services.AddApplicationInsightsTelemetry(opt => opt.EnableActiveTelemetryConfigurationSetup = true);

This is available from Microsoft.ApplicationInsights v2.15 onwards. At least, exists a workaround, but the official way to get the configuration properly is injecting the options from class ApplicationInsightsServiceOptions.

I'll work on the sink code in the next few days to try a complete solution valid for versions pre and post 2.15 of AppInsights.

Hope it helps someone!

simbaja commented 3 years ago

Hi,

I've struggled with this problem just these days, and finally, I found a solution to still use the standard appsettings.json configuration. Despite Telemetryconfiguration.Active is deprecated, the .net team creates a configurable property to still maintain its usage in case your code needed, as it's the case. (Issue #1953) What I've added in the Startup class is a line like this:

services.AddApplicationInsightsTelemetry(opt => opt.EnableActiveTelemetryConfigurationSetup = true);

This is available from Microsoft.ApplicationInsights v2.15 onwards. At least, exists a workaround, but the official way to get the configuration properly is injecting the options from class ApplicationInsightsServiceOptions.

I'll work on the sink code in the next few days to try a complete solution valid for versions pre and post 2.15 of AppInsights.

Hope it helps someone!

Thanks for the reply, and this does work for some use cases. However, it's only available when using the AspNetCore version of the library, not when using service worker version of the library. They are different and incompatible in this regard. Additionally, this option only copies the configuration settings over to the Active configuration. If you have further customized anything else (processors, initializors, etc) those are not copied (which, when using the MSFT packages, they internally do through DI). Furthermore, you essentially will have two copies of these modules even if you replicate all of the modules because the DI-based and Active configuration are still internally separate. This may lead to hard to figure out correlation issues from my understanding.

Hopefully your work in the sink itself will be fruitful. The only way I've found to fully leverage DI in the existing code base is by injecting the config through reflection, which is not good.

nblumhardt commented 2 years ago

Hi! Some help investigating more deeply and proposing a solution or documentation updates would be helpful here, if anyone is able to help out?

dominik-weber commented 2 years ago

Hi everyone, it has been some time since I have implemented AI+Serilog, but this is the code i'm using currently.

Setup Application insights like so (using the parameterless overload), no need to add the EnableActiveTelemetryConfigurationSetup workaround:

        public void ConfigureServices(IServiceCollection services)
        {
            services.AddApplicationInsightsTelemetry();
            //...
        }

To configure Serilog, I have created an extension method that i'm reusing accross multiple projects. I have extracted the default filter levels from the Visual Studio template for ASP.NET Core apps, feel free to adjust to your needs 🙂

    static class LoggingConfigExtensions
    {
        /// <summary>
        /// Initializes the Serilog <paramref name="logger"/> with default configuration.
        /// </summary>
        public static LoggerConfiguration ConfigureCustomLogging(this LoggerConfiguration logger, IServiceProvider? services)
        {
            _ = logger ?? throw new ArgumentNullException(nameof(logger));

            logger
                // default log level settings from ASP.NET Core Visual Studio template (appsettings.json)
                .MinimumLevel.Information()
                .MinimumLevel.Override("Microsoft", LogEventLevel.Warning)
                .MinimumLevel.Override("Microsoft.Hosting.Lifetime", LogEventLevel.Information)
                // remove noisy HttpClient logs
                .MinimumLevel.Override("System", LogEventLevel.Warning)
                .Enrich.FromLogContext()
                .WriteTo.Console();

            // service provider is not available during app startup
            if (services is not null)
            {
                var telemetry = services.GetRequiredService<TelemetryConfiguration>();
                logger.WriteTo.ApplicationInsights(telemetry, TelemetryConverter.Traces);
            }

            return logger;
        }
    }

I call the above extension method in the Program.cs in two places, before and after the hostbuild is created:

    public class Program
    {
        public static int Main(string[] args)
        {
            // Setup Serilog
            // https://github.com/serilog/serilog-aspnetcore
            Log.Logger = new LoggerConfiguration()
                // Because we can't access appsettings before creating the HostBuilder we'll use a bootstrap logger
                // without configuration specific initialization first and replace it after the HostBuilder was created.
                // See https://github.com/serilog/serilog-aspnetcore#two-stage-initialization
                .ConfigureCustomLogging(null)
                .CreateBootstrapLogger();

            // ...
            CreateHostBuilder(args).Build().Run();
            // ...
        }

        public static IHostBuilder CreateHostBuilder(string[] args) =>
            Host.CreateDefaultBuilder(args)
                .UseSerilog((context, services, logger) => logger.ConfigureCustomLogging(services))
                .ConfigureWebHostDefaults(webBuilder =>
                {
                    webBuilder.UseStartup<Startup>();
                });
    }

In the first place, the static Log.Logger is initialized. This is done before setting up the HostBuilder, ie. before dependency injection becomes available. Because we can't inject the Application Insights configuration yet, the logger will only log to the console in this phase (please note that I'm passing null for the IServiceProvider). Use Log.Information(..), Log.Warning(..) etc. to log messages.

After the HostBuilder is created, we create a new serilog logger and initialize it using the same extension method. This time we'll pass the service provider to the extension, which will in turn inject the TelemetryConfiguration class that was registered by Application Insights to read the configuration from. To my knowledge this will also update Log.Logger to use the new AI-enabled logger, but of course ideally you would inject ILogger<Something> at this point to log messages.

Hope this helps 🙂

nblumhardt commented 2 years ago

That looks nice, @dominik-weber - thanks for sharing your setup! We should possibly show something more like this in the README 🤔

dominik-weber commented 2 years ago

Thanks @nblumhardt :) there is actually an example on the application insights sink readme which looks fairly similar to my setup: https://github.com/serilog-contrib/serilog-sinks-applicationinsights#telemetryconfigurationactive-is-deprecated-in-the-app-insights-sdk-for-net-core-what-do-i-do

edit: looks like you found it already ;)

simbaja commented 2 years ago

Perhaps I'm missing something here (I admit I'm skimming through this), but all of the methods proposed so far seem to involve code instead of configuration. It's been a little while since I looked through this, but the only way I saw to resolve this without having to resort to code (either a solution like what @dominik-weber provides, or a solution similar to my hacky one) is to refactor the sink configuration/setup code. The problem is that deep in the bowels of the sink setup, they rely on TelemetryConfiguration.Active, so you have to pass something via code to it to change that behavior.

nblumhardt commented 2 years ago

Thanks @simbaja for chiming in. It's more that likely that I'm missing something 😅

The only place the sink touches TelemetryConfiguration.Active is in the WriteTo.ApplicationInsights() configuration methods, but in those cases a TelemetryConfiguration can be passed through instead:

https://github.com/serilog-contrib/serilog-sinks-applicationinsights/blob/dev/src/Serilog.Sinks.ApplicationInsights/LoggerConfigurationApplicationInsightsExtensions.cs#L42

A clear description of the problem would help a lot, I don't use App Insights or know anything about its API, unfortunately - is the problem just related to JSON configuration of AI? Thanks! 🙏

simbaja commented 2 years ago

To summarize the original post a little bit, there are two problems:

  1. The sink should not rely upon TelemetryConfiguration.Active, it is not guaranteed to be initialized and even when it is initialized is not guaranteed to be correctly initialized and consistent with DI configurations (see the original post for details). The default configuration of this sink will lead to unexpected results for the unwary.
  2. Because the extension configuration methods depend on TelemetryConfiguration.Active, and, ultimately, the Serilog.ReadFrom.Configuration methods call one of these extensions to configure the logger, as far as I see, there's no way to configure this sink fully in a declarative manner. This leads to issues when you are supporting multiple environments and can't hardcode your logging setup (which is what the solutions in this thread do).

Right now, the hacky solution that injects the right configuration/client IF you are using AI as part of your logging works (it doesn't require you to set anything up, but will adjust the configuration if it finds something).

Resolving this issue in a less hacky manner would probably require some changes to those extension methods to not use TelemetryConfiguration.Active and perhaps rely upon DI if an explicit configuration/client isn't provided.

Technically, there's also a third problem, the InstrumentationKey-based extension method should probably be deprecated altogether since it's not really configuring anything properly, but that's a little outside the scope of this issue.

nblumhardt commented 2 years ago

Ah I see; so the title of the ticket should probably be _Use sink via JSON configuration without relying on TelemetryConfiguration.Active? Thanks!

@dominik-weber @simbaja I don't suppose you (or anyone following along) would be interested in working through a resolution of some kind?

Serilog.Extensions.Hosting now provides a ReadFrom.Services() method that might form a useful basis for a solution, instead of relying on ReadFrom.Configuration() (which has no access to services). The root of the issue seems to be that the sink should always use an injected TelemetryClient, so there's really no nice way for ReadFrom.Configuration() to work without Active. A few moving parts (especially how to get configuration into the sink), but seems solvable. What do you think?