microsoft / ApplicationInsights-dotnet

ApplicationInsights-dotnet
MIT License
565 stars 287 forks source link

Add support for connection string to Microsoft.ApplicationInsights.NLogTarget #2858

Open saidi-adot opened 8 months ago

saidi-adot commented 8 months ago

Add support for connection string to Microsoft.ApplicationInsights.NLogTarget https://github.com/microsoft/ApplicationInsights-dotnet/issues/2714

Fix Issue # .

Changes

(Please provide a brief description of the changes here.)

Checklist

For significant contributions please make sure you have completed the following items:

The PR will trigger build, unit tests, and functional tests automatically. Please follow these instructions to build and test locally.

Notes for authors:

Notes for reviewers: We support comment build triggers

saidi-adot commented 8 months ago

@TimothyMothra @cijothomas - Can you please review the PR

cijothomas commented 8 months ago

https://github.com/microsoft/ApplicationInsights-dotnet/pull/2750#issuecomment-1771068342

saidi-adot commented 8 months ago

#2750 (comment)

@rajkumar-rangaraj and @TimothyMothra - Can you please let us know your thoughts on this.

snakefoot commented 6 months ago

@cijothomas If NLog-target changes from using obsolete/legacy TelemetryClient (depending on on TelemtryConfiguration.Active), to instead create its own local TelemetryClient using connectionString. Would you accept the pull-request ?

snakefoot commented 6 months ago

@cijothomas Polite poke. If NLog-target changes from using obsolete/legacy TelemetryClient (depending on on TelemtryConfiguration.Active), to instead create its own local TelemetryClient using connectionString. Would you accept the pull-request ?

cijothomas commented 6 months ago

@cijothomas Polite poke. If NLog-target changes from using obsolete/legacy TelemetryClient (depending on on TelemtryConfiguration.Active), to instead create its own local TelemetryClient using connectionString. Would you accept the pull-request ?

I'll let @rajkumar-rangaraj @TimothyMothra to comment on if this repo plan to accept new features, given the OTel movement.

From a technical standpoint, creating own TelemetryClient would mean it'll have a config, different from the rest of the application, right?

snakefoot commented 6 months ago

@cijothomas From a technical standpoint, creating own TelemetryClient would mean it'll have a config, different from the rest of the application, right?

Yes like this:

                var telemetryConfiguration = TelemetryConfiguration.CreateDefault();
                telemetryConfiguration.ConnectionString = connectionString;
                this.telemetryClient = new TelemetryClient(telemetryConfiguration);

Thus not depend on deprecated TelemtryConfiguration.Active, that requires one to explicit configure EnableActiveTelemetryConfigurationSetup to make the NLog-target work. Giving a better user-experience.

snakefoot commented 5 months ago

@cijothomas + @rajkumar-rangaraj + @TimothyMothra Are you willing to accept pull-request that allows NLog to use ConnectionString and no longer use the now deprecated TelemtryConfiguration.Active ?

This will give a better user-experience, as people doesn't have to explicit set EnableActiveTelemetryConfigurationSetup

ahanneman-adot commented 3 months ago

@cijothomas + @rajkumar-rangaraj + @TimothyMothra Please accept this Pull Request.

Azure is ending support for InstrumentationKey in March 2025 forcing everyone to switch to ConnectionString. We really like using NLog and it huge be a huge effort for us to switch off of NLog. We cannot be losing application logs, we need to be able to continue to support our applications.

snakefoot commented 3 months ago

Created fork #2897 that changes to use isolated TelemetryClient when using ConnectionString.