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

TC.Active is obsolete and we should re-think its use in logging adapters #1457

Closed
Dmitry-Matveev opened this issue Jul 10, 2019 · 11 comments
Closed
Labels

Comments

@Dmitry-Matveev
Copy link
Member

TelemetryConfiguration.Active is now obsolete for .NET Core target.

Several of logging adapters rely on TC.Active to get the currently used and/or modified configuration from the main SDK installation. At the same time, these projects are multi-targeted to compile for .NET Core and .NET Full.

This approach works for .NET Full but not for .NET Core where TC.Active is not the one in use.
This is not a regression - adapters have used TC.Active from the beginning, but the issue was highlighted by obsoleting TC.Active.

@TimothyMothra TimothyMothra transferred this issue from microsoft/ApplicationInsights-dotnet-logging Dec 4, 2019
@GeeWee
Copy link

GeeWee commented Dec 6, 2019

@mia01
Copy link

mia01 commented Dec 26, 2019

Hello
Is there any action to be taken on this? I believe nlog has also the same issue although I'm not 100% certain. I raised a StackOverflow question about it here: https://stackoverflow.com/questions/59481261/application-insights-adding-cloud-role-name-via-custom-itelemetryinitializer-in

@cijothomas
Copy link
Contributor

@mia01 For logging adaptors other than Ilogger, I believe your only option would be to continue using TC.Active, even though its obsolete.

@mia01
Copy link

mia01 commented Dec 27, 2019

@cijothomas I've only noticed this with console apps. With web apps I've been able to use:

services.addSingeton<ITelemetryInitializers, CustomInitializer>();

Do you know why this is the case?

@cijothomas
Copy link
Contributor

For console app, https://docs.microsoft.com/en-us/azure/azure-monitor/app/worker-service this is the recommended approach. And initializers can be added the same way as web. It only integrates well with ilogger based logging, so nlog would require some manual steps.

@mia01
Copy link

mia01 commented Dec 29, 2019

@cijothomas Thanks for responding to my comment. Just doing some more investigation I could see there is a difference between the TelemetryConfigurationOptions class for ASP and TelemetryConfigurationOptions for workerservice you'll find the ASP one has a fallback mechanism where it adds all the initialisers to TelemetryConfiguration.Active. That would explain what I mentioned in my previous comment. What can we do about this? Also looking at the ApplicationInsightsTarget class for nlog adaptor I can see it wasn't updated to use DI to get the TelemetryClient which would have resolved the issue we are facing I wonder why that is? Admittedly I may not have a full understanding of the big picture but I wonder why was this missed out at the time of deprecating TelemetryConfiguration.Active and instead the warning suppressed?
For now what I've been doing in my console app is TelemetryConfiguration.Active.TelemetryInitializers.Add(new CustomInitializer()); but this leaves a bad taste because of the warning of using something obsolete.

@cijothomas
Copy link
Contributor

Populating TelemetryConfiguration.Active is done in Asp.Net Core not as a fallback mechanism, but because it was previously doing it, and hence continuing to do so to maintain backward compatibility.

WorkerServiceSDK is a totally new package, and it never had TelemetryConfiguration.Active from the beginning and no backward compatibility to maintain. So it doesn't populate .Active.

Unfortunately - I don't have any better solution than using .ACTIVE for .net core console apps integration with NLog.

@github-actions
Copy link

This issue is stale because it has been open 300 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions
Copy link

This issue is stale because it has been open 300 days with no activity. Remove stale label or this will be closed in 7 days. Commenting will instruct the bot to automatically remove the label.

@github-actions github-actions bot added the stale label Jul 16, 2022
@snakefoot
Copy link
Contributor

NLog 5.0 makes it possible to resolve service-dependencies, so one can acquire the Telemetry-interface from the ServiceProvider.

It would ofcourse require bumping Microsoft.ApplicationInsights.NLogTarget to depend on NLog 5.0

@github-actions github-actions bot removed the stale label Jul 17, 2022
@github-actions
Copy link

This issue is stale because it has been open 300 days with no activity. Remove stale label or this will be closed in 7 days. Commenting will instruct the bot to automatically remove the label.

@github-actions github-actions bot added the stale label May 13, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants