Skip to content

Small refactoring to remove duplicate code around SocketException detection#3123

Merged
andrewlock merged 1 commit into
masterfrom
andrew/socket-exception-refactor
Aug 30, 2022
Merged

Small refactoring to remove duplicate code around SocketException detection#3123
andrewlock merged 1 commit into
masterfrom
andrew/socket-exception-refactor

Conversation

@andrewlock

@andrewlock andrewlock commented Aug 25, 2022

Copy link
Copy Markdown
Member

Summary of changes

Extracts helper method for determining if an exception is a SocketException

Reason for change

We have the exact same logic duplicated in multiple places

Implementation details

Created an extension method called IsSocketException(),

Test coverage

Added some unit tests

Other details

@andrewlock
andrewlock requested review from a team as code owners August 25, 2022 08:56
@andrewlock

andrewlock commented Aug 25, 2022

Copy link
Copy Markdown
Member Author

Benchmarks Report 🐌

Benchmarks for #3123 compared to master:

  • All benchmarks have the same speed
  • All benchmarks have the same 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.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master WriteAndFlushEnrichedTraces net472 709μs 488ns 1.89μs 0.353 0 0 3.18 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 462μs 125ns 486ns 0 0 0 2.58 KB
#3123 WriteAndFlushEnrichedTraces net472 713μs 149ns 556ns 0.357 0 0 3.18 KB
#3123 WriteAndFlushEnrichedTraces netcoreapp3.1 459μs 148ns 573ns 0 0 0 2.58 KB
Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net472 185ns 0.162ns 0.607ns 0.0676 0 0 425 B
master AllCycleSimpleBody netcoreapp3.1 241ns 0.181ns 0.7ns 0.00579 0 0 424 B
master AllCycleMoreComplexBody net472 202ns 0.276ns 1.03ns 0.0637 0 0 401 B
master AllCycleMoreComplexBody netcoreapp3.1 237ns 0.169ns 0.632ns 0.00558 0 0 400 B
master BodyExtractorSimpleBody net472 268ns 0.348ns 1.35ns 0.0573 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 239ns 0.262ns 0.98ns 0.00367 0 0 272 B
master BodyExtractorMoreComplexBody net472 14.5μs 12.3ns 47.7ns 1.2 0.0217 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 12.3μs 19.2ns 74.2ns 0.0921 0 0 6.75 KB
#3123 AllCycleSimpleBody net472 185ns 0.191ns 0.741ns 0.0675 0 0 425 B
#3123 AllCycleSimpleBody netcoreapp3.1 238ns 0.18ns 0.675ns 0.00583 0 0 424 B
#3123 AllCycleMoreComplexBody net472 182ns 0.135ns 0.505ns 0.0637 0 0 401 B
#3123 AllCycleMoreComplexBody netcoreapp3.1 236ns 0.383ns 1.49ns 0.00557 0 0 400 B
#3123 BodyExtractorSimpleBody net472 257ns 0.242ns 0.938ns 0.0574 0 0 361 B
#3123 BodyExtractorSimpleBody netcoreapp3.1 232ns 0.175ns 0.678ns 0.00372 0 0 272 B
#3123 BodyExtractorMoreComplexBody net472 14.5μs 11.2ns 43.5ns 1.21 0.0219 0 7.62 KB
#3123 BodyExtractorMoreComplexBody netcoreapp3.1 12.9μs 16.9ns 63.4ns 0.0904 0 0 6.75 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 net472 0ns 0ns 0ns 0 0 0 0 b
master SendRequest netcoreapp3.1 179μs 89.8ns 348ns 0.179 0 0 20.57 KB
#3123 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3123 SendRequest netcoreapp3.1 182μs 130ns 503ns 0.271 0 0 20.57 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 net472 1.86μs 0.526ns 1.9ns 0.15 0.000929 0 947 B
master ExecuteNonQuery netcoreapp3.1 1.4μs 0.566ns 2.19ns 0.0126 0 0 936 B
#3123 ExecuteNonQuery net472 1.76μs 0.641ns 2.48ns 0.15 0.000875 0 947 B
#3123 ExecuteNonQuery netcoreapp3.1 1.4μs 0.668ns 2.5ns 0.0125 0 0 936 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 net472 2.51μs 1.09ns 4.09ns 0.184 0 0 1.16 KB
master CallElasticsearch netcoreapp3.1 1.49μs 0.407ns 1.41ns 0.0149 0 0 1.1 KB
master CallElasticsearchAsync net472 2.64μs 0.648ns 2.51ns 0.205 0 0 1.29 KB
master CallElasticsearchAsync netcoreapp3.1 1.63μs 0.499ns 1.87ns 0.0163 0 0 1.22 KB
#3123 CallElasticsearch net472 2.46μs 0.669ns 2.59ns 0.183 0 0 1.16 KB
#3123 CallElasticsearch netcoreapp3.1 1.49μs 0.366ns 1.27ns 0.015 0 0 1.1 KB
#3123 CallElasticsearchAsync net472 2.63μs 2.54ns 9.83ns 0.205 0 0 1.29 KB
#3123 CallElasticsearchAsync netcoreapp3.1 1.67μs 2.14ns 7.72ns 0.0158 0 0 1.22 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 net472 2.56μs 4.82ns 18.7ns 0.224 0 0 1.41 KB
master ExecuteAsync netcoreapp3.1 1.66μs 2.37ns 8.85ns 0.0178 0 0 1.34 KB
#3123 ExecuteAsync net472 2.54μs 9.33ns 36.1ns 0.223 0 0 1.41 KB
#3123 ExecuteAsync netcoreapp3.1 1.67μs 3.31ns 12.4ns 0.0182 0 0 1.34 KB
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master SendAsync net472 5.78μs 9.75ns 37.7ns 0.438 0 0 2.77 KB
master SendAsync netcoreapp3.1 3.71μs 5.19ns 19.4ns 0.0358 0 0 2.6 KB
#3123 SendAsync net472 5.73μs 10.4ns 37.7ns 0.438 0 0 2.77 KB
#3123 SendAsync netcoreapp3.1 3.53μs 8.91ns 34.5ns 0.0358 0 0 2.6 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 net472 2.96μs 0.684ns 2.56ns 0.287 0 0 1.81 KB
master EnrichedLog netcoreapp3.1 2.42μs 6.37ns 23ns 0.0253 0 0 1.85 KB
#3123 EnrichedLog net472 3.06μs 1.15ns 4.47ns 0.287 0 0 1.81 KB
#3123 EnrichedLog netcoreapp3.1 2.45μs 1.14ns 4.25ns 0.025 0 0 1.85 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 net472 151μs 86.3ns 334ns 0.677 0.226 0 4.65 KB
master EnrichedLog netcoreapp3.1 115μs 145ns 561ns 0.0578 0 0 4.49 KB
#3123 EnrichedLog net472 150μs 231ns 896ns 0.677 0.226 0 4.65 KB
#3123 EnrichedLog netcoreapp3.1 114μs 164ns 636ns 0 0 0 4.49 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 net472 5.71μs 9.34ns 35ns 0.569 0.00286 0 3.59 KB
master EnrichedLog netcoreapp3.1 4.45μs 6.83ns 25.5ns 0.0519 0 0 3.91 KB
#3123 EnrichedLog net472 5.51μs 20.4ns 78.9ns 0.569 0.00272 0 3.59 KB
#3123 EnrichedLog netcoreapp3.1 4.46μs 8.35ns 32.4ns 0.0536 0 0 3.91 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 net472 2.26μs 0.661ns 2.56ns 0.217 0 0 1.37 KB
master SendReceive netcoreapp3.1 1.78μs 0.577ns 2.23ns 0.0179 0 0 1.32 KB
#3123 SendReceive net472 2.23μs 0.703ns 2.53ns 0.218 0 0 1.37 KB
#3123 SendReceive netcoreapp3.1 1.74μs 0.614ns 2.3ns 0.018 0 0 1.32 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 net472 5.06μs 2.19ns 8.21ns 0.353 0 0 2.23 KB
master EnrichedLog netcoreapp3.1 4.27μs 1.92ns 7.44ns 0.0235 0 0 1.8 KB
#3123 EnrichedLog net472 5.03μs 2.03ns 7.86ns 0.354 0 0 2.23 KB
#3123 EnrichedLog netcoreapp3.1 4.2μs 1.57ns 5.67ns 0.0232 0 0 1.8 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 net472 1.12μs 0.301ns 1.13ns 0.128 0 0 810 B
master StartFinishSpan netcoreapp3.1 913ns 1.07ns 4.14ns 0.0104 0 0 760 B
master StartFinishScope net472 1.34μs 0.834ns 3.12ns 0.141 0 0 891 B
master StartFinishScope netcoreapp3.1 1.04μs 0.329ns 1.18ns 0.0118 0 0 880 B
#3123 StartFinishSpan net472 1.15μs 0.306ns 1.14ns 0.129 0 0 810 B
#3123 StartFinishSpan netcoreapp3.1 939ns 0.603ns 2.09ns 0.0102 0 0 760 B
#3123 StartFinishScope net472 1.34μs 0.628ns 2.35ns 0.142 0 0 891 B
#3123 StartFinishScope netcoreapp3.1 1.12μs 0.292ns 1.09ns 0.0118 0 0 880 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 net472 1.46μs 0.507ns 1.9ns 0.141 0 0 891 B
master RunOnMethodBegin netcoreapp3.1 1.15μs 0.385ns 1.49ns 0.0115 0 0 880 B
#3123 RunOnMethodBegin net472 1.56μs 0.634ns 2.45ns 0.142 0 0 891 B
#3123 RunOnMethodBegin netcoreapp3.1 1.16μs 0.499ns 1.93ns 0.0116 0 0 880 B

@andrewlock

andrewlock commented Aug 25, 2022

Copy link
Copy Markdown
Member Author

Code Coverage Report 📊

✔️ Merging #3123 into master will not change line coverage
✔️ Merging #3123 into master will not change branch coverage
✔️ Merging #3123 into master will will decrease complexity by 16

master #3123 Change
Lines 17190 / 23534 17190 / 23508
Lines % 73% 73% 0% ✔️
Branches 10221 / 14538 10217 / 14512
Branches % 70% 70% 0% ✔️
Complexity 15640 15624 -16 ✔️

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #3123 Change
Lines % 73% 73% 0% ✔️
Branches % 70% 70% 0% ✔️
Complexity 15640 15624 -16 ✔️

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change
Datadog.Trace.Ci.CIVisibility 5% ✔️ 6% ✔️ 0 ✔️

The following classes were added in #3123:

File Line coverage Branch coverage Complexity
Datadog.Trace.Util.Http.HttpExceptionExtensions 100% 100% 4

View the full reports for further details:

bouwkast
bouwkast approved these changes Aug 25, 2022
View reviewed changes

@bouwkast bouwkast left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice!

lucaspimentel
lucaspimentel reviewed Aug 26, 2022
View reviewed changes
Comment thread tracer/src/Datadog.Trace/Util/Http/HttpExceptionExtensions.cs

namespace Datadog.Trace.Util.Http;

internal static class HttpExceptionExtensions

@lucaspimentel lucaspimentel Aug 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be called ExceptionExtensions since the only method is an extension on Exception, not HttpException? (level of care low)

@andrewlock andrewlock Aug 30, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, probably 🤔 I made it HttpExceptionExtensions because I wanted to put it in the Http sub namespace, to reduce the places it's in scope. I'll leave it there for now, but happy to shift it later

lucaspimentel
lucaspimentel approved these changes Aug 26, 2022
View reviewed changes

@lucaspimentel lucaspimentel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice deduplication, thanks

dromanol
dromanol approved these changes Aug 29, 2022
View reviewed changes

@dromanol dromanol left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👌

zacharycmontoya
zacharycmontoya approved these changes Aug 29, 2022
View reviewed changes

@zacharycmontoya zacharycmontoya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Love it

@andrewlock
andrewlock merged commit 68e01ee into master Aug 30, 2022
@andrewlock
andrewlock deleted the andrew/socket-exception-refactor branch August 30, 2022 07:36
@github-actions github-actions Bot added this to the vNext milestone Aug 30, 2022
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
@bouwkast bouwkast bouwkast approved these changes
@zacharycmontoya zacharycmontoya zacharycmontoya approved these changes
@dromanol dromanol dromanol approved these changes

Assignees

No one assigned

Labels

type:refactor

Projects

None yet

Milestone

2.15.0

Development

Successfully merging this pull request may close these issues.

5 participants

@andrewlock @lucaspimentel @bouwkast @zacharycmontoya @dromanol