-
Notifications
You must be signed in to change notification settings - Fork 358
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
updating AppInsights nugets #2213
Conversation
@@ -21,7 +21,7 @@ public static class ApplicationInsightsLoggingBuilderExtensions | |||
public static ILoggingBuilder AddApplicationInsights( | |||
this ILoggingBuilder builder) | |||
{ | |||
return builder.AddApplicationInsights(null); | |||
return AddApplicationInsights(builder, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an ambiguous reference... I'm assuming the new AspNetCore package includes the AddApplicationInsights()
extension. Wasn't sure if it was worth updating to that or not...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to update to our new loggerprovider. But please consider marking this method as obsoltete, and provide a new one with AppApplicationInsightsWebJob
@cijothomas could you please have a look? @brettsam, there was some caveat around the ILogger provider. Now the WebJobs ILogger and AppInsihgts ILogger have the same name for the provider. We've been wondering if it affects anything and if it creates any confusion. @cijothomas wanted to look into this more precisely and see if we need to do anything else. |
It does create some confusion... I'm not sure that we can change it on our end in v2, however. In v3 I will rename it to be more WebJobs/Functions-specific. We already call out that no one should ever call Although that calls out |
AddApplicationInsightsTelemetry() on IServiceCollection or UseApplicationInsights() on IWebHostBuilder - both internally does the same, and now enables Ilogger collection Warning and above. Since docs suggest customers to NOT do this, i think we are fine. Due to conflicting name of the Provider alias, configuring logging level appsettings.json should be validated. In code, its possible to use fully qualified name, but not sure about config based approach. Edit: |
@@ -21,7 +21,7 @@ public static class ApplicationInsightsLoggingBuilderExtensions | |||
public static ILoggingBuilder AddApplicationInsights( | |||
this ILoggingBuilder builder) | |||
{ | |||
return builder.AddApplicationInsights(null); | |||
return AddApplicationInsights(builder, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to update to our new loggerprovider. But please consider marking this method as obsoltete, and provide a new one with AppApplicationInsightsWebJob
Updating App Insights nugets -- which also brought along a new extension method
AddApplicationInsights()
, which conflicts with ours. To make this more straightforward, I've marked this obsolete and renamed itAddApplicationInsightsWebJobs
.