Skip to content

[CIVisibility] - Fix InternalFlushEventsAsync task creation#3148

Merged
tonyredondo merged 1 commit into
masterfrom
tony/fix-ciagentlesswriter
Sep 1, 2022
Merged

[CIVisibility] - Fix InternalFlushEventsAsync task creation#3148
tonyredondo merged 1 commit into
masterfrom
tony/fix-ciagentlesswriter

Conversation

@tonyredondo

@tonyredondo tonyredondo commented Sep 1, 2022

Copy link
Copy Markdown
Member

Summary of changes

This PR fixes a problematic behaviour in the task creation of the InternalFlushEventsAsync task.

Reason for change

The current implementation uses the Task.Factory.StartNew method without unwrapping the inner Task in a sync loop, that means that when the InternalFlushEventsAsync hits an await line, the task will complete.

Implementation details

The PR replaces the use of Task.Factory.StartNew with Task.Run that handles this automatically by unwrapping the inner task created by InternalFlushEventsAsync

@tonyredondo tonyredondo self-assigned this Sep 1, 2022
@tonyredondo
tonyredondo requested review from a team as code owners September 1, 2022 09:48
@andrewlock

andrewlock commented Sep 1, 2022

Copy link
Copy Markdown
Member

Benchmarks Report 🐌

Benchmarks for #3148 compared to master:

  • 1 benchmarks are slower, with geometric mean 1.143
  • 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 711μs 423ns 1.64μs 0.355 0 0 3.18 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 463μs 124ns 463ns 0 0 0 2.58 KB
#3148 WriteAndFlushEnrichedTraces net472 710μs 344ns 1.29μs 0.355 0 0 3.18 KB
#3148 WriteAndFlushEnrichedTraces netcoreapp3.1 474μs 148ns 573ns 0 0 0 2.59 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 183ns 0.137ns 0.514ns 0.0675 0 0 425 B
master AllCycleSimpleBody netcoreapp3.1 238ns 0.22ns 0.854ns 0.00577 0 0 424 B
master AllCycleMoreComplexBody net472 184ns 0.164ns 0.615ns 0.0637 0 0 401 B
master AllCycleMoreComplexBody netcoreapp3.1 233ns 0.243ns 0.939ns 0.0055 0 0 400 B
master BodyExtractorSimpleBody net472 265ns 0.43ns 1.66ns 0.0573 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 226ns 0.193ns 0.747ns 0.00375 0 0 272 B
master BodyExtractorMoreComplexBody net472 14.3μs 9.58ns 35.8ns 1.21 0.0213 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 12.2μs 11.2ns 43.3ns 0.0912 0 0 6.75 KB
#3148 AllCycleSimpleBody net472 186ns 0.879ns 3.52ns 0.0675 9.2E-05 0 425 B
#3148 AllCycleSimpleBody netcoreapp3.1 236ns 0.306ns 1.18ns 0.00575 0 0 424 B
#3148 AllCycleMoreComplexBody net472 183ns 0.127ns 0.492ns 0.0637 0 0 401 B
#3148 AllCycleMoreComplexBody netcoreapp3.1 233ns 0.295ns 1.14ns 0.0055 0 0 400 B
#3148 BodyExtractorSimpleBody net472 261ns 0.354ns 1.33ns 0.0574 0 0 361 B
#3148 BodyExtractorSimpleBody netcoreapp3.1 220ns 0.208ns 0.806ns 0.00374 0 0 272 B
#3148 BodyExtractorMoreComplexBody net472 14.2μs 8.01ns 30ns 1.21 0.0214 0 7.62 KB
#3148 BodyExtractorMoreComplexBody netcoreapp3.1 12.2μs 21ns 81.4ns 0.0914 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 180μs 105ns 407ns 0.268 0 0 20.57 KB
#3148 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3148 SendRequest netcoreapp3.1 182μs 222ns 860ns 0.182 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.85μs 0.969ns 3.63ns 0.15 0.000929 0 947 B
master ExecuteNonQuery netcoreapp3.1 1.49μs 0.436ns 1.63ns 0.0126 0 0 936 B
#3148 ExecuteNonQuery net472 1.84μs 0.853ns 3.31ns 0.15 0.000915 0 947 B
#3148 ExecuteNonQuery netcoreapp3.1 1.46μs 0.307ns 1.15ns 0.0123 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.53μs 0.96ns 3.59ns 0.183 0 0 1.16 KB
master CallElasticsearch netcoreapp3.1 1.6μs 0.627ns 2.35ns 0.015 0 0 1.1 KB
master CallElasticsearchAsync net472 2.68μs 0.987ns 3.82ns 0.205 0 0 1.29 KB
master CallElasticsearchAsync netcoreapp3.1 1.66μs 0.654ns 2.53ns 0.0165 0 0 1.22 KB
#3148 CallElasticsearch net472 2.48μs 0.738ns 2.76ns 0.183 0 0 1.16 KB
#3148 CallElasticsearch netcoreapp3.1 1.54μs 0.734ns 2.84ns 0.0146 0 0 1.1 KB
#3148 CallElasticsearchAsync net472 2.53μs 0.942ns 3.65ns 0.205 0 0 1.29 KB
#3148 CallElasticsearchAsync netcoreapp3.1 1.68μs 0.475ns 1.84ns 0.0159 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.57μs 4.99ns 19.3ns 0.224 0 0 1.41 KB
master ExecuteAsync netcoreapp3.1 1.7μs 4.53ns 16.3ns 0.0176 0 0 1.34 KB
#3148 ExecuteAsync net472 2.58μs 5.73ns 22.2ns 0.224 0 0 1.41 KB
#3148 ExecuteAsync netcoreapp3.1 1.72μs 4.33ns 16.8ns 0.0181 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.7μs 13ns 50.4ns 0.439 0 0 2.77 KB
master SendAsync netcoreapp3.1 3.63μs 9.08ns 35.2ns 0.0359 0 0 2.6 KB
#3148 SendAsync net472 5.71μs 15ns 56.1ns 0.44 0 0 2.77 KB
#3148 SendAsync netcoreapp3.1 3.66μs 4.32ns 16.7ns 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 3.12μs 1.72ns 6.2ns 0.288 0 0 1.81 KB
master EnrichedLog netcoreapp3.1 2.43μs 0.409ns 1.42ns 0.0244 0 0 1.85 KB
#3148 EnrichedLog net472 3.19μs 3.02ns 11.7ns 0.287 0 0 1.81 KB
#3148 EnrichedLog netcoreapp3.1 2.52μs 0.932ns 3.61ns 0.0254 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 150μs 219ns 847ns 0.678 0.226 0 4.65 KB
master EnrichedLog netcoreapp3.1 117μs 178ns 691ns 0.0575 0 0 4.49 KB
#3148 EnrichedLog net472 151μs 168ns 650ns 0.68 0.227 0 4.65 KB
#3148 EnrichedLog netcoreapp3.1 116μs 237ns 917ns 0.0576 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.5μs 12.5ns 48.5ns 0.568 0.00275 0 3.59 KB
master EnrichedLog netcoreapp3.1 4.48μs 8.43ns 32.6ns 0.0533 0 0 3.91 KB
#3148 EnrichedLog net472 5.59μs 14.9ns 57.6ns 0.569 0.00278 0 3.59 KB
#3148 EnrichedLog netcoreapp3.1 4.5μs 11.7ns 43.6ns 0.0523 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.23μs 1.21ns 4.51ns 0.217 0 0 1.37 KB
master SendReceive netcoreapp3.1 1.8μs 0.593ns 2.22ns 0.0179 0 0 1.32 KB
#3148 SendReceive net472 2.24μs 1.06ns 3.68ns 0.218 0 0 1.37 KB
#3148 SendReceive netcoreapp3.1 1.92μs 0.947ns 3.67ns 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 5.14μs 1.12ns 4.03ns 0.353 0 0 2.23 KB
master EnrichedLog netcoreapp3.1 4.29μs 2.18ns 8.14ns 0.0237 0 0 1.8 KB
#3148 EnrichedLog net472 4.98μs 1.3ns 4.87ns 0.354 0 0 2.23 KB
#3148 EnrichedLog netcoreapp3.1 4.4μs 3.99ns 15.5ns 0.0237 0 0 1.8 KB
Benchmarks.Trace.SpanBenchmark - Slower ⚠️ Same allocations ✔️

Slower ⚠️ in #3148

Benchmark diff/base Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.SpanBenchmark.StartFinishScope‑netcoreapp3.1 1.143 1,065.12 1,217.48

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master StartFinishSpan net472 1.1μs 0.39ns 1.46ns 0.128 0 0 810 B
master StartFinishSpan netcoreapp3.1 942ns 0.385ns 1.44ns 0.0103 0 0 760 B
master StartFinishScope net472 1.39μs 0.328ns 1.23ns 0.141 0 0 891 B
master StartFinishScope netcoreapp3.1 1.06μs 0.372ns 1.34ns 0.0119 0 0 880 B
#3148 StartFinishSpan net472 1.16μs 0.426ns 1.48ns 0.129 0 0 810 B
#3148 StartFinishSpan netcoreapp3.1 949ns 0.625ns 2.42ns 0.0104 0 0 760 B
#3148 StartFinishScope net472 1.45μs 4.44ns 17.2ns 0.141 0 0 891 B
#3148 StartFinishScope netcoreapp3.1 1.21μs 3.42ns 13.2ns 0.0119 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.671ns 2.6ns 0.141 0 0 891 B
master RunOnMethodBegin netcoreapp3.1 1.18μs 1.28ns 4.94ns 0.0117 0 0 880 B
#3148 RunOnMethodBegin net472 1.54μs 0.377ns 1.46ns 0.141 0 0 891 B
#3148 RunOnMethodBegin netcoreapp3.1 1.16μs 0.503ns 1.88ns 0.0122 0 0 880 B

@andrewlock

andrewlock commented Sep 1, 2022

Copy link
Copy Markdown
Member

Code Coverage Report 📊

✔️ Merging #3148 into master will not change line coverage
✔️ Merging #3148 into master will not change branch coverage
✔️ Merging #3148 into master will not change complexity

master #3148 Change
Lines 17457 / 23771 17507 / 23769
Lines % 73% 74% 0% ✔️
Branches 10329 / 14621 10359 / 14621
Branches % 71% 71% 0% ✔️
Complexity 15784 15784 0 ✔️

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #3148 Change
Lines % 73% 74% 0% ✔️
Branches % 71% 71% 0% ✔️
Complexity 15784 15784 0 ✔️

The following classes have significant coverage changes.

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

View the full reports for further details:

@tonyredondo
tonyredondo merged commit 1965d30 into master Sep 1, 2022
@tonyredondo
tonyredondo deleted the tony/fix-ciagentlesswriter branch September 1, 2022 12:38
@github-actions github-actions Bot added this to the vNext milestone Sep 1, 2022
kevingosse pushed a commit that referenced this pull request Sep 1, 2022
@tonyredondo
Fix Internal Flush initialization task creation (#3148)
648eab6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewlock andrewlock andrewlock approved these changes
+1 more reviewer
@kevingosse kevingosse kevingosse approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tonyredondo tonyredondo

Labels

area:ci-visibility type:bug

Projects

None yet

Milestone

2.15.0

Development

Successfully merging this pull request may close these issues.

3 participants

@tonyredondo @andrewlock @kevingosse