Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: d22f64d660
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness and operability issues in the new inventory reporter (retry attempt semantics vs PR description, inaccurate attempt logging, and several silent error-swallowing/log-level concerns) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new background “inventory reporter” to the datadog-serverless-compat mini-agent so supported serverless compat workloads (Azure Functions + GCP Gen1 Cloud Functions) can periodically emit an inventory payload to the Datadog metadata intake for Fleet Automation visibility.
Changes:
- Spawns a background Tokio task at startup to run the inventory reporter (gated via
DD_SERVERLESS_COMPAT_INVENTORY_ENABLED=true). - Introduces
inventory.rsto build workload identity + payload and send it with bounded retry and optional GCP metadata-server fallback. - Adds new crate dependencies needed for payload generation and process UUIDs (
serde,serde_json,uuid, and Tokiotime).
File summaries
| File | Description |
|---|---|
| crates/datadog-serverless-compat/src/main.rs | Spawns the inventory reporter task during agent startup. |
| crates/datadog-serverless-compat/src/inventory.rs | Implements inventory gating, identity derivation, payload construction, and send/retry logic + unit tests. |
| crates/datadog-serverless-compat/Cargo.toml | Adds dependencies and enables Tokio time feature to support reporting. |
| Cargo.lock | Updates lockfile for newly added dependencies. |
Review details
Suppressed comments (3)
crates/datadog-serverless-compat/src/inventory.rs:210
- This log message reports the 0-based loop index as an “attempt” count, so it under-reports by 1 (e.g., last attempt logs “after 2 attempts” when 3 total attempts were made). Consider logging
attempt + 1or renaming the field to retries.
warn!(
"inventory: transport error after {attempt} attempts \
(report_reason={report_reason}, error={e})"
);
crates/datadog-serverless-compat/src/inventory.rs:425
- The metadata-server request transport error is dropped via
.ok()?, which makes it hard to understand why region/project resolution failed (you only get a later “identity unavailable” warning). Logging the reqwest error here would make troubleshooting much easier.
.get(&url)
.header("Metadata-Flavor", "Google")
.send()
.await
.ok()?;
crates/datadog-serverless-compat/src/inventory.rs:438
- Reading the metadata-server response body and logging the parsed value currently uses
.ok()?(drops the error) and logs the value at INFO. Consider logging the read error and lowering the value log to DEBUG to avoid leaking project IDs into INFO logs.
let body = resp.text().await.ok()?;
let result = parse(body.trim());
info!("inventory: GCP metadata server {label}: {:?}", result);
result
- Files reviewed: 3/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| // Runtime: prefer DD_SERVERLESS_COMPAT_RUNTIME (set by language package); | ||
| // fall back to FUNCTIONS_WORKER_RUNTIME injected by Azure. | ||
| let runtime = env::var("DD_SERVERLESS_COMPAT_RUNTIME") |
There was a problem hiding this comment.
Where does DD_SERVERLESS_COMPAT_RUNTIME come from? Is that set in the Serverless Compatibility layer today?
There was a problem hiding this comment.
It is an optional handoff intended for the language package wrapping the compat binary; it is not set by this Rust binary today. When absent, the reporter uses the platform-derived runtime from AzureMetadata or the GCP runtime environment. I added a test covering the optional override behavior.
There was a problem hiding this comment.
Are there plans to set this value somewhere in the future? If not, I'd remove it to reduce any complexity introduced by an unused feature.
There was a problem hiding this comment.
Agreed. We removed DD_SERVERLESS_COMPAT_RUNTIME. The runtime now comes only from the Azure or GCP platform metadata, which avoids carrying an unused override.
|
|
||
| // Runtime version: prefer DD_SERVERLESS_COMPAT_RUNTIME_VERSION (language package), | ||
| // then FUNCTIONS_WORKER_RUNTIME_VERSION, then language-specific vars. | ||
| let runtime_ver = env::var("DD_SERVERLESS_COMPAT_RUNTIME_VERSION") |
There was a problem hiding this comment.
Where doesDD_SERVERLESS_COMPAT_RUNTIME_VERSION come from? Is that set in the Serverless Compatibility layer today?
There was a problem hiding this comment.
Like DD_SERVERLESS_COMPAT_RUNTIME, this is an optional language-package handoff and is not set by the Rust binary itself today. If it is absent, Azure uses FUNCTIONS_WORKER_RUNTIME_VERSION through AzureMetadata, while GCP derives the language version from the platform runtime variables.
There was a problem hiding this comment.
Agreed. We also removed DD_SERVERLESS_COMPAT_RUNTIME_VERSION. Azure and GCP now build the canonical runtime value directly from platform metadata.
5a10d29 to
4bb2152
Compare
4bb2152 to
1c5c819
Compare
apiarian-datadog
left a comment
apiarian-datadog
left a comment
There was a problem hiding this comment.
looks okay, but lets make sure serverless-compat engineers look at this.
64d97d8 to
0ab5886
Compare
0ab5886 to
b7d0f84
Compare
| let intake_url = match build_intake_url(&self.dd_site) { | ||
| Ok(url) => url, | ||
| Err(error) => { | ||
| warn!("inventory: invalid DD_SITE, skipping {report_reason} report: {error}"); | ||
| return; | ||
| } | ||
| }; |
There was a problem hiding this comment.
build_intake_url should probably be build once at startup rather than on every report of telemetry from the inventory agent.
There was a problem hiding this comment.
Fixed. build_intake_url now runs once when the reporter starts. The validated reqwest::Url is stored in Reporter and reused for the startup and periodic reports.
b7d0f84 to
40b65bc
Compare
40b65bc to
7b31be2
Compare
| run: | | ||
| VERSION="$(./scripts/resolve-serverless-compat-version.sh "$INPUT_VERSION")" | ||
| if [[ -n "$VERSION" ]]; then | ||
| echo "DD_SERVERLESS_COMPAT_VERSION=$VERSION" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
Maybe rename this environment variable to DD_SERVERLESS_COMPAT_BINARY_VERSION to differentiate it from the environment variable that's used to store the .NET, Java, Node.js, Python, Go packages?
There was a problem hiding this comment.
Fixed in deb1294. The workflow now exports DD_SERVERLESS_COMPAT_BINARY_VERSION, and both payload reads use that name. DD_SERVERLESS_COMPAT_VERSION remains reserved for the language-specific package version, which is sent as serverless_compat_runtime_version.
| - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
| with: | ||
| sparse-checkout: npm | ||
| sparse-checkout-cone-mode: false |
There was a problem hiding this comment.
Have you tested this workflow? How will the resolve-serverless-compat-version.sh script run if it's not checked out in the job?
There was a problem hiding this comment.
Fixed in 23818d5. The package job now includes scripts/resolve-serverless-compat-version.sh in its sparse checkout before the version step runs. I verified the script is present and executable with the same checkout patterns. The reusable build job already uses a full checkout.
7cb9051 to
deb1294
Compare
06c24b8 to
2223b3e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2223b3e0c0
ℹ️ 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".
| if !is_metadata_enabled(&ProcessEnv) || !is_supported(&env_type) { | ||
| return; |
There was a problem hiding this comment.
For every existing Azure or GCP deployment that does not set the newly introduced DD_SERVERLESS_COMPAT_METADATA_ENABLED=true, this returns before sending anything; a repo-wide search also finds no deployment or workflow that provisions the variable. Consequently, the startup and periodic reports promised by this PR remain inactive by default behind an undocumented opt-in. Either enable reporting by default with a documented opt-out, or explicitly document and provision this gate.
AGENTS.md reference: AGENTS.md:L64-L65
Useful? React with 👍 / 👎.
| metadata["serverless_compat_binary_version"] = | ||
| Value::String(compat_binary_version.to_string()); | ||
| } | ||
| metadata["dd_site"] = Value::String(dd_site.to_string()); |
There was a problem hiding this comment.
When users set the PR-documented DD_SERVERLESS_COMPAT_RUNTIME or DD_SERVERLESS_COMPAT_RUNTIME_VERSION, payload construction never reads either variable, so the platform-derived runtime value is still reported. This makes the advertised override ineffective for runtimes whose platform metadata is missing or incorrect; apply those values here or remove the unsupported settings from the PR description.
AGENTS.md reference: AGENTS.md:L64-L65
Useful? React with 👍 / 👎.
| exit 0 | ||
| fi | ||
|
|
||
| if [[ ! "$version" =~ ^[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z.-]+)?(\+[0-9A-Za-z.-]+)?$ ]]; then |
There was a problem hiding this comment.
When an operator supplies a version containing empty prerelease or build identifiers, such as 1.2.3-.. or 1.2.3+a..b, this regex accepts it even though npm requires published package versions to be parseable by node-semver. The publish workflow therefore completes every platform build before failing at npm publish, while the release workflow can embed the malformed value in its binaries; validate with a SemVer-aware tool or enforce nonempty dot-separated identifiers and the numeric leading-zero rules. npm package.json documentation
Useful? React with 👍 / 👎.
Summary
Adds the
serverless_compat_agentinventory reporter to the shared Compat mini-agent so Azure Functions and GCP Gen1 Cloud Functions appear in Fleet Automation.DD_SERVERLESS_COMPAT_METADATA_ENABLED=true; startup reporting is then immediate, with periodic reports every 30 minutesK_SERVICEandFUNCTION_TARGETremain supported; Gen2 uses the serverless-init sidecar pathDD_SERVERLESS_COMPAT_BINARY_VERSION; runtimeDD_SERVERLESS_COMPAT_VERSIONreports the installed language-wrapper version separatelyplatform_versionand hostname are intentionally absentStack
This PR is based on #176 and remains the end-to-end-testable top of the stack. Its original URL, comments, and review history are preserved.
Testing
cargo test --package datadog-serverless-compat-inventory --locked --offlinecargo test --package datadog-serverless-compat --locked --offlinecargo clippy --package datadog-serverless-compat-inventory --package datadog-serverless-compat --all-targets --locked --offline -- -D warningsgit diff --checkWhat is not here
No deploy scripts, test logs, or npm binaries; those stay local or on the prototype branch (
nina/svls-9604-inventory-payload).Related