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

Deprecating TelemetryConfiguration.Active singleton #1152

Closed
lmolkova opened this issue Jun 8, 2019 · 40 comments
Closed

Deprecating TelemetryConfiguration.Active singleton #1152

lmolkova opened this issue Jun 8, 2019 · 40 comments
Milestone

Comments

@lmolkova
Copy link
Member

lmolkova commented Jun 8, 2019

With ApplicationInsights SDK 2.11-beta1 we are deprecating TelemetryConfiguration.Active on .NET core apps, see #1148.

ApplicationInsights exposes TelemetryConfiguration.Active singleton to enable auto-configuration scenarios in the absence of a dependency injection framework.

It is used on ASP.NET Classic apps (in Microsoft.ApplicationInsights.Web SDK). The configuration is read from the ApplicationInsights.config file by auto-collectors when an application starts.
User can access configuration that is used by auto-collectors and is able to customize it.

It is not the case for ASP.NET Core application where Dependency Injection is part of the framework - ApplicationInsights configuration is done and could be accessed and customized through DI container. The details and examples could be found here; https://docs.microsoft.com/en-us/azure/azure-monitor/app/asp-net-core

Active singleton creates issues typical for static singletons - lifetime it tight to process, testing is complicated. This is especially problematic in ASP.NET core apps, Azure WebJobs, and generic host apps, where one process may host multiple applications. So there ApplicationInsights SDK configures an instance of TelemetryConfiguration per host. It frequently results in confusion around which instance of configuration to use: one in the DI container or Active.

@macrogreg
Copy link
Contributor

@lmolkova In the context of this change, what is the recommended replacement for the popular pattern of creating an instance of TelemetryClient via the parameter-less ctor at multiple places, instead of carrying around an instance?
Will TelemetryClient() use something other than TelemetryConfiguration.Active?
If so - what? If not - what is the recommendation? I was that doing

var client = TelemetryClient();
client.TrackStuff(..);

at multiple places is a very common usage pattern.

Thanks!

@lmolkova
Copy link
Member Author

lmolkova commented Aug 6, 2019

[EDITED]
In console apps, where ApplicationInsights SDK or AppInsights ILogger provider are used, get TelemetryConfiguration instance from the DI container and create TelemetryClient whenever you want.

Check out how you can customize configuration for ILogger provider and ASP.NET Core SDK

When building container yourself, you decide how to customize the configuration and register it in the container. The baseline: you can just register TelemetryConfiguration instance and still use dependency injection to resolve it. Pay attention to nuances like

  • when registering an instance without a factory, an instance is not disposed with the host, so always use factories to register disposable instances
  • use services.Configure<TelemetryConfiguration>, but be careful with correlation and add OperationCorrelationTelemetryInitializer to ensure correlation still works:
  services.Configure<TelemetryConfiguration>(
     (o) => {
       o.InstrumentationKey = "123";
       o.TelemetryInitializers.Add(new OperationCorrelationTelemetryInitializer());
     });

If you don't want to use DI, and have pure console app, create config once and pass it around however you want.

When creating configuration instance yourself, use TelemetryConfiguration.CreateDefault() and not the parameterless constructor (to enable correlation):

var config = TelemetryConfiguration.CreateDefault();
var client = new TelemetryClient(config);

Keep one TelemetryConfiguration per app lifetime (a few is also fine), but you may create as many clients as you want.
Do not forget to dispose TelemetryConfiguration when gracefully shutting down the application.

@pharring
Copy link
Member

pharring commented Aug 6, 2019

I think we advise using IOptions<TelemetryConfiguration> instead of a raw TelemetryConfiguration instance. At least that's what you get with the ILogger extension.

If building the DI container yourself, of course you're free to do whatever you want, but I think we prefer services.Configure<TelemetryConfiguration>(...) over services.AddSingleton<TelemetryConfiguration>(...).

@lmolkova
Copy link
Member Author

lmolkova commented Aug 6, 2019

yes, thanks for the clarification, @pharring, update comment above

@bjornharrtell
Copy link

I've used TelemetryConfiguration.Active to programmatically configure the configuration used with Microsoft.ApplicationInsights.NLogTarget. From the above information, it's not clear to me what the rewrite strategy should be in the case of Microsoft.ApplicationInsights.NLogTarget.

@xantari
Copy link

xantari commented Nov 27, 2019

This is causing a lot of confusion in logging frameworks like Serilog. We now don't have a way to catch early startup failures. See here: serilog-contrib/serilog-sinks-applicationinsights#121

@cijothomas
Copy link
Contributor

@xantari Could you open a new issue describing the issues with serilog intergrations?

@kinetiq
Copy link

kinetiq commented Dec 26, 2019

This is difficult since it's used on my ServiceBase, which is inherited over 400 times in my project... I don't even think there's a Resharper refactoring for this type of cascading dependency quagmire.

However, if someone does in fact have a way to automate this with Resharper or otherwise, this might be a good place to mention it...

@kinetiq
Copy link

kinetiq commented Dec 26, 2019

This is difficult since it's used on my ServiceBase, which is inherited over 400 times in my project... I don't even think there's a Resharper refactoring for this type of cascading dependency quagmire.

However, if someone does in fact have a way to automate this with Resharper or otherwise, this might be a good place to mention it...

Actually, I think I found it: https://stackoverflow.com/questions/6543078/resharper-or-visual-studio-shortcut-to-cascade-changes-to-a-constructor

@iSeiryu
Copy link

iSeiryu commented Jan 23, 2020

So how do we make logs in the Startup.cs file now? Before we would create the configuration of ApplicationInsights inside Program.cs and then inject TelemetryClient into Startup's constructor:

public Startup(IConfiguration configuration, TelemetryClient insights)
{
    _configuration = configuration;
    _insights = insights;
}

But now the instance of TelemetryClient is not going to be available in Startup.ConfigureServices() method.

@hansmbakker
Copy link

hansmbakker commented Jan 28, 2020

I received code that uses new TelemetryClient() to setup mock objects in tests (in the core application it is done properly using DI).

For this new TelemetryClient() instances in my tests I get a warning that it is deprecated with a link to this issue - how can we best mock a TelemetryClient instead?

Should I just do

var config = TelemetryConfiguration.CreateDefault();
var client = new TelemetryClient(config);

as per #1152 (comment)?

@SychevIgor
Copy link
Contributor

SychevIgor commented Feb 14, 2020

Microsoft, you mentioned that we should use "codeless" application insights integration for azure functions, but problem is that this integration ignore all soft of parent/child relations, w3c context context and make disturbed tracing - impossible. https://github.com/w3c/correlation-context/

From my perspective - suggest only "codeless" is a bit too strong best practice.

@bjornharrtell
Copy link

The issue with Microsoft.ApplicationInsights.NLogTarget persists and is related to #1457 and specifically that the obsolete method is used at

#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
with suppressed warnings and no way to override.

@nmaarse
Copy link

nmaarse commented Mar 2, 2020

Can you please update the readme file on the applicationinsight pages?
https://github.com/serilog/serilog-sinks-applicationinsights
The example provided still uses the obsolete version. Can you provide an example that reads the intrumentation key from the environment?

@iamalexmang
Copy link

