Fix all build warnings and errors by andrewlock · Pull Request #3324 · DataDog/dd-trace-dotnet

andrewlock · 2022-10-07T14:10:57Z

Summary of changes

Fixes all benign build warnings and errors in CI 🎉:aww_yeah:🎉

Reason for change

Currently, this is a successful build:

After the change, just a lot of green, no more errors or warnings

Implementation details

There were several changes required:

Stop the native build from writing to stderr every time

ExternalProject_Add doesn't add the --quiet flag, which means git clone writes to stderr. In the macos build, this is interpreted by Azure DevOps as a build error. That's annoying. Instead, do the git clone manually so we can specify --quiet, and just use ExternalProject_Add to work with the pre-cloned repo. Also added --depth 1 for good measure.

Annoying, ARM64 apparently didn't like this change, so using the "noisy" version in that case. It's not actually noisy, because it's in a container, so doesn't matter too much, but it does bug me why it's different when we are running the same code in the same container 😬

Add missing newline to publish test results step in native unit tests

Otherwise this was interpreted as a single pattern: No test result files matching tracer/test/**/test*.xml profiler/_build/bin/**/test*.xml were found.

Fix platform compatibility checks in `Process.Start` sample

In .NET 5 and .NET 6, the compiler verifies that code works on all platforms, if not, we get warnings:

D:\a\1\s\tracer\test\test-applications\integrations\Samples.ProcessStart\Program.cs(28,17): warning CA1416: This call site is reachable on all platforms. 'Process.Start(string, string, string, SecureString, string)' is supported on: 'windows'.

Workaround it by adding Debug.Assert(OperatingSystem.IsWindows());, which apparently works, even in Release mode.

Handle multi-line log files in the profiler logs

The profiler outputs various "multi-line" logs. The CheckLogsForErrors step was using a more primitive version which was adding a warning for all of the subsequent lines.

Workaround AzureDevops complaining about .NET Core 2.1

The UseDotNet@2 task (inexplicably IMO) writes a warning every time you install an out-of-date .NET core version that is in their cache:

NET Core version you specfied 2.1.x is out of support and will be removed from hosted agents soon. Please refer to https://aka.ms/dotnet-core-support for more information about the .NET support policy.

Note that this isn't saying "you won't be able to install .NET Core 2.1 soon".

To workaround it, install using the dotnet-install.ps1 script instead (which we already do for 32 bit anyway). As part of this, renamed the install-dotnet-sdk-32bit file to install-dotnet-sdk-manually, removed some of the duplication, and update to support both x86 and x64

Test coverage

The proof of the pudding is in the eating.

Other details

Supersedes #3261

zacharycmontoya

Looks great! You're a true watchman of our repo

andrewlock · 2022-10-07T17:39:51Z

Code Coverage Report 📊

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

	master	#3324	Change
Lines	`19233` / `26585`	`19269` / `26585`
Lines %	`72%`	`72%`	`0%` ✔️
Branches	`11419` / `16497`	`11435` / `16497`
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`17816`	`17816`	`0` ✔️

View the full report for further details:

Datadog.Trace Breakdown ✔️

	master	#3324	Change
Lines %	`72%`	`72%`	`0%` ✔️
Branches %	`69%`	`69%`	`0%` ✔️
Complexity	`17816`	`17816`	`0` ✔️

The following classes have significant coverage changes.

File	Line coverage change	Branch coverage change	Complexity change
Datadog.Trace.Debugger.PInvoke.DebuggerNativeMethods	`6%` ✔️	`0%` ✔️	`0` ✔️
Datadog.Trace.ClrProfiler.AutoInstrumentation.Http.HttpClient.WinHttpHandler.WinHttpHandlerIntegration	`100%` ✔️	`0%` ✔️	`0` ✔️

View the full reports for further details:

gleocadie · 2022-10-08T10:49:06Z

+set(fmt_GIT_REPO https://github.com/DataDog/fmt.git)
+set(fmt_GIT_TAG 5.3.0)
+if (ISARM64)
+    ExternalProject_Add(fmt
+        GIT_REPOSITORY "${fmt_GIT_REPO}"
+        GIT_TAG "${fmt_GIT_TAG}"
        GIT_CONFIG advice.detachedHead=false
        TIMEOUT 5
        CMAKE_ARGS -DCMAKE_POSITION_INDEPENDENT_CODE=TRUE -DFMT_TEST=0 -DFMT_DOC=0
        PREFIX "${EXTERNAL_INSTALL_LOCATION}"
        INSTALL_COMMAND ""
        )
+else ()
+    # This doesn't seem to work on arm64, but I'm not sure why :shrug:
+    execute_process(
+            COMMAND git clone --quiet --depth 1 --branch "${fmt_GIT_TAG}" --config advice.detachedHead=false "${fmt_GIT_REPO}"
+            WORKING_DIRECTORY "${OUTPUT_DEPS_DIR}"
+            TIMEOUT 5
+            )
+    ExternalProject_Add(fmt
+            CMAKE_ARGS -DCMAKE_POSITION_INDEPENDENT_CODE=TRUE -DFMT_TEST=0 -DFMT_DOC=0
+            SOURCE_DIR "${EXTERNAL_INSTALL_LOCATION}"
+            PREFIX "${EXTERNAL_INSTALL_LOCATION}"
+            INSTALL_COMMAND ""
+            )
+endif()


same comment as #3261 (comment)

a bit too complex.
you could try this instead

ExternalProject_Add(fmt DOWNLOAD_COMMAND git clone --quiet --depth 1 --branch "${fmt_GIT_TAG}" --config advice.detachedHead=false "${fmt_GIT_REPO}" TIMEOUT 5 CMAKE_ARGS -DCMAKE_POSITION_INDEPENDENT_CODE=TRUE -DFMT_TEST=0 -DFMT_DOC=0 . SOURCE_DIR "${EXTERNAL_INSTALL_LOCATION}" PREFIX "${EXTERNAL_INSTALL_LOCATION}" INSTALL_COMMAND "" )

…et core 2.1 Generalized and simplified(?) the install 32 bit sdk step so that it works with 64 bit too, and has less duplication

Co-authored-by: Zach Montoya <[email protected]>

andrewlock · 2022-10-10T10:26:03Z

Benchmarks Report 🐌

Benchmarks for #3324 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	788μs	672ns	2.6μs	0.393	3.22 KB
master	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	500μs	156ns	582ns	0	2.62 KB
#3324	`WriteAndFlushEnrichedTraces`	net472	787μs	548ns	2.12μs	0.393	3.22 KB
#3324	`WriteAndFlushEnrichedTraces`	netcoreapp3.1	527μs	179ns	647ns	0	2.63 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`AllCycleSimpleBody`	net472	205ns	0.425ns	1.65ns	0.0727	0	457 B
master	`AllCycleSimpleBody`	netcoreapp3.1	253ns	0.11ns	0.411ns	0.00624	0	456 B
master	`AllCycleMoreComplexBody`	net472	193ns	0.0554ns	0.192ns	0.0688	0	433 B
master	`AllCycleMoreComplexBody`	netcoreapp3.1	257ns	0.173ns	0.648ns	0.00592	0	432 B
master	`BodyExtractorSimpleBody`	net472	276ns	0.238ns	0.922ns	0.0574	0	361 B
master	`BodyExtractorSimpleBody`	netcoreapp3.1	236ns	0.122ns	0.441ns	0.00379	0	272 B
master	`BodyExtractorMoreComplexBody`	net472	15.7μs	10.5ns	40.6ns	1.2	0.0155	7.62 KB
master	`BodyExtractorMoreComplexBody`	netcoreapp3.1	13.6μs	3.52ns	13.2ns	0.0886	0	6.75 KB
#3324	`AllCycleSimpleBody`	net472	198ns	0.064ns	0.248ns	0.0727	0	457 B
#3324	`AllCycleSimpleBody`	netcoreapp3.1	253ns	0.073ns	0.273ns	0.00634	0	456 B
#3324	`AllCycleMoreComplexBody`	net472	206ns	0.128ns	0.48ns	0.0688	0	433 B
#3324	`AllCycleMoreComplexBody`	netcoreapp3.1	254ns	0.214ns	0.802ns	0.00592	0	432 B
#3324	`BodyExtractorSimpleBody`	net472	276ns	0.995ns	3.85ns	0.0573	0	361 B
#3324	`BodyExtractorSimpleBody`	netcoreapp3.1	233ns	0.12ns	0.448ns	0.00373	0	272 B
#3324	`BodyExtractorMoreComplexBody`	net472	15.5μs	7.53ns	29.1ns	1.21	0.0155	7.62 KB
#3324	`BodyExtractorMoreComplexBody`	netcoreapp3.1	12.9μs	3.63ns	14.1ns	0.0903	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	179μs	165ns	638ns	0.267	20.68 KB
#3324	`SendRequest`	net472	0ns	0ns	0ns	0	0 b
#3324	`SendRequest`	netcoreapp3.1	180μs	154ns	597ns	0.269	20.68 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.86μs	0.995ns	3.85ns	0.155	0.000934	979 B
master	`ExecuteNonQuery`	netcoreapp3.1	1.47μs	0.942ns	3.4ns	0.0132	0	968 B
#3324	`ExecuteNonQuery`	net472	1.86μs	0.94ns	3.64ns	0.155	0.000935	979 B
#3324	`ExecuteNonQuery`	netcoreapp3.1	1.47μs	0.682ns	2.64ns	0.0132	0	968 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`CallElasticsearch`	net472	2.46μs	1.13ns	4.4ns	0.188	1.19 KB
master	`CallElasticsearch`	netcoreapp3.1	1.58μs	1.14ns	4.42ns	0.015	1.13 KB
master	`CallElasticsearchAsync`	net472	2.67μs	0.842ns	3.03ns	0.21	1.32 KB
master	`CallElasticsearchAsync`	netcoreapp3.1	1.64μs	1.93ns	6.97ns	0.0164	1.25 KB
#3324	`CallElasticsearch`	net472	2.53μs	0.709ns	2.56ns	0.189	1.19 KB
#3324	`CallElasticsearch`	netcoreapp3.1	1.59μs	0.79ns	2.95ns	0.0151	1.13 KB
#3324	`CallElasticsearchAsync`	net472	2.64μs	1.11ns	4.32ns	0.21	1.32 KB
#3324	`CallElasticsearchAsync`	netcoreapp3.1	1.62μs	0.514ns	1.85ns	0.0163	1.25 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`ExecuteAsync`	net472	2.73μs	0.935ns	3.62ns	0.231	1.45 KB
master	`ExecuteAsync`	netcoreapp3.1	1.79μs	1.88ns	7.05ns	0.0187	1.38 KB
#3324	`ExecuteAsync`	net472	2.84μs	0.758ns	2.94ns	0.23	1.45 KB
#3324	`ExecuteAsync`	netcoreapp3.1	1.78μs	0.616ns	2.3ns	0.0187	1.38 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendAsync`	net472	5.92μs	1.7ns	6.37ns	0.444	2.8 KB
master	`SendAsync`	netcoreapp3.1	3.67μs	1.81ns	6.52ns	0.0348	2.63 KB
#3324	`SendAsync`	net472	5.93μs	2.21ns	8.55ns	0.444	2.8 KB
#3324	`SendAsync`	netcoreapp3.1	3.7μs	1.51ns	5.45ns	0.0347	2.63 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	3.07μs	1.96ns	7.58ns	0.298	1.88 KB
master	`EnrichedLog`	netcoreapp3.1	2.46μs	1.27ns	4.91ns	0.0259	1.91 KB
#3324	`EnrichedLog`	net472	3.22μs	0.984ns	3.55ns	0.298	1.88 KB
#3324	`EnrichedLog`	netcoreapp3.1	2.55μs	0.757ns	2.93ns	0.0254	1.91 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Gen 1	Allocated
master	`EnrichedLog`	net472	152μs	105ns	406ns	0.682	0.227	4.72 KB
master	`EnrichedLog`	netcoreapp3.1	118μs	259ns	1μs	0.0591	0	4.55 KB
#3324	`EnrichedLog`	net472	152μs	147ns	570ns	0.688	0.229	4.72 KB
#3324	`EnrichedLog`	netcoreapp3.1	118μs	226ns	877ns	0	0	4.55 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.87μs	1.56ns	5.84ns	0.577	0.00295	3.65 KB
master	`EnrichedLog`	netcoreapp3.1	4.35μs	1.55ns	5.99ns	0.0545	0	3.98 KB
#3324	`EnrichedLog`	net472	5.81μs	1.16ns	4.34ns	0.578	0.0029	3.65 KB
#3324	`EnrichedLog`	netcoreapp3.1	4.38μs	1.24ns	4.49ns	0.0549	0	3.98 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`SendReceive`	net472	2.32μs	1.27ns	4.4ns	0.222	1.4 KB
master	`SendReceive`	netcoreapp3.1	1.74μs	0.409ns	1.47ns	0.0183	1.35 KB
#3324	`SendReceive`	net472	2.25μs	2ns	7.73ns	0.223	1.4 KB
#3324	`SendReceive`	netcoreapp3.1	1.83μs	0.433ns	1.68ns	0.0183	1.35 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`EnrichedLog`	net472	5.1μs	1.8ns	6.75ns	0.364	2.3 KB
master	`EnrichedLog`	netcoreapp3.1	4.48μs	3.52ns	13.2ns	0.0243	1.86 KB
#3324	`EnrichedLog`	net472	5.16μs	2.21ns	8.58ns	0.364	2.3 KB
#3324	`EnrichedLog`	netcoreapp3.1	4.38μs	6.88ns	25.7ns	0.0239	1.86 KB

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`StartFinishSpan`	net472	1.1μs	0.383ns	1.48ns	0.133	842 B
master	`StartFinishSpan`	netcoreapp3.1	987ns	0.263ns	0.949ns	0.0107	792 B
master	`StartFinishScope`	net472	1.42μs	0.361ns	1.3ns	0.147	923 B
master	`StartFinishScope`	netcoreapp3.1	1.12μs	0.522ns	1.95ns	0.0123	912 B
#3324	`StartFinishSpan`	net472	1.17μs	0.271ns	1.01ns	0.134	842 B
#3324	`StartFinishSpan`	netcoreapp3.1	915ns	0.21ns	0.726ns	0.0111	792 B
#3324	`StartFinishScope`	net472	1.44μs	2.42ns	9.39ns	0.146	923 B
#3324	`StartFinishScope`	netcoreapp3.1	1.09μs	0.613ns	2.29ns	0.0125	912 B

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

Raw results

Branch	Method	Toolchain	Mean	StdError	StdDev	Gen 0	Allocated
master	`RunOnMethodBegin`	net472	1.58μs	0.492ns	1.84ns	0.146	923 B
master	`RunOnMethodBegin`	netcoreapp3.1	1.28μs	0.553ns	2.07ns	0.0121	912 B
#3324	`RunOnMethodBegin`	net472	1.52μs	0.592ns	2.29ns	0.146	923 B
#3324	`RunOnMethodBegin`	netcoreapp3.1	1.18μs	0.329ns	1.27ns	0.0123	912 B

gleocadie

🎉 nice

andrewlock added the area:builds project files, build scripts, pipelines, versioning, releases, packages label Oct 7, 2022

andrewlock requested review from a team as code owners October 7, 2022 14:10

andrewlock force-pushed the andrew/fix-all-build-warnings branch from ba60a00 to abff712 Compare October 7, 2022 15:24

This comment has been minimized.

Sign in to view

zacharycmontoya reviewed Oct 7, 2022

View reviewed changes

Comment thread .azure-pipelines/steps/install-dotnet-sdk-manually.yml Outdated

zacharycmontoya approved these changes Oct 7, 2022

View reviewed changes

lucaspimentel approved these changes Oct 7, 2022

View reviewed changes

zacharycmontoya mentioned this pull request Oct 7, 2022

Stop the native build from writing to stderr every time #3261

Closed

gleocadie requested changes Oct 8, 2022

View reviewed changes

andrewlock and others added 8 commits October 10, 2022 09:32

Try stopping the native build from erroring every time

a3d0566

arm64 apparently doesn't like the new approach

80d3bda

Add missing newline to publish test results step

b7fa7a1

Fix platform compatibility warning in Process.Start sample

16b132b

Try working around the stupid build warning by manually installing .n…

80b69f5

…et core 2.1 Generalized and simplified(?) the install 32 bit sdk step so that it works with 64 bit too, and has less duplication

Handle multi-line logs in profiler logs, so we don't get warnings

72c8cfb

Update .azure-pipelines/steps/install-dotnet-sdk-manually.yml

423c260

Co-authored-by: Zach Montoya <[email protected]>

Try alternative clone code

42a25d0

andrewlock force-pushed the andrew/fix-all-build-warnings branch from 1a267ce to 42a25d0 Compare October 10, 2022 08:32

andrewlock added 3 commits October 10, 2022 09:59

Well, that didn't work

32b3fa2

Lets try this

b7a7c36

#dropthedot

404c299

gleocadie self-requested a review October 10, 2022 11:49

gleocadie approved these changes Oct 10, 2022

View reviewed changes

andrewlock merged commit 2e0dd24 into master Oct 11, 2022

andrewlock deleted the andrew/fix-all-build-warnings branch October 11, 2022 14:36

github-actions Bot added this to the vNext milestone Oct 11, 2022

andrewlock mentioned this pull request Oct 13, 2022

Fix dotnet install in build #3348

Merged

Uh oh!

Conversation

andrewlock commented Oct 7, 2022

Summary of changes

Reason for change

Implementation details

Stop the native build from writing to stderr every time

Add missing newline to publish test results step in native unit tests

Fix platform compatibility checks in Process.Start sample

Handle multi-line log files in the profiler logs

Workaround AzureDevops complaining about .NET Core 2.1

Test coverage

Other details

Uh oh!

This comment has been minimized.

Uh oh!

zacharycmontoya left a comment

Choose a reason for hiding this comment

Uh oh!

andrewlock commented Oct 7, 2022

Code Coverage Report 📊

Datadog.Trace Breakdown ✔️

Uh oh!

gleocadie Oct 8, 2022

Choose a reason for hiding this comment

Uh oh!

andrewlock commented Oct 10, 2022

Benchmarks Report 🐌

Benchmark details

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Raw results

Uh oh!

gleocadie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix platform compatibility checks in `Process.Start` sample