enhancement(agent-data-plane): let the binary declare its own identity - #2346
Open
jszwedko wants to merge 8 commits into
Open
enhancement(agent-data-plane): let the binary declare its own identity#2346jszwedko wants to merge 8 commits into
jszwedko wants to merge 8 commits into
Conversation
saluki-metadata baked the application's name and version into a shared library at compile time, which it fundamentally can't do per-binary: one compiled copy is shared by every binary in the workspace. The only lever scoped to a single build command was the process environment, so every build entry point had to independently remember the same four values. Thirteen places, and the drift they produced has been the subject of the last two PRs. Split the eight fields by what they actually describe. Name and version identify a specific application, so ADP now declares them with `declare_app_details!` and registers them during bootstrap; the version comes from Cargo. Git hash, build time, dev build, and target arch describe the build rather than the application, so they stay exactly where they were, in saluki-metadata's build script. Ordering is enforced structurally rather than by convention: AppBootstrapper takes the details and registers them before initializing any subsystem, so the one library consumer that runs during bootstrap (the logging subagent prefix) can't observe an unregistered application. ADP's own call sites use the constant directly, which covers `version` running before bootstrap. Net effect: APP_FULL_NAME, APP_SHORT_NAME, APP_IDENTIFIER, and APP_VERSION stop being build inputs and disappear from the Makefile, both Dockerfiles, GitLab CI, both Windows scripts, and the AIX script. A bare `cargo build` now produces a correctly identified binary, which is what prompted this in the first place.
dd-octo-sts
Bot
added
area/core
Binary Size Analysis (Agent Data Plane)Baseline: 0f47357 · Comparison: 3cabab8 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
jszwedko
commented
Aug 14, 2026
handle_version_command's signature change wasn't buying anything: APP_DETAILS is a crate const, so the function can just read it. The actual requirement is only that it not use get_app_details(), since it can run before bootstrap registers.
`version` returns early, before configuration is loaded, so that reporting the version never depends on there being usable config. That made run_inner's Version arm dead code which quietly implied the opposite. Assert the invariant instead, so removing the early return fails loudly rather than silently turning `version` into a config-requiring command.
…mments Drop the comments narrating which values moved out of the build tooling, and restore the build metadata description in the release docs. The comments described the change rather than the current state, which isn't useful to a later reader.
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 ( |
…tstrap Application details are a compile-time constant with no relationship to configuration, so routing them through AppBootstrapper coupled them to the config-driven bootstrap phase for no reason: from_configuration took an argument unrelated to configuration, and `version` needed a special case to read the constant directly because it runs beforehand. Registering at the top of main is both simpler and strictly earlier, which also closes the window where config loading and logging translation ran with the details unregistered. saluki-app and the version command revert to their original shapes.
dd-octo-sts
Bot
removed
the
area/observability
jszwedko
commented
Aug 14, 2026
| Action::Dogstatsd(cmd) => handle_dogstatsd_command(local_config, cmd).await, | ||
| Action::Version(v) => handle_version_command(v.json).await, | ||
| // Handled before bootstrap, so that reporting the version never depends on there being usable configuration. | ||
| Action::Version(_) => unreachable!("version is handled before bootstrap"), |
Collaborator
Author
There was a problem hiding this comment.
This is an unrelated change, but 🤖 noticed it while I was in the area.
jszwedko
marked this pull request as ready for review
August 14, 2026 22:48
Registering the application's details is a one-line call at the top of main, and forgetting it degrades silently to reporting an unknown application. No other test in the workspace can catch that: library unit tests have no main, so they legitimately observe the same unregistered fallback. Exercise the real binary instead. Verified by moving the registration below the version early-return, which the compiler accepts and this test rejects. Parsing the output as JSON also pins the version command to running before logging is initialized, since log output on stdout would otherwise break it.
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
ADP registers its identity before every repository consumer can read it, including CLI version reporting, logging, endpoints, and IPC registration. The remaining metadata stays compile-time populated, and all repository build entry points were updated consistently with the removed identity variables.
🤖 Datadog Autotest · Commit 8956d37 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
![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: 8956d37fbe
ℹ️ 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".
Vale flags the bare keyword as a misspelling, and the surrounding code already refers to it in backticks.
…ally The check only ran when some build metadata had already been supplied, so declaring a release build while supplying none of it skipped the check entirely and shipped placeholders -- reachable through a direct Docker build, since the Dockerfile defaults both arguments to empty. That gate was based on a mistaken premise: that tag-pipeline test and lint jobs supply no metadata. They run through the Makefile, which exports it for every recipe, so they carry real values and satisfy the check like any other build.
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
1 participant
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.
Human Summary
Move static metadata or inferable metadata (version) into ADP to remove the need to set some of the
APP_variables. This, I think, will remove one source of bugs resulting from inconsistent setting as was seen in #2343 and enable easier local development (don't need to remember to set the variables). Instead, we only set the metadata that is only known at build time (like 🥁 the build time).AI Summary
saluki-metadatabakes the application's name and version into a shared library at compile time — something it fundamentally cannot do per-binary, since one compiled copy is shared by every binary in the workspace. The only lever scoped to a single build command is the process environment, which is why every build entry point has had to independently remember the same four values. That's thirteen places, and the drift it produces is what the last two PRs were cleaning up. This removes the cause rather than the symptoms.The fix is to split the eight fields by what they actually describe:
full_name,short_name,identifier,versiongit_hash,build_time,dev_build,target_archADP declares the left column once, in
main.rs:The version comes from Cargo —
env!inside the macro expands in the calling crate, so it picks up ADP's version, andCARGO_PKG_VERSION_{MAJOR,MINOR,PATCH}replaces the hand-rolled version splitting in the build script. The right column is genuinely shared (one build has one git hash regardless of how many binaries it produces), so it stays put — as does the release-metadata guard from #2345, narrowed to the fields it still covers.target_archhas to stay there regardless:TARGETis only set for build scripts.Registration happens as the first statement of
main. The details are a compile-time constant with no relationship to configuration, so they're registered before anything else rather than being routed through the config-driven bootstrap phase — which also means the window where config loading and logging translation run is already covered. Theget_app_details()signature is unchanged, so every existing call site is untouched, includingsaluki-appand theversioncommand.Net effect:
APP_FULL_NAME,APP_SHORT_NAME,APP_IDENTIFIER, andAPP_VERSIONstop being build inputs entirely and disappear from the Makefile,Dockerfile.agent-data-plane,.gitlab/build.yml, both Windows scripts, the AIX script, and both antithesis Dockerfiles. This also makes the value fixes in #2343 and the metadata completion in #2345 moot by deletion — those lines no longer exist to drift.Also included
run_inner'sAction::Versionarm was unreachable —versionreturns early, before configuration is loaded, so that reporting the version never depends on usable config. The dead arm quietly implied the opposite, so it now asserts the invariant instead: removing the early return fails loudly rather than silently turningversioninto a config-requiring command.Behavior changes
cargo buildnow produces a correctly identified binary. This was the original motivation, and it's per-binary rather than the workspace-wide.cargo/config.tomldefault we ruled out earlier.v12345-abc1234(a pipeline slug) as the version; they now report the real version, matching every other platform. No other platform's version string changes — the container and tarball paths already used the Cargo version.ADP_APP_VERSIONsurvives in the Makefile, but only because release artifacts are named after it.Test plan
cargo buildwith no environment at all reportsAgent Data Plane/data-plane/adp/1.6.0— previouslyunknownmake build-adpreports the same, plus a real git hashagent-data-plane versionstill printsv1.6.0-<sha>; the leadingvis display-only and unaffectedcargo clippy --workspace --all-targetsclean; 1235 tests pass acrosssaluki-metadata,saluki-app,saluki-components,datadog-agent-commons, andagent-data-planetests/version_cli.rsasserts the built binary reports its real identity — verified it actually catches a regression by moving the registration below the version early-return, which compiles fine and fails the testFollow-up, not in this PR
Library unit tests observe the unregistered fallback, because there's no
main()to register. That's true today as well (the build script yieldsunknownundercargo test), so nothing regresses —endpoints.rsasserts0-0-0-unknown.agentagainst itself either way. What changes is that it becomes fixable: identity can now be injected at runtime. Doing it properly needs an idempotent helper, since aOnceLockis shared across parallel tests in one binary.