Allow generating a snapshot of the MSI contents#8270
Merged
Conversation
andrewlock
added
the
area:builds
BenchmarksBenchmark execution time: 2026-03-06 11:56:05 Comparing candidate commit 2b3dfb6 in PR branch Found 9 performance improvements and 6 performance regressions! Performance is the same for 162 metrics, 15 unstable metrics. scenario:Benchmarks.Trace.AgentWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.AspNetCoreBenchmark.SendRequest netcoreapp3.1
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces net6.0
scenario:Benchmarks.Trace.CIVisibilityProtocolWriterBenchmark.WriteAndFlushEnrichedTraces netcoreapp3.1
scenario:Benchmarks.Trace.CharSliceBenchmark.OptimizedCharSlice netcoreapp3.1
scenario:Benchmarks.Trace.ElasticsearchBenchmark.CallElasticsearchAsync net472
scenario:Benchmarks.Trace.GraphQLBenchmark.ExecuteAsync net472
scenario:Benchmarks.Trace.Iast.StringAspectsBenchmark.StringConcatAspectBenchmark netcoreapp3.1
scenario:Benchmarks.Trace.Log4netBenchmark.EnrichedLog netcoreapp3.1
scenario:Benchmarks.Trace.SerilogBenchmark.EnrichedLog netcoreapp3.1
scenario:Benchmarks.Trace.SingleSpanAspNetCoreBenchmark.SingleSpanAspNetCore net6.0
scenario:Benchmarks.Trace.SingleSpanAspNetCoreBenchmark.SingleSpanAspNetCore netcoreapp3.1
scenario:Benchmarks.Trace.SpanBenchmark.StartFinishSpan net6.0
scenario:Benchmarks.Trace.TraceAnnotationsBenchmark.RunOnMethodBegin net6.0
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8270) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-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 highlighted 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). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8270) - mean (75ms) : 72, 78
master - mean (75ms) : 73, 77
section Bailout
This PR (8270) - mean (79ms) : 77, 81
master - mean (79ms) : 78, 81
section CallTarget+Inlining+NGEN
This PR (8270) - mean (1,080ms) : 1033, 1127
master - mean (1,089ms) : 1047, 1130
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8270) - mean (115ms) : 112, 118
master - mean (117ms) : 113, 120
section Bailout
This PR (8270) - mean (116ms) : 114, 118
master - mean (118ms) : 115, 121
section CallTarget+Inlining+NGEN
This PR (8270) - mean (774ms) : 712, 837
master - mean (766ms) : 708, 823
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8270) - mean (104ms) : 101, 107
master - mean (104ms) : 101, 107
section Bailout
This PR (8270) - mean (105ms) : 102, 107
master - mean (105ms) : 103, 107
section CallTarget+Inlining+NGEN
This PR (8270) - mean (757ms) : 682, 832
master - mean (768ms) : 707, 828
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8270) - mean (103ms) : 100, 106
master - mean (103ms) : 100, 105
section Bailout
This PR (8270) - mean (104ms) : 102, 106
master - mean (103ms) : 101, 105
section CallTarget+Inlining+NGEN
This PR (8270) - mean (684ms) : 656, 712
master - mean (681ms) : 651, 712
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8270) - mean (195ms) : 188, 202
master - mean (195ms) : 191, 199
section Bailout
This PR (8270) - mean (198ms) : 195, 201
master - mean (199ms) : 196, 202
section CallTarget+Inlining+NGEN
This PR (8270) - mean (1,154ms) : 1092, 1215
master - mean (1,155ms) : 1095, 1215
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8270) - mean (279ms) : 274, 284
master - mean (279ms) : 272, 286
section Bailout
This PR (8270) - mean (280ms) : 272, 288
master - mean (279ms) : 275, 284
section CallTarget+Inlining+NGEN
This PR (8270) - mean (949ms) : 911, 987
master - mean (950ms) : 913, 986
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8270) - mean (271ms) : 266, 276
master - mean (274ms) : 268, 279
section Bailout
This PR (8270) - mean (271ms) : 266, 275
master - mean (272ms) : 269, 276
section CallTarget+Inlining+NGEN
This PR (8270) - mean (934ms) : 907, 962
master - mean (934ms) : 903, 965
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8270) - mean (272ms) : 266, 278
master - mean (270ms) : 265, 275
section Bailout
This PR (8270) - mean (271ms) : 267, 275
master - mean (270ms) : 266, 275
section CallTarget+Inlining+NGEN
This PR (8270) - mean (836ms) : 810, 862
master - mean (836ms) : 813, 859
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
andrewlock
force-pushed
the
andrew/msi-snapshot
branch
from
March 4, 2026 17:27
ba74861 to
e8bba96
Compare
Merged
andrewlock
marked this pull request as ready for review
March 5, 2026 18:08
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14e4b48bcf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
andrewlock
force-pushed
the
andrew/msi-snapshot
branch
from
March 6, 2026 11:14
14e4b48 to
2b3dfb6
Compare
bouwkast
approved these changes
Mar 11, 2026
| Version: '' | ||
| LibDdwaf.32: | ||
| Attributes: 1536 | ||
| Component_: Datadog.Tracer.Native.32 |
Collaborator
There was a problem hiding this comment.
Gonna be honest, not sure what I'm lookin at entirely here but all the other components are teh same as the main yaml thing
Member
Author
There was a problem hiding this comment.
not sure what I'm lookin at entirely here
You and me both 😅 It's basically the "internal tables" of the MSI, so shouldn't change, I just wanted to make sure I could tell if the WiX
Gonna be honest, not sure what I'm lookin at entirely here but all the other components are teh same as the main yaml thing
As best as I can tell, it's because of how this is currently configured - ddwaf is added to the same component as the tracer:
dd-trace-dotnet/shared/src/msi-installer/Tracer/Files.wxs
Lines 29 to 38 in fb1ebae
I don't think it should be, because it's the only case of this pattern we have, but also, I guess it doesn't matter 😅
andrewlock
added a commit
that referenced
this pull request
Mar 13, 2026
## Summary of changes Updates our MSI project to use [Wix 5.x.x](https://docs.firegiant.com/wix/whatsnew/#whats-new-in-wix-v5) instead of Wix 3 ## Reason for change [Wix 3 was deprecated a year ago](https://docs.firegiant.com/wix/wix3/), and is generally clunky and hard to use, as it relies on a global install + .NET Framework 3.5. The newer versions of Wix use newer SDK-style projects, are deployed as nuget packages, and can just be built with a normal `dotnet build` - Wix 4: Quite a big change - Wix 5: Pretty much back-compatible with 4 - Wix 6: [Shifted licensing model](https://docs.firegiant.com/wix/whatsnew/#open-source-maintenance-fee) - we need to look into this if we want to upgrade further. - Wix 7: As above ## Implementation details This was entirely 🤖 driven, but [there's also a .NET tool](https://docs.firegiant.com/wix/whatsnew/#convert-wix-authoring-from-the-command-line) to help with the conversion. _Mostly_ the changes are just "annoying", e.g. moving values from being element text to a `Value` property, etc. ## Test coverage At the end of the day, the generated MSI is _essentially_ the same as confirmed by the snapshots created in #8270. The changes all appear to be benign changes in hashing algorithms, or renaming of wix properties. What's more, I tested the install, and it _looks_ the same (and works), and the MSI tests all pass, which is obviously the important thing! 😄 <img width="495" height="387" alt="image" src="https://github.com/user-attachments/assets/54995470-b846-419c-9f18-e07c1daae127" /> <img width="495" height="387" alt="image" src="https://github.com/user-attachments/assets/0997801f-7143-4c25-88d4-d66ad2404968" /> <img width="495" height="387" alt="image" src="https://github.com/user-attachments/assets/63381b12-f312-4b45-9ae3-d2c77b23d377" /> <img width="495" height="387" alt="image" src="https://github.com/user-attachments/assets/320758a9-f0b7-42e8-a1db-4c8b4b8514ad" /> <img width="495" height="387" alt="image" src="https://github.com/user-attachments/assets/65e1fa04-f02e-42bf-8fcd-efe704000901" /> ## Other details The removal of the `Win64="yes"` and `Win64="$(var.Win64)"` attributes were the main thing I was unsure about. There _is_ [a `Bitness` attribute now](https://docs.firegiant.com/wix/schema/wxs/component/), with values `default`, `always32`, or `always64`, which is pretty much equivalent. However, seeing as we _only_ produce an x64 installer, and not an x86 installer, I think this is essentially just legacy cruft which is ok to remove. We _might_ regret that choice if/when we need an arm64 installer, but I think we'll need to look at everything again at that point anyway, so I don't think it's worth worrying about 😄 --------- Co-authored-by: Claude <[email protected]>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Adds a "snapshot generator" for the MSI contents
Reason for change
We've discussed updating/switching to newer versions of Wix, and we want to make sure we don't regress anything
Implementation details
Uses the WixToolset.Dtf.WindowsInstaller nuget package to read the contents of the MSI. We then scrub values which we expect to change (version numbers, filesizes etc) and dump the values out as a yaml file (could have done any format, we already had a transient reference to yamldotnet, I just made it explicit)
Test coverage
Tested with a couple of MSIs from master, and they pass.
Other details
I considered an alternative, where we try to understand the impact of installing the MSI, which on the surface is what we really care about, but seemed like a much harder prospect 😅