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

[Sql Instrumentation] Unify exposed public API #3900

Merged

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Nov 14, 2022

Relates to #3793

Changes

  • Exposes the same API for all targets to fix package resolution issues when consuming Sql instrumentation into netstandard2.0 projects.

Public API Changes

namespace OpenTelemetry.Instrumentation.SqlClient
{
    public class SqlClientInstrumentationOptions
    {
-#if NETFRAMEWORK
-       public bool SetDbStatement { get; set; }
-#else
        public bool SetDbStatementForStoredProcedure { get; set; } = true;
        public bool SetDbStatementForText { get; set; }
        public Func<object, bool> Filter { get; set; }
        public bool RecordException { get; set; }
-#endif
        public Action<Activity, string, object> Enrich { get; set; }
        public bool EnableConnectionLevelAttributes { get; set; }
    }
}

TODOs

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

@CodeBlanch CodeBlanch requested a review from a team November 14, 2022 18:31
@codecov
Copy link

codecov bot commented Nov 14, 2022

Codecov Report

Merging #3900 (d01059f) into main (3919a2a) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3900      +/-   ##
==========================================
- Coverage   87.38%   87.31%   -0.08%     
==========================================
  Files         279      279              
  Lines       10782    10781       -1     
==========================================
- Hits         9422     9413       -9     
- Misses       1360     1368       +8     
Impacted Files Coverage Δ
...mentation.Http/HttpClientInstrumentationOptions.cs 100.00% <ø> (ø)
...ation.SqlClient/SqlClientInstrumentationOptions.cs 98.66% <ø> (-0.02%) ⬇️
...ent/Implementation/SqlEventSourceListener.netfx.cs 87.09% <100.00%> (ø)
...tation/OpenTelemetryProtocolExporterEventSource.cs 85.00% <0.00%> (-15.00%) ⬇️
...porter.OpenTelemetryProtocol/OtlpMetricExporter.cs 72.72% <0.00%> (-13.64%) ⬇️
src/OpenTelemetry/BatchExportProcessor.cs 82.24% <0.00%> (-1.87%) ⬇️

Copy link
Member

@vishweshbankwar vishweshbankwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@CodeBlanch CodeBlanch merged commit f770047 into open-telemetry:main Nov 14, 2022
@CodeBlanch CodeBlanch deleted the sql-instrumentation-publicapi branch November 14, 2022 22:33
@vishweshbankwar
Copy link
Member

@CodeBlanch - Don't we need to keep this information on Readme?

On .NET Framework, unlike .NET Core, the instrumentation capabilities for both [Microsoft.Data.SqlClient](https://www.nuget.org/packages/Microsoft.Data.SqlClient/) and System.Data.SqlClient` are limited:

  • Microsoft.Data.SqlClient
    always exposes both the stored procedure name and the full query text but
    doesn't allow for more granular control to turn either on/off depending on
    CommandType.
  • System.Data.SqlClient only exposes stored procedure names and not the full
    query text.`

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