What confuses me is that all the documentation available today (e.g. https://docs.microsoft.com/en-us/azure/azure-monitor/app/asp-net-core#how-can-i-track-telemetry-thats-not-automatically-collected: A singleton instance of TelemetryClient is already registered in the DependencyInjection container) suggests that dependency injection happens out of the box, yet that is not true or at least not what I'm experiencing: a TelemetryClient is NOT already registered in the DependencyInjection container.

Specifically, I'm adding a TelemetryClient object as a parameter to the constructor, and it does not contain either of the properties, initializers etc. configured.

In the past, I would configure a TelemetryClient object by instantiating it during service configuration, but since that option is obsolete, I'm confused how dependency injection should be handled now.
Do you mind elaborating that, please?

image

@cijothomas
Copy link
Contributor

What confuses me is that all the documentation available today (e.g. https://docs.microsoft.com/en-us/azure/azure-monitor/app/asp-net-core#how-can-i-track-telemetry-thats-not-automatically-collected: A singleton instance of TelemetryClient is already registered in the DependencyInjection container) suggests that dependency injection happens out of the box, yet that is not true or at least not what I'm experiencing: a TelemetryClient is NOT already registered in the DependencyInjection container.

Specifically, I'm adding a TelemetryClient object as a parameter to the constructor, and it does not contain either of the properties, initializers etc. configured.

In the past, I would configure a TelemetryClient object by instantiating it during service configuration, but since that option is obsolete, I'm confused how dependency injection should be handled now.
Do you mind elaborating that, please?

image

Please open a new issue with the description of what you are facing along with a minimal repro.

@iamalexmang
Copy link

Which repo. This one? I've already opened one related to the docs, but that's a different repo.

@cijothomas
Copy link
Contributor

Which repo. This one? I've already opened one related to the docs, but that's a different repo.

Please share the link. What I am saying is - open new issue instead of commenting on closed ones. Closed ones are not actively tracked.

@iamalexmang
Copy link

That's fair - good point. I've created a new issue inside this repo (see mention above). The one for the docs is MicrosoftDocs/azure-docs#59590

@mehric
Copy link

mehric commented Dec 30, 2020

Hi,

I am using TelemetryClient for adding correlation between 2 azure functions communicating with each other using Azure Event Grid. Unlike the Service Bus, the end-to-end transactions don't appear on the logs out of the box with the Event Grid.

However, I am hitting a problem here.

  • When I am injecting the TelemetryConfiguration into my function, it is completely empty.
  • When I am injecting the IOptions<TelemetryConfiguration> to my function, the TelemetryConfiguration is there but missing the actual config, even the instrumentation key.
  • When I am injecting my customized TelemetryConfiguration as below, it works but the end-to-end transaction logs get weird and loses significant correlations in the logs:
return services
    .AddSingleton(sp =>
    {
        var telemetryConfiguration = TelemetryConfiguration.CreateDefault();
        telemetryConfiguration.TelemetryInitializers.Add(new OperationCorrelationTelemetryInitializer());
        var config = sp.GetService<IConfiguration>();
        var instrumentationKey = config.GetValue<string?>("APPINSIGHTS_INSTRUMENTATIONKEY");
        if (instrumentationKey != null)
            telemetryConfiguration.InstrumentationKey = instrumentationKey;
        return telemetryConfiguration;
    });

However, when using the deprecated TelemetryConfiguration.Active it works perfectly. I noticed the Active config has more initializers in addition to the OperationCorrelationTelemetryInitializer, namely speaking:

  • Microsoft.Azure.WebJobs.Logging.ApplicationInsights.WebJobsRoleEnvironmentTelemetryInitializer
  • Microsoft.Azure.WebJobs.Logging.ApplicationInsights.WebJobsTelemetryInitializer

Not sure if that is there reason, but I couldn't find any good documentation or solution for this! Is there an alternative solution to make this work like the TelemetryConfiguration.Active?

@cijothomas
Copy link
Contributor

@mehric Please open an issue in the Azure Web Jobs SDK where the relevant code is hosted.

@mehric
Copy link

mehric commented Jan 7, 2021

Hi @cijothomas,

Thanks for the reply, however, I don't believe this is an issue with the Web Jobs SDK. The issue is that there was a way to get the active telemetry configuration before but it is deprecated now. Unfortunately, I couldn't find a working alternative for it and would appreciate it if someone could point out a working solution for it.

I actually created another issue here for it: #2144

@cijothomas
Copy link
Contributor

@mehric WebJob/Functions uses this SDK is a special way. Hence I suggest to open it in their repo.
If you have issues in a non Functions environment, please feel free to open a new issue in this repo, with steps to repro and we can definitely help.

@adambielecki
Copy link

In terms of unit tests the code that is using injected telemetry client through the method AddApplicationInsightsTelemetry() how can we mock telemetry client? What would be recommended best practice?
Thanks.

@chrisfcarroll
Copy link

So excuse my asking on a 2yr closed issue but I've read the thread this far and I still don't quite see:

What's the solution for logging startup ?

Yesterday my Azure web app told me 'http 500.30 failed to start' and without startup logging I'm blind.

@cijothomas
Copy link
Contributor

So excuse my asking on a 2yr closed issue but I've read the thread this far and I still don't quite see:

What's the solution for logging startup ?

Yesterday my Azure web app told me 'http 500.30 failed to start' and without startup logging I'm blind.

Please review the docs (https://docs.microsoft.com/en-us/azure/azure-monitor/app/ilogger) and open a new issue if required. This question doesn't seem to be related to this issue.

@imercerwillow
Copy link

Does the advice above change with the new AddApplicationInsightsTelemetryWorkerService feature?

https://docs.microsoft.com/en-us/azure/azure-monitor/app/worker-service

@asjvarsity
Copy link

asjvarsity commented Mar 24, 2023

In terms of unit tests the code that is using injected telemetry client through the method AddApplicationInsightsTelemetry() how can we mock telemetry client? What would be recommended best practice? Thanks.

did you find a solution to this?
@cijothomas

@pharring
Copy link
Member

@asjvarsity Please see #342 (comment) for an example of using a mock TelemetryChannel for unit testing.

@papadeltasierra
Copy link

FYI there are live public links to this article but the general public do not seem to have access to this article. For example https://learn.microsoft.com/en-us/azure/azure-monitor/app/api-custom-events-metrics is publicly accessible but then you cannot follow the link to this page from a public account.

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