Skip to content

Refactor transport strategies code#3126

Merged
shurivich merged 4 commits into
masterfrom
andrew/refactor-transport-strategies
Aug 29, 2022
Merged

Refactor transport strategies code#3126
shurivich merged 4 commits into
masterfrom
andrew/refactor-transport-strategies

Conversation

@andrewlock

@andrewlock andrewlock commented Aug 25, 2022

Copy link
Copy Markdown
Member

Summary of changes

  • Remove the duplication from TracesTransportStrategy and TelemetryTransportStrategy

Reason for change

The code for choosing the correct IApiRequestFactory based on ExporterSettings and target framework is…convoluted. Currently we duplicate most of this in TracesTransportStrategy and TelemetryTransportStrategy. However, for data streams monitoring I need to create a new one which will be another copy, and @shurivich also needs a new one to support UDS etc..

Implementation details

Extract the common bits, pass the rest in as parameters, or Func<T> where appropriate. I also removed the static helpers on DatadogHttpClient as didn't feel right to keep creating extra methods there (and they're now unused anyway).

Test coverage

Should be covered by all existing tests

Other details

I think this is a prerequisite for necessary changes to #3125.

@andrewlock
andrewlock requested a review from a team as a code owner August 25, 2022 14:50
@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock
andrewlock force-pushed the andrew/refactor-transport-strategies branch from a8f772c to 0958492 Compare August 25, 2022 16:26
@andrewlock

andrewlock commented Aug 25, 2022

Copy link
Copy Markdown
Member Author

Benchmarks Report 🐌

Benchmarks for #3126 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 712μs 623ns 2.41μs 0.355 0 0 3.18 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 467μs 74.1ns 257ns 0 0 0 2.58 KB
#3126 WriteAndFlushEnrichedTraces net472 711μs 418ns 1.51μs 0.355 0 0 3.18 KB
#3126 WriteAndFlushEnrichedTraces netcoreapp3.1 457μs 466ns 1.81μs 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 186ns 0.122ns 0.472ns 0.0676 0 0 425 B
master AllCycleSimpleBody netcoreapp3.1 238ns 0.24ns 0.93ns 0.0059 0 0 424 B
master AllCycleMoreComplexBody net472 184ns 0.141ns 0.526ns 0.0638 0 0 401 B
master AllCycleMoreComplexBody netcoreapp3.1 236ns 0.274ns 1.06ns 0.00551 0 0 400 B
master BodyExtractorSimpleBody net472 256ns 0.257ns 0.963ns 0.0573 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 222ns 0.269ns 1.01ns 0.00377 0 0 272 B
master BodyExtractorMoreComplexBody net472 14.6μs 16.6ns 62.1ns 1.2 0.0218 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 12.1μs 12.7ns 49.2ns 0.0916 0 0 6.75 KB
#3126 AllCycleSimpleBody net472 200ns 0.157ns 0.606ns 0.0676 0 0 425 B
#3126 AllCycleSimpleBody netcoreapp3.1 238ns 0.234ns 0.908ns 0.00584 0 0 424 B
#3126 AllCycleMoreComplexBody net472 183ns 0.226ns 0.877ns 0.0637 0 0 401 B
#3126 AllCycleMoreComplexBody netcoreapp3.1 236ns 0.242ns 0.938ns 0.00544 0 0 400 B
#3126 BodyExtractorSimpleBody net472 257ns 0.263ns 0.984ns 0.0573 0 0 361 B
#3126 BodyExtractorSimpleBody netcoreapp3.1 219ns 0.226ns 0.845ns 0.00365 0 0 272 B
#3126 BodyExtractorMoreComplexBody net472 14.8μs 12.3ns 47.7ns 1.2 0.0148 0 7.62 KB
#3126 BodyExtractorMoreComplexBody netcoreapp3.1 12.7μs 13.9ns 52.1ns 0.0884 0 0 6.75 KB
Benchmarks.Trace.AspNetCoreBenchmark - Unknown 🤷 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 181μs 129ns 498ns 0.271 0 0 20.57 KB
#3126 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3126 SendRequest netcoreapp3.1 180μs 123ns 476ns 0.18 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 1.01ns 3.79ns 0.15 0.00092 0 947 B
master ExecuteNonQuery netcoreapp3.1 1.46μs 0.6ns 2.33ns 0.0124 0 0 936 B
#3126 ExecuteNonQuery net472 1.76μs 0.85ns 3.29ns 0.15 0.000883 0 947 B
#3126 ExecuteNonQuery netcoreapp3.1 1.42μs 0.89ns 3.45ns 0.0126 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.57μs 1.12ns 4.32ns 0.183 0 0 1.16 KB
master CallElasticsearch netcoreapp3.1 1.5μs 0.393ns 1.47ns 0.015 0 0 1.1 KB
master CallElasticsearchAsync net472 2.77μs 0.645ns 2.41ns 0.204 0 0 1.29 KB
master CallElasticsearchAsync netcoreapp3.1 1.69μs 0.39ns 1.46ns 0.0162 0 0 1.22 KB
#3126 CallElasticsearch net472 2.39μs 0.484ns 1.75ns 0.183 0 0 1.16 KB
#3126 CallElasticsearch netcoreapp3.1 1.53μs 1.25ns 4.68ns 0.0147 0 0 1.1 KB
#3126 CallElasticsearchAsync net472 2.62μs 0.386ns 1.5ns 0.205 0 0 1.29 KB
#3126 CallElasticsearchAsync netcoreapp3.1 1.62μs 0.5ns 1.94ns 0.0162 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.49μs 8.5ns 32.9ns 0.224 0 0 1.41 KB
master ExecuteAsync netcoreapp3.1 1.67μs 3.62ns 14ns 0.0184 0 0 1.34 KB
#3126 ExecuteAsync net472 2.58μs 7.61ns 29.5ns 0.223 0 0 1.41 KB
#3126 ExecuteAsync netcoreapp3.1 1.67μs 3.28ns 12.7ns 0.0179 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.63μs 16.8ns 65.1ns 0.439 0 0 2.77 KB
master SendAsync netcoreapp3.1 3.61μs 9.47ns 36.7ns 0.0356 0 0 2.6 KB
#3126 SendAsync net472 5.51μs 8.69ns 33.7ns 0.438 0 0 2.77 KB
#3126 SendAsync netcoreapp3.1 3.48μs 5.29ns 20.5ns 0.0345 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 3.12μs 2.36ns 9.12ns 0.287 0 0 1.81 KB
master EnrichedLog netcoreapp3.1 2.47μs 0.628ns 2.35ns 0.0247 0 0 1.85 KB
#3126 EnrichedLog net472 3.2μs 1.55ns 5.57ns 0.287 0 0 1.81 KB
#3126 EnrichedLog netcoreapp3.1 2.53μs 2.46ns 9.54ns 0.0252 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 311ns 1.12μs 0.679 0.226 0 4.65 KB
master EnrichedLog netcoreapp3.1 117μs 120ns 465ns 0.0586 0 0 4.49 KB
#3126 EnrichedLog net472 151μs 252ns 977ns 0.67 0.223 0 4.65 KB
#3126 EnrichedLog netcoreapp3.1 118μs 152ns 590ns 0.0583 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.49μs 19.9ns 77ns 0.57 0.00271 0 3.59 KB
master EnrichedLog netcoreapp3.1 4.21μs 10.8ns 40.5ns 0.0533 0 0 3.91 KB
#3126 EnrichedLog net472 5.53μs 14.1ns 54.7ns 0.57 0.00277 0 3.59 KB
#3126 EnrichedLog netcoreapp3.1 4.31μs 13.5ns 52.1ns 0.0534 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.31μs 1.28ns 4.8ns 0.218 0 0 1.37 KB
master SendReceive netcoreapp3.1 1.76μs 2.43ns 9.1ns 0.0185 0 0 1.32 KB
#3126 SendReceive net472 2.23μs 1.18ns 4.27ns 0.217 0 0 1.37 KB
#3126 SendReceive netcoreapp3.1 1.75μs 0.834ns 3.23ns 0.0174 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 4.92μs 1.78ns 6.88ns 0.353 0 0 2.23 KB
master EnrichedLog netcoreapp3.1 4.29μs 1.5ns 5.62ns 0.0235 0 0 1.8 KB
#3126 EnrichedLog net472 5.06μs 1.97ns 7.37ns 0.353 0 0 2.23 KB
#3126 EnrichedLog netcoreapp3.1 4.33μs 2.59ns 10ns 0.0238 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.16μs 0.289ns 1.08ns 0.129 0 0 810 B
master StartFinishSpan netcoreapp3.1 898ns 0.31ns 1.12ns 0.0104 0 0 760 B
master StartFinishScope net472 1.38μs 0.274ns 0.987ns 0.141 0 0 891 B
master StartFinishScope netcoreapp3.1 1.05μs 0.535ns 2ns 0.0121 0 0 880 B
#3126 StartFinishSpan net472 1.13μs 0.697ns 2.7ns 0.128 0 0 810 B
#3126 StartFinishSpan netcoreapp3.1 900ns 0.322ns 1.11ns 0.0104 0 0 760 B
#3126 StartFinishScope net472 1.39μs 0.376ns 1.46ns 0.141 0 0 891 B
#3126 StartFinishScope netcoreapp3.1 1.08μs 0.255ns 0.919ns 0.0116 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.55μs 0.327ns 1.22ns 0.141 0 0 891 B
master RunOnMethodBegin netcoreapp3.1 1.13μs 0.457ns 1.71ns 0.0119 0 0 880 B
#3126 RunOnMethodBegin net472 1.49μs 0.512ns 1.98ns 0.141 0 0 891 B
#3126 RunOnMethodBegin netcoreapp3.1 1.16μs 0.421ns 1.63ns 0.0115 0 0 880 B

@andrewlock

andrewlock commented Aug 25, 2022

Copy link
Copy Markdown
Member Author

Code Coverage Report 📊

✔️ Merging #3126 into master will not change line coverage
✔️ Merging #3126 into master will not change branch coverage
✔️ Merging #3126 into master will will decrease complexity by 4

master #3126 Change
Lines 17163 / 23534 17131 / 23542
Lines % 73% 73% 0% ✔️
Branches 10200 / 14538 10194 / 14534
Branches % 70% 70% 0% ✔️
Complexity 15640 15636 -4 ✔️

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #3126 Change
Lines % 73% 73% 0% ✔️
Branches % 70% 70% 0% ✔️
Complexity 15640 15636 -4 ✔️

The following classes have significant coverage changes.

File Line coverage change Branch coverage change Complexity change
Datadog.Trace.Ci.GitInfo -17% ⛔ -11% ⛔ 0 ✔️
Datadog.Trace.Agent.DiscoveryService.DiscoveryService -12% ⛔ 0% ✔️ 0 ✔️
Datadog.Trace.Agent.Transports.HttpStreamRequestFactory -12% ⛔ 0% ✔️ 0 ✔️
Datadog.Trace.Debugger.ProbeStatuses.ProbeStatusPoller 0% ✔️ 6% ✔️ 0 ✔️
Datadog.Trace.Ci.CIVisibility 5% ✔️ 6% ✔️ 0 ✔️
Datadog.Trace.Agent.TracesTransportStrategy 8% ✔️ 0% ✔️ -4 ✔️

The following classes were added in #3126:

File Line coverage Branch coverage Complexity
Datadog.Trace.Agent.AgentTransportStrategy 96% 100% 5
Datadog.Trace.Telemetry.Transports.TelemetryAgentHttpHeaderHelper 80% 100% 3

1 classes were removed from Datadog.Trace in #3126

View the full reports for further details:

@shurivich shurivich mentioned this pull request Aug 29, 2022
Merged
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.

LGTM. It's always good to extract common code.

Comment thread tracer/src/Datadog.Trace/Telemetry/Transports/TelemetryTransportStrategy.cs
tcpTimeout: Timeout,
TelemetryHttpHeaderNames.GetDefaultAgentHeaders(),
() => new TelemetryAgentHttpHeaderHelper(),
uri => UriHelpers.Combine(uri, TelemetryConstants.AgentTelemetryEndpoint));

@dromanol dromanol Aug 29, 2022

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.

Nice refactor 👍

@shurivich
shurivich merged commit b3386dd into master Aug 29, 2022
@shurivich
shurivich deleted the andrew/refactor-transport-strategies branch August 29, 2022 09:41
@github-actions github-actions Bot added this to the vNext milestone Aug 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@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.

3 participants

@andrewlock @dromanol @shurivich