microsoft / ApplicationInsights-dotnet-server

Microsoft Application Insights for .NET Web Applications
https://azure.microsoft.com/services/application-insights/
133 stars 67 forks source link

Adding a flag to enable/disable collection of SQL Command text #1285

Closed stebet closed 4 years ago

stebet commented 5 years ago

Adding a boolean flag to the DependencyTrackingTelemetryModule which instructs the SQL processors to enable/disable collecting SQL command text.

Also added tests to verify the functionality.

Perhaps there is a better place for that flag than on the module itself since it's propagated down to the other classes, but it made sense to put it there.

This relates to https://github.com/microsoft/ApplicationInsights-aspnetcore/issues/980 as well as

1282

cijothomas commented 5 years ago

@Dmitry-Matveev @SergeyKanzhelev I am fine with this PR changes. I could use one of your review as well especially on the part where we are changing SQL Text to be opt-in for .net core customers now.

lmolkova commented 5 years ago

LGTM.

regarding defaults, I suggest

Dmitry-Matveev commented 5 years ago

Looks good. Regarding defaults, can you please check with Dave? It may very well be a privacy requirement to have it opt-in only anyway (reqs recently were updated for several privacy considerations), in this case, yes, we're fixing a bug.

stebet commented 5 years ago

Distinguishing between the libraries in the collection logic would make it a bit more complicated as well, instead of just treating both libraries the same. Will look into it though if you want to split the default logic between the old and new library.

TimothyMothra commented 5 years ago

Adding a reminder here, we need to inform the support teams about this change. Although it's a bug, we're changing the default behavior and this may negatively impact customers.

stebet commented 5 years ago

Is there anything else needed from me on this PR? Any changes required?

cijothomas commented 5 years ago

@stebet nothing needed from you. We are discussing internally about the default setting. Will be back with finding here. (about whether we want sqltext ON by default or OFF by default)