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

Removing the SQL command text from the dependency name for Framework #1723

Merged
merged 5 commits into from
Mar 27, 2020

Conversation

stebet
Copy link
Contributor

@stebet stebet commented Mar 10, 2020

Removing the SQL command text from the dependency name for Framework processing to reduce event bloat.

Changes

When using the Microsoft.Data.SqlClient NuGet package for SQL statements, which now provides the SQL Command Text, the SQL dependency telemetry are all bloated with the SQL Command text for ad-hoc queries when running under .NET Framework.

For comparison, the .NET Core SQL dependency collector does not use the command text in the dependency name, and just relies on the Data property to provide that information, so this should bring more parity between .NET Framework and .NET Core SQL dependencies, and reduce the amount of data sent when running under the .NET Framework and executing ad-hoc statements (like when using Entity Framework).

Checklist

  • I ran Unit Tests locally.
  • CHANGELOG.md updated with one line description of the fix, and a link to the original issue if available.

Stefán J. Sigurðarson added 2 commits March 10, 2020 13:41
@stebet
Copy link
Contributor Author

stebet commented Mar 10, 2020

Here is an example of what the dependency telemetry currently looks like in the Performance->Dependencies blade in AppInsights. The long blurred out segments are the SQL Command Text, which should only be needed in the "Data" property and shouldn't be a part of the name. This might be handy for stored procs, but it is dreadful for long ad-hoc queries.

image

For comparison, here is what a .NET Core SQL Dependency list currently looks like:
image

@stebet
Copy link
Contributor Author

stebet commented Mar 23, 2020

Is this something that can be looked at @TimothyMothra? Don't know if I'm supposed to add a reviewer or if that's all on your end :)

@TimothyMothra
Copy link
Member

Hi @stebet !
Our work schedules have been interrupted and we fell behind in reviews. Thanks for the reminder.
This looks straight forward to me, I'll queue the builds now.

@TimothyMothra
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@TimothyMothra
Copy link
Member

@cijothomas I'm okay with this change. I just wanted to double-check with you... do you know if anything was dependent on this behavior?

@cijothomas
Copy link
Contributor

Thanks. The full sql text was supposed to be part of .Data only . Thanks for fixing this.

@TimothyMothra
Copy link
Member

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@TimothyMothra TimothyMothra merged commit fe320a4 into microsoft:develop Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants