[CIVisibility] - Counters based Code Coverage by tonyredondo · Pull Request #3134 · DataDog/dd-trace-dotnet

tonyredondo · 2022-08-29T07:49:49Z

Summary of changes

This PR changes the algorithm of the Code Coverage feature from an individual instructions list to arrays of counters.

Reason for change

The current list based implementation works well for code with not many instructions or loops, those individual instructions are later grouped using linq to aggregate the number of times an instruction was executed. The new algorithm removes the individual instructions and keeps an int[] array with the counters for a given method, so the data gets aggregated in place.

For performance comparison, using the following test:

    [Fact]
    public void HeavyTest()
    {
        var tsk1 = Task.Run(() => Proc());
        var tsk2 = Task.Run(() => Proc());
        var tsk3 = Task.Run(() => Proc());
        var tsk4 = Task.Run(() => Proc());
        var tsk5 = Task.Run(() => Proc());
        var tsk6 = Task.Run(() => Proc());
        var tsk7 = Task.Run(() => Proc());
        var tsk8 = Task.Run(() => Proc());
        var tsk9 = Task.Run(() => Proc());
        var tsk10 = Task.Run(() => Proc());

        Task.WaitAll(tsk1, tsk2, tsk3, tsk4, tsk5, tsk6, tsk7, tsk8, tsk9, tsk10);
    }
    private void Proc()
    {
        for (var x = 0; x < 100; x++)
        {
            for (var i = 0; i < 10_000; i++)
            {
                var a = 1000;
                var b = 2000;
                var k = Math.Sqrt(Math.Sin(a) * Math.Cos(b)) + i;
            }
        }
    }

We have these results:

The code inside both for loops are executed 1.000.000 times on 10 parallel tasks. In the current code coverage algorithm this makes the instructions list to grow several times and after that a linq query is applied to the whole list.

In the new algorithm we are only incrementing counters in an array.

Implementation details

At the end is a complete rewrite of the code coverage implementation, by using counters we also removes all the strings we were defining with the filepath and the ranges, and we rely in the .pdb information already available (using the vendored version of dnlib).

Test coverage

Snapshot has been updated with the new coverage format.

…n the method body directly.

andrewlock

LGTM, though I confess, my eyes glazed over several times 🙈

andrewlock · 2022-09-01T13:42:31Z

+                _ciVisibilitySettings = CIVisibilitySettings.FromDefaultSources();
+
+                // Read the DD_DOTNET_TRACER_HOME environment variable
+                _tracerHome = Util.EnvironmentHelpers.GetEnvironmentVariable("DD_DOTNET_TRACER_HOME");


Not related to this PR, but we've been discussing about being able to get rid of this variable down the line, because the native loader sets its own env vars that we know are right. I wonder if it would make sense to use those env vars here etc

Well, I need this because I have to inject (copy to the bin folder) the Datadog.Trace assembly. And I need to choose the right version for the target application (.NET462, netstandard, .NET 6). So yeah, if there's another way to get the root folder where all the tracer assemblies are I'm open to change it.

andrewlock · 2022-09-01T13:44:56Z

-        private void BackupFolder(string folder)
-        {
-            var destinationFolder = Path.Combine(folder, _dateTime.ToString("yyyyMMddHHmmss"));
-            _logger?.Debug($"Backup folder: {destinationFolder}");
-            Copy(folder, destinationFolder);
-        }


Just wondering why you decided this wasn't required any more?

We never use this backup, and we don't have plans to do a restore in CI. This was done more for debugging purposes.

andrewlock · 2022-09-01T13:51:27Z

+namespace Datadog.Trace.Ci.Coverage.Metadata
+{
+    public abstract class ModuleCoverageMetadata
+    {
+        protected readonly int[][] Metadata;
+        protected ModuleCoverageMetadata() { }
+    }
+}


This probably doesn't need to be public right?

Sadly nope, this abstract class is inherited by the new Metadata type we emit in the target assembly. So, to bypass all visibility checks this needs to be public.

andrewlock · 2022-09-01T13:53:02Z

-				scope.Report(2814874321813540uL);
+				counters[7]++;
 				if (i >= 100)
 				{
 					break;
 				}
-				scope.Report(3096280579112974uL, 3377772735758374uL);
+				counters[3]++;
+				counters[4]++;
 				Console.WriteLine(i);
-				scope.Report(3659230532665358uL, 2814912976519209uL);
+				counters[5]++;
+				counters[6]++;


counters[7] is at the top, is that right? 🤔

Yup, awesome right!? this is the IL -> C# converter fault, the original is a for loop and the comparison is done after each loop that's why the debugging (sequencePoint) offset has this value. And when we try to get the C# code from that rewritten IL, the tool is unable to detect the for loop anymore.

andrewlock · 2022-09-01T13:53:46Z

+		public ModuleCoverage()
+		{
+			Metadata = new int[2][];
+			Metadata[0] = new int[0];


I guess this could use Array.Empty<int>()?

Sure, now I have to emit it in IL :D

done in 99e3387

… tony/coverage-improvements

OmerRaviv

Marking as approved without actually reviewing just to unblock you (since CODEOWNERs definitions are tricky ,;) )

andrewlock · 2022-09-01T16:54:08Z

Benchmarks Report 🐌

Benchmarks for #3134 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	Allocated
master	`WriteAndFlushEnrichedTraces`	net472	706μs	284ns	1.1μs	0.355	3.18 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	461μs	245ns	950ns	0	2.58 KB
#3134	`WriteAndFlushEnrichedTraces`	net472	707μs	447ns	1.73μs	0.355	3.18 KB
#3134	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	458μs	187ns	725ns	0	2.58 KB

Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	186ns	0.247ns	0.856ns	0.0675	0	425 B
master	`AllCycleSimpleBody`	netcoreapp3.1	238ns	0.307ns	1.19ns	0.00585	0	424 B
master	`AllCycleMoreComplexBody`	net472	189ns	0.158ns	0.59ns	0.0637	0	401 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	234ns	0.178ns	0.666ns	0.00545	0	400 B
master	`BodyExtractorSimpleBody`	net472	274ns	0.253ns	0.981ns	0.0574	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	221ns	0.312ns	1.21ns	0.00372	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	14.2μs	10.8ns	41.6ns	1.2	0.0211	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.1μs	18.7ns	70ns	0.0911	0	6.75 KB
#3134	`AllCycleSimpleBody`	net472	183ns	0.191ns	0.716ns	0.0675	0	425 B
#3134	`AllCycleSimpleBody`	netcoreapp3.1	238ns	0.271ns	0.94ns	0.00576	0	424 B
#3134	`AllCycleMoreComplexBody`	net472	184ns	0.139ns	0.519ns	0.0637	0	401 B
#3134	`AllCycleMoreComplexBody`	netcoreapp3.1	231ns	0.127ns	0.491ns	0.00548	0	400 B
#3134	`BodyExtractorSimpleBody`	net472	264ns	0.674ns	2.52ns	0.0573	0	361 B
#3134	`BodyExtractorSimpleBody`	netcoreapp3.1	222ns	0.367ns	1.42ns	0.00373	0	272 B
#3134	`BodyExtractorMoreComplexBody`	net472	14.4μs	70.4ns	282ns	1.21	0.0213	7.62 KB
#3134	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.4μs	13.3ns	51.4ns	0.0922	0	6.75 KB

Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
master	`SendRequest`	netcoreapp3.1	186μs	236ns	885ns	0.185	20.57 KB
#3134	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3134	`SendRequest`	netcoreapp3.1	183μs	218ns	846ns	0.182	20.57 KB

Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`ExecuteNonQuery`	net472	1.83μs	0.466ns	1.75ns	0.15	0.00091	947 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.46μs	3.32ns	12.9ns	0.0124	0	936 B
#3134	`ExecuteNonQuery`	net472	1.87μs	0.558ns	2.09ns	0.15	0.00093	947 B
#3134	`ExecuteNonQuery`	netcoreapp3.1	1.45μs	0.448ns	1.68ns	0.013	0	936 B

Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.52μs	0.779ns	2.91ns	0.183	1.16 KB
master	`CallElasticsearch`	netcoreapp3.1	1.63μs	4.34ns	16.8ns	0.0144	1.1 KB
master	`CallElasticsearchAsync`	net472	2.78μs	0.73ns	2.63ns	0.204	1.29 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.66μs	0.616ns	2.22ns	0.0166	1.22 KB
#3134	`CallElasticsearch`	net472	2.45μs	0.775ns	2.9ns	0.183	1.16 KB
#3134	`CallElasticsearch`	netcoreapp3.1	1.58μs	0.568ns	2.2ns	0.0143	1.1 KB
#3134	`CallElasticsearchAsync`	net472	2.63μs	1.12ns	4.34ns	0.205	1.29 KB
#3134	`CallElasticsearchAsync`	netcoreapp3.1	1.58μs	0.586ns	2.19ns	0.0166	1.22 KB

Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	2.7μs	6.42ns	24.9ns	0.224	1.41 KB
master	`ExecuteAsync`	netcoreapp3.1	1.72μs	4.35ns	16.8ns	0.0178	1.34 KB
#3134	`ExecuteAsync`	net472	2.65μs	4.24ns	16.4ns	0.223	1.41 KB
#3134	`ExecuteAsync`	netcoreapp3.1	1.76μs	3.06ns	11.8ns	0.0175	1.34 KB

Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	5.6μs	12.8ns	49.6ns	0.438	2.77 KB
master	`SendAsync`	netcoreapp3.1	3.51μs	8.23ns	30.8ns	0.0354	2.6 KB
#3134	`SendAsync`	net472	5.81μs	14.7ns	54.9ns	0.438	2.77 KB
#3134	`SendAsync`	netcoreapp3.1	3.66μs	8.02ns	31.1ns	0.0348	2.6 KB

Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.25μs	2.64ns	10.2ns	0.288	1.81 KB
master	`EnrichedLog`	netcoreapp3.1	2.59μs	7.73ns	29.9ns	0.0253	1.85 KB
#3134	`EnrichedLog`	net472	3.24μs	3.43ns	12.8ns	0.287	1.81 KB
#3134	`EnrichedLog`	netcoreapp3.1	2.65μs	2.13ns	8.24ns	0.0251	1.85 KB

Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	150μs	150ns	580ns	0.686	0.229	4.65 KB
master	`EnrichedLog`	netcoreapp3.1	116μs	263ns	1.02μs	0	0	4.49 KB
#3134	`EnrichedLog`	net472	153μs	248ns	959ns	0.692	0.231	4.65 KB
#3134	`EnrichedLog`	netcoreapp3.1	115μs	192ns	744ns	0.0574	0	4.49 KB

Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	5.6μs	12.9ns	48.3ns	0.568	0.00277	3.59 KB
master	`EnrichedLog`	netcoreapp3.1	4.48μs	11.2ns	41.8ns	0.0537	0	3.91 KB
#3134	`EnrichedLog`	net472	5.7μs	11ns	42.6ns	0.568	0.00285	3.59 KB
#3134	`EnrichedLog`	netcoreapp3.1	4.47μs	4.97ns	18.6ns	0.0516	0	3.91 KB

Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	2.34μs	2.78ns	10.8ns	0.217	1.37 KB
master	`SendReceive`	netcoreapp3.1	1.81μs	0.835ns	3.01ns	0.0181	1.32 KB
#3134	`SendReceive`	net472	2.34μs	3.58ns	13.9ns	0.218	1.37 KB
#3134	`SendReceive`	netcoreapp3.1	1.82μs	0.674ns	2.61ns	0.0182	1.32 KB

Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	5.08μs	1.81ns	7.03ns	0.354	2.23 KB
master	`EnrichedLog`	netcoreapp3.1	4.43μs	1.3ns	5.04ns	0.0245	1.8 KB
#3134	`EnrichedLog`	net472	4.98μs	2.82ns	10.6ns	0.352	2.23 KB
#3134	`EnrichedLog`	netcoreapp3.1	4.29μs	1.81ns	7ns	0.024	1.8 KB

Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	1.13μs	0.246ns	0.921ns	0.128	810 B
master	`StartFinishSpan`	netcoreapp3.1	923ns	0.166ns	0.623ns	0.0102	760 B
master	`StartFinishScope`	net472	1.39μs	0.484ns	1.74ns	0.141	891 B
master	`StartFinishScope`	netcoreapp3.1	1.13μs	0.385ns	1.44ns	0.0118	880 B
#3134	`StartFinishSpan`	net472	1.13μs	0.441ns	1.53ns	0.129	810 B
#3134	`StartFinishSpan`	netcoreapp3.1	927ns	0.465ns	1.74ns	0.0102	760 B
#3134	`StartFinishScope`	net472	1.37μs	1.22ns	4.74ns	0.141	891 B
#3134	`StartFinishScope`	netcoreapp3.1	1.08μs	0.654ns	2.45ns	0.0123	880 B

Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.5μs	0.712ns	2.76ns	0.141	891 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.22μs	1.06ns	4.09ns	0.0121	880 B
#3134	`RunOnMethodBegin`	net472	1.54μs	2.96ns	11.5ns	0.141	891 B
#3134	`RunOnMethodBegin`	netcoreapp3.1	1.15μs	0.506ns	1.89ns	0.0117	880 B

zacharycmontoya

LGTM, nice improvement!

andrewlock · 2022-09-01T17:39:17Z

Code Coverage Report 📊

✔️ Merging #3134 into master will not change line coverage
✔️ Merging #3134 into master will not change branch coverage
⛔ Merging #3134 into master will will increase complexity by 56

	master	#3134	Change
Lines	`17420` / `23769`	`17492` / `23816`
Lines %	`73%`	`73%`	`0%` ✔️
Branches	`10309` / `14621`	`10350` / `14675`
Branches %	`71%`	`71%`	`0%` ✔️
Complexity	`15784`	`15840`	`56` ⛔

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3134	Change
Lines %	`73%`	`73%`	`0%` ✔️
Branches %	`71%`	`71%`	`0%` ✔️
Complexity	`15784`	`15840`	`56` ⛔

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Telemetry.Transports.JsonTelemetryTransport	`-9%` ⛔	`-17%` ⛔	`0` ✔️
Datadog.Trace.Ci.Coverage.DefaultCoverageEventHandler	`0%` ✔️	`-30%` ⛔	`33` ⛔
Datadog.Trace.Ci.CIVisibility	`5%` ✔️	`6%` ✔️	`0` ✔️
Datadog.Trace.Agent.Transports.SocketHandlerRequestFactory	`7%` ✔️	`0%` ✔️	`0` ✔️
Datadog.Trace.Ci.Coverage.CoverageContextContainer	`17%` ✔️	`-75%` ⛔	`3` ⛔

The following classes were added in #3134:

File	Line coverage	Branch coverage	Complexity
Datadog.Trace.Ci.Coverage.CoverageReporter`1	`0%`	`0%`	`13`
Datadog.Trace.Ci.Coverage.Metadata.ModuleCoverageMetadata	`0%`	`100%`	`4`
Datadog.Trace.Ci.Coverage.MethodValues	`0%`	`0%`	`2`
Datadog.Trace.Ci.Coverage.ModuleValue	`0%`	`0%`	`2`
Datadog.Trace.Ci.Coverage.TypeValues	`0%`	`0%`	`2`

2 classes were removed from Datadog.Trace in #3134

View the full reports for further details:

tonyredondo added 2 commits August 23, 2022 14:54

Remove Backup process.

15aface

Refactor and changes of the Coverage algorithm

a089c6a

tonyredondo self-assigned this Aug 29, 2022

Merge branch 'master' into tony/coverage-improvements

99732b7

github-actions Bot added the area:ci-visibility label Aug 29, 2022