feat(config): adds support for full suite of otlp_config.receiver keys - #2308
Conversation
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 ( |
Binary Size Analysis (Agent Data Plane)Baseline: e760adf · Comparison: 7762e11 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
webern
left a comment
webern
left a comment
There was a problem hiding this comment.
Headed in the right direction, but instead of doing a one-time patch and checking-in the resultant, patched, schema, we need to deserialize and do the patching in memory at build.rs execution.
Your version of things here in the PR currently works and gives us what we want, but it cannot be maintained because the core schema will be overwritten when we update it.
In general, both schema's should be checked into our repository in a pristine state, no edits or patches. Anything we need to do with them happens in-memory during build.rs.
There was a problem hiding this comment.
We cannot "patch" the core schema like this because we need to be able to overwrite with a new version.
What we need to do instead is vendor (i.e. check-in to this repo) the otel configuration schema (or the relevant parts of it) here in, e.g. schema/otel next to schema/core. Then during build.rs we deserialize both and perform the "patch" in memory before handing it off to code gen or other processes.
dd-octo-sts
Bot
added
area/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: 0f88b4cc38
ℹ️ 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".
| pub fn load_composed_schema(datadog_schema: &Path, otel_schema_dir: &Path) -> Result<serde_yaml::Value, Error> { | ||
| let mut datadog_schema = load_resolved_schema(datadog_schema)?; | ||
| let otel_receiver = load_otel_receiver(otel_schema_dir)?; | ||
| patch_receiver(&mut datadog_schema, otel_receiver)?; |
There was a problem hiding this comment.
When a receiver key exists only in the newly vendored OTel schema, composing it into schema_keys makes the overlay and generators classify it as a Datadog key even though the repository's source-class rule says keys absent from schema/core/ are Saluki-only. If one of these currently unknown entries is later marked supported, it will incorrectly enter the Datadog witness/environment path instead of SalukiOnly; add the keys to the canonical core schema if the Agent owns them, or retain their Saluki-only classification.
AGENTS.md reference: AGENTS.md:L36-L38
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is incorrect. The Core Agent does "own" these keys, but the processing is delegated and offloaded to the embedded OTEL Collector. It would be misleading to classify them as Saluki-only. Also, it was decided that composition should happen in-memory at build time.
There was a problem hiding this comment.
I agree, this is by design as we have though about this and decided that, since these appear in Datadog configuration, and affect the observable behavior of the Agent (by being copied into the embedded OTeL collector), these are effectively Datadog configurations. Furthermore, doing this makes all of our downstream processes work correctly for said keys.
| let Some(receiver) = otlp_config.get_mut("properties").and_then(|v| v.get_mut("receiver")) else { | ||
| return Ok(()); | ||
| }; | ||
|
|
||
| *receiver = otel_receiver; |
There was a problem hiding this comment.
Whenever load_composed_schema processes the real core schema, replacing the entire receiver node discards Datadog-specific metadata already present there. For example, the core schema declares gRPC and HTTP endpoint defaults of localhost:4317 and localhost:4318, while the replacement OTel nodes declare no defaults, so schema_gen::load_schema now emits default: None even though DatadogConfiguration is still generated from the unpatched core schema and applies those defaults. Merge the additional OTel properties while retaining attributes from overlapping core nodes so generated metadata and runtime behavior remain consistent.
AGENTS.md reference: AGENTS.md:L36-L38
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a good catch. Fixed in (95e873b). We should honor the DataDog defaults for otlp_config.receiver since they were configured for our specific use cases.
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The composed OTLP schema resolves the new receiver keys and representative nested/list/duration shapes correctly, but both build scripts stopped watching referenced core schema files. Later edits to files such as apm_config.yaml can leave generated runtime configuration and test metadata stale.
📊 Validated against 8 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 0f88b4c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
|
||
| let schema_dir = files.schema.parent().expect("schema file must have a parent directory"); | ||
| println!("cargo:rerun-if-changed={}", schema_dir.display()); | ||
| println!("cargo:rerun-if-changed={}", files.datadog_schema.display()); |
There was a problem hiding this comment.
Incremental builds can compile stale configuration structs, environment bindings, classifiers, registry entries, and documentation after a referenced core schema changes.
Assertion details
- Input: Build once, edit a referenced schema such as
schema/core/apm_config.yamlwithout touchingcore_schema.yaml, then build again. - Expected:
Both build scripts should watch the parent core schema directory and the OTel schema directory so every consumed schema change reruns generation. - Actual: Both changed build scripts now emit
rerun-if-changedonly forcore_schema.yaml, althoughload_resolved_schemarecursively readsapm_config.yaml,logs_config.yaml, and many other sibling files. Cargo therefore considers the generators fresh after those referenced files change. A complete fix must restore the core-directory watcher in bothconfig/build.rsandconfig-testing/build.rs, while retaining the new OTel-directory watcher.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
lucastemb
changed the title
webern
left a comment
webern
left a comment
There was a problem hiding this comment.
The only thing is that I think the gen_datadog_config is not using the merged schema.
The other comments are less immediate.
| let schema_map = schema_gen::load_schema(&schema_path); | ||
|
|
||
| classifier_gen::generate(&overlay, &schema_map, &manifest_dir); | ||
| datadog_config_gen::generate(&overlay, &schema_path, &schema_map, &manifest_dir); |
There was a problem hiding this comment.
Could we pass the composed schema to datadog_config_gen instead of this raw path? schema_map uses the composed schema, but datadog_config_gen::generate reloads this path with load_resolved_schema. Typed configuration generation therefore still sees the unpatched core schema. The issue requires every build consumer to use the same composed schema.
Put differently... any caller that is using the schema should take the composed schema instead of a file path.
| } | ||
|
|
||
| /// Merge the resolved OTel receiver subtree into the Datadog schema's `otlp_config.receiver` while | ||
| /// honoring default values not native to OTel. |
There was a problem hiding this comment.
I think OTeL defaults are an unsolved problem. They don't exist in the spec, apparently. So as soon as we enable one of these keys, we are going to have to figure out its default and how that value arrives into the code.
I think we can move forward for now, but we will hit this next.
| } | ||
|
|
||
| /// Resolve a single `$ref` target, returning the definition value and its source `$defs`. | ||
| fn resolve_otel_ref_target( |
There was a problem hiding this comment.
Somewhere in here we should either mark these as no_env (I think?) with a TODO, that says see if the Agent accepts these by DD_WHATEVER. Or, if feeling ambitious, maybe do an experiment to see if these can be set by DD env var and if so, use the pattern to populate the right vars here.
There was a problem hiding this comment.
After poking around (and running a correctness test) it seems like only the 13 that are advertised in core_schema.yaml can be set via DD env var. According to the LLM, it seems that no-env won't suppress generation of DD_* env entries, but I'm unclear what the intended effect is.
There was a problem hiding this comment.
Fixed in b9a7bdd, but admittedly, I'm not sure if this is what you were looking for
webern
left a comment
webern
left a comment
There was a problem hiding this comment.
I want to unblock this merge because I will be out for three days next week and my review basically amounts to, this looks good and we probably still need to deal with defaults, which we can do in a future PR, or as we enable the fields.
…ull configuration suite
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
#2308) ## Summary <!-- Please provide a brief summary about what this PR does. This should help the reviewers give feedback faster and with higher quality. --> As part of the ongoing work related to OTLP Ingest, we noticed that some of the embedded OTEL configuration options were not being broadcasted. This PR seeks to explicitly list all `otlp_config.receiver` options that are recognized. This will lay the ground work for a series of future PRs addressed at accepting or rejecting support for the full suite of config keys. The full list can be seen [here](https://github.com/open-telemetry/opentelemetry-collector/blob/main/receiver/otlpreceiver/config.schema.yaml) or [here](https://github.com/open-telemetry/opentelemetry-collector/blob/main/receiver/otlpreceiver/README.md) ## Change Type - [ ] Bug fix - [x] New feature - [ ] Non-functional (chore, refactoring, docs) - [ ] Performance ## How did you test this PR? <!-- Please how you tested these changes here --> N/A ## References <!-- Please list any issues closed by this PR. --> <!-- - Closes: <issue link> --> - Closes: #2283 <!-- Any other issues or PRs relevant to this PR? Feel free to list them here. --> Co-authored-by: lucas.tembras <lucas.tembras@datadoghq.com> 8d372a5
2 participants
Summary
As part of the ongoing work related to OTLP Ingest, we noticed that some of the embedded OTEL configuration options were not being broadcasted. This PR seeks to explicitly list all
otlp_config.receiveroptions that are recognized. This will lay the ground work for a series of future PRs addressed at accepting or rejecting support for the full suite of config keys.The full list can be seen here or here
Change Type
How did you test this PR?
N/A
References