Use dependency injection to configure ApplicationInsights #2
Conversation
| @@ -53,6 +170,7 @@ public static class ApplicationInsightsLoggerExtensions | |||
| return loggerFactory; | |||
| } | |||
|
|
|||
| [Obsolete("Use 'AddApplicationInsights' IHostBuilder extension method instead.", true)] | |||
There was a problem hiding this comment.
I think we're okay to just rip these out rather than marking them obsolete. When we merge this branch it's going to be a massive breaking change anyway.
| { | ||
| services.AddSingleton<ITelemetryInitializer, WebJobsRoleEnvironmentTelemetryInitializer>(); | ||
| services.AddSingleton<ITelemetryInitializer, WebJobsTelemetryInitializer>(); | ||
| services.AddSingleton<ITelemetryInitializer, WebJobsSanitizingInitializer>(); |
There was a problem hiding this comment.
Should these be TryAddEnumerable since you're trying to register multiple initializers?
There was a problem hiding this comment.
adding multiple services with the same service type is ok (while they have different implementation type).
It's also possible to use TryAddEnumerable (thanks, I did not know about it!)
services.TryAddEnumerable(new[] {
ServiceDescriptor.Describe(serviceType: typeof(ITelemetryInitializer), implementationType: typeof(WebJobsRoleEnvironmentTelemetryInitializer), lifetime: ServiceLifetime.Singleton),
ServiceDescriptor.Describe(serviceType: typeof(ITelemetryInitializer), implementationType: typeof(WebJobsTelemetryInitializer), lifetime: ServiceLifetime.Singleton),
ServiceDescriptor.Describe(serviceType: typeof(ITelemetryInitializer), implementationType: typeof(WebJobsSanitizingInitializer), lifetime: ServiceLifetime.Singleton)
});But i think it looks more complicated and verbose, so I'd prefer to keep current implementation
There was a problem hiding this comment.
If AddSingleton works, by all means use it. We didn't think that this call would work: provider.GetServices<ITelemetryInitializer>() unless you tried to register it as an enumerable. I thought that it would overwrite your previous service registration and leave you with only a single ITelemetryInitializer. But it looks like it does work. Nice :-)
| TelemetryClient client = new TelemetryClient(configuration); | ||
|
|
||
| string assemblyVersion = GetAssemblyFileVersion(typeof(JobHost).Assembly); | ||
| client.Context.GetInternalContext().SdkVersion = $"webjobs: {assemblyVersion}"; |
There was a problem hiding this comment.
Cool. We'll need to do this in the Functions repo (we change this to "azurefunctions: " for better tracking) -- but this should be pretty easy.
| .Use((next) => new FilteringTelemetryProcessor(filter, next)); | ||
|
|
||
| SamplingPercentageEstimatorSettings samplingSettings = | ||
| provider.GetService<IOptions<SamplingPercentageEstimatorSettings>>()?.Value; |
There was a problem hiding this comment.
@lmolkova this looks good to me. I'll let @brettsam chime in if he has any additional comments, but we should have this merged in.
I wanted to give you a heads up first as we're planning on rebasing this on the current dev branch and I don't want to do it before your work is merged and we have a green light from you that we don't have any pending commits. I don't want to leave you with a headache to deal with after we force push to this branch.
|
I agree -- let's go ahead and merge. |
|
sorry I'm out on a conference. I think it's ready, plrase merge it ! I will submit other work items with a new PRs. |
|
Thanks! Merged. We'll keep you updated. You may find that the branch has been rebased on dev when you try to pull again next time. |
This change implements PR1 from Azure#1731.
Please let me know if I can remove
DefaultTelemetryClientFactorycompletelyMotivation
Now, configuration of applicationInsights is done via DefaultTelemetryClientFactory.
With enabling dependency collection and correlation this is going to grow and become complicated.
It's also hard to tune for users.
Another issue is that if AppInsights enables new features, Azure Functions would need to change the code to enable them as well.
Proposal (implemented)
Leverage Dependency Injection to instantiate and control the lifetime of AppInsights instances. Hide it beyond
IHostBuilder.ConfigureApplicationInsights(Configure/Add/Use) extension. Eventually, ApplicationInsights should provide such extension for non-Web Hosts and maintain it (now it works in AspNetCore SDK for web hosts).This will enable simple modification of configuration in a typical ApplicationInsights and DI way:
Azure#1556
Azure#1618
Azure#1416
/cc @brettsam @fabiocav