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

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

Closed
batizar opened this issue Jun 4, 2021 · 28 comments

Comments

@batizar
Copy link

batizar commented Jun 4, 2021

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

batizar commented Jun 26, 2021

Hi, anybody?!

@rodion-m
Copy link

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

services.AddApplicationInsightsTelemetry(options => options.ConnectionString = s);

@Sebazzz
Copy link

Sebazzz commented Aug 26, 2021

Also:

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

@Sebazzz
Copy link

Sebazzz commented Aug 26, 2021

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

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

@sebader
Copy link

sebader commented Mar 22, 2022

@nblumhardt
Copy link
Contributor

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

@shahiddev
Copy link

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

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

Is there any news on this?

@nblumhardt
Copy link
Contributor

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

sebader commented May 2, 2022

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

@nblumhardt
Copy link
Contributor

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

@nblumhardt nblumhardt changed the title Sink is not working when using APPINSIGHTS_CONNECTIONSTRING Cannot configure sink using APPINSIGHTS_CONNECTIONSTRING without relying on TelemetryClient service May 2, 2022
@zyofeng
Copy link

zyofeng commented May 7, 2022

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

I'm not aware of any, @zyofeng

@zyofeng
Copy link

zyofeng commented May 11, 2022

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

zyofeng commented May 11, 2022

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?

b5d6f04

@wesselkranenborg
Copy link

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

Sebazzz commented May 30, 2022

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

I'm fine with a breaking change if necessary 👍

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

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

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

zyofeng commented Jun 19, 2022

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

@zyofeng
Copy link

zyofeng commented Jun 19, 2022

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

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

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

zyofeng commented Oct 25, 2022

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

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 ?

  • If yes, why ?
  • If not, can we create a "bug" for that or a "feature request" ?

What do you think ?

@zyofeng
Copy link

zyofeng commented Oct 26, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants