Skip to content

[CIVisibility] - Coverage fixes and improvements#3170

Merged
tonyredondo merged 8 commits into
masterfrom
tony/coverage-coretypes-fixes
Sep 6, 2022
Merged

[CIVisibility] - Coverage fixes and improvements#3170
tonyredondo merged 8 commits into
masterfrom
tony/coverage-coretypes-fixes

Conversation

@tonyredondo

@tonyredondo tonyredondo commented Sep 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Summary of changes

This PR contains fixes and improvements of the coverage algorithm.

  1. Avoid the usage of module.ImportReference(typeof...) pattern that can add a new AssemblyReference for system types (in cases where the tool compiled in .NET 6.0 modifies a netcoreapp3.1 or netstandard assembly). This is an actual issue that can leave the assembly with two runtimes references.

  2. Improves the resolver by manually searching in the test output folder.

  3. Improves the logger to use also the managed standard tracer logger. (The test output one truncates errors and exceptions).

  4. Adds a Warning message when a .NET core signed assembly contains the InternalsVisibleTo attribute and a snk file is not provided.

Reason for change

After running some tests from the Test environment repo some issues were identified and this PR solves that.

@tonyredondo tonyredondo self-assigned this Sep 5, 2022
@tonyredondo tonyredondo added the type:enhancement Improvement to an existing feature label Sep 5, 2022
@tonyredondo
tonyredondo marked this pull request as ready for review September 5, 2022 17:29
@tonyredondo
tonyredondo requested a review from a team as a code owner September 5, 2022 17:29
@andrewlock

This comment has been minimized.

Sign in to view
andrewlock
andrewlock approved these changes Sep 6, 2022
View reviewed changes
Comment thread tracer/src/Datadog.Trace.Coverage.collector/AssemblyProcessor.cs
Comment thread tracer/src/Datadog.Trace.Coverage.collector/AssemblyProcessor.cs
Comment thread tracer/src/Datadog.Trace.Coverage.collector/AssemblyProcessor.cs
Comment thread tracer/src/Datadog.Trace.Coverage.collector/AssemblyProcessor.cs
Comment on lines +534 to +554
private string GetDatadogTracer(TracerTarget tracerTarget)
{
// Get the Datadog.Trace path
string targetFolder = "net461";
switch (tracerTarget)
{
case TracerTarget.Net461:
targetFolder = "net461";
break;
case TracerTarget.Netstandard20:
targetFolder = "netstandard2.0";
break;
case TracerTarget.Netcoreapp31:
targetFolder = "netcoreapp3.1";
break;
case TracerTarget.Net60:
targetFolder = "net6.0";
break;
}

return Path.Combine(_tracerHome, targetFolder, "Datadog.Trace.dll");

@andrewlock andrewlock Sep 6, 2022

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.

Seeing this code again bothers me - it makes it a pain to add/drop targets! I feel like we should have a single c# file that contains this logic and then we link to it in the project as needs be.

Not for this PR obviously, just a thought 🙂

@tonyredondo tonyredondo Sep 6, 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.

I agree, this can be useful for others.

@andrewlock

This comment has been minimized.

Sign in to view
@andrewlock

andrewlock commented Sep 6, 2022

Copy link
Copy Markdown
Member

Benchmarks Report 🐌

Benchmarks for #3170 compared to master:

  • 1 benchmarks are faster, with geometric mean 1.155
  • 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 723μs 755ns 2.83μs 0.359 0 0 3.18 KB
master WriteAndFlushEnrichedTraces netcoreapp3.1 480μs 1.95μs 7.31μs 0 0 0 2.58 KB
#3170 WriteAndFlushEnrichedTraces net472 713μs 769ns 2.98μs 0.365 0 0 3.17 KB
#3170 WriteAndFlushEnrichedTraces netcoreapp3.1 456μs 174ns 627ns 0 0 0 2.58 KB
Benchmarks.Trace.AppSecBodyBenchmark - Faster 🎉 Same allocations ✔️

Faster 🎉 in #3170

Benchmark base/diff Base Median (ns) Diff Median (ns) Modality
Benchmarks.Trace.AppSecBodyBenchmark.AllCycleMoreComplexBody‑net472 1.155 232.64 201.49

Raw results

Branch Method Toolchain Mean StdError StdDev Gen 0 Gen 1 Gen 2 Allocated
master AllCycleSimpleBody net472 185ns 0.0773ns 0.3ns 0.0676 0 0 425 B
master AllCycleSimpleBody netcoreapp3.1 237ns 0.338ns 1.31ns 0.00585 0 0 424 B
master AllCycleMoreComplexBody net472 234ns 1.06ns 4.11ns 0.0637 0 0 401 B
master AllCycleMoreComplexBody netcoreapp3.1 247ns 0.28ns 1.09ns 0.00549 0 0 400 B
master BodyExtractorSimpleBody net472 262ns 0.175ns 0.655ns 0.0574 0 0 361 B
master BodyExtractorSimpleBody netcoreapp3.1 219ns 0.322ns 1.2ns 0.00376 0 0 272 B
master BodyExtractorMoreComplexBody net472 14.5μs 8.62ns 32.3ns 1.21 0.0217 0 7.62 KB
master BodyExtractorMoreComplexBody netcoreapp3.1 12μs 17.6ns 65.9ns 0.0897 0 0 6.75 KB
#3170 AllCycleSimpleBody net472 187ns 0.134ns 0.521ns 0.0675 0 0 425 B
#3170 AllCycleSimpleBody netcoreapp3.1 236ns 0.258ns 1ns 0.00577 0 0 424 B
#3170 AllCycleMoreComplexBody net472 202ns 0.136ns 0.508ns 0.0638 0 0 401 B
#3170 AllCycleMoreComplexBody netcoreapp3.1 232ns 0.255ns 0.986ns 0.00547 0 0 400 B
#3170 BodyExtractorSimpleBody net472 267ns 0.27ns 1.05ns 0.0574 0 0 361 B
#3170 BodyExtractorSimpleBody netcoreapp3.1 237ns 0.191ns 0.687ns 0.00366 0 0 272 B
#3170 BodyExtractorMoreComplexBody net472 14.3μs 9.11ns 34.1ns 1.21 0.0217 0 7.62 KB
#3170 BodyExtractorMoreComplexBody netcoreapp3.1 11.9μs 19.1ns 71.3ns 0.0889 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 114ns 427ns 0.271 0 0 20.57 KB
#3170 SendRequest net472 0ns 0ns 0ns 0 0 0 0 b
#3170 SendRequest netcoreapp3.1 181μs 151ns 584ns 0.27 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.8μs 0.753ns 2.71ns 0.15 0.000899 0 947 B
master ExecuteNonQuery netcoreapp3.1 1.61μs 0.786ns 3.04ns 0.0121 0 0 936 B
#3170 ExecuteNonQuery net472 1.89μs 0.845ns 3.27ns 0.15 0.000939 0 947 B
#3170 ExecuteNonQuery netcoreapp3.1 1.5μs 1.05ns 4.05ns 0.0127 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 1.19ns 4.62ns 0.183 0 0 1.16 KB
master CallElasticsearch netcoreapp3.1 1.62μs 0.771ns 2.89ns 0.0146 0 0 1.1 KB
master CallElasticsearchAsync net472 2.64μs 0.732ns 2.74ns 0.204 0 0 1.29 KB
master CallElasticsearchAsync netcoreapp3.1 1.64μs 2.07ns 7.74ns 0.0165 0 0 1.22 KB
#3170 CallElasticsearch net472 2.47μs 1.28ns 4.95ns 0.183 0 0 1.16 KB
#3170 CallElasticsearch netcoreapp3.1 1.53μs 1.04ns 3.89ns 0.0145 0 0 1.1 KB
#3170 CallElasticsearchAsync net472 2.59μs 1.42ns 5.52ns 0.205 0 0 1.29 KB
#3170 CallElasticsearchAsync netcoreapp3.1 1.7μs 0.664ns 2.48ns 0.0161 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.65μs 5.89ns 22.8ns 0.226 0 0 1.42 KB
master ExecuteAsync netcoreapp3.1 1.68μs 4.42ns 17.1ns 0.0185 0 0 1.34 KB
#3170 ExecuteAsync net472 2.64μs 6.14ns 23.8ns 0.225 0 0 1.42 KB
#3170 ExecuteAsync netcoreapp3.1 1.68μs 3.61ns 14ns 0.0183 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.64μs 8.3ns 32.2ns 0.439 0 0 2.77 KB
master SendAsync netcoreapp3.1 3.62μs 10.3ns 39.7ns 0.0348 0 0 2.6 KB
#3170 SendAsync net472 5.66μs 15.1ns 58.3ns 0.439 0 0 2.77 KB
#3170 SendAsync netcoreapp3.1 3.64μs 5.76ns 22.3ns 0.0343 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.23μs 3.78ns 14.6ns 0.287 0 0 1.81 KB
master EnrichedLog netcoreapp3.1 2.53μs 1.19ns 4.43ns 0.0253 0 0 1.85 KB
#3170 EnrichedLog net472 3.38μs 1.4ns 5.41ns 0.287 0 0 1.81 KB
#3170 EnrichedLog netcoreapp3.1 2.59μs 0.97ns 3.63ns 0.0248 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 109ns 392ns 0.672 0.224 0 4.65 KB
master EnrichedLog netcoreapp3.1 117μs 213ns 826ns 0.0595 0 0 4.49 KB
#3170 EnrichedLog net472 153μs 192ns 719ns 0.686 0.229 0 4.65 KB
#3170 EnrichedLog netcoreapp3.1 118μs 208ns 805ns 0.0584 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.62μs 7.97ns 29.8ns 0.568 0.00278 0 3.59 KB
master EnrichedLog netcoreapp3.1 4.33μs 9.47ns 36.7ns 0.0535 0 0 3.91 KB
#3170 EnrichedLog net472 5.62μs 11.6ns 43.4ns 0.568 0.00281 0 3.59 KB
#3170 EnrichedLog netcoreapp3.1 4.54μs 9.2ns 35.6ns 0.0518 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.32μs 4.05ns 15.2ns 0.217 0 0 1.37 KB
master SendReceive netcoreapp3.1 1.85μs 1.37ns 5.12ns 0.0176 0 0 1.32 KB
#3170 SendReceive net472 2.32μs 3.2ns 12.4ns 0.218 0 0 1.37 KB
#3170 SendReceive netcoreapp3.1 1.88μs 1.49ns 5.56ns 0.0178 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.02μs 1.33ns 5.15ns 0.352 0 0 2.23 KB
master EnrichedLog netcoreapp3.1 4.23μs 1.3ns 5.05ns 0.0233 0 0 1.8 KB
#3170 EnrichedLog net472 5.06μs 2.52ns 9.42ns 0.353 0 0 2.23 KB
#3170 EnrichedLog netcoreapp3.1 4.32μs 1.44ns 5.41ns 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.15μs 0.556ns 2.15ns 0.129 0 0 810 B
master StartFinishSpan netcoreapp3.1 935ns 3.25ns 12.1ns 0.0106 0 0 760 B
master StartFinishScope net472 1.45μs 0.761ns 2.95ns 0.141 0 0 891 B
master StartFinishScope netcoreapp3.1 1.06μs 0.62ns 2.32ns 0.0123 0 0 880 B
#3170 StartFinishSpan net472 1.22μs 0.372ns 1.39ns 0.129 0 0 810 B
#3170 StartFinishSpan netcoreapp3.1 965ns 0.314ns 1.18ns 0.0101 0 0 760 B
#3170 StartFinishScope net472 1.39μs 0.915ns 3.54ns 0.141 0 0 891 B
#3170 StartFinishScope netcoreapp3.1 1.07μs 0.275ns 1.03ns 0.0117 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.5μs 0.828ns 3.1ns 0.141 0 0 891 B
master RunOnMethodBegin netcoreapp3.1 1.12μs 0.342ns 1.28ns 0.0118 0 0 880 B
#3170 RunOnMethodBegin net472 1.5μs 0.802ns 3.11ns 0.141 0 0 891 B
#3170 RunOnMethodBegin netcoreapp3.1 1.21μs 0.469ns 1.82ns 0.0121 0 0 880 B

@andrewlock

andrewlock commented Sep 6, 2022

Copy link
Copy Markdown
Member

Code Coverage Report 📊

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

master #3170 Change
Lines 17633 / 24209 17703 / 24209
Lines % 73% 73% 0% ✔️
Branches 10433 / 14900 10473 / 14900
Branches % 70% 70% 0% ✔️
Complexity 16087 16087 0 ✔️

View the full report for further details:

Datadog.Trace Breakdown ✔️

master #3170 Change
Lines % 73% 73% 0% ✔️
Branches % 70% 70% 0% ✔️
Complexity 16087 16087 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.Ci.GitInfo 5% ✔️ 8% ✔️ 0 ✔️

View the full reports for further details:

@tonyredondo
tonyredondo merged commit 9c5882e into master Sep 6, 2022
@tonyredondo
tonyredondo deleted the tony/coverage-coretypes-fixes branch September 6, 2022 13:53
@github-actions github-actions Bot added this to the vNext milestone Sep 6, 2022
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
@nachoBonafonte nachoBonafonte nachoBonafonte approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tonyredondo tonyredondo

Labels

area:ci-visibility type:enhancement Improvement to an existing feature

Projects

None yet

Milestone

2.15.0

Development

Successfully merging this pull request may close these issues.

3 participants

@tonyredondo @andrewlock @nachoBonafonte