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

Enable SQL Command text for non-stored procs in EventSource events for .NET Framework. #242

Merged
merged 1 commit into from Oct 4, 2019

Conversation

stebet
Copy link
Contributor

@stebet stebet commented Oct 3, 2019

When using System.Data.SqlClient, .NET Core provides the command text for all SQL operations but .NET Framework only does so for stored procedures in the EventSource events.

This has made it hard in the past to track down problematic SQL Statement by using the EventSource events, for example when using SQL dependency tracking in Application Insights in .NET Framework applications.

Since Microsoft.Data.SqlClient is a separate package, I think we have an excellent opportunity to change this behavior and bring .NET Framework up to speed with .NET Core, to more easily track SQL statements through the event source, without needing to install an additional profiler.

Would love to get input from @lmolkova and @cijothomas for this change as well, since they are familiar with the Application Insights side of things here.

@cijothomas
Copy link

@cijothomas cijothomas commented Oct 3, 2019

Thanks @stebet This will be a huge value add to customers to get full sql without additional steps, which are sometimes non-trivial (https://docs.microsoft.com/en-us/azure/azure-monitor/app/asp-net-dependencies#advanced-sql-tracking-to-get-full-sql-query)

If this is approved, we need corresponding changes in application insights side as well. Also we want to enable full sql under a feature flag only so as to let users turn it off. (https://github.com/microsoft/ApplicationInsights-aspnetcore/issues/980)

cc: @SergeyKanzhelev @Dmitry-Matveev who would know a lot more of additional historical information about sql as well.

@stebet
Copy link
Contributor Author

@stebet stebet commented Oct 3, 2019

I actually got the SQL command to show up in Application Insights without any changes needed there when I tested this change locally. Don't know if you'd want to change anything other than the feature flag on the Application Insights side of things?

Example from me repro app:

{
    "name": "Microsoft.ApplicationInsights.Dev.RemoteDependency",
    "time": "2019-10-03T09:55:06.9817477Z",
    "tags": {
        "ai.cloud.roleInstance": "*redacted_machine_name*",
        "ai.user.id": "Yrl8e",
        "ai.operation.id": "f6040a3049d9eb43b6d5e729f8cf7b1c",
        "ai.operation.parentId": "|f6040a3049d9eb43b6d5e729f8cf7b1c.3272d3e70294844b.",
        "ai.operation.name": "GET Home/Index",
        "ai.location.ip": "::1",
        "ai.internal.sdkVersion": "rddf:2.11.0-21563",
        "ai.internal.nodeName": "*redacted_machine_name*"
    },
    "data": {
        "baseType": "RemoteDependencyData",
        "baseData": {
            "ver": 2,
            "name": "SELECT name FROM sys.tables",
            "id": "|f6040a3049d9eb43b6d5e729f8cf7b1c.5deeaba979d30045.",
            "data": "SELECT name FROM sys.tables",
            "duration": "00:00:00.0270801",
            "success": true,
            "type": "SQL",
            "target": "*redacted_server_name* | *redacted_db_name*",
            "properties": {
                "DeveloperMode": "true",
                "_MS.ProcessedByMetricExtractors": "(Name:'Dependencies', Ver:'1.1')"
            }
        }
    }
}

@SergeyKanzhelev
Copy link

@SergeyKanzhelev SergeyKanzhelev commented Oct 3, 2019

My understanding was that this was intentionally limited to SP name only to minimize the need for an additional security review and exposing any configuration setting.

I'm all for this change. I'd let code owners to decide whether they are willing to expose this information to the customers and improve SQL issues troubleshooting and manageability experiences.

@stebet
Copy link
Contributor Author

@stebet stebet commented Oct 3, 2019

Agreed, especially since this is the default in .NET Core, and moving code over to this new library is a conscious decision requiring code changes, so there is an opportunity here to make this change i.m.o.

@lmolkova
Copy link

@lmolkova lmolkova commented Oct 3, 2019

we had an agreement with SQL team (@David-Engel) around the instrumentation of Microsoft.SqlClient with DiagnosticSource - it brings other benefits such as end-to-end correlation and flexibility of using raw SQL commands in the payload. AFAIK this in the backlog.

@cheenamalhotra
Copy link
Member

@cheenamalhotra cheenamalhotra commented Oct 3, 2019

@stebet @cijothomas
Would grab your attention to issue #52 if you can confirm we can close that issue.

Thanks!

@David-Engel
Copy link
Contributor

@David-Engel David-Engel commented Oct 4, 2019

I agree. This seems like a good change in general and for parity between the two targets (netcore/netfx).

@cheenamalhotra cheenamalhotra added this to the 1.1.0-preview2 milestone Oct 4, 2019
@cheenamalhotra cheenamalhotra added this to In progress in SqlClient v1.1.0 via automation Oct 4, 2019
@cheenamalhotra cheenamalhotra merged commit 65d034e into dotnet:master Oct 4, 2019
6 checks passed
SqlClient v1.1.0 automation moved this from In progress to Done Oct 4, 2019
@stebet stebet deleted the commandTextInEventSource branch Oct 7, 2019
yukiwongky pushed a commit to yukiwongky/SqlClient that referenced this issue Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants