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

Add null reference checks to elasticsearch7 integration #4918

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

andrewlock
Copy link
Member

Summary of changes

Fix the null checks in the elasticsearch7 integration

Reason for change

We know we're getting some NullReferenceException in the elasticsearch7 integration

Implementation details

Sprinkle some #nullable enable and watch the errors shout

Test coverage

Covered by existing tests

Other details

@andrewlock andrewlock added type:bug area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) area:integrations labels Nov 22, 2023
@andrewlock andrewlock requested a review from a team as a code owner November 22, 2023 18:03
@datadog-ddstaging
Copy link

datadog-ddstaging bot commented Nov 22, 2023

Datadog Report

Branch report: andrew/fix-elasticsearch7-nullreference
Commit report: 2420319

dd-trace-dotnet: 0 Failed, 0 New Flaky, 303780 Passed, 939 Skipped, 44m 22.84s Wall Time

@andrewlock andrewlock force-pushed the andrew/fix-elasticsearch7-nullreference branch from 3ab075c to 2420319 Compare November 23, 2023 10:25
@andrewlock
Copy link
Member Author

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 (4918) - mean (72ms)  : 63, 81
     .   : milestone, 72,
    master - mean (70ms)  : 62, 78
     .   : milestone, 70,

    section CallTarget+Inlining+NGEN
    This PR (4918) - mean (991ms)  : 971, 1012
     .   : milestone, 991,
    master - mean (998ms)  : 982, 1013
     .   : milestone, 998,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (4918) - mean (105ms)  : 102, 109
     .   : milestone, 105,
    master - mean (106ms)  : 103, 109
     .   : milestone, 106,

    section CallTarget+Inlining+NGEN
    This PR (4918) - mean (691ms)  : 665, 716
     .   : milestone, 691,
    master - mean (686ms)  : 667, 705
     .   : milestone, 686,

Loading
gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (4918) - mean (90ms)  : 84, 96
     .   : milestone, 90,
    master - mean (90ms)  : 87, 92
     .   : milestone, 90,

    section CallTarget+Inlining+NGEN
    This PR (4918) - mean (658ms)  : 632, 684
     .   : milestone, 658,
    master - mean (661ms)  : 634, 687
     .   : milestone, 661,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (4918) - mean (189ms)  : 186, 192
     .   : milestone, 189,
    master - mean (188ms)  : 186, 191
     .   : milestone, 188,

    section CallTarget+Inlining+NGEN
    This PR (4918) - mean (1,136ms)  : 1114, 1158
     .   : milestone, 1136,
    master - mean (1,136ms)  : 1115, 1157
     .   : milestone, 1136,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (4918) - mean (273ms)  : 269, 277
     .   : milestone, 273,
    master - mean (272ms)  : 268, 276
     .   : milestone, 272,

    section CallTarget+Inlining+NGEN
    This PR (4918) - mean (1,096ms)  : 1061, 1132
     .   : milestone, 1096,
    master - mean (1,092ms)  : 1066, 1118
     .   : milestone, 1092,

Loading
gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (4918) - mean (261ms)  : 258, 265
     .   : milestone, 261,
    master - mean (261ms)  : 257, 265
     .   : milestone, 261,

    section CallTarget+Inlining+NGEN
    This PR (4918) - mean (1,060ms)  : 1030, 1089
     .   : milestone, 1060,
    master - mean (1,059ms)  : 1036, 1081
     .   : milestone, 1059,

Loading

@andrewlock
Copy link
Member Author

Throughput/Crank Report:zap:

Throughput results for AspNetCoreSimpleController comparing the following branches/commits:

Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red.

Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards!

gantt
    title Throughput Linux x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (4918) (11.583M)   : 0, 11583011
    master (11.936M)   : 0, 11935977
    benchmarks/2.9.0 (11.683M)   : 0, 11682730

    section Automatic
    This PR (4918) (8.058M)   : 0, 8057722
    master (8.057M)   : 0, 8056732
    benchmarks/2.9.0 (8.466M)   : 0, 8466123

    section Trace stats
    This PR (4918) (8.276M)   : 0, 8276367
    master (8.454M)   : 0, 8453900

    section Manual
    This PR (4918) (10.379M)   : 0, 10379117
    master (10.418M)   : 0, 10418295

    section Manual + Automatic
    This PR (4918) (7.937M)   : 0, 7936665
    master (7.686M)   : 0, 7685677

    section Version Conflict
    This PR (4918) (6.846M)   : 0, 6846473
    master (6.904M)   : 0, 6903820

Loading
gantt
    title Throughput Linux arm64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (4918) (9.328M)   : 0, 9328107
    master (9.525M)   : 0, 9524598
    benchmarks/2.9.0 (9.501M)   : 0, 9501348

    section Automatic
    This PR (4918) (6.305M)   : 0, 6305217
    master (6.555M)   : 0, 6554923

    section Trace stats
    This PR (4918) (6.831M)   : 0, 6830788
    master (6.953M)   : 0, 6952541

    section Manual
    This PR (4918) (8.359M)   : 0, 8358990
    master (8.392M)   : 0, 8391646

    section Manual + Automatic
    This PR (4918) (6.209M)   : 0, 6209111
    master (6.195M)   : 0, 6195294

    section Version Conflict
    This PR (4918) (5.560M)   : 0, 5560088
    master (5.699M)   : 0, 5699220

Loading
gantt
    title Throughput Windows x64 (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    This PR (4918) (10.718M)   : 0, 10718222
    master (10.167M)   : 0, 10166663
    benchmarks/2.9.0 (10.646M)   : 0, 10646425

    section Automatic
    This PR (4918) (7.349M)   : 0, 7349136
    master (7.303M)   : 0, 7303491
    benchmarks/2.9.0 (7.738M)   : 0, 7737732

    section Trace stats
    This PR (4918) (7.807M)   : 0, 7807395
    master (7.689M)   : 0, 7688613

    section Manual
    This PR (4918) (9.268M)   : 0, 9268233
    master (9.014M)   : 0, 9014102

    section Manual + Automatic
    This PR (4918) (7.226M)   : 0, 7226268
    master (7.010M)   : 0, 7010349

    section Version Conflict
    This PR (4918) (6.425M)   : 0, 6425189
    master (6.207M)   : 0, 6206537

Loading
gantt
    title Throughput Linux x64 (ASM) (Total requests) 
    dateFormat  X
    axisFormat %s
    section Baseline
    master (7.369M)   : 0, 7368949
    benchmarks/2.9.0 (7.766M)   : 0, 7765742

    section No attack
    master (1.955M)   : 0, 1954927
    benchmarks/2.9.0 (3.225M)   : 0, 3225290

    section Attack
    master (1.570M)   : 0, 1570391
    benchmarks/2.9.0 (2.447M)   : 0, 2447478

    section Blocking
    master (3.259M)   : 0, 3258698

    section IAST default
    master (6.413M)   : 0, 6413148

    section IAST full
    master (5.753M)   : 0, 5752925

    section Base vuln
    master (0.951M)   : 0, 951131

    section IAST vuln
    master (0.859M)   : 0, 858699

Loading

@andrewlock andrewlock merged commit 6fce37c into master Nov 23, 2023
54 of 56 checks passed
@andrewlock andrewlock deleted the andrew/fix-elasticsearch7-nullreference branch November 23, 2023 13:55
@github-actions github-actions bot added this to the vNext milestone Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:integrations area:tracer The core tracer library (Datadog.Trace, does not include OpenTracing, native code, or integrations) identified-by:telemetry type:bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants