-
Notifications
You must be signed in to change notification settings - Fork 287
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
Add support for connection string to Microsoft.ApplicationInsights.NLogTarget #2858
base: main
Are you sure you want to change the base?
Add support for connection string to Microsoft.ApplicationInsights.NLogTarget #2858
Conversation
Adding new methods to Public Api
@TimothyMothra @cijothomas - Can you please review the PR |
@rajkumar-rangaraj and @TimothyMothra - Can you please let us know your thoughts on this. |
@@ -118,10 +129,21 @@ protected override void InitializeTarget() | |||
this.telemetryClient = new TelemetryClient(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe implement like this:
string connectionString = this.connectionStringLayout.Render(LogEventInfo.CreateNullEvent());
// Check if nlog application insights target has connectionstring in config file then
// configure new telemetryclient with the connectionstring otherwise using legacy instrumentationkey.
if (!string.IsNullOrWhiteSpace(connectionString))
{
var telemetryConfiguration = TelemetryConfiguration.CreateDefault();
telemetryConfiguration.ConnectionString = connectionString;
this.telemetryClient = new TelemetryClient(telemetryConfiguration);
}
else
{
#pragma warning disable CS0618 // Type or member is obsolete: TelemtryConfiguration.Active is used in TelemetryClient constructor.
this.telemetryClient = new TelemetryClient();
#pragma warning restore CS0618 // Type or member is obsolete
string instrumentationKey = this.instrumentationKeyLayout.Render(LogEventInfo.CreateNullEvent());
if (!string.IsNullOrWhiteSpace(instrumentationKey))
{
this.telemetryClient.Context.InstrumentationKey = instrumentationKey;
}
}
This will resolve the issue about not to modify TelemtryConfiguration.Active
@cijothomas If NLog-target changes from using obsolete/legacy |
@cijothomas Polite poke. If NLog-target changes from using obsolete/legacy |
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 |
Yes like this:
Thus not depend on deprecated |
@cijothomas + @rajkumar-rangaraj + @TimothyMothra Are you willing to accept pull-request that allows NLog to use ConnectionString and no longer use the now deprecated This will give a better user-experience, as people doesn't have to explicit set |
@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. |
Created fork #2897 that changes to use isolated TelemetryClient when using ConnectionString. |
Add support for connection string to Microsoft.ApplicationInsights.NLogTarget #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