No longer depend on Obsolete AppInsights APIs #7745
Conversation
xavierdecoster
requested review from
agr,
cristinamanum,
dannyjdev,
joelverhagen,
loic-sharma,
ryuyu,
shishirx34,
skofman1 and
zhhyu
as code owners
| @@ -127,7 +126,6 @@ protected void ConfigureGalleryServices(IServiceCollection services, IConfigurat | |||
| services.AddScoped<IDeleteAccountService, DeleteAccountService>(); | |||
|
|
|||
| services.AddScoped<IUserService, AccountDeleteUserService>(); | |||
| services.AddScoped<ITelemetryClient>(sp => { return TelemetryClientWrapper.Instance; }); | |||
There was a problem hiding this comment.
This was already configured here.
xavierdecoster
force-pushed
the
dev-ai-init
branch
from
845d70f to
aaba398
Compare
|
|
||
| ApplicationInsightsConfiguration applicationInsightsConfiguration; | ||
|
|
||
| if (heartbeatIntervalSeconds > 0) |
There was a problem hiding this comment.
I see that ApplicationInsightsConfiguration is initialized twice. Here and in DefaulyDependenciesModule. Why is this needed?
There was a problem hiding this comment.
I was wondering the same thing, but didn't get to digging into it more until now.
The OwinStartup class loads the DefaultDependenciesModule through a call to app.UseAutofacInjection here, which in turn calls builder.RegisterAssemblyModules(currentAssembly); here.
My thinking is that we should consolidate everything within DefaultDependenciesModule and remove additional telemetry configuration from OwinStartup.
| } | ||
| } | ||
| // Register the ILoggerFactory and configure AppInsights. | ||
| var applicationInsightsConfiguration = ConfigureApplicationInsights(configuration); |
There was a problem hiding this comment.
applicationInsightsConfiguration [](start = 16, length = 32)
Does this override the configuration we have in a applicationinsights.config? One important thing we configured is not to sample: https://docs.microsoft.com/en-us/azure/azure-monitor/app/sampling
Please verify that we don't get sampling back with this change. I think looking at the runtime config with debugger should be sufficient.
There was a problem hiding this comment.
xavierdecoster
force-pushed
the
dev-ai-init
branch
2 times, most recently
from
8fa9940 to
8c95215
Compare
xavierdecoster
force-pushed
the
dev-ai-init
branch
from
8c95215 to
bb5a8eb
Compare
This PR updates the Gallery (and the jobs within the solution) to use the new Application Insights initialization logic introduced in ServerCommon 2.60.0 and NuGet.Jobs (current dev branch).
Goal is to move off obsolete AppInsights APIs, such as
TelemetryConfiguration.Active.In doing so, some legacy code and static classes required refactoring. Work in progress, but opening the PR already to collect feedback early on.
Also needs to consume a to-be-released update of
NuGet.Services.Logging(PR pending)