Fix v2 branch building #5969
Merged
Fix v2 branch building #5969
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## 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.
andrewlock
added
the
area:builds
kevingosse
approved these changes
Aug 30, 2024
Datadog ReportBranch report: ✅ 0 Failed, 347026 Passed, 1806 Skipped, 14h 25m 16.44s Total Time |
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). gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5969) - mean (73ms) : 64, 81
. : milestone, 73,
section CallTarget+Inlining+NGEN
This PR (5969) - mean (1,031ms) : 1007, 1055
. : milestone, 1031,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5969) - mean (110ms) : 106, 114
. : milestone, 110,
section CallTarget+Inlining+NGEN
This PR (5969) - mean (713ms) : 695, 730
. : milestone, 713,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5969) - mean (92ms) : 90, 95
. : milestone, 92,
section CallTarget+Inlining+NGEN
This PR (5969) - mean (669ms) : 645, 693
. : milestone, 669,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5969) - mean (190ms) : 187, 194
. : milestone, 190,
section CallTarget+Inlining+NGEN
This PR (5969) - mean (1,116ms) : 1096, 1136
. : milestone, 1116,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5969) - mean (277ms) : 272, 282
. : milestone, 277,
section CallTarget+Inlining+NGEN
This PR (5969) - mean (882ms) : 862, 903
. : milestone, 882,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5969) - mean (264ms) : 260, 268
. : milestone, 264,
section CallTarget+Inlining+NGEN
This PR (5969) - mean (861ms) : 843, 880
. : milestone, 861,
|
Benchmarks Report for tracer 🐌Benchmarks for #5969 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 - Slower
|
| Benchmark | diff/base | Base Median (ns) | Diff Median (ns) | Modality |
|---|---|---|---|---|
| Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin‑net6.0 | 1.123 | 582.23 | 654.05 |
Raw results
| Branch | Method | Toolchain | Mean | StdError | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| master | RunOnMethodBegin |
net6.0 | 582ns | 0.413ns | 1.6ns | 0.00987 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
netcoreapp3.1 | 899ns | 1.01ns | 3.77ns | 0.00949 | 0 | 0 | 696 B |
| master | RunOnMethodBegin |
net472 | 1.02μs | 1.54ns | 5.96ns | 0.104 | 0 | 0 | 658 B |
| #5969 | RunOnMethodBegin |
net6.0 | 654ns | 0.395ns | 1.53ns | 0.00961 | 0 | 0 | 696 B |
| #5969 | RunOnMethodBegin |
netcoreapp3.1 | 1μs | 1.16ns | 4.5ns | 0.00902 | 0 | 0 | 696 B |
| #5969 | RunOnMethodBegin |
net472 | 1.1μs | 0.947ns | 3.67ns | 0.104 | 0 | 0 | 658 B |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
The
release/2.xbranch is broken since I updated the VMsReason 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
Test coverage
This is the test, hopefully it works 🤞