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.

Sign up for GitHub

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

Jump to bottom

Protect the connection string tags extractor from an invalid connection string (#5956 -> v2) #5960

Merged
merged 1 commit into from
Aug 29, 2024
Merged

Protect the connection string tags extractor from an invalid connection string (#5956 -> v2) #5960

merged 1 commit into from
Aug 29, 2024

Conversation

tonyredondo
Copy link
Member

@tonyredondo tonyredondo commented Aug 28, 2024 •
edited by andrewlock
Loading

Summary of changes

This PR fixes an scenario when the connection string could not be parsed by avoiding the exception.

Reason for change

Seen in Error tracking:

image

Implementation details

Wrap the method in a try catch block, and return default on that invalid connection string.
The caller method already returns default in other cases.

Test coverage

This is an small fix, I didn't write a test case for this... yet.

Other details

Backport of #5956

@tonyredondo @andrewlock
Protect the connection string tags extractor from an invalid connecti…
8961d91 
…on string (#5956)

## Summary of changes

This PR fixes an scenario when the connection string could not be parsed
by avoiding the exception.

## Reason for change

Seen in Error tracking:

<img width="1127" alt="image"
src="https://github.com/user-attachments/assets/436de8fe-b6b8-43d7-a894-5554bb4e04eb">

## Implementation details

Wrap the method in a try catch block, and return default on that invalid
connection string.
The caller method already returns `default` in other cases.

## Test coverage

This is an small fix, I didn't write a test case for this... yet.

## Other details
<!-- Fixes #{issue} -->

<!-- ⚠️ Note: where possible, please obtain 2 approvals prior to
merging. Unless CODEOWNERS specifies otherwise, for external teams it is
typically best to have one review from a team member, and one review
from apm-dotnet. Trivial changes do not require 2 reviews. -->

---------

Co-authored-by: Andrew Lock <[email protected]>
@tonyredondo tonyredondo requested a review from a team as a code owner August 28, 2024 14:33
@tonyredondo tonyredondo self-assigned this Aug 28, 2024
@andrewlock andrewlock changed the title Protect the connection string tags extractor from an invalid connection string (#5956) Protect the connection string tags extractor from an invalid connection string (#5956 -> v2) Aug 28, 2024
@lucaspimentel
Copy link
Member

This is an small fix, I didn't write a test case for this... yet.

ahem

@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Aug 28, 2024 •
edited
Loading

Datadog Report

Branch report: tony/fix-connectionstring-cache-issue-v2
Commit report: 8961d91
Test service: dd-trace-dotnet

✅ 0 Failed, 341392 Passed, 1756 Skipped, 14h 37m 43.74s Total Time
⌛ 1 Performance Regression

⌛ Performance Regressions vs Default Branch (1)

  • Baseline - Samples.FakeDbCommand.windows.netcoreapp31.json.scenarios 114.76ms (+5.36ms, +5%) - Details

@lucaspimentel lucaspimentel added area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) identified-by:telemetry labels Aug 28, 2024
@andrewlock
Copy link
Member

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:

  • Welch test with statistical test for significance of 5%
  • Only results indicating a difference greater than 5% and 5 ms are considered.

Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.

Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5960) - mean (76ms)  : 65, 86
     .   : milestone, 76,

    section CallTarget+Inlining+NGEN
    This PR (5960) - mean (1,031ms)  : 1011, 1051
     .   : milestone, 1031,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5960) - mean (115ms)  : 105, 124
     .   : milestone, 115,

    section CallTarget+Inlining+NGEN
    This PR (5960) - mean (712ms)  : 692, 732
     .   : milestone, 712,
Loading 
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5960) - mean (94ms)  : 90, 99
     .   : milestone, 94,

    section CallTarget+Inlining+NGEN
    This PR (5960) - mean (667ms)  : 645, 690
     .   : milestone, 667,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5960) - mean (191ms)  : 187, 195
     .   : milestone, 191,

    section CallTarget+Inlining+NGEN
    This PR (5960) - mean (1,114ms)  : 1092, 1136
     .   : milestone, 1114,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5960) - mean (276ms)  : 270, 281
     .   : milestone, 276,

    section CallTarget+Inlining+NGEN
    This PR (5960) - mean (880ms)  : 858, 902
     .   : milestone, 880,
Loading 
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (5960) - mean (264ms)  : 260, 268
     .   : milestone, 264,

    section CallTarget+Inlining+NGEN
    This PR (5960) - mean (865ms)  : 844, 885
     .   : milestone, 865,
Loading

@andrewlock
Copy link
Member

Benchmarks Report for tracer 🐌

Benchmarks for #5960 compared to master:

  • All benchmarks have the same speed
  • 1 benchmarks have more allocations

The following thresholds were used for comparing the benchmark speeds:

  • Mann–Whitney U test with statistical test for significance of 5%
  • Only results indicating a difference greater than 10% and 0.3 ns are considered.

Allocation changes below 0.5% are ignored.

Benchmark details

Benchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartStopWithChild net6.0 7.78μs 42.8ns 249ns 0.0153 0.00767 0 5.43 KB
master StartStopWithChild netcoreapp3.1 10.1μs 53.3ns 287ns 0.0143 0.00478 0 5.62 KB
master StartStopWithChild net472 16μs 45.7ns 177ns 1.02 0.308 0.103 6.06 KB
#5960 StartStopWithChild net6.0 7.54μs 39.8ns 199ns 0.0183 0.00733 0 5.43 KB
#5960 StartStopWithChild netcoreapp3.1 9.72μs 54.2ns 343ns 0.0136 0.00454 0 5.62 KB
#5960 StartStopWithChild net472 15.9μs 26ns 97.5ns 1.01 0.308 0.0948 6.06 KB
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 461μs 173ns 670ns 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 627μs 219ns 821ns 0 0 0 2.7 KB
master WriteAndFlushEnrichedTraces net472 843μs 626ns 2.34μs 0.422 0 0 3.3 KB
#5960 WriteAndFlushEnrichedTraces net6.0 486μs 375ns 1.45μs 0 0 0 2.7 KB
#5960 WriteAndFlushEnrichedTraces netcoreapp3.1 654μs 301ns 1.17μs 0 0 0 2.7 KB
#5960 WriteAndFlushEnrichedTraces net472 841μs 400ns 1.55μs 0.419 0 0 3.3 KB
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendRequest net6.0 189μs 1.07μs 7.46μs 0.183 0 0 18.45 KB
master SendRequest netcoreapp3.1 210μs 1.15μs 6.9μs 0.207 0 0 20.61 KB
master SendRequest net472 0.00134ns 0.00058ns 0.00209ns 0 0 0 0 b
#5960 SendRequest net6.0 189μs 1.08μs 8.05μs 0.188 0 0 18.45 KB
#5960 SendRequest netcoreapp3.1 209μs 1.02μs 6.05μs 0.202 0 0 20.61 KB
#5960 SendRequest net472 0.00197ns 0.000721ns 0.00279ns 0 0 0 0 b
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ More allocations ⚠️

More allocations ⚠️ in #5960

Benchmark Base Allocated Diff Allocated Change Change %
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces‑netcoreapp3.1 41.61 KB 41.94 KB 332 B 0.80%

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net6.0 549μs 2μs 7.5μs 0.561 0 0 41.6 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 672μs 3.51μs 16.5μs 0.343 0 0 41.61 KB
master WriteAndFlushEnrichedTraces net472 863μs 2.5μs 9.37μs 8.87 2.66 0.443 53.29 KB
#5960 WriteAndFlushEnrichedTraces net6.0 556μs 1.44μs 5.39μs 0.563 0 0 41.54 KB
#5960 WriteAndFlushEnrichedTraces netcoreapp3.1 693μs 3.33μs 12.9μs 0.327 0 0 41.94 KB
#5960 WriteAndFlushEnrichedTraces net472 888μs 3.41μs 13.2μs 8.54 2.25 0.45 53.29 KB
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteNonQuery net6.0 1.3μs 1.25ns 4.69ns 0.0143 0 0 1.02 KB
master ExecuteNonQuery netcoreapp3.1 1.69μs 1.09ns 4.23ns 0.0134 0 0 1.02 KB
master ExecuteNonQuery net472 2.05μs 1.98ns 7.68ns 0.156 0 0 987 B
#5960 ExecuteNonQuery net6.0 1.29μs 0.854ns 3.2ns 0.0143 0 0 1.02 KB
#5960 ExecuteNonQuery netcoreapp3.1 1.83μs 0.901ns 3.37ns 0.0135 0 0 1.02 KB
#5960 ExecuteNonQuery net472 2.01μs 1.81ns 6.78ns 0.156 0 0 987 B
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master CallElasticsearch net6.0 1.23μs 1.08ns 4.18ns 0.0135 0 0 976 B
master CallElasticsearch netcoreapp3.1 1.59μs 0.549ns 2.12ns 0.0127 0 0 976 B
master CallElasticsearch net472 2.43μs 2.36ns 9.15ns 0.158 0 0 995 B
master CallElasticsearchAsync net6.0 1.32μs 0.505ns 1.95ns 0.0132 0 0 952 B
master CallElasticsearchAsync netcoreapp3.1 1.64μs 1.11ns 4.31ns 0.0137 0 0 1.02 KB
master CallElasticsearchAsync net472 2.59μs 2.15ns 8.33ns 0.166 0 0 1.05 KB
#5960 CallElasticsearch net6.0 1.12μs 1.24ns 4.81ns 0.0134 0 0 976 B
#5960 CallElasticsearch netcoreapp3.1 1.55μs 0.626ns 2.26ns 0.013 0 0 976 B
#5960 CallElasticsearch net472 2.39μs 1.58ns 6.1ns 0.158 0 0 995 B
#5960 CallElasticsearchAsync net6.0 1.3μs 0.933ns 3.61ns 0.0132 0 0 952 B
#5960 CallElasticsearchAsync netcoreapp3.1 1.62μs 0.64ns 2.4ns 0.0139 0 0 1.02 KB
#5960 CallElasticsearchAsync net472 2.54μs 2.05ns 7.93ns 0.167 0.00126 0 1.05 KB
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master ExecuteAsync net6.0 1.19μs 1.25ns 4.68ns 0.0131 0 0 952 B
master ExecuteAsync netcoreapp3.1 1.65μs 0.699ns 2.62ns 0.0124 0 0 952 B
master ExecuteAsync net472 1.8μs 1.49ns 5.76ns 0.145 0 0 915 B
#5960 ExecuteAsync net6.0 1.32μs 0.75ns 2.9ns 0.0132 0 0 952 B
#5960 ExecuteAsync netcoreapp3.1 1.65μs 0.656ns 2.45ns 0.0132 0 0 952 B
#5960 ExecuteAsync net472 1.68μs 1.09ns 4.23ns 0.145 0 0 915 B
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net6.0 4.36μs 22.5ns 103ns 0.0309 0 0 2.22 KB
master SendAsync netcoreapp3.1 5.13μs 1.83ns 7.08ns 0.036 0 0 2.76 KB
master SendAsync net472 7.83μs 1.88ns 7.3ns 0.497 0 0 3.15 KB
#5960 SendAsync net6.0 4.26μs 1.97ns 7.62ns 0.0295 0 0 2.22 KB
#5960 SendAsync netcoreapp3.1 5.17μs 2.92ns 10.9ns 0.036 0 0 2.76 KB
#5960 SendAsync net472 7.78μs 3.9ns 15.1ns 0.497 0 0 3.15 KB
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 1.58μs 0.864ns 3.35ns 0.0236 0 0 1.64 KB
master EnrichedLog netcoreapp3.1 2.37μs 1.05ns 4.06ns 0.0227 0 0 1.64 KB
master EnrichedLog net472 2.72μs 1.61ns 6.23ns 0.25 0 0 1.57 KB
#5960 EnrichedLog net6.0 1.5μs 0.568ns 2.12ns 0.0233 0 0 1.64 KB
#5960 EnrichedLog netcoreapp3.1 2.39μs 0.663ns 2.39ns 0.0215 0 0 1.64 KB
#5960 EnrichedLog net472 2.76μs 3.11ns 12.1ns 0.249 0 0 1.57 KB
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 116μs 176ns 682ns 0 0 0 4.28 KB
master EnrichedLog netcoreapp3.1 119μs 248ns 962ns 0 0 0 4.28 KB
master EnrichedLog net472 146μs 167ns 580ns 0.654 0.218 0 4.46 KB
#5960 EnrichedLog net6.0 116μs 232ns 869ns 0.0574 0 0 4.28 KB
#5960 EnrichedLog netcoreapp3.1 118μs 213ns 796ns 0.0583 0 0 4.28 KB
#5960 EnrichedLog net472 149μs 386ns 1.49μs 0.666 0.222 0 4.46 KB
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 3.04μs 1.23ns 4.76ns 0.0303 0 0 2.2 KB
master EnrichedLog netcoreapp3.1 4.16μs 1.6ns 6.2ns 0.0293 0 0 2.2 KB
master EnrichedLog net472 4.86μs 1.56ns 6.03ns 0.319 0 0 2.02 KB
#5960 EnrichedLog net6.0 3.11μs 1.14ns 4.43ns 0.0312 0 0 2.2 KB
#5960 EnrichedLog netcoreapp3.1 4.2μs 0.925ns 3.34ns 0.0296 0 0 2.2 KB
#5960 EnrichedLog net472 4.83μs 1.34ns 5.19ns 0.319 0 0 2.02 KB
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendReceive net6.0 1.39μs 1.79ns 6.69ns 0.0162 0 0 1.14 KB
master SendReceive netcoreapp3.1 1.72μs 0.612ns 2.29ns 0.0155 0 0 1.14 KB
master SendReceive net472 2.19μs 1.97ns 7.37ns 0.183 0 0 1.16 KB
#5960 SendReceive net6.0 1.39μs 1.23ns 4.76ns 0.016 0 0 1.14 KB
#5960 SendReceive netcoreapp3.1 1.82μs 0.844ns 3.27ns 0.0155 0 0 1.14 KB
#5960 SendReceive net472 2.12μs 2.17ns 8.41ns 0.183 0 0 1.16 KB
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master EnrichedLog net6.0 2.68μs 0.963ns 3.73ns 0.0228 0 0 1.6 KB
master EnrichedLog netcoreapp3.1 3.95μs 12.1ns 43.5ns 0.0216 0 0 1.65 KB
master EnrichedLog net472 4.43μs 3.34ns 12.9ns 0.322 0 0 2.04 KB
#5960 EnrichedLog net6.0 2.78μs 0.931ns 3.48ns 0.0223 0 0 1.6 KB
#5960 EnrichedLog netcoreapp3.1 4.02μs 3.25ns 12.6ns 0.022 0 0 1.65 KB
#5960 EnrichedLog net472 4.26μs 2.75ns 10.7ns 0.323 0 0 2.04 KB
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net6.0 411ns 0.897ns 3.48ns 0.00819 0 0 576 B
master StartFinishSpan netcoreapp3.1 548ns 0.337ns 1.31ns 0.0077 0 0 576 B
master StartFinishSpan net472 661ns 0.663ns 2.57ns 0.0918 0 0 578 B
master StartFinishScope net6.0 475ns 0.232ns 0.899ns 0.00986 0 0 696 B
master StartFinishScope netcoreapp3.1 720ns 0.655ns 2.54ns 0.00937 0 0 696 B
master StartFinishScope net472 846ns 0.367ns 1.37ns 0.105 0 0 658 B
#5960 StartFinishSpan net6.0 397ns 0.243ns 0.942ns 0.00797 0 0 576 B
#5960 StartFinishSpan netcoreapp3.1 572ns 0.571ns 2.21ns 0.00774 0 0 576 B
#5960 StartFinishSpan net472 665ns 0.787ns 3.05ns 0.0918 0 0 578 B
#5960 StartFinishScope net6.0 475ns 0.406ns 1.57ns 0.00983 0 0 696 B
#5960 StartFinishScope netcoreapp3.1 764ns 0.985ns 3.69ns 0.00919 0 0 696 B
#5960 StartFinishScope net472 812ns 0.708ns 2.74ns 0.104 0 0 658 B
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master RunOnMethodBegin net6.0 692ns 0.436ns 1.69ns 0.00972 0 0 696 B
master RunOnMethodBegin netcoreapp3.1 936ns 0.401ns 1.55ns 0.00938 0 0 696 B
master RunOnMethodBegin net472 1.11μs 0.79ns 3.06ns 0.104 0 0 658 B
#5960 RunOnMethodBegin net6.0 679ns 0.751ns 2.91ns 0.00953 0 0 696 B
#5960 RunOnMethodBegin netcoreapp3.1 931ns 0.873ns 3.38ns 0.00927 0 0 696 B
#5960 RunOnMethodBegin net472 1.11μs 1.54ns 5.74ns 0.104 0 0 658 B

@tonyredondo tonyredondo merged commit 58ba195 into release/2.x Aug 29, 2024
62 of 65 checks passed
@tonyredondo tonyredondo deleted the tony/fix-connectionstring-cache-issue-v2 branch August 29, 2024 08:59
@github-actions github-actions bot added this to the vNext-v2 milestone Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucaspimentel lucaspimentel lucaspimentel approved these changes

Assignees

@tonyredondo tonyredondo

Labels
area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) identified-by:telemetry
Projects
None yet
Milestone
2.58.0
Development

Successfully merging this pull request may close these issues.
3 participants
@tonyredondo @lucaspimentel @andrewlock