fix: update pprof prebuild cleanup for @datadog/pprof 5.14.4 - #813
Conversation
joeyzhao2018
left a comment
joeyzhao2018
left a comment
There was a problem hiding this comment.
it's safe to ship as long as the pipeline is clean
There was a problem hiding this comment.
Pull request overview
Updates @datadog/pprof prebuild cleanup for its 5.14.4 layout.
Changes:
- Removes non-Linux and musl binaries.
- Updates ABI-specific cleanup patterns.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: 17e3dc7624
ℹ️ 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".
|
@codex review |
The @datadog/pprof package restructured its prebuilds in 5.14.4, breaking the Dockerfile cleanup that removes unused platform/ABI binaries from the Lambda layer: 1. Platform directories renamed: linuxglibc-x64 -> linux-x64, linuxglibc-arm64 -> linux-arm64. The old linuxmusl-* dirs no longer exist as separate dirs; musl .node files now live alongside glibc inside linux-x64/ and linux-arm64/. 2. File naming changed: node-111.node -> dd_pprof.node.abi111.node, so the old glob patterns matched nothing. This caused 36 files (8.6 MB) of unnecessary prebuilds to ship in the layer instead of 8 files (2.1 MB). The fix updates the cleanup to use the new directory/file naming and removes musl binaries via glob, bringing the layer down to 6 files (1.8 MB).
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
d40c6c0 to
1ec755e
Compare
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
What does this PR do?
Updates the Dockerfile prebuild cleanup for
@datadog/pprofto match the package's new directory and file naming structure introduced in 5.14.4.Motivation
PR #783 bumped
@datadog/pproffrom 5.13.2 to 5.14.4, which restructured the prebuilds in two ways that silently broke the Dockerfile cleanup (lines 53-66):1. Platform directory names changed
linuxglibc-x64/→linux-x64/linuxglibc-arm64/→linux-arm64/linuxmusl-x64/andlinuxmusl-arm64/no longer exist as separate directories. The musl.nodefiles now live alongside glibc insidelinux-x64/andlinux-arm64/with add_pprof.musl.node.abi*.nodenaming pattern.The Dockerfile removed
linuxmusl-arm64/andlinuxmusl-x64/as directories, which are now no-ops. The musl binaries ship in the layer even though Lambda runs glibc (Amazon Linux 2).2. File naming convention changed
node-111.node→dd_pprof.node.abi111.nodenode-120.node→dd_pprof.node.abi120.nodeThe glob
prebuilds/*/node-111.nodematches nothing in 5.14.4, so all ABI-specific removals are also no-ops.Size impact:
.nodefiles.nodefilesThe regression was ~6.5 MB of unnecessary prebuilds shipping in the layer — 3.2 MB of musl binaries that Lambda can't use, plus unused ABIs for Node runtimes Lambda doesn't support.
This was identified in this Slack thread.
Testing Guidelines
Verified the cleanup logic by downloading
@datadog/pprof5.14.4 from npm and simulating the new Dockerfilerm -rfcommands against the actual prebuilds directory. After the new cleanup, 6 files remain (glibc, x64 + arm64, ABIs for Node 18/20/22), totaling 1.8 MB — down from 36 files / 8.6 MB with the old broken cleanup, and slightly smaller than the 8 files / 2.1 MB on the old 5.13.2.End-to-end layer size measurements
Built the Node 20 layer (
NODE_VERSION=20.19) at the commit immediately before this PR merged (5095655) and at the merge commit (2d3f33d).The repository's layer-size check fails before the PR because the extracted layer is over the 24 MiB limit (25,680 KiB vs. 24,576 KiB). It passes after the PR at 19,954 KiB. Both archives pass the 9 MiB compressed-size limit.
Additional Notes
The fix keeps ABIs for Node 18 (abi108), Node 20 (abi115), and Node 22 (abi127) — the currently supported Lambda Node runtimes. If new Node versions are added to Lambda, the ABI removal list will need updating. The existing TODOs about splitting x64/arm64 into separate images and shipping per-platform bindings still apply.
Types of Changes
Check all that apply