Try to head off future build issues#5770
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 298881 Passed, 1587 Skipped, 12h 23m 48.78s Total Time |
22e90fc to
0a59c1e
Compare
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:
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). |
Benchmarks Report for tracer 🐌Benchmarks for #5770 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.ActivityBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.TraceAnnotationsBenchmark - Faster 🎉 Same allocations ✔️
|
| Benchmark | base/diff | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0 | 1.115 | 677.94 | 608.21 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | RunOnMethodBegin |
net6.0 | 678ns | 0.279ns | 1.08ns | 0.00987 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 945ns | 0.656ns | 2.54ns | 0.00948 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
net472 | 1.11μs | 0.407ns | 1.58ns | 0.104 | 0 | 0 | 658 B |
| #5770 | RunOnMethodBegin |
net6.0 | 608ns | 0.408ns | 1.58ns | 0.00985 | 0 | 0 | 696 B |
| #5770 | RunOnMethodBegin |
netcoreapp3.1 | 940ns | 0.353ns | 1.27ns | 0.00944 | 0 | 0 | 696 B |
| #5770 | RunOnMethodBegin |
net472 | 1.09μs | 1.66ns | 6.44ns | 0.104 | 0 | 0 | 658 B |
Throughput/Crank Report ⚡Throughput results for AspNetCoreSimpleController comparing the following branches/commits: Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red. Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards! gantt
title Throughput Linux x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5770) (11.706M) : 0, 11705613
master (11.924M) : 0, 11924044
benchmarks/2.9.0 (11.883M) : 0, 11882814
section Automatic
This PR (5770) (7.950M) : 0, 7950178
master (7.828M) : 0, 7827944
benchmarks/2.9.0 (8.446M) : 0, 8445613
section Trace stats
master (8.153M) : 0, 8153092
section Manual
master (11.617M) : 0, 11616636
section Manual + Automatic
This PR (5770) (7.256M) : 0, 7256160
master (7.365M) : 0, 7364918
section DD_TRACE_ENABLED=0
master (10.799M) : 0, 10798925
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5770) (9.438M) : 0, 9438129
master (9.551M) : 0, 9551120
benchmarks/2.9.0 (9.760M) : 0, 9759697
section Automatic
This PR (5770) (6.588M) : 0, 6587606
master (6.610M) : 0, 6609773
section Trace stats
master (6.890M) : 0, 6889536
section Manual
master (9.385M) : 0, 9384897
section Manual + Automatic
This PR (5770) (6.197M) : 0, 6196862
master (6.104M) : 0, 6103569
section DD_TRACE_ENABLED=0
master (8.918M) : 0, 8917738
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5770) (9.999M) : 0, 9998839
section Automatic
This PR (5770) (6.705M) : 0, 6705428
section Manual + Automatic
This PR (5770) (6.291M) : 0, 6291119
|
|
When this is merged, we should also bump to the latest .NET 8 build version as it likely includes fixes we need |
0a59c1e to
fe1376b
Compare
fe1376b to
9001630
Compare
## Summary of changes Replaces the ruby tool `fpm` with `nfpm` and `tar` ## Reason for change `fpm` is a ruby tool, which means we have to install ruby. This has historically caused a bunch of issues at various points when dependencies change (e.g. we add/update anything or change dockerfiles). [`nfpm` is a go tool](https://github.com/goreleaser/nfpm) created specifically to do pretty much the same thing as `fpm`, but without requiring ruby. The only slightly annoying thing is that it doesn't support raw "tar" so we have to use the built-in `tar` for that (although that's not a big hardship). ## Implementation details - Install `nfpm` in the docker images and remove the `fpm` install (and removing all the other ruby-related code) - Call `tar` to pack the `tar` images, and `nfpm` to pack the `deb/rpm` packages - `nfpm` is a "config" based tool, so we dump the yaml config to a file, and pass that to the execution - I have validated the nfpm and fpm deb output are essentially identical, there are just a few nuances - `fpm` adds a `License` field to the `.deb` header, but that's non standard. [The correct approach](https://www.debian.org/doc/debian-policy/ch-docs.html#copyright-information) is to include a specific license file. See [this issue](goreleaser/nfpm#847) for more details. - Similarly, `fpm` adds a `Vendor` field that is non standard (but `Maintainer` is standard and is added). - `fpm` adds `Relocations: /var/datadog` to the `.rpm` header, so I have replicated that for nfpm, but I don't know that we actually _want_ that... If we let people relocate the headers, then the documentation is wrong for the profiler env vars etc. Left it as-is anyway, to reduce chance of breaking people, just something I spotted. - The `"Apache License 2.0"` license string used for `.rpm` previously is not a valid value, as defined by fedora, so switched it to the supported moniker [here](https://docs.fedoraproject.org/en-US/legal/allowed-licenses/). - The `tar` files were "accidentally" including the before/after scripts, so I've made sure to still include them, incase anyone was relying on those (they run the `createLogPath.sh` file, and create a symlink to dd-dotnet in `/usr/bin`) ## Test coverage [Ran a full test with all installers here](https://dev.azure.com/datadoghq/dd-trace-dotnet/_build/results?buildId=162539&view=results) - ignore the couple of failures in the dotnet tool smoke tests, those are failures on `master` I need to fix 😅 Will manually compare the output of this run too: - [x] tar (`tar -tzvf *.tar.gz`) - The same, except the before/after scripts weren't being made executable. Fixed - [x] deb (`dpkg --info *.deb` and `dpkg -c *.deb`) - Minor differences: - nfpm adds the directories as entries - nfpm doesn't have the non-standard License field (unlike fpm) - nfpm adds the license in the required copyright path - nfpm doesn't include a dummy changelog.gz file (unlike fpm) - [x] rpm (`rpm -qipl --scripts *.rpm`) - Minor differences, nfpm adds the directories as entries, nothing significant though ## Other details Stacked on - #5770 because we need to change the dockerfiles, and we need the Centos7 fixes in there at the very least. We'll need to update the VMSS images though after making these changes so I've been delaying until that makes sense up to this point.
## Summary of changes Adds support for the tracer on alpine on ARM64 ## Reason for change We currently support x64 on glibc/alpine but only glibc on arm64. This closes that matrix which makes it easier to understand ## Implementation details There's several layers, but at a high level - Anywhere where we detect platform, make sure to include `linux-musl-arm64` as well as `linux-arm64` - Build the tracer + profiler on alpine arm64 There were several difficulties - The ruby-based fpm bundler we use doesn't work on alpine arm64, hence #5905 - I couldn't get our existing alpine.build.dockerfile to compile on arm64 (where we currently compile llvm/clang16 on alpine14). - To work around it, created a separate alpine.build.arm64.dockerfile image which uses alpine:18 (the first version that includes clang16) as the base. - Obviously this gives us different requirements in arm64, but I think that's fine because a) this is a new feature b) we already have differences on the glibc requirements for arm64 c) We only support .NET 5+ anyway on arm64 (.NET 6 really given .NET 5 is mostly gone) and they ship alpine:18 images for .NET 6+ - I created a multi arch base image from the two base images ([as some bloke with a blog describes here](https://andrewlock.net/combining-multiple-docker-images-into-a-multi-arch-image/)) - I had difficulty compiling the profiler on arm64 initially (we require it for some crash tracking functionality), as we were getting `undefined reference to `_Uaarch64_get_accessors_int'` when trying to compile libunwind. @gleocadie tracked it down to [this issue](libunwind/libunwind#788), and fixed it by just not building the libunwind tests 🙇♂️ - The sqlite IAST instrumentation tests on alpine arm64 were failing in .NET 5. I _think_ that's because the sqlite package we're using there doesn't support it (which makes sense) so just disabled the tests for that scenario. They pass in .NET 6+ so I'm not worried about it. - Removed the "noop" smoke tests stage (there's nothing to run in it currently) - We can't even install .NET Core 2.1 using the dotnet install script on alpine arm64, so bailing out. We can probably bail out of more too, but that's not _necessary_ so haven't done it yet. There's still one outstanding issue I can't get my head around, on _debian x64_ (i.e. not something that should have changed) The `OnEolFrameworkInSsi_WhenForwarderPathExists_CallsForwarderWithExpectedTelemetry` tests is failing by crashing. It's bizarre, I can't figure it out, if anyone has any ideas, please let me know 😅 ## Test coverage I've run full installer + full TFM tests [here](https://dev.azure.com/datadoghq/dd-trace-dotnet/_build/results?buildId=162869&view=results) 🤞 ## Other details Stacked on - #5770 - #5905 as they both change key parts of the build + will mean we need to rebuild the VMs Fixes #3850 --------- Co-authored-by: Gregory LEOCADIE <[email protected]>
## Summary of changes - Vendor the `dotnet-install.sh` and `dotnet-install.ps1` scripts into the repo - Replace the centos7 repo references with vault based ones ## Reason for change - https://dotnet.microsoft.com/ went down recently, which meant we couldn't download `dotnet-install.sh` or `dotnet-install.ps1`, which broke a bunch of things, so vendor it. - Centos7 recently shut down their repo feed, which means you can no longer pull packages. Use vault repo instead until we deprecate centos7 entirely ## Implementation details - Vendor the scripts - Replace downloading of the script with direct reference - do some `sed` to replace the centos7 repo ## Test coverage Largely, this is the test, if it all works, I think we're good ## Other details Supersedes #5759 Requires updating the VMs <!--⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. -->
## Summary of changes Replaces the ruby tool `fpm` with `nfpm` and `tar` ## Reason for change `fpm` is a ruby tool, which means we have to install ruby. This has historically caused a bunch of issues at various points when dependencies change (e.g. we add/update anything or change dockerfiles). [`nfpm` is a go tool](https://github.com/goreleaser/nfpm) created specifically to do pretty much the same thing as `fpm`, but without requiring ruby. The only slightly annoying thing is that it doesn't support raw "tar" so we have to use the built-in `tar` for that (although that's not a big hardship). ## Implementation details - Install `nfpm` in the docker images and remove the `fpm` install (and removing all the other ruby-related code) - Call `tar` to pack the `tar` images, and `nfpm` to pack the `deb/rpm` packages - `nfpm` is a "config" based tool, so we dump the yaml config to a file, and pass that to the execution - I have validated the nfpm and fpm deb output are essentially identical, there are just a few nuances - `fpm` adds a `License` field to the `.deb` header, but that's non standard. [The correct approach](https://www.debian.org/doc/debian-policy/ch-docs.html#copyright-information) is to include a specific license file. See [this issue](goreleaser/nfpm#847) for more details. - Similarly, `fpm` adds a `Vendor` field that is non standard (but `Maintainer` is standard and is added). - `fpm` adds `Relocations: /var/datadog` to the `.rpm` header, so I have replicated that for nfpm, but I don't know that we actually _want_ that... If we let people relocate the headers, then the documentation is wrong for the profiler env vars etc. Left it as-is anyway, to reduce chance of breaking people, just something I spotted. - The `"Apache License 2.0"` license string used for `.rpm` previously is not a valid value, as defined by fedora, so switched it to the supported moniker [here](https://docs.fedoraproject.org/en-US/legal/allowed-licenses/). - The `tar` files were "accidentally" including the before/after scripts, so I've made sure to still include them, incase anyone was relying on those (they run the `createLogPath.sh` file, and create a symlink to dd-dotnet in `/usr/bin`) ## Test coverage [Ran a full test with all installers here](https://dev.azure.com/datadoghq/dd-trace-dotnet/_build/results?buildId=162539&view=results) - ignore the couple of failures in the dotnet tool smoke tests, those are failures on `master` I need to fix 😅 Will manually compare the output of this run too: - [x] tar (`tar -tzvf *.tar.gz`) - The same, except the before/after scripts weren't being made executable. Fixed - [x] deb (`dpkg --info *.deb` and `dpkg -c *.deb`) - Minor differences: - nfpm adds the directories as entries - nfpm doesn't have the non-standard License field (unlike fpm) - nfpm adds the license in the required copyright path - nfpm doesn't include a dummy changelog.gz file (unlike fpm) - [x] rpm (`rpm -qipl --scripts *.rpm`) - Minor differences, nfpm adds the directories as entries, nothing significant though ## Other details Stacked on - #5770 because we need to change the dockerfiles, and we need the Centos7 fixes in there at the very least. We'll need to update the VMSS images though after making these changes so I've been delaying until that makes sense up to this point.
## Summary of changes The `release/2.x` branch is broken since I updated the VMs ## Reason for change I updated VMs yesterday, which cleared the build cache. I knew that would slow down release/2.x builds but I forgot it would completely break them due to the centos7 mess. ## Implementation details Backported some PRs to fix the layer caching etc - #5770 - #5905 - #5933 (just the changes in the alpine dockerfile) ## Test coverage This is the test, hopefully it works 🤞
Summary of changes
dotnet-install.shanddotnet-install.ps1scripts into the repoReason for change
dotnet-install.shordotnet-install.ps1, which broke a bunch of things, so vendor it.Implementation details
sedto replace the centos7 repoTest coverage
Largely, this is the test, if it all works, I think we're good
Other details
Supersedes #5759
Requires updating the VMs