fix(agent-data-plane): fail the build when release metadata is missing - #2345
fix(agent-data-plane): fail the build when release metadata is missing#2345jszwedko wants to merge 4 commits into
Conversation
saluki-metadata falls back to placeholder values ("unknown", "0.0.0") for any APP_*
variable a build doesn't supply. That keeps a bare `cargo build` working, but it also
means a release build whose tooling forgot or misspelled one of these variables ships
silently, reporting "unknown" to users and telemetry. Two such cases were already live:
the ADP image build never declared ARG APP_DEV_BUILD, so tagged release images were
marked as dev builds (and emitted a "-dev-<sha>" suffixed version label), and the
Windows build set APP_BUILD_DATE where saluki-metadata reads APP_BUILD_TIME, so Windows
binaries reported no build time at all.
Fix both, then make the class of bug non-silent: once a build supplies application
identity and declares itself a release build, saluki-metadata's build script now fails
if any metadata is still at its placeholder default.
The check is deliberately gated on identity being supplied rather than on APP_DEV_BUILD
alone. APP_DEV_BUILD is set at the workflow level in CI, so on a tag pipeline it is also
present for jobs that only run cargo test/clippy; keying off it alone would have failed
the whole release pipeline.
Also drops the dead APP_BUILD_DATE assignments in the Makefile (the value reached cargo
via the global export at the top of the file, so this was misleading rather than broken),
stamps a real timestamp in the AIX script since it defaults to a release build, and
corrects the build metadata docs.
dd-octo-sts
Bot
added
area/core
Ten lines was out of step with the rest of the file. Keep only what can't be read off the code: why placeholders fail, and why the condition isn't APP_DEV_BUILD alone.
Binary Size Analysis (Agent Data Plane)Baseline: 0f47357 · Comparison: cd28248 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
![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: 6b3c59c987
ℹ️ 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".
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Tagged Windows builds still default to development metadata because their container never receives APP_DEV_BUILD; this bypasses the new release guard and preserves the incorrect -dev-<sha> telemetry version on shipped Windows artifacts.
🤖 Datadog Autotest · Commit 6b3c59c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
The Windows release build runs inside `docker run` with an explicit -e allowlist, so the workflow-level APP_DEV_BUILD never reached it. Tagged Windows artifacts were therefore marked as dev builds, and the new release metadata check skipped them entirely. Only the release-zip definition gets it (the FIPS variant extends the same definition). The integration job is deliberately left alone: windows-integration-tests.ps1 supplies only partial identity, so forwarding the flag there would trip the check on tag pipelines.
The script set only APP_FULL_NAME, APP_SHORT_NAME, and APP_GIT_HASH, leaving the identifier, version, and build time to fall back to placeholders. That partial set is a trap now that saluki-metadata rejects placeholder metadata on release builds: the job would fail the moment anyone forwarded APP_DEV_BUILD into its container. Fill in the rest, taking the version from ADP's manifest the same way the Makefile does.
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (5)Experiments configured
Bounds Checks: ✅ Passed (5)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |
| # Windows PowerShell 5.1 (the default `powershell.exe` in the LTSC2022 build image) doesn't | ||
| # have Get-Date's -AsUTC switch (added in PS 7.1). ToUniversalTime() works on both. | ||
| $env:APP_BUILD_DATE = (Get-Date).ToUniversalTime().ToString("yyyy-MM-ddTHH:mm:ssZ") | ||
| $env:APP_BUILD_TIME = (Get-Date).ToUniversalTime().ToString("yyyy-MM-ddTHH:mm:ssZ") |
There was a problem hiding this comment.
Was using the wrong name.
| # were ever handed APP_DEV_BUILD=false. | ||
| $env:APP_FULL_NAME = "Agent Data Plane" | ||
| $env:APP_SHORT_NAME = "data-plane" | ||
| $env:APP_IDENTIFIER = "adp" |
There was a problem hiding this comment.
Sets missing variables.
1 participant
Human Summary
Ensure that the ADP metadata is populated for release builds. This actually caught and fixes an issue with the Windows and AIX builds using the wrong env var name for the build time.
Separately, also caught an issue with the docker image build where it wasn't propagating
APP_DEV_BUILD.AI Summary
saluki-metadatasilently substitutes placeholders (unknown,0.0.0,0000-00-00 00:00:00) for anyAPP_*variable a build doesn't supply. That's what makes a barecargo buildwork, but it also means a release build whose tooling forgot or misspelled one of those variables ships without anyone noticing — the binary just reportsunknownto users and telemetry. Two instances were already live inmain, which is what prompted this. Rather than only fixing them, this makes the whole class of bug loud: once a build supplies application identity and declares itself a release build, the build script fails instead of quietly falling back.This is the first step of a broader cleanup of how ADP's build metadata is plumbed; it's self-contained and doesn't depend on where that lands.
The two live bugs
.gitlab/build.ymlpasses--build-arg APP_DEV_BUILD, butdocker/Dockerfile.agent-data-planenever declared a matchingARG, so Docker dropped it and the build script fell back totrue. Tagged release images therefore emittedrunning{version="1.6.0-dev-<sha>"}instead of1.6.0.windows-build-adp.ps1setAPP_BUILD_DATE, butsaluki-metadatareadsAPP_BUILD_**TIME**, and the Windows job sets nothing else — so those binaries reported0000-00-00 00:00:00.Why the check isn't keyed on
APP_DEV_BUILDaloneAPP_DEV_BUILDis set at the workflow level in.gitlab-ci.yml, so on a tag pipeline it's in scope for every job — including the.gitlab/test.ymljobs (unit tests, clippy, miri), which have norules:and no reason to supply application metadata. Enforcing purely onAPP_DEV_BUILD=falsewould have failed the entire release pipeline. The guard therefore only engages when something actually tried to identify the application, which no test or lint job does. Forgetting a single variable still trips it, since the others remain set.Also included
Small fixes that keep the guard honest rather than papering over it: the Makefile's dead
APP_BUILD_DATEassignments are corrected (the right value already reached cargo via the globalexportat the top of the file, so these were misleading rather than broken),build-adp-aix.shnow stamps a real timestamp since it defaults to a release build and would otherwise trip the new check, and the build-metadata docs are brought in line with reality.Test plan
Verified the guard against the full matrix by invoking
cargo build -p saluki-metadatawith each environment, confirming no false positives on the paths that must keep working:APP_DEV_BUILD=falseonly)APP_BUILD_TIMEforgottenAPP_IDENTIFIERmisspelledmake -n build-adpconfirms the correctedAPP_BUILD_TIMEreaches cargo