Add full null checking to MongoDB and AWS.SDK integrations #4815
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewlock
commented
Nov 3, 2023
There was a problem hiding this comment.
This proxy is apparently unused
andrewlock
commented
Nov 3, 2023
| namespace Datadog.Trace.ClrProfiler.AutoInstrumentation.MongoDb | ||
| { | ||
| /// <summary> | ||
| /// MongoDB.Bson.BsonDocument interface for duck-typing | ||
| /// </summary> | ||
| internal class BsonDocumentProxy | ||
| internal interface IBsonDocumentProxy : IDuckType |
There was a problem hiding this comment.
As per recommendations - favor interface proxies over classes
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. 19 occurrences of : + _dd.git.commit.sha: aaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbb,
+ _dd.git.repository_url: https://github.com/DataDog/dd-trace-dotnet,
27 occurrences of : + _dd.git.commit.sha: aaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbb,
+ _dd.git.repository_url: https://github.com/DataDog/dd-trace-dotnet
|
andrewlock
commented
Nov 3, 2023
| { | ||
| return default; | ||
| } | ||
| public BsonElementStruct GetElement(int index); |
There was a problem hiding this comment.
The BsonElement this is ducktyping is a struct, so the return type here must be non-nullable. Currently you get a weird runtime exception if you try to use this:
//👇 Can't use this
public BsonElementStruct? GetElement(int index);as it gives this:
Unable to access IWireProtocol.Command properties. Datadog.Trace.DuckTyping.DuckTypeDuckCopyStructDoesNotContainsAnyField: The [DuckCopy] struct 'System.Nullable`1[[Datadog.Trace.ClrProfiler.AutoInstrumentation.MongoDb.BsonElementStruct, Datadog.Trace, Version=2.41.0.0, Culture=neutral, PublicKeyToken=def86d061d0d2eeb]]' does not contains any public field. Remember that DuckCopy proxies must be declared using fields instead of properties.
It's incorrect to use BsonElementStruct? in this case, but the exception we throw is "wrong". I'll follow up separately so that we check for the struct/Nullable<struct> mismatch and throw an appropriate exception
andrewlock
changed the title
Datadog ReportBranch report: ✅ |
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:
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 (4815) - mean (70ms) : 64, 75
. : milestone, 70,
master - mean (71ms) : 62, 80
. : milestone, 71,
section CallTarget+Inlining+NGEN
This PR (4815) - mean (1,000ms) : 976, 1024
. : milestone, 1000,
master - mean (1,004ms) : 981, 1027
. : milestone, 1004,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4815) - mean (106ms) : 103, 110
. : milestone, 106,
master - mean (106ms) : 103, 109
. : milestone, 106,
section CallTarget+Inlining+NGEN
This PR (4815) - mean (689ms) : 672, 707
. : milestone, 689,
master - mean (687ms) : 671, 702
. : milestone, 687,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4815) - mean (90ms) : 89, 92
. : milestone, 90,
master - mean (91ms) : 87, 94
. : milestone, 91,
section CallTarget+Inlining+NGEN
This PR (4815) - mean (655ms) : 631, 678
. : milestone, 655,
master - mean (655ms) : 635, 674
. : milestone, 655,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4815) - mean (187ms) : 184, 190
. : milestone, 187,
master - mean (187ms) : 185, 189
. : milestone, 187,
section CallTarget+Inlining+NGEN
This PR (4815) - mean (1,133ms) : 1110, 1156
. : milestone, 1133,
master - mean (1,129ms) : 1110, 1148
. : milestone, 1129,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4815) - mean (270ms) : 266, 275
. : milestone, 270,
master - mean (271ms) : 266, 275
. : milestone, 271,
section CallTarget+Inlining+NGEN
This PR (4815) - mean (1,098ms) : 1076, 1120
. : milestone, 1098,
master - mean (1,093ms) : 1068, 1117
. : milestone, 1093,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (4815) - mean (261ms) : 258, 265
. : milestone, 261,
master - mean (261ms) : 256, 265
. : milestone, 261,
section CallTarget+Inlining+NGEN
This PR (4815) - mean (1,062ms) : 1028, 1095
. : milestone, 1062,
master - mean (1,065ms) : 1037, 1093
. : milestone, 1065,
|
duncanista
approved these changes
Nov 3, 2023
Benchmarks Report 🐌Benchmarks for #4815 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Asm.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Asm.AppSecWafBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Faster 🎉 Same allocations ✔️
|
| Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync‑net6.0 | 1.171 | 1,334.94 | 1,140.17 | |
| Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearch‑netcoreapp3.1 | 1.120 | 1,449.44 | 1,293.96 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | CallElasticsearch |
net6.0 | 1.17μs | 0.542ns | 2.03ns | 0.0133 | 0 | 0 | 936 B |
| master | CallElasticsearch |
netcoreapp3.1 | 1.45μs | 0.696ns | 2.41ns | 0.0125 | 0 | 0 | 936 B |
| master | CallElasticsearch |
net472 | 2.36μs | 0.476ns | 1.84ns | 0.152 | 0 | 0 | 955 B |
| master | CallElasticsearchAsync |
net6.0 | 1.34μs | 0.453ns | 1.69ns | 0.0127 | 0 | 0 | 912 B |
| master | CallElasticsearchAsync |
netcoreapp3.1 | 1.42μs | 1.81ns | 6.79ns | 0.0127 | 0 | 0 | 984 B |
| master | CallElasticsearchAsync |
net472 | 2.45μs | 2.57ns | 9.94ns | 0.16 | 0 | 0 | 1.01 KB |
| #4815 | CallElasticsearch |
net6.0 | 1.24μs | 0.705ns | 2.64ns | 0.013 | 0 | 0 | 936 B |
| #4815 | CallElasticsearch |
netcoreapp3.1 | 1.29μs | 0.629ns | 2.43ns | 0.0124 | 0 | 0 | 936 B |
| #4815 | CallElasticsearch |
net472 | 2.41μs | 0.447ns | 1.73ns | 0.151 | 0 | 0 | 955 B |
| #4815 | CallElasticsearchAsync |
net6.0 | 1.14μs | 0.31ns | 1.2ns | 0.0126 | 0 | 0 | 912 B |
| #4815 | CallElasticsearchAsync |
netcoreapp3.1 | 1.49μs | 0.854ns | 3.19ns | 0.0134 | 0 | 0 | 984 B |
| #4815 | CallElasticsearchAsync |
net472 | 2.43μs | 0.876ns | 3.39ns | 0.16 | 0 | 0 | 1.01 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.2μs | 0.736ns | 2.85ns | 0.0126 | 0 | 0 | 912 B |
| master | ExecuteAsync |
netcoreapp3.1 | 1.55μs | 0.45ns | 1.68ns | 0.0124 | 0 | 0 | 912 B |
| master | ExecuteAsync |
net472 | 1.72μs | 1.43ns | 5.53ns | 0.139 | 0 | 0 | 875 B |
| #4815 | ExecuteAsync |
net6.0 | 1.19μs | 1.96ns | 7.58ns | 0.0126 | 0 | 0 | 912 B |
| #4815 | ExecuteAsync |
netcoreapp3.1 | 1.58μs | 0.452ns | 1.69ns | 0.0118 | 0 | 0 | 912 B |
| #4815 | ExecuteAsync |
net472 | 1.64μs | 0.897ns | 3.35ns | 0.139 | 0.000821 | 0 | 875 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 | 3.7μs | 1.27ns | 4.74ns | 0.026 | 0 | 0 | 1.9 KB |
| master | SendAsync |
netcoreapp3.1 | 4.45μs | 2.37ns | 8.87ns | 0.0333 | 0 | 0 | 2.43 KB |
| master | SendAsync |
net472 | 7.13μs | 3.16ns | 12.2ns | 0.472 | 0 | 0 | 2.99 KB |
| #4815 | SendAsync |
net6.0 | 3.87μs | 1.64ns | 6.13ns | 0.0252 | 0 | 0 | 1.9 KB |
| #4815 | SendAsync |
netcoreapp3.1 | 4.54μs | 1.64ns | 6.12ns | 0.0317 | 0 | 0 | 2.43 KB |
| #4815 | SendAsync |
net472 | 7.09μs | 2.48ns | 9.59ns | 0.473 | 0 | 0 | 2.99 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.3μs | 0.483ns | 1.81ns | 0.0218 | 0 | 0 | 1.57 KB |
| master | EnrichedLog |
netcoreapp3.1 | 1.97μs | 2.68ns | 10ns | 0.0212 | 0 | 0 | 1.57 KB |
| master | EnrichedLog |
net472 | 2.31μs | 1.14ns | 4.27ns | 0.237 | 0 | 0 | 1.49 KB |
| #4815 | EnrichedLog |
net6.0 | 1.33μs | 0.36ns | 1.25ns | 0.0222 | 0 | 0 | 1.57 KB |
| #4815 | EnrichedLog |
netcoreapp3.1 | 2.1μs | 1.14ns | 4.27ns | 0.0216 | 0 | 0 | 1.57 KB |
| #4815 | EnrichedLog |
net472 | 2.4μs | 1.74ns | 6.28ns | 0.236 | 0 | 0 | 1.49 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 | 113μs | 82.3ns | 319ns | 0.0569 | 0 | 0 | 4.21 KB |
| master | EnrichedLog |
netcoreapp3.1 | 117μs | 178ns | 688ns | 0.0582 | 0 | 0 | 4.21 KB |
| master | EnrichedLog |
net472 | 146μs | 53.6ns | 201ns | 0.656 | 0.219 | 0 | 4.38 KB |
| #4815 | EnrichedLog |
net6.0 | 111μs | 82.3ns | 319ns | 0.0553 | 0 | 0 | 4.21 KB |
| #4815 | EnrichedLog |
netcoreapp3.1 | 118μs | 175ns | 654ns | 0 | 0 | 0 | 4.21 KB |
| #4815 | EnrichedLog |
net472 | 147μs | 237ns | 919ns | 0.67 | 0.223 | 0 | 4.38 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 | 2.92μs | 0.773ns | 2.89ns | 0.0305 | 0 | 0 | 2.13 KB |
| master | EnrichedLog |
netcoreapp3.1 | 3.82μs | 1.69ns | 6.55ns | 0.0286 | 0 | 0 | 2.13 KB |
| master | EnrichedLog |
net472 | 4.49μs | 1.79ns | 6.69ns | 0.307 | 0 | 0 | 1.93 KB |
| #4815 | EnrichedLog |
net6.0 | 2.91μs | 1.13ns | 4.22ns | 0.0291 | 0 | 0 | 2.13 KB |
| #4815 | EnrichedLog |
netcoreapp3.1 | 3.88μs | 0.812ns | 2.93ns | 0.0292 | 0 | 0 | 2.13 KB |
| #4815 | EnrichedLog |
net472 | 4.58μs | 2.4ns | 8.64ns | 0.306 | 0 | 0 | 1.93 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.36μs | 0.644ns | 2.41ns | 0.0156 | 0 | 0 | 1.1 KB |
| master | SendReceive |
netcoreapp3.1 | 1.62μs | 0.599ns | 2.32ns | 0.0147 | 0 | 0 | 1.1 KB |
| master | SendReceive |
net472 | 2.05μs | 1.99ns | 7.18ns | 0.177 | 0 | 0 | 1.12 KB |
| #4815 | SendReceive |
net6.0 | 1.28μs | 0.888ns | 3.44ns | 0.0152 | 0 | 0 | 1.1 KB |
| #4815 | SendReceive |
netcoreapp3.1 | 1.63μs | 0.944ns | 3.66ns | 0.0147 | 0 | 0 | 1.1 KB |
| #4815 | SendReceive |
net472 | 1.86μs | 1.88ns | 7.27ns | 0.177 | 0.000924 | 0 | 1.12 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.57μs | 1.36ns | 5.26ns | 0.0206 | 0 | 0 | 1.53 KB |
| master | EnrichedLog |
netcoreapp3.1 | 3.46μs | 1.43ns | 5.53ns | 0.0206 | 0 | 0 | 1.58 KB |
| master | EnrichedLog |
net472 | 4.1μs | 2.13ns | 8.25ns | 0.31 | 0 | 0 | 1.96 KB |
| #4815 | EnrichedLog |
net6.0 | 2.7μs | 1.75ns | 6.79ns | 0.0203 | 0 | 0 | 1.53 KB |
| #4815 | EnrichedLog |
netcoreapp3.1 | 3.57μs | 3.54ns | 13.2ns | 0.0213 | 0 | 0 | 1.58 KB |
| #4815 | EnrichedLog |
net472 | 4μs | 1.25ns | 4.85ns | 0.311 | 0 | 0 | 1.96 KB |
Benchmarks.Trace.SpanBenchmark - Faster 🎉 Same allocations ✔️
Faster 🎉 in #4815
Benchmark
base/diff
Base Median (ns)
Diff Median (ns)
Modality
Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net472
1.152
733.73
636.94
| Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.SpanBenchmark.StartFinishSpan‑net472 | 1.152 | 733.73 | 636.94 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | StartFinishSpan |
net6.0 | 442ns | 0.305ns | 1.14ns | 0.00759 | 0 | 0 | 536 B |
| master | StartFinishSpan |
netcoreapp3.1 | 636ns | 0.206ns | 0.797ns | 0.00731 | 0 | 0 | 536 B |
| master | StartFinishSpan |
net472 | 734ns | 0.149ns | 0.558ns | 0.0852 | 0 | 0 | 538 B |
| master | StartFinishScope |
net6.0 | 507ns | 0.112ns | 0.435ns | 0.00927 | 0 | 0 | 656 B |
| master | StartFinishScope |
netcoreapp3.1 | 673ns | 0.17ns | 0.613ns | 0.00874 | 0 | 0 | 656 B |
| master | StartFinishScope |
net472 | 804ns | 0.71ns | 2.75ns | 0.098 | 0 | 0 | 618 B |
| #4815 | StartFinishSpan |
net6.0 | 460ns | 0.226ns | 0.875ns | 0.00762 | 0 | 0 | 536 B |
| #4815 | StartFinishSpan |
netcoreapp3.1 | 591ns | 2.66ns | 10.3ns | 0.00725 | 0 | 0 | 536 B |
| #4815 | StartFinishSpan |
net472 | 637ns | 0.115ns | 0.445ns | 0.0853 | 0 | 0 | 538 B |
| #4815 | StartFinishScope |
net6.0 | 530ns | 0.306ns | 1.18ns | 0.00926 | 0 | 0 | 656 B |
| #4815 | StartFinishScope |
netcoreapp3.1 | 716ns | 0.283ns | 0.981ns | 0.00886 | 0 | 0 | 656 B |
| #4815 | StartFinishScope |
net472 | 831ns | 0.363ns | 1.41ns | 0.0979 | 0 | 0 | 618 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 | 612ns | 0.365ns | 1.41ns | 0.00923 | 0 | 0 | 656 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 849ns | 0.239ns | 0.925ns | 0.00888 | 0 | 0 | 656 B |
| master | RunOnMethodBegin |
net472 | 981ns | 0.452ns | 1.75ns | 0.0977 | 0 | 0 | 618 B |
| #4815 | RunOnMethodBegin |
net6.0 | 627ns | 0.313ns | 1.83ns | 0.00911 | 0 | 0 | 656 B |
| #4815 | RunOnMethodBegin |
netcoreapp3.1 | 789ns | 0.267ns | 0.998ns | 0.00914 | 0 | 0 | 656 B |
| #4815 | RunOnMethodBegin |
net472 | 1.02μs | 0.358ns | 1.34ns | 0.0977 | 0 | 0 | 618 B |
pierotibou
approved these changes
Nov 6, 2023
| @@ -15,6 +15,8 @@ | |||
| runtime-id: Guid_2, | |||
| span.kind: internal, | |||
| version: 1.0.0, | |||
| _dd.git.commit.sha: aaaaaaaaaaaaaaaaaaaaabbbbbbbbbbbbbbbbbbbbbbb, | |||
There was a problem hiding this comment.
I don't get why the snapshots are changed with those changes
There was a problem hiding this comment.
These are due to the .NET 8 update. The .NET 8 SDK adds sourcelink by default to all apps. Which means we then extract them and add them to the tags (for source code integration support).
They're added here, because this snapshot doesn't run by "default", only when you do the comprehensive testing. And due to space issues we can't run comprehensive testing on more than one integration at a time. So I couldn't necessarily update all the snapshots in the original .NET 8 PR.
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 (4815) (10.876M) : 0, 10876186
master (11.190M) : 0, 11189506
benchmarks/2.38.0 (11.867M) : 0, 11866956
benchmarks/2.9.0 (11.214M) : 0, 11213739
section Automatic
This PR (4815) (7.455M) : 0, 7455085
master (7.662M) : 0, 7661933
benchmarks/2.38.0 (8.176M) : 0, 8175636
benchmarks/2.9.0 (8.133M) : 0, 8132928
section Trace stats
This PR (4815) (7.876M) : 0, 7875914
master (7.979M) : 0, 7978675
benchmarks/2.38.0 (8.450M) : 0, 8450370
section Manual
This PR (4815) (9.615M) : 0, 9614994
master (9.992M) : 0, 9991660
benchmarks/2.38.0 (10.334M) : 0, 10334368
section Manual + Automatic
This PR (4815) (7.263M) : 0, 7262933
master (7.444M) : 0, 7444109
benchmarks/2.38.0 (7.750M) : 0, 7750484
section Version Conflict
This PR (4815) (6.516M) : crit ,0, 6515615
master (6.943M) : 0, 6943141
benchmarks/2.38.0 (7.137M) : 0, 7136691
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (4815) (9.648M) : 0, 9647691
master (9.472M) : 0, 9471773
benchmarks/2.38.0 (9.548M) : 0, 9548121
benchmarks/2.9.0 (9.641M) : 0, 9640617
section Automatic
This PR (4815) (6.863M) : 0, 6862528
master (6.761M) : 0, 6760928
benchmarks/2.38.0 (6.747M) : 0, 6747227
section Trace stats
This PR (4815) (6.932M) : 0, 6931535
master (6.896M) : 0, 6895808
benchmarks/2.38.0 (6.815M) : 0, 6814846
section Manual
This PR (4815) (8.487M) : 0, 8487160
master (8.385M) : 0, 8385337
benchmarks/2.38.0 (8.263M) : 0, 8263131
section Manual + Automatic
This PR (4815) (6.303M) : 0, 6303256
master (6.241M) : 0, 6241427
benchmarks/2.38.0 (6.275M) : 0, 6275411
section Version Conflict
This PR (4815) (5.820M) : 0, 5820040
master (5.795M) : 0, 5794842
benchmarks/2.38.0 (5.670M) : 0, 5669744
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (4815) (9.579M) : 0, 9579403
master (9.519M) : 0, 9519293
benchmarks/2.38.0 (9.007M) : 0, 9007066
benchmarks/2.9.0 (9.513M) : 0, 9513070
section Automatic
This PR (4815) (6.797M) : 0, 6797111
master (6.777M) : 0, 6777138
benchmarks/2.38.0 (6.423M) : 0, 6422756
benchmarks/2.9.0 (7.072M) : 0, 7072421
section Trace stats
This PR (4815) (6.975M) : 0, 6975297
master (7.084M) : 0, 7084302
benchmarks/2.38.0 (6.721M) : 0, 6720838
section Manual
This PR (4815) (8.629M) : 0, 8628730
master (8.397M) : 0, 8397251
benchmarks/2.38.0 (7.977M) : 0, 7977494
section Manual + Automatic
This PR (4815) (6.528M) : 0, 6527612
master (6.486M) : 0, 6485815
benchmarks/2.38.0 (6.349M) : 0, 6349066
section Version Conflict
This PR (4815) (6.030M) : 0, 6029608
master (5.962M) : 0, 5961832
benchmarks/2.38.0 (5.682M) : 0, 5681654
gantt
title Throughput Linux x64 (ASM) (Total requests)
dateFormat X
axisFormat %s
section Baseline
master (7.504M) : 0, 7503869
benchmarks/2.38.0 (7.578M) : 0, 7577544
benchmarks/2.9.0 (7.918M) : 0, 7917536
section No attack
master (2.135M) : 0, 2134909
benchmarks/2.38.0 (2.184M) : 0, 2184279
benchmarks/2.9.0 (3.233M) : 0, 3233469
section Attack
master (1.725M) : 0, 1724804
benchmarks/2.38.0 (1.711M) : 0, 1710542
benchmarks/2.9.0 (2.584M) : 0, 2583750
section Blocking
master (3.406M) : 0, 3406408
benchmarks/2.38.0 (3.486M) : 0, 3486124
section IAST default
master (6.764M) : 0, 6763534
section IAST full
master (6.158M) : 0, 6158423
section Base vuln
master (0.943M) : 0, 943414
section IAST vuln
master (0.917M) : 0, 917290
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
?to all the mongodb ducktypes to ensure full checking?to all the AWS SDK ducktypes to ensure full checkingReason for change
We know that the mongo and AWS SDK are generating a lot of errors. Given the try-catch we currently have, these seem like the most likely issues (without further details)
Implementation details
Followed the advice from the documentation on handling nullability.
Test coverage
Ran "full" test runs for both the MongoDb tests and the AWS.SDK.SQS (to test the sdk integration). There were some missing tags on the snapshots from the .NET 8 upgrade, but I added them before making the change and confirmed the full tests passed both before and after my changes
Other details
I wonder if we should add
when (ex is not DuckTypeException)to the try catch handlers, so that we disable the integration if a duck typing error happens? 🤔 If aDuckTypeExceptionhappens in production, it will likely be an issue on every invocation, so this exception typically triggers us to disable the integration. By using a blankettry{}catch(Exception ex)we are potentially causing more issues than letting them bubble up? I didn't make the change in this PR because we know that's not the source of the errors we're aware of.