serilog-contrib / serilog-sinks-applicationinsights

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

Cannot configure sink using `APPINSIGHTS_CONNECTIONSTRING` without relying on `TelemetryClient` service #163

Closed batizar closed 2 years ago

batizar commented 3 years ago

Hi, it is recommended in Microsoft docs to use Application Insights connection string over instrumentation key but when adding application insights with that sink will not submit any logs or metrics to Application Insights, when I switch back to using instrumentation key it will start working again.

https://docs.microsoft.com/en-us/azure/azure-monitor/app/ilogger#example-programcs

Thanks.

batizar commented 3 years ago

Hi, anybody?!

rodion-m commented 3 years ago

Because of this issue I had to delete this package and use AI directly:

services.AddApplicationInsightsTelemetry(options => options.ConnectionString = s);
Sebazzz commented 3 years ago

Also:

New Azure regions require the use of connection strings instead of instrumentation keys.

Sebazzz commented 3 years ago

You need to manually initialize the Telemetry Configuration:

.WriteTo.ApplicationInsights(CreateAppInsightsTelemetryConfiguration(), new TraceTelemetryConverter(), LogEventLevel.Information)

Unfortunately, this does not work via appsettings.json based configuration because static methods are only called for interface or abstract type parameters.

nblumhardt commented 3 years ago

Sorry about the delay. Hoping to help catch up on the backlog, here - #168 is tracking.

sebader commented 2 years ago

Any updates on this? Instrumentation key-based ingestion has now a deprecation date set https://azure.microsoft.com/en-us/updates/technical-support-for-instrumentation-key-based-global-ingestion-in-application-insights-will-end-on-31-march-2025/

nblumhardt commented 2 years ago

@shahiddev is this one you'd be interested in shepherding through?

shahiddev commented 2 years ago

Hey @nblumhardt I can try and take a look - aware of the deprecation but wasn't aware of new regions not allowing connection strings

nblumhardt commented 2 years ago

Awesome :-)

@augustoproiete, I'm not 100% across what you have in mind for team structure, but I've invited @shahiddev to the org with maintainer rights in this repo. Drop me a line if there's a particular setup you had in mind.

Let me know if I can help get the ball rolling on anything :-)

wesselkranenborg commented 2 years ago

Is there any news on this?

nblumhardt commented 2 years ago

Using:

.WriteTo.ApplicationInsights(services.GetRequiredService<TelemetryConfiguration>(), TelemetryConverter.Traces))

should work around this successfully, and is currently the suggested method of configuration. Can anyone please confirm?

sebader commented 2 years ago

@nblumhardt this does not work when you need to initialize the logger earlier than in the Startup.cs/ConfigureServices already

nblumhardt commented 2 years ago

@sebader gotcha! So the title of this issue should be something more like _Cannot configure sink using APPINSIGHTS_CONNECTIONSTRING without relying on TelemetryClient service_? Thanks 👍

It seems like adding a connection string option to the service configuration should be pretty straightforward, is anyone able to help out/take a shot at it?

zyofeng commented 2 years ago

Some Azure services (i.e. Azure Functions) automatically injects Application Insights services, as a result I think services.GetRequiredService() should always be the go to configuration.

Is there any reason why one would prefer to let the sink setup application insights?

nblumhardt commented 2 years ago

I'm not aware of any, @zyofeng

zyofeng commented 2 years ago

If I understand this correctly, you are trying to log start up errors such as one in Program.cs file What I end up doing, is to create a boostrap logger and load the IConfiguration in using Configuration Builder

        var configuration = new ConfigurationBuilder()
            .SetBasePath(System.IO.Directory.GetCurrentDirectory())
            .AddJsonFile("appsettings.json", true)
            .AddJsonFile($"appsettings.{System.Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT")}.json", true)
            .AddEnvironmentVariables()
            .Build();

        var config = TelemetryConfiguration.CreateDefault();

        if (!string.IsNullOrEmpty(configuration["APPLICATIONINSIGHTS_CONNECTION_STRING"]))
            config.ConnectionString = configuration["APPLICATIONINSIGHTS_CONNECTION_STRING"];

        Log.Logger = new LoggerConfiguration()
            .ConfigureLogger(new TelemetryClient(config), configuration)
            .CreateBootstrapLogger();

try { var host = await new HostBuilder().ConfigureWebHost(webBuilder => { webBuilder.UseTestServer().UseStartup(); }); } catch (Exception ex) { Log.Fatal("Startup Error: {@Exception}", ex); throw; }

zyofeng commented 2 years ago

I have added to my PR to switch from using InstrumentationKey to ConnectionString, however this is obviously going to be a breaking change so maybe it should be in 4.0?

https://github.com/serilog-contrib/serilog-sinks-applicationinsights/pull/191/commits/b5d6f046fc6eff5ce32d92494ed2fd559993ec2c

wesselkranenborg commented 2 years ago

@zyofeng: if it's implemented in this way it's indeed a breaking change. You could also implement a new extension method with this version and leave the instrumentationkey implementation there and mark it as obsolete. Then you allow for a migration path for users. Just a suggestion so it can already be incorporated in version 3.x

Sebazzz commented 2 years ago

It would not be a problem to make it a breaking change IMHO. The instrumentation key implementation will stop working in Azure soon anyway.

nblumhardt commented 2 years ago

I'm fine with a breaking change if necessary :+1:

What we really need right now is a full proposal for how the sink should be changed (ideally with a PR) - is anyone able to pick this up?

I don't use AI at all, so it's very unlikely I'll put this together myself, but I can help review etc. if someone with the knowledge/access to AI can work through it.

nblumhardt commented 2 years ago

I've merged @zyofeng's PR to dev, and it's on NuGet as 4.0.0-dev-00030.

https://github.com/serilog-contrib/serilog-sinks-applicationinsights/pull/194

Can someone please check that:

  1. 4.0.0-dev-00030 works as expected with an instrumentation key supplied, and
  2. The README is correct with respect to JSON config: https://github.com/serilog-contrib/serilog-sinks-applicationinsights#configuring-with-readfromconfiguration

If we can verify those two items, I'll merge/release 4.0.0, as there's not much else I can do on #184 right now. Thanks!

zyofeng commented 2 years ago

I tried both methods (adding the sink programmatically and through configuration) and they seem to work fine to me image

zyofeng commented 2 years ago

One thing, not sure if this is intentionally or not, but if I put APPLICATIONINSIGHTS_CONNECTION_STRING in appsettings.config rather than as an environment variable it does not get picked up, I would suggest reading the connection string from IConfiguration["APPLICATIONINSIGHTS_CONNECTION_STRING"] because .net core's approach with merging multiple config sources. Technically I can have APPLICATIONINSIGHTS_CONNECTION_STRING configured in a keyvault or Azure App Configuration and shared across multiple containers/apps.

nblumhardt commented 2 years ago

@zyofeng thanks for checking it out. Calling this ticket done 👍 - will merge 4.0 as soon as I can write the notes.

Support for the connection string variable is via the app insights SDK and not tied to any of the .NET configuration system. I think any method that sets the corresponding environment variable should work.

simon-knu commented 2 years ago

Hi @zyofeng @nblumhardt

I need some clarification about that part One thing, not sure if this is intentionally or not, but if I put APPLICATIONINSIGHTS_CONNECTION_STRING in appsettings.config rather than as an environment variable it does not get picked up

I need to set the "APPLICATIONINSIGHTS_CONNECTION_STRING" through the appsettings.json and not through Environment Variable but it doesn't seems to work..

Is it intentional that this not working with the appsettings.json config ? Or is it a bug ? Like @zyofeng said, it should be possible to use both case because : Technically I can have APPLICATIONINSIGHTS_CONNECTION_STRING configured in a keyvault or Azure App Configuration and shared across multiple containers/apps.

So, what about that ?

zyofeng commented 2 years ago

Because you can instantiate the telemetry client and pass that into the sink as a param. You can just read the connectionstring from iconfig if needed.

simon-knu commented 2 years ago

I don't think I get what you said

I don't understand why we should do something different if we don't set the environment variable ...

The "APPLICATIONINSIGHTS_CONNECTION_STRING " should be handle if it's set in the environment variable or set in appsettings.json. For the moment, only the environment variable way is working

Is it done on purpose ?

What do you think ?

zyofeng commented 2 years ago

If you are after some kind of justification I guess here are two. Because the stock Microsoft application insight lib uses env variable As @nblumhardt mentioned loading it from appsettings or rather iconfiguration in general requires tying the library to Microsoft Options pattern