Skip to content
This repository has been archived by the owner on Jun 10, 2020. It is now read-only.

Deprecate extension methods on WebHostBuilder #919

Closed
cijothomas opened this issue Jul 9, 2019 · 3 comments
Closed

Deprecate extension methods on WebHostBuilder #919

cijothomas opened this issue Jul 9, 2019 · 3 comments

Comments

@cijothomas
Copy link
Contributor

cijothomas commented Jul 9, 2019

Proposing to deprecate UseApplicationInsights() extension methods on IWebHostBuilder, https://github.com/microsoft/ApplicationInsights-aspnetcore/blob/develop/src/Microsoft.ApplicationInsights.AspNetCore/Extensions/ApplicationInsightsWebHostBuilderExtensions.cs#L18

The alternate option is to use the AddApplicationInsightsTelemetry() extensions methods on IServiceCollection.
https://github.com/microsoft/ApplicationInsights-aspnetcore/blob/develop/src/Microsoft.ApplicationInsights.AspNetCore/Extensions/ApplicationInsightsExtensions.cs#L65

Reasons:
Former did not allow overrides to customize behavior, so all public docs were already recommending the latter approach.
Visual Studio AI Tools used the Former approach, and this has created customer confusion about which one to use.
With Asp.Net Core 3.0, default templates use IHostBuilder, as opposed to IWebHostBuilder - this already means customers have to use extension methods on IServiceCollection. As of preview 7, the Visual Studio integrated enablement is broken due to this reason. Users should follow non-VS doc to workaround(https://docs.microsoft.com/en-us/azure/azure-monitor/app/asp-net-core#enable-application-insights-server-side-telemetry-no-visual-studio)

If there are no concerns, will ask VS Team to modify "Add Application Insights" experience to use extension methods on IServiceCollection.

@cijothomas
Copy link
Contributor Author

@lmolkova @Dmitry-Matveev @MS-TimothyMothra please share any concerns.

@Dmitry-Matveev
Copy link
Member

If the approach is switched in VS - will VS extension assume that the exising projects instrumented in the old way are not instrumented and will try to add AddApplicationInsightsTelemetry() in addition to UseApplicationInsights()?

Other than that, deprecation should not be an issue - I guess you are suggesting to obsolete in 2.* and drop in 3.*?

@cijothomas
Copy link
Contributor Author

I'll check how VS Extension detects sdk presence. While adding both methods are not harmful (i.e there wont be double addition as methods internally check if already added), we surely don't want to have that confusing experience.

Yes to Obsolete in 2., and Drop in 3., following standard deprecation processes.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants