[SqlClient] Add support for Filter expression #3743

Driedas · 2022-10-07T17:38:28Z

Changes

Adds support for using a Filter expression for SqlClient Intrumentation to optionally filter out telemetry based on the current SqlCommand. Heavily inspired by the AspNetCore Instrumentation filter implementation. Updated the readme.md file with a short description and a sample.

@cijothomas Initially intended as completion of PR 3342 but started from scratch instead.

Appropriate CHANGELOG.md updated for non-trivial changes
Design discussion issue #
Changes in public API reviewed

linux-foundation-easycla · 2022-10-07T17:38:31Z

The committers listed above are authorized under a signed CLA.

✅ login: Driedas / name: Pavel Steinl (87fc4b6)

codecov · 2022-10-07T19:12:20Z

Codecov Report

Merging #3743 (48e069e) into main (1a2103d) will increase coverage by 0.16%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3743      +/-   ##
==========================================
+ Coverage   87.18%   87.34%   +0.16%     
==========================================
  Files         277      277              
  Lines       10209    10228      +19     
==========================================
+ Hits         8901     8934      +33     
+ Misses       1308     1294      -14

Impacted Files	Coverage Δ
...ient/Implementation/SqlClientDiagnosticListener.cs	`85.00% <100.00%> (+2.39%)`	⬆️
...lementation/SqlClientInstrumentationEventSource.cs	`75.00% <100.00%> (+10.29%)`	⬆️
...ation.SqlClient/SqlClientInstrumentationOptions.cs	`98.66% <100.00%> (+0.01%)`	⬆️
src/OpenTelemetry/ProviderExtensions.cs	`81.81% <0.00%> (-9.10%)`	⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	`73.52% <0.00%> (-5.89%)`	⬇️
...Telemetry/Metrics/PeriodicExportingMetricReader.cs	`72.72% <0.00%> (-5.46%)`	⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs	`97.65% <0.00%> (+0.78%)`	⬆️
src/OpenTelemetry/BatchExportProcessor.cs	`84.11% <0.00%> (+1.86%)`	⬆️
...metryProtocol/Implementation/ActivityExtensions.cs	`95.13% <0.00%> (+3.24%)`	⬆️
...ZPages/Implementation/ZPagesExporterEventSource.cs	`62.50% <0.00%> (+6.25%)`	⬆️
... and 3 more

...penTelemetry.Instrumentation.SqlClient/Implementation/SqlClientInstrumentationEventSource.cs

src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md

src/OpenTelemetry.Instrumentation.SqlClient/SqlClientInstrumentationOptions.cs

src/OpenTelemetry.Instrumentation.SqlClient/.publicApi/net462/PublicAPI.Unshipped.txt

vishweshbankwar · 2022-10-11T15:56:22Z

@cijothomas @utpilla - Could you please approve running the workflow?

src/OpenTelemetry.Instrumentation.SqlClient/README.md

cijothomas · 2022-10-12T20:24:08Z

@Driedas Can you resolve the conflict?

cijothomas

Thanks for your contribution! Changes looks good to me.

Driedas · 2022-10-12T20:37:25Z

@Driedas Can you resolve the conflict?

Github doesn't let me do it via its UI (even though it says it's been resolved), I can rebase the whole PR to HEAD main though...

Also, not sure what to do with those 2 failing checks, neither of those seem related to the work I've done and all tests pass locally on my machine 🤷‍♂️

src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md

src/OpenTelemetry.Instrumentation.SqlClient/README.md

vishweshbankwar · 2022-10-13T17:04:44Z

@Driedas Can you resolve the conflict?

Github doesn't let me do it via its UI (even though it says it's been resolved), I can rebase the whole PR to HEAD main though...

Also, not sure what to do with those 2 failing checks, neither of those seem related to the work I've done and all tests pass locally on my machine 🤷‍♂️

@Driedas - The failing check is from linux environment.
Failed OpenTelemetry.Instrumentation.SqlClient.Tests.SqlClientTests.ShouldNotCollectTelemetryWhenFilterEvaluatesToFalse [91 ms]
Error Message:
System.PlatformNotSupportedException : LocalDB is not supported on this platform.
Stack Trace:
at Microsoft.Data.SqlClient.SNI.LocalDB.GetLocalDBConnectionString(String localDbInstance)
at Microsoft.Data.SqlClient.SNI.SNIProxy.GetLocalDBDataSource(String fullServerName, Boolean& error)
at Microsoft.Data.SqlClient.SNI.SNIProxy.CreateConnectionHandle(Object callbackObject, String fullServerName, Boolean ignoreSniOpenTimeout, Int64

For the merge conflicts - you could resolve it locally and push changes.

Driedas · 2022-10-13T17:56:09Z

@vishweshbankwar ok, will resolve locally, but what do we do about that failing test? Should I replace the connection string with SqlConnectionString that the SuccessfulCommandTest is using? Is that a database that is available in the CI environment?

vishweshbankwar · 2022-10-13T19:19:48Z

@vishweshbankwar ok, will resolve locally, but what do we do about that failing test? Should I replace the connection string with SqlConnectionString that the SuccessfulCommandTest is using? Is that a database that is available in the CI environment?

@Driedas I think you can copy this test into a new one to include filter.

Driedas · 2022-10-13T20:56:06Z

@vishweshbankwar Done, replaced the implementation of the test helper method, should now hopefully run on Linux as well

vishweshbankwar

LGTM

Driedas · 2022-10-13T21:11:18Z

@vishweshbankwar also, squashed it all into a single commit and updated to latest main to fix the README.md conflict

Driedas · 2022-10-14T09:09:58Z

@vishweshbankwar I've moved the private helper further down in the class, as per the SA1204 error, and rebased to latest main...

...penTelemetry.Instrumentation.SqlClient/Implementation/SqlClientInstrumentationEventSource.cs

cijothomas · 2022-10-14T21:35:38Z

src/OpenTelemetry.Instrumentation.SqlClient/README.md

+## Filter
+
+This option allows to filter out activities based on the properties of the
+`SqlCommand` object being instrumented using a `Func<object, bool>`.


https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Instrumentation.AspNetCore#filter the wording here is slightly better. Not blocking. There are plans to change the return type from bool to more easy-to-use Enums like DropActivity, ReportActivity etc, soon.

cijothomas · 2022-10-17T16:42:32Z

Hold off from merging, until @utpilla completes a release today. This PR itself is good, but we might want to make some breaking changes to the Filter signature across all libraries. If that happens, want to do it in one shot.

* [Logs] Fix buffered log scopes being reused (#3731) * Fix buffered log scopes being reused. * CHANGELOG update. * Test fixes. Co-authored-by: Cijo Thomas <cithomas@microsoft.com> * clarify that Prometheus HttpListner is not for production at this moment (#3737) * [HttpClient] Export spans corresponding to retries (#3732) * Minor update to OTLP readme (#3739) * Nit fixes to prometheus readme * Add minor clarification about OTLP logs * Update workflow (#3741) * Minor improvement to log message (#3742) * [SDK + Jaeger] Support loading environment variables from IConfiguration in Traces & Metrics (#3720) * Support retrieval of environment variables through IConfiguration in SDK. * Update Jaeger to load environment variables through IConfiguration. * Warning fix. * CHANGELOG patch. * Bug fixes. * Warning cleanup. * Code review. * [Zipkin] Support loading envvars from IConfiguration (#3759) * Support loading envvars from IConfiguration in Zipkin exporter. * CHANGELOG patch. * SqlClient Instrumentation to leverage native Activity Status. (#3751) * [Metrics] Update default buckets for Explicit Bucket Histogram from spec (#3722) * [Logs] Fix: Respect AttributeValueLengthLimit when building otlp LogRecord data (#3684) * Respect SdkConfiguration.AttributeValueLengthLimit so otlp data points are not rejected by monitoring tools * Respect maxAttributeCount and use OtlpKeyValueTransformer in AddStringAttribute and in AddIntAttribute * Extend CHANGELOG.md Co-authored-by: Mikel Blanchard <mblanchard@macrosssoftware.com> * Bump actions/setup-dotnet from 3.0.1 to 3.0.2 (#3764) Bumps [actions/setup-dotnet](https://github.com/actions/setup-dotnet) from 3.0.1 to 3.0.2. - [Release notes](https://github.com/actions/setup-dotnet/releases) - [Commits](actions/setup-dotnet@v3.0.1...v3.0.2) --- updated-dependencies: - dependency-name: actions/setup-dotnet dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * [Repo] Attempting to stabilize the API Compatibility CI job (#3766) * Attempting to stablize the API Compatibility CI. * Tweak. * More logging. * Increase build logging level for api compat ci job. * Tweak. * Tweak. * Revert extra logging in ci job. * Switched some logging back to debug. * Adding MinMax to Histograms (#2735) * Logging state during building of TracerProvider (#3746) * [HttpClient] Add unit tests for `RecordException` case (#3761) * Add a separate example project for Logs redaction (#3744) * Update CHANGELOG for 1.4.0-beta.2 release (#3772) * [SDK + Otlp] Support loading envvars from IConfiguration (#3760) * Updated Otlp Trace & Metrics exporters to load envvars from IConfiguration. * Patch CHANGELOGs. * Fix up otlp log exporter for SdkOptions changes. * Revert SdkOptions public api. * Restore tests. * Fix benchmarks. * SdkLimitOptions IConfiguration test. * OtlpExporterOptions IConfiguration test. * MetricReaderOptions IConfiguration test. * Bug fix. * Nit. * [SqlClient] Add support for Filter expression (#3743) * [SDK, Jaeger, Zipkin, & Otlp] Support loading envvars for BatchExportActivityProcessorOptions from IConfiguration (#3776) * Support loading envvars for ExportActivityProcessorOptions & BatchExportActivityProcessorOptions from IConfiguration. * Update src/OpenTelemetry/CHANGELOG.md Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com> * Patch CHANGELOG. * Unit test. Co-authored-by: Cijo Thomas <cithomas@microsoft.com> Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com> * Remove Env from CI `DOTNET_MULTILEVEL_LOOKUP = 1` (#3779) * Link to .NET docs about different ports (#3704) * Update DS to rc2 (#3781) * ConsoleLogExporter to output full exception (#3784) * [ASP.NET Core] Add back netstandard2.0 and 2.1 targets (#3755) * [HttpClient] Add back netstandard2.0 target (#3787) * Add back netstandard2.0 target * changelog * public api files * Bump System.Text.Json version due to CVE-2021-26701 (#3789) * Auto-generated Semantic Conventions (#2069) * [SDK] Support dependency injection in ResourceBuilder and load envvars from IConfiguration (#3782) * Add Vishwesh as an Approver (#3783) Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com> * Move more SDK docs to docs folder (#3794) * [Prometheus AspNetCore] Support named options in pipeline extensions (#3780) * Support named options in Prometheus AspNetCore pipeline extensions. * Patch CHANGELOG. Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com> * [Logs] UnitTest: LogRecord attribute limits (#3758) * Unittest for LogRecord attribute limits * Remove maxValueLength from LogRecordExtensions.AddIntAttribute. Change requested from #3684 (comment) * Pr commits addressed Co-authored-by: Mikel Blanchard <mblanchard@macrosssoftware.com> * Add MinMax to console and doc additions (#3795) * Mark private and internal classes as sealed (#3799) Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com> * Move more docs to docs folder (#3801) * [ASP.NET Core] Update enrich callbacks to use specific type. (#3749) * [Http] Update enrich callbacks for http (#3792) * [SDK] Support dependency injection in the GetDefaultResource API (#3798) * Support dependency injection in the GetDefaultResource API. * CHANGELOG patch. * Test tweaks. Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com> * Fix circular reference issue building up tracer provider. (#3803) * [SDK] Add some missing nullable annotations (#3796) * Added some missing nullable annotations in SDK. * Handle null span names in SamplingParameters. * Warning cleanup. * Warning cleanup. * Added link to issue in comment. Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com> * Guard.Range now uses invariant culture for error message (#3778) Co-authored-by: Utkarsh Umesan Pillai <utpilla@microsoft.com> * Fix circular reference issue building up meter provider. (#3806) Co-authored-by: Utkarsh Umesan Pillai <utpilla@microsoft.com> * Add missing end of code block backticks (#3807) * More doc tweaks (#3805) * More doc tweaks * remove draft staatus * Update grpc client enrich callbacks (#3804) * Port refactor from main-logs branch. (#3808) Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com> * Merge fixes. * Merge fixes. * Merge fix. Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Cijo Thomas <cithomas@microsoft.com> Co-authored-by: Reiley Yang <reyang@microsoft.com> Co-authored-by: Vishwesh Bankwar <vishweshbankwar@users.noreply.github.com> Co-authored-by: Cijo Thomas <cijo.thomas@gmail.com> Co-authored-by: Yun-Ting Lin <yunl@microsoft.com> Co-authored-by: Sebastian Schoder Moreno <35150382+schoder-moreno@users.noreply.github.com> Co-authored-by: Jonathan Wilhelm <Jonathan.wilhelm@sage.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Michael Maxwell <micmax@microsoft.com> Co-authored-by: ggoel <gaurav.goel111@gmail.com> Co-authored-by: Utkarsh Umesan Pillai <utpilla@microsoft.com> Co-authored-by: Pavel Steinl <pavel.steinl@gmail.com> Co-authored-by: Piotr Kiełkowicz <pkiekowicz@splunk.com> Co-authored-by: aristotelos <arisvd@gmail.com> Co-authored-by: Joao Grassi <joao@joaograssi.com> Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com> Co-authored-by: benhall_io <3179852+benbhall@users.noreply.github.com>

Driedas requested a review from a team October 7, 2022 17:38

vishweshbankwar reviewed Oct 10, 2022

View reviewed changes

...penTelemetry.Instrumentation.SqlClient/Implementation/SqlClientInstrumentationEventSource.cs Show resolved Hide resolved

vishweshbankwar reviewed Oct 11, 2022

View reviewed changes

src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md Outdated Show resolved Hide resolved

vishweshbankwar reviewed Oct 11, 2022

View reviewed changes

src/OpenTelemetry.Instrumentation.SqlClient/SqlClientInstrumentationOptions.cs Outdated Show resolved Hide resolved

vishweshbankwar reviewed Oct 11, 2022

View reviewed changes

src/OpenTelemetry.Instrumentation.SqlClient/.publicApi/net462/PublicAPI.Unshipped.txt Outdated Show resolved Hide resolved

cijothomas reviewed Oct 12, 2022

View reviewed changes

src/OpenTelemetry.Instrumentation.SqlClient/README.md Outdated Show resolved Hide resolved

cijothomas approved these changes Oct 12, 2022

View reviewed changes

utpilla reviewed Oct 13, 2022

View reviewed changes

src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md Outdated Show resolved Hide resolved

utpilla reviewed Oct 13, 2022

View reviewed changes

src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md Outdated Show resolved Hide resolved

utpilla reviewed Oct 13, 2022

View reviewed changes

src/OpenTelemetry.Instrumentation.SqlClient/README.md Show resolved Hide resolved

vishweshbankwar approved these changes Oct 13, 2022

View reviewed changes

cijothomas reviewed Oct 14, 2022

View reviewed changes

...penTelemetry.Instrumentation.SqlClient/Implementation/SqlClientInstrumentationEventSource.cs Outdated Show resolved Hide resolved

[SqlClient] Add support for Filter expression

d674cb7

cijothomas reviewed Oct 14, 2022

View reviewed changes

cijothomas approved these changes Oct 14, 2022

View reviewed changes

Merge branch 'main' into main

51dbcf0

cijothomas and others added 2 commits October 17, 2022 18:23

Merge branch 'main' into main

0de9d40

Merge branch 'main' into main

48e069e

cijothomas merged commit 3f53be6 into open-telemetry:main Oct 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SqlClient] Add support for Filter expression #3743

[SqlClient] Add support for Filter expression #3743

Driedas commented Oct 7, 2022

linux-foundation-easycla bot commented Oct 7, 2022 •

edited

Loading

codecov bot commented Oct 7, 2022 •

edited

Loading

vishweshbankwar commented Oct 11, 2022

cijothomas commented Oct 12, 2022

cijothomas left a comment

Driedas commented Oct 12, 2022 •

edited

Loading

vishweshbankwar commented Oct 13, 2022

Driedas commented Oct 13, 2022

vishweshbankwar commented Oct 13, 2022

Driedas commented Oct 13, 2022

vishweshbankwar left a comment

Driedas commented Oct 13, 2022

Driedas commented Oct 14, 2022

cijothomas Oct 14, 2022

cijothomas commented Oct 17, 2022

[SqlClient] Add support for Filter expression #3743

[SqlClient] Add support for Filter expression #3743

Conversation

Driedas commented Oct 7, 2022

Changes

linux-foundation-easycla bot commented Oct 7, 2022 • edited Loading

codecov bot commented Oct 7, 2022 • edited Loading

Codecov Report

vishweshbankwar commented Oct 11, 2022

cijothomas commented Oct 12, 2022

cijothomas left a comment

Choose a reason for hiding this comment

Driedas commented Oct 12, 2022 • edited Loading

vishweshbankwar commented Oct 13, 2022

Driedas commented Oct 13, 2022

vishweshbankwar commented Oct 13, 2022

Driedas commented Oct 13, 2022

vishweshbankwar left a comment

Choose a reason for hiding this comment

Driedas commented Oct 13, 2022

Driedas commented Oct 14, 2022

cijothomas Oct 14, 2022

Choose a reason for hiding this comment

cijothomas commented Oct 17, 2022

linux-foundation-easycla bot commented Oct 7, 2022 •

edited

Loading

codecov bot commented Oct 7, 2022 •

edited

Loading

Driedas commented Oct 12, 2022 •

edited

Loading