Skip to content
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

Add a flag to send SQL Command text as Data for non-stored procedures. #1606

Closed
stebet opened this issue Dec 12, 2017 · 20 comments
Closed

Add a flag to send SQL Command text as Data for non-stored procedures. #1606

stebet opened this issue Dec 12, 2017 · 20 comments

Comments

@stebet
Copy link
Contributor

stebet commented Dec 12, 2017

A little bit related to microsoft/ApplicationInsights-dotnet-server#193

Add a flag/configuraiton value or some other way to the DependencyCollector to allow applications to insert the text of the SQL Command being executed into the Data property of the DependencyTelemetry for SQL dependencies in .NET Framework applications. This works as intended for .NET Core apps, but not for .NET Framework.

I realize some data might potentially be sensitive (although with proper query parameterization that should not happen) but it's should be up to the user to allow that text to be in there instead of it always being just empty. The default behavior could be kept as is to prevent people from automatically opting-in to potentially send sensitive data.

For big queries, it might be a good idea to also have a setting that would truncate the text after X characters.

@SergeyKanzhelev
Copy link
Contributor

@cijothomas , @lmolkova were there any updates for this? I remember something was added to Data recently

@isaacrlevin
Copy link

I have a customer that is curious about anyway to log the parameters being passed for an EF call. We know we can get the params and command and add track events inline with the EF call, but that seems hacky. I would love to have a TelemetryInitializer where I can do this for ALL SQL calls.

@cijothomas
Copy link
Contributor

I cannot recollect any recent changes to SQL Dependencies..

@SergeyKanzhelev
Copy link
Contributor

@cijothomas @lmolkova is it feasible? Or this info is not available in DiagnosticsSource?

@cijothomas
Copy link
Contributor

@isaacrlevin
Copy link

@cijothomas not really following where I can get the parameters using the class you gave me. Can you provide a gist?

@cijothomas
Copy link
Contributor

@isaac2004 are you using .net core app? then sql query should be captured automatically without doing anything..

@isaacrlevin
Copy link

@cijothomas are you sure? This is an ASP.NET Core 2.2 app, parameter is not present
image

@cijothomas
Copy link
Contributor

@isaac2004 As per this code (https://github.com/Microsoft/ApplicationInsights-dotnet-server/blob/develop/Src/DependencyCollector/Shared/Implementation/SqlClientDiagnostics/SqlClientDiagnosticSourceListener.cs#L125) you should be getting full sql text.
Can you share the SDK version you are using? are you on .net framework?
If you can give a small repro app, i can investigate to see why sql query is not capture.

@isaacrlevin
Copy link

Before I do that, I want to ensure we are talking about the same thing. The above screenshot is a SQL dependency call using EFCore. By design, EF calls are parameterized but you are saying I should be able to collect the raw sql with where clause values via DependencyTelemetry object?

@cijothomas
Copy link
Contributor

@isaac2004 looking at your screenshot above, it appears that 'Data' already has the full SQL query. This issue was about not getting Sql Command in data, right?

@isaacrlevin
Copy link

No it was about getting raw sql. This is parameterized sql

@cijothomas
Copy link
Contributor

@isaac2004 thanks for clarifying. You are right We don't collect parameters yet. (Haven't explored if it is feasible or not)

@isaacrlevin
Copy link

Yep figured. I know I can get it inline. I wonder if I can pass the EF transaction into a Telemetry Initializer

@stebet
Copy link
Contributor Author

stebet commented Jun 7, 2019

I created the issue originally for .NET Framework apps, where the SQL text is not put into the .Data on the DependencyTelemetry objects for SQL calls. It works fine on .NET Core.

@TimothyMothra
Copy link
Member

Hello @stebet,
This works in .NET Core because there are additional libraries that support collecting the SQL text.

For .NET Framework, you'll need to install the "Instrumentation Engine" which is distributed with the Status Monitor product.

See here for instructions depending on your environment:
https://docs.microsoft.com/azure/azure-monitor/app/asp-net-dependencies#advanced-sql-tracking-to-get-full-sql-query

If you don't want to install the Status Monitor product (msi), you can check out Status Monitor v2 (powershell) which provides an option to only install the Instrumentation Engine. V2 is currently in a public preview, but we expect to GA soon.

@stebet
Copy link
Contributor Author

stebet commented Jun 11, 2019

Hi @MS-TimothyMothra and thank you for your response.

I know it works in .NET Core. It would be great to get it to work without the instrumentation engine in .NET Framework but I realize that might involve changes to both the SQL Client libraries as well as the DiagnosticSource providers/listeners in AppInsights, so I created this ticket to see how feasible it was.

I haven't checked if it works out of the box in Microsoft.Data.SqlClient (the new library) though. That might provide a way to make it work without risking breakage of older stuff (since it's a new library/dependency)?

@lmolkova
Copy link
Member

Microsoft.Data.SqlClient

AFAIK this is now pure replication of .NET Fx SQL client. We are working with SQL team to make diagnostics better in future in Microsoft.Data.SqlClient.

@stebet
Copy link
Contributor Author

stebet commented Jun 19, 2019

@lmolkova That's fantastic to hear. Even just getting feature parity with .NET Core without additional profilers/instrumentation engines would go a long way.

Thanks for your responses :)

@TimothyMothra TimothyMothra transferred this issue from microsoft/ApplicationInsights-dotnet-server Jan 10, 2020
@TimothyMothra TimothyMothra added this to the Future milestone Jan 10, 2020
@stebet
Copy link
Contributor Author

stebet commented Mar 10, 2020

Closing this issue as this is now possible using the Microsoft.Data.SqlClient package and can now be opted into starting with 2.14.

@stebet stebet closed this as completed Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants