Add command text to SQL command

[jaredcnance]

Issue #, if available: None

Description of changes: Adds the parameterized SQL command to the metadata. I tried adding it using the AddSqlInformation API, but it isn't available in the console/UI so I suspect there is a requisite backend change to handle this. I don't really expect this to get merged, but hope to either get some advice on how this should be done today, or start a discussion on how it should be done in the future.

Example content of CommandText (generated by EntityFrameworkCore):

SELECT TOP(1) [e].[Id], [e].[Name]\nFROM [users] AS [e]\nWHERE [e].[Id] = @__get_User_0

It would be nice to strip the newlines out or pretty print them. That probably shouldn't be done application side since it could result in lots of string allocations, but it might make sense on the console side of things, especially if it receives special treatment as SQL command text.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[yogiraj07]

Hi @jaredcnance ,
AWS X-Ray records sanitized query in sql namespace under sanitized_query field.
So the correct way to use is recorder. AddSqlInformation("sanitized_query","query to be recorded"); instead of meta data. So can you please update this PR here

We will get our security team review it as soon as possible.

In the mean time could you confirm CommandText would be sufficient to record. The API reference documentation doesn't mention anything about stripping or recording user data.

Thank you for the contribution.

Best
Yogi

[jaredcnance]

@yogiraj07 I've updated the PR to use sanitized_query instead. CommandText is the raw query less the paramterized values. In general, user information should be specified as query parameters.

In the absence of an ORM you would write your queries like:

const string userId = "my-id";
using (var conn = new SqlConnection(connString))
{
    var cmd = new SqlCommand("Select * From users Where userId=@userId", conn);
    cmd.Parameters.Add(new SqlParameter("@userId", userId));

    // the command text will not contain the parameter values
    Assert.DoesNotContain(cmd.CommandText, userId);

    // send the query
    await conn.OpenAsync();
    using(var reader = await cmd.ExecuteReaderAsync())
        while(await reader.ReadAsync()) { /* ... */ }
}

This puts the burden on the developers but also avoids any magic in-process string replacement and unnecessary allocations. There are also performance benefits when using parameterized queries (e.g. SQLServer caches the query execution plan instead of creating a new one).

It seems that there is a mix of positions on this across the SDKs. The response for Java in January was "not yet". But it seems like it is already supported in go and python.

Could this be put behind a global options flag that's disabled by default? At a minimum, I think CollectSqlInformation should be protected virtual to make subclassing easier:

aws-xray-sdk-dotnet/sdk/src/Handlers/SqlServer/TraceableSqlCommand.netstandard.cs

Line 443 in 831c7e6

private void CollectSqlInformation()

[yogiraj07]

Hi @jaredcnance ,
CollectSqlInformation is a helper method. I think, making Intercept() and InterceptAsync() method protected is a good choice considering maintainability of the code and giving flexibility for the users to write their own implementation on the main method.

[jaredcnance]

@yogiraj07 I agree that Intercept/InterceptAsync should likely be protected virtual but I would contend that CollectSqlInformation should also be overridable because of the targeted nature of the method and a known use case. I don't think adding this method to the public API surface area adds much risk or complexity considering the nature of the method.

All I want to do is add data to the published payload—whether or not CollectSqlInformation is the best way to do it or not is debatable. But, as an application developer, I shouldn't be concerned with beginning and ending subsegments and I shouldn't have to override 2 methods just to add the sanitized_query. Having some hook that allows developers to add their own data to XRay's SQL payload without having to C&P any boilerplate seems like a reasonable request.

[yogiraj07]

Hi @jaredcnance ,
I think adding a feature flag to opt-in for recording sanitized query, to all TraceableSqlCommand constructors is a good idea. By default this flag will be false and no query will be recorded. This will help in conscious decision.

In addition to this, I think Intercept/InterceptAsync can be protected virtual, if the users want there own implementation of the interceptors.

Can you please make the changes to both .NET and .NET Core platform.

Thanks,
Yogi

[yogiraj07]

Hi @jaredcnance ,
Would you like to provide some update? Or else I can work on this PR for its completion.

Please let me know.

Thanks,
Yogi

[jaredcnance]

@yogiraj07 i have the implementation completed already but it's my opinion that there need to be tests on this before it rolls out. The current design makes testing TraceableSqlCommand incredibly difficult for a few reasons. The first is that no abstraction has been created for the underlying SqlCommand. There are a couple issues here:

1. SqlCommand is sealed
2. We depend on return types that are specific to the SqlCommand type rather than the abstract DbCommand return types.

So, mocking/stubbing the sql operations is not easy without some ugly decisions (such as creating a SqlCommandFactory that returns a proxy to SqlCommand), so I decided to see if I could inherit TraceableSqlCommand and test the protected methods (Intercept) directly. This led me to the next issue:

3. TraceableSqlCommand has an implicit dependency on AWSXRayRecorder.Instance.Instance returns the concrete type AWSXRayRecorder rather than the interface IAWSXRayRecorder. Consumers of this member depend on methods that don't exist on the interface.

So, at a minimum we need to:

1. Refactor all consumers of AWSXRayRecorder.Instance so that they depend on IAWSXRayRecorder. This necessarily involves expanding the existing interface which is a breaking change unless you use C#8's default interface methods.

2. Either make the setter for AWSXRayRecorder.Instance internal with the addition of InternalsVisibleTo("AWSXRayRecorder.UnitTests, ...) or make the dependency on IAWSXRayRecorder explicit. This will allow the test to mock the recorder, but would be a breaking change.

In addition to the tests, I think configuring the collection of SQL queries should be 2 fold:

1. Global option on XRayOptions
2. Local option in constructor TraceableSqlCommand(bool collectSqlQueries = false) that if provided overrides the global option.

Let me know what your thoughts are and I can finish this work up.

Also: this repo needs automated tests for CI. I am working on OSX and can't build the full-framework version (net45 runtime).

[yogiraj07]

Hi @jaredcnance ,
I worked internally on your comments.
AWSXRayRecorder.Instance return type probably is not the problem, but Intercept() accessing Connection.
With research on mocking this class and your analysis, I think going ahead with SqlCommandFactory may be the path forward. This will not be a breaking change.

I think, XRayOptions and constructor options, as 2 way opt-in way is okay. However, we can document it clearly the precedence of constructor value is over the XRayOptions value. By default, in either case, the value will be false.

We also need tests for XRayOptions.

Once the PR is finalize, I will do a fast follow-up for NET45.

Sure, I have CI for the repo in the backlog and will prioritize it soon.

[yogiraj07]

Hi @jaredcnance ,
Minor changes, but logically it looks good. I will do local testing soon. Thanks for the updates.

Best,
Yogi

[yogiraj07]

Hi @jaredcnance , Thank you for the update. I would expedite internal security review process and merge the PR at the earliest. As a fast follow-up, after merging the PR, I would add this support to NET45 platform.

[yogiraj07]

Hi @jaredcnance ,
Apologies in delay in response.
I am performing a test to set "CollectSqlQueries : true" in appsettings.json.

This is my json

{
  "XRay": {
    "DisableXRayTracing": "false",
    "SamplingRuleManifest": "sampling-rules.json",
    "UseRuntimeErrors": "true",
    "CollectSqlQueries": "true"
  }
}

However, ShouldCollectSqlText returns false.

The reason is, CollectSqlQueries key is not read from appsetting.json file. You would need to add key here: ConfigurationExtensions.

[jaredcnance]

Ah, I see. Instead of the GetSetting methods, I would have expected the IConfiguration["XRay"] block to be directly deserialized to XRayOptions e.g.:

services.Configure<XRayOptions>(Configuration.GetSection("XRay"));

// or

var options = new XRayOptions();
Configuration.GetSection("XRay").Bind(options);

But, for now I can make the change to add the CollectSqlQueries key.

[yogiraj07]

@jaredcnance , good point. For the moment you can also add unit test for the new configuration.

[rtablada]

Any update on this @jaredcnance or @yogiraj07? What is missing to get this merged and a version published?

[yogiraj07]

@jaredcnance , can you submit another revision with necessary changes requested? I have added CI, the current PR fails while building since the changes are only for NET Core. The CI will help you to make changes only for NET Core platform. If you are okay, I can make those changes on your behalf and expedite the implementation for NET 45 framework.

[jaredcnance]

@yogiraj07 go ahead and commit on top of my branch, I don't think I will have cycles to work on this anytime soon.