Make SQL settings available unconditionally

[ghost]

Fixes #1862.

Changes

SQL settings should be available regardless of the compile target because .netstandard libraries can be run in framework or .net core. Currently you get runtime exceptions about methods not found when using certain properties and then executing under .net framework.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Changes in public API reviewed

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Jann Roder (45db513f36377b8c2b558f4c3da39d80cae690c4)

[codecov]

Codecov Report

Merging #1864 (d4f9969) into main (110b5c2) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1864      +/-   ##
==========================================
+ Coverage   83.79%   83.87%   +0.07%     
==========================================
  Files         251      251              
  Lines        8857     8857              
==========================================
+ Hits         7422     7429       +7     
+ Misses       1435     1428       -7     

Impacted Files	Coverage Δ
...ation.SqlClient/SqlClientInstrumentationOptions.cs	98.64% <100.00%> (ø)
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	73.52% <0.00%> (-8.83%)	⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs	56.25% <0.00%> (-6.25%)	⬇️
...tation/OpenTelemetryProtocolExporterEventSource.cs	85.00% <0.00%> (+10.00%)	⬆️
...mentation/ExportClient/BaseOtlpGrpcExportClient.cs	62.50% <0.00%> (+12.50%)	⬆️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs	50.00% <0.00%> (+14.28%)	⬆️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs	59.09% <0.00%> (+22.72%)	⬆️

[mbakalov]

Ah, this confusing API again! Thanks @roederja2!

For context, this was first added in #1599 and then slightly renamed in #1645. The issue we were trying to solve then was this:

• It is nice to have granular control for Text/SP capturing, and the netcore versions of System.Data.SqlClient and Microsoft.Data.SqlClient provide us that.
• However on netfx we only get the sqlCmd.CommandText and the instrumentation has no context on whether it is Text or SP.

So back then we decided to have different API:

• For netfx we just have bool SetDbStatement
• For netcore we have both bool SetDbStatementForStoredProcedure and bool SetDbStatementForText

The "netcore" options above actually get compiled for the netstandard2.0 target though, which now makes the following scenario (described in #1862 that this PR is about) possible:

• consoleApp -> classLib -> Otel SDK + SqlClient instrumentation, where
• consoleApp is targeting net48
• classLib is targeting netstandard2.0

Now because classLib is netstandard, it uses the bool SetDbStatementForStoredProcedure api, which compiles fine but crashes with a runtime error when called from a net48 process.

I am not sure what the best way to go about this. I think typically the SDK should be configured by the host code (consoleApp) targeting specific runtime, not the library code. But it looks like there are legitimate scenarios where configuring it in a netstandard library is desirable. Besides this SqlClient api issue, can anyone think of any other problems this might cause?

If we want to change the api, there are several options:

• Option 1 - do what this PR suggests, have all 3 settings available on all platforms, explain in comments/docs how they are meant to be used.
• Option 2 - only keep SetDbStatementForText/SetDbStatementForStoredProcedure. Make the "Text" control all behavior on netfx. The SP setting will just be ignored. Will need some comments/docs as well, but maybe this is a bit clearer?
• Option 3 - only have a single setting SetDbStatement that controls everything on all platforms. Simplest api, but lose ability for granular control on netcore.

Not sure which one is best 🤷

@CodeBlanch , @cijothomas , @roederja2 - any thoughts?

[ghost]

I like option 2.

The reason why I have the configuration in a netstandard library is that it is the same for all our services - some of which are already .net core while some are still on framework.

[ghost]

Looking at the comment for SetDbStatement it seems it's not even consistent what it does on .net framework depending on which SQL client you use. So why don't we get rid of SetDbStatement and make SetDbStatementForStoredProcedure do what it says on .net core and .net framework when using Microsoft.Data.SqlClient and if you are using System.Data.SqlClient on .net framework you need to set SetDbStatementForText to see anything.

[mbakalov]

Should this be something like:

bool captureText = this.options.SetDbStatementForText && eventData.EventSource == this.mdsEventSource;
bool captureSp = this.options.SetDbStatementForStoredProcedure && eventData.EventSource == this.adoNetEventSource;

if (captureText || captureSp) { ... }

[CodeBlanch]

The logic looks good to me, but it could lead to some confusion. If you have SetDbStatementForText=true & SetDbStatementForStoredProcedure=false and eventData.EventSource != this.adoNetEventSource then you could still end up with stored procedures, which you told it you didn't want. Right?

That is probably not the end of the world, but maybe it would be more clear as an enum?

public enum SqlCommandTextCaptureMode
{
   None,
   StoredProcedure,
   StoredProducureAndText
}

🤷

[ghost]

But then you couldn't capture only Text which is currently possibly under .net core, ok you could turn the enum into flags... But personally I would leave it as it is.

[CodeBlanch]

Ya I did that intentionally. My thinking was... having the two isn't particularly useful to anyone. Is there a use case where you would want Text and not Stored Procedures? I doubt it 😄 IMO most people will do Stored Procedures, some will also kick on Text. But Text and NOT Stored Procedures? The enum simplifies it and is trying to avoid letting users configure something that is impossible.

[ghost]

StoredProducureAndText is still impossible on .NET framework with System.Data.SqlClient . In any case this will be a breaking change of the API for .NET core users. So far it's only breaking for framework users. As I said, I think the way it is now is fine, but I will change it to an enum if that will get the PR merged :)

[mbakalov]

@roederja2 - thanks! I think this is more clear than what we had before! Still confusing a bit, but can't get away from that with the difference between the event sources it seems. I also like that we can now capture SP on System.Data.SqlClient by default - that's useful (and safe).

I've added a couple comments - not sure if I misunderstood the behavior you intended for SetDbStatementForText on adoNet?

Would like @cijothomas and @CodeBlanch to take a look as well :)

[ghost]

I think there are some tests that spuriously fail. How do I re-run the failed task?

[mbakalov]

I think there are some tests that spuriously fail. How do I re-run the failed task?

Don't think you can unfortunately 😞 . Can only push new commits. Currently only maintainers can re-trigger CI, could be good to have a bot to let more folks do that though. I'll open an issue/suggestion for that to discuss!

[cijothomas]

I think there are some tests that spuriously fail. How do I re-run the failed task?

Simply Close/Open the PR and it'll retrigger every CI. (you can't do individual CI trigger)

[ghost]

Anything else to be done here? Should I change the interface to use an Enum instead. I can confirm that this change works in my setup now to get the sproc name logged by default - using System.Data.SqlClient with net48.

[ghost]

I can't see the codecov report - get a 403 error

[cijothomas]

LGTM. This does solves the original runtime issue.
The SetDbStatement config part is not ideal for .NET FW, but given the inherent limitation, this seems a good tradeoff.

[cijothomas]

@mbakalov @CodeBlanch Do you have concerns with this or are we okay for merge?

[mbakalov]

@mbakalov @CodeBlanch Do you have concerns with this or are we okay for merge?

Sorry been away from this for some time! Looks good to me as well!

[ghost]

I just noticed that the HTTP instrumentation has a similar problem. I think it is generally not a good idea to use conditional compilation on public API method signatures in .net standard libraries.

[ghost]

Ping

[cijothomas]

I just noticed that the HTTP instrumentation has a similar problem. I think it is generally not a good idea to use conditional compilation on public API method signatures in .net standard libraries.

I have reached out to .NET team for an official guidance on this issue. (We discussed this in last SIG meeting and decided to get an official recommendation from .NET team before making changes). Will share an update here once we hear back. Thanks for your patience!

[ghost]

Thanks for the update. Here is my suggested fix for the Http instrumentation: #2005

[ghost]

Any update on this?

[ghost]

@cijothomas : Ping

[AndyBevan]

Is there any update when we might get this moved forward? We are in the process of migrating a large solution across to .net standard with a view to ultimately switching pieces out to dotnet core. We're struggling to implement SQL telemetry as our DB layer is in dotnet standard however the main web application is still dotnet framework so we run into this exact problem.

Thanks for all the hard work on this - much appreciated.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Jann Röder (309a8d5, 6b1731c, 8276f6a, aff4da3, 4450183, 0d81214, e1cefe8, 12bf4bb, 6d64312, be60aec, 4d8da68, 8de1a7d)
• ✅ Cijo Thomas (0ec6c63, 9abda42)

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.