[CI Visibility] Add support for coverlet.msbuild coverage reporting #6284

tonyredondo · 2024-11-14T11:37:58Z

Summary of changes

This PR add support to automatic reporting code coverage percentage from the coverlet.msbuild package.

Reason for change

We added support to automatic reporting from the coverlet datacollector but we were missing the automatic reporting from the coverlet msbuild task.

Implementation details

For some reason we are hitting an edge case in the DuckType library were Type.GetType returns null when using a partial type name and not a qualified one. I did a modification to the ducktype library to handle this edge case.

Test coverage

Works on my machine 🤷🏻 🤣 .

Now seriously... creating a test for this specific feature will require a refactor to the way we run/publish the samples apps. Currently we build a publish them in a different step to the artifacts folder and run them later in the test. coverlet.msbuild is not compatible with that because it contains a series of msbuild tasks that applies new targets to build the covered binary and then get the coverage values after the execution. Something that we cannot do right now.

Because this is a niche, best effort, and easy to test locally functionality. I decided to skip a custom test for this one. I'm evaluating on adding the test in the Test Visibility test-environment that it would be easier to do.

For the ducktyping change if this PR passes we are ok to go because a change of behaviour in the normal case is not expected.

Other details

datadog-ddstaging · 2024-11-14T12:21:26Z

Datadog Report

Branch report: tony/civisibility-support-for-coverlet-msbuild
Commit report: 60fe5cb
Test service: dd-trace-dotnet

✅ 0 Failed, 454629 Passed, 2757 Skipped, 19h 0m 39.07s Total Time

andrewlock

LGTM in general, just minor questions about potentially refactoring it...

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Testing/DotnetTest/DotnetCommon.cs

andrewlock · 2024-11-15T12:23:12Z

tracer/src/Datadog.Trace/DuckTyping/DuckType.Methods.cs

@@ -168,7 +168,7 @@ private static void CreateMethods(
                        DuckTypeTargetMethodNotFoundException.Throw(proxyMethodDefinition);
                    }

-                    targetMethod = targetMethod.MakeGenericMethod(proxyDuckAttribute.GenericParameterTypeNames.Select(name => Type.GetType(name, throwOnError: true)!).ToArray());
+                    targetMethod = targetMethod.MakeGenericMethod(proxyDuckAttribute.GenericParameterTypeNames.Select(name => GetTypeFromPartialName(name, throwOnError: true)!).ToArray());


Do we have to use it here? It complicates things because of the throwOnError...

not for my case but the same could happen with GenericParameterTypeNames also declared in the DuckAttribute.

yeah... we still don't really know why it's necessary 😂

tracer/src/Datadog.Trace/DuckTyping/DuckType.Statics.cs

andrewlock · 2024-11-15T12:28:54Z

tracer/src/Datadog.Trace/DuckTyping/DuckType.Methods.cs

@@ -383,7 +383,7 @@ private static void CreateReverseProxyMethods(TypeBuilder? proxyTypeBuilder, Typ
                }

                Type[] parameterTypes = proxyMethodDuckAttributeParameterTypeNames
-                                                                .Select(pName => Type.GetType(pName, throwOnError: false))


Actually, do we have a good reason we were using false here? 😟 It feels like we might actually be hiding strange issues which we won't know about in that case...

Yeah because that change can be really impactful, I'll try to do that in another PR, is a change of behavior of the SelectMethod

andrewlock

LGTM 👍

andrewlock · 2024-11-15T16:09:15Z

Execution-Time Benchmarks Report ⏱️

Execution-time results for samples comparing the following branches/commits:

Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:

Welch test with statistical test for significance of 5%
Only results indicating a difference greater than 5% and 5 ms are considered.

Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.

Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).

gantt
    title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6284) - mean (73ms)  : 61, 85
     .   : milestone, 73,
    master - mean (72ms)  : 63, 81
     .   : milestone, 72,

    section CallTarget+Inlining+NGEN
    This PR (6284) - mean (1,108ms)  : 1088, 1128
     .   : milestone, 1108,
    master - mean (1,107ms)  : 1082, 1132
     .   : milestone, 1107,

gantt
    title Execution time (ms) FakeDbCommand (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6284) - mean (108ms)  : 106, 110
     .   : milestone, 108,
    master - mean (108ms)  : 105, 111
     .   : milestone, 108,

    section CallTarget+Inlining+NGEN
    This PR (6284) - mean (763ms)  : 747, 778
     .   : milestone, 763,
    master - mean (770ms)  : 753, 786
     .   : milestone, 770,

gantt
    title Execution time (ms) FakeDbCommand (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6284) - mean (91ms)  : 89, 93
     .   : milestone, 91,
    master - mean (92ms)  : 90, 94
     .   : milestone, 92,

    section CallTarget+Inlining+NGEN
    This PR (6284) - mean (716ms)  : 701, 731
     .   : milestone, 716,
    master - mean (725ms)  : 709, 741
     .   : milestone, 725,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6284) - mean (190ms)  : 185, 195
     .   : milestone, 190,
    master - mean (191ms)  : 185, 196
     .   : milestone, 191,

    section CallTarget+Inlining+NGEN
    This PR (6284) - mean (1,211ms)  : 1180, 1242
     .   : milestone, 1211,
    master - mean (1,213ms)  : 1190, 1237
     .   : milestone, 1213,

gantt
    title Execution time (ms) HttpMessageHandler (.NET Core 3.1) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6284) - mean (275ms)  : 272, 279
     .   : milestone, 275,
    master - mean (276ms)  : 271, 280
     .   : milestone, 276,

    section CallTarget+Inlining+NGEN
    This PR (6284) - mean (939ms)  : 920, 958
     .   : milestone, 939,
    master - mean (945ms)  : 929, 960
     .   : milestone, 945,

gantt
    title Execution time (ms) HttpMessageHandler (.NET 6) 
    dateFormat  X
    axisFormat %s
    todayMarker off
    section Baseline
    This PR (6284) - mean (264ms)  : 261, 267
     .   : milestone, 264,
    master - mean (265ms)  : 260, 270
     .   : milestone, 265,

    section CallTarget+Inlining+NGEN
    This PR (6284) - mean (926ms)  : 908, 945
     .   : milestone, 926,
    master - mean (930ms)  : 907, 952
     .   : milestone, 930,

github-actions bot added the area:ci-visibility label Nov 14, 2024

tonyredondo marked this pull request as ready for review November 14, 2024 11:55

tonyredondo requested review from a team as code owners November 14, 2024 11:55

tonyredondo force-pushed the tony/civisibility-support-for-coverlet-msbuild branch from 1a45cbf to b261d56 Compare November 14, 2024 15:48

[CI Visibility] Add support for coverlet.msbuild coverage reporting

cbd97b2

tonyredondo force-pushed the tony/civisibility-support-for-coverlet-msbuild branch from b261d56 to cbd97b2 Compare November 14, 2024 22:28

DataDog deleted a comment from andrewlock Nov 14, 2024

andrewlock reviewed Nov 15, 2024

View reviewed changes

tonyredondo added 3 commits November 15, 2024 16:14

Addressing pr comments

5bcd7ae

remove unused using

8ffa972

remove blank line

60fe5cb

andrewlock approved these changes Nov 15, 2024

View reviewed changes

DataDog deleted a comment from andrewlock Nov 15, 2024

tonyredondo merged commit 43ad231 into master Nov 15, 2024
76 of 79 checks passed

tonyredondo deleted the tony/civisibility-support-for-coverlet-msbuild branch November 15, 2024 17:42

github-actions bot added this to the vNext-v3 milestone Nov 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CI Visibility] Add support for coverlet.msbuild coverage reporting #6284

[CI Visibility] Add support for coverlet.msbuild coverage reporting #6284

tonyredondo commented Nov 14, 2024 •

edited

Loading

datadog-ddstaging bot commented Nov 14, 2024 •

edited

Loading

andrewlock left a comment

andrewlock Nov 15, 2024

tonyredondo Nov 15, 2024

andrewlock Nov 15, 2024

andrewlock Nov 15, 2024

tonyredondo Nov 15, 2024

andrewlock left a comment

andrewlock commented Nov 15, 2024

[CI Visibility] Add support for coverlet.msbuild coverage reporting #6284

[CI Visibility] Add support for coverlet.msbuild coverage reporting #6284

Conversation

tonyredondo commented Nov 14, 2024 • edited Loading

Summary of changes

Reason for change

Implementation details

Test coverage

Other details

datadog-ddstaging bot commented Nov 14, 2024 • edited Loading

Datadog Report

andrewlock left a comment

Choose a reason for hiding this comment

andrewlock Nov 15, 2024

Choose a reason for hiding this comment

tonyredondo Nov 15, 2024

Choose a reason for hiding this comment

andrewlock Nov 15, 2024

Choose a reason for hiding this comment

andrewlock Nov 15, 2024

Choose a reason for hiding this comment

tonyredondo Nov 15, 2024

Choose a reason for hiding this comment

andrewlock left a comment

Choose a reason for hiding this comment

andrewlock commented Nov 15, 2024

Execution-Time Benchmarks Report ⏱️

tonyredondo commented Nov 14, 2024 •

edited

Loading

datadog-ddstaging bot commented Nov 14, 2024 •

edited

Loading