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.
|
|
||
| // 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.
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.
b7d0f84 to
40b65bc
Compare
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.K_SERVICEandFUNCTION_TARGETremain supported; Gen2 uses the serverless-init sidecar pathDD_SERVERLESS_COMPAT_VERSIONremains the highest-precedence overrideDD_SERVERLESS_COMPAT_RUNTIMEandDD_SERVERLESS_COMPAT_RUNTIME_VERSIONoverride platform-derived runtime metadataplatform_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
serverless-compatflavor andserverless_compat_agenttable writesserverless_compat_agentschema