fix(config): recognize duration keys at their default in the config classifier#1958
Conversation
…ifier The config classifier compared a key's incoming value against its schema default using exact JSON equality. Duration keys broke this: the schema default is a Go duration string (e.g. `10s`) while the Datadog Agent transmits durations over the config stream as integer nanoseconds, so a value at its default (e.g. `tls_handshake_timeout` = 10000000000) never matched and was flagged as an override, emitting a spurious "Unsupported configuration key" warning. Carry the schema's `format: duration` marker through to each classifier entry and normalize both sides to nanoseconds before comparing, so any duration-typed key at its default is correctly recognized. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Binary Size Analysis (Agent Data Plane)Baseline: 9b8116a · Comparison: 44b4303 · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
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 ( |
| /// Supports the unit suffixes Go's `time.ParseDuration` accepts (`ns`, `us`/`µs`/`μs`, `ms`, `s`, | ||
| /// `m`, `h`), an optional leading sign, and fractional components. Returns `None` if the string is | ||
| /// not a well-formed duration. | ||
| fn parse_go_duration_nanos(input: &str) -> Option<i128> { |
There was a problem hiding this comment.
This can be refactored out to share the implementation from saluki-config.
webern
left a comment
webern
left a comment
There was a problem hiding this comment.
Approved as a quick fix, with a recommended follow-up (see long comment).
Basically, I think this is closer to right than #1959, but the design I pasted in (even if it's not 100% right as written, not sure, but...) gets us even closer than this to actually dealing with this datatype.
| /// Whether this key holds a `format: duration` value. The Agent sends durations as integer | ||
| /// nanoseconds while `default` is a Go duration string, so the default check normalizes both | ||
| /// sides before comparing. | ||
| pub is_duration: bool, |
There was a problem hiding this comment.
I spent some time on designing what I think is a proper fix and that design is below. It solves a few problems and models the actual data type correctly. You can merge your fix as is, but if you do I might suggest posting this as a follow up issue (which I can take a stab at). It might have enough fidelity to nearly one-shot it as is from the write-up, though there's a decision buried in there about how we stop rewriting the "Go duration" algorithm over and over again 😅
The major benefits of the design below
- We catch this at compile time. If the format is duration and default is not parsable as a duration, our build breaks.
- We properly model the specified datatype at the source and all downstream codegen can do the right thing™ with it.
1. Model format: duration as an effective field type
The vendored Agent schema is JSON Schema in YAML form. I checked the resolved schema, and the only format value we currently see is duration. Those entries are all type: number, format: duration. (Well, actually on main it is now corrected to type: string, format: duration)
Given that, I think is_duration is modeling this at the wrong level. The source schema has type + format, but our generated FieldType is already the effective type used by codegen/smoke tests/classification. So I would rather fold format: duration into FieldType::Duration than carry FieldType::Float plus an is_duration side channel.
lib/datadog-agent/config-overlay-model/src/schema_gen.rs
pub enum FieldType {
String,
Bool,
Integer,
Float,
StringList,
Duration,
Unknown,
}lib/datadog-agent/config-overlay-model/src/schema_gen.rs
fn parse_value_type(value: &Value) -> FieldType {
match (
value.get("type").and_then(|v| v.as_str()),
value.get("format").and_then(|v| v.as_str()),
) {
(Some("string"), Some("duration")) => FieldType::Duration,
// Should we break the build on nonsense like this?
(_, Some("duration")) => panic!("illogical duration field {show_helpful_stuff}"),
(Some("string"), _) => FieldType::String,
(Some("boolean"), _) => FieldType::Bool,
(Some("integer"), _) => FieldType::Integer,
(Some("number"), _) => FieldType::Float,
(Some("array"), _) => {
let item_type = value.get("items").and_then(|v| v.get("type")).and_then(|v| v.as_str());
if item_type == Some("string") {
FieldType::StringList
} else {
FieldType::Unknown
}
}
_ => FieldType::Unknown,
}
}If we later want raw JSON Schema fidelity, I would model that explicitly as something like json_type + format. But FieldType + is_duration is neither the raw schema model nor a clean effective-type model; it is a one-off side channel that future schema gotchas are likely to copy.
2. Teach smoke tests about duration too
I would also make the config-testing registry understand this as a real value type. That avoids needing bespoke test_json values for every future duration key.
lib/datadog-agent/config-testing/src/config_registry/mod.rs
pub enum ValueType {
Bool,
String,
Integer,
Float,
StringList,
Duration,
}lib/datadog-agent/config-testing/src/smoke_test.rs
fn test_json_value(value_type: ValueType) -> serde_json::Value {
match value_type {
ValueType::String => json!(TEST_STRING_VALUE),
ValueType::Bool => json!(TEST_BOOL_VALUE),
ValueType::StringList => json!(TEST_STRING_LIST_VALUE),
ValueType::Integer => json!(42i64),
ValueType::Float => json!(1.5f64),
ValueType::Duration => json!("42s"),
}
}lib/datadog-agent/config-testing/src/smoke_test.rs
fn json_value_to_env_string(value: &serde_json::Value, value_type: ValueType) -> String {
match value_type {
ValueType::Bool => value
.as_bool()
.map(|b| b.to_string())
.unwrap_or_else(|| "true".to_string()),
ValueType::Integer => value
.as_i64()
.map(|n| n.to_string())
.unwrap_or_else(|| "42".to_string()),
ValueType::Float => value
.as_f64()
.map(|f| f.to_string())
.unwrap_or_else(|| "1.5".to_string()),
ValueType::String => value.as_str().unwrap_or(TEST_STRING_VALUE).to_string(),
ValueType::StringList => value
.as_array()
.map(|arr| arr.iter().filter_map(|v| v.as_str()).collect::<Vec<_>>().join(" "))
.unwrap_or_else(|| TEST_STRING_LIST_VALUE.join(" ")),
ValueType::Duration => value.as_str().unwrap_or("42s").to_string(),
}
}Individual overlay entries should only need test_json: '"20s"' or similar when the generic duration test value is bad for that specific key.
3. Store parsed duration defaults, not parse schema defaults during classification
The other thing I would change is the default representation. Duration defaults are static generated metadata. I do not think the classifier should parse the schema default on every classification. Codegen should parse and canonicalize the duration default once, and fail the build if the schema default is invalid.
For example:
lib/datadog-agent/config/src/classifier/mod.rs
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum DefaultValue {
Missing,
Json(&'static str),
DurationNanos(u64),
}Then generated classifier data can carry a typed default:
lib/datadog-agent/config/src/classifier/classifier_data.rs
ClassifierEntry {
yaml_path: "tls_handshake_timeout",
aliases: &[],
support_level: SupportLevel::Incompatible(Severity::Medium),
pipeline_affinity: PipelineAffinity::CrossCutting,
default: DefaultValue::DurationNanos(10_000_000_000),
}Non-duration defaults can stay JSON-backed:
lib/datadog-agent/config/src/classifier/classifier_data.rs
ClassifierEntry {
yaml_path: "min_tls_version",
aliases: &[],
support_level: SupportLevel::Partial,
pipeline_affinity: PipelineAffinity::CrossCutting,
default: DefaultValue::Json("\"tlsv1.2\""),
}Then the runtime default check switches on the already-typed default:
lib/datadog-agent/config/src/classifier/classifier.rs
fn is_default_value(default: DefaultValue, value: &Value) -> bool {
match default {
DefaultValue::Json(default_str) => serde_json::from_str::<Value>(default_str)
.map(|default_value| *value == default_value)
.unwrap_or(false),
DefaultValue::DurationNanos(default_ns) => duration_value_as_nanos(value)
.map(|value_ns| value_ns == default_ns)
.unwrap_or(false),
DefaultValue::Missing => match value {
Value::Null => true,
Value::String(s) => s.is_empty(),
_ => false,
},
}
}Runtime still needs to normalize the incoming value because the Agent config stream may send durations as integer nanoseconds:
lib/datadog-agent/config/src/classifier/classifier.rs
fn duration_value_as_nanos(value: &Value) -> Option<u64> {
match value {
Value::Number(n) => n.as_u64(),
Value::String(s) => parse_duration(s).ok().map(|d| d.as_nanos() as u64),
_ => None,
}
}4. Avoid duplicating DurationString
This PR's duration parser duplicates the existing saluki-config::DurationString / parse_duration behavior. I agree datadog-agent-config should not depend on saluki-config; that would pull a leaf-ish Datadog config crate toward a much heavier, higher-level crate.
There are also existing crates that may solve the leaf-crate problem without making our crates depend on each other:
If one of these is acceptable after careful vetting, I would prefer taking that dependency over continuing to rewrite this parser in multiple places. The vetting should include at least: license, maintenance/activity, dependency footprint, exact Go time.ParseDuration compatibility, overflow behavior, negative duration behavior, and whether the API returns the unit we want without lossy conversion.
This is especially attractive because duration parsing has a pile of small edge cases (µs vs μs, fractional units, signs, overflow, bare 0, malformed input). The existence of this PR is a good signal that we should stop locally reimplementing that algorithm.
One possible shape if we pick a crate:
lib/datadog-agent/config/Cargo.toml
[dependencies]
go-parse-duration = "0.1"lib/saluki-config/Cargo.toml
[dependencies]
go-parse-duration = "0.1"Then saluki-config::DurationString can remain our domain type, but delegate parsing to the vetted implementation:
lib/saluki-config/src/duration_string.rs
fn parse_string(s: &str) -> Result<Duration, ParseDurationError> {
let nanos = go_parse_duration::parse_duration(s).map_err(|err| ...)?;
Duration::from_nanos(nanos.try_into().map_err(|_| ParseDurationError::Negative)?).into()
}And classifier codegen can use the same parser for schema defaults without depending on saluki-config:
lib/datadog-agent/config/build/classifier_gen.rs
fn schema_default_expr(yaml_path: &str, schema_map: &IndexMap<String, FieldInfo>) -> String {
let Some(info) = schema_map.get(yaml_path) else {
return "DefaultValue::Missing".to_string();
};
match (&info.value_type, &info.default) {
(FieldType::Duration, Some(default)) => {
let default_value: serde_json::Value =
serde_json::from_str(default).expect("schema duration default must be JSON");
let default_str = default_value
.as_str()
.expect("schema duration default must be a string");
let nanos = go_parse_duration::parse_duration(default_str)
.expect("schema duration default must be a valid Go duration");
format!("DefaultValue::DurationNanos({})", nanos)
}
(_, Some(default)) => {
format!("DefaultValue::Json(\"{}\")", escape_str(default))
}
(_, None) => "DefaultValue::Missing".to_string(),
}
}If neither crate passes vetting, then I think the fallback should be to move our existing duration parsing primitive into a smaller lower-level shared crate/module and have both saluki-config and datadog-agent-config use it. The important part is that the parser has a single owner.
The main point: I like the bugfix direction, but I would rather put the duration-specific behavior into the generated type/default model than add is_duration and reparsing logic to the classifier path.
There was a problem hiding this comment.
👍 I like the explicit modeling of duration types. Started a draft follow-up with this direction here: #1966
…lassifier-duration-default # Conflicts: # lib/datadog-agent/config/src/classifier/classifier_data.rs
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
jszwedko/fix-config-classifier-duration-default
branch
…lassifier (#1958) ## Summary The nightly integration suite (which runs against the latest Datadog Agent master rather than the pinned release) started failing because ADP emitted a spurious `Unsupported configuration key` warning for `tls_handshake_timeout`. The key was at its default the whole time — the classifier just couldn't tell, because it compares the Agent-supplied value against the schema default with exact JSON equality, and duration keys don't survive that comparison: the schema expresses their default as a Go duration string (`10s`) while the Agent transmits durations over the config stream as integer nanoseconds (`10000000000`). The result is a false "override" for any duration key the Agent populates at its default. This restores the intended behavior — a key sitting at its default is never treated as an incompatible override — by teaching the classifier that duration defaults and duration values are the same quantity in two encodings. ```mermaid flowchart LR A["Agent config stream<br/>tls_handshake_timeout = 10000000000 (ns)"] --> C{is_default?} S["Schema default<br/>10s (Go duration string)"] --> C C -- "before: String != Number<br/>=> false => WARN" --> W["Unsupported configuration key"] C -- "after: normalize both to ns<br/>10e9 == 10e9 => true" --> OK["Recognized as default (no warning)"] ``` The fix threads the schema's `format: duration` marker through codegen into each `ClassifierEntry` (`is_duration`), and `is_default_value` normalizes both sides to nanoseconds before comparing. It applies to every duration-typed key in the classifier, not just `tls_handshake_timeout`. No schema/overlay edits were required. ## Test plan - [x] New unit tests in `classifier.rs`: a duration key sent as nanoseconds is recognized as its default; a non-default nanosecond value is not; and `parse_go_duration_nanos` covers the common Go duration forms (`10s`, `500ms`, `1h30m0s`, `1.5h`, `250µs`, `-5s`, bare `0`, and malformed input). - [x] Reproduced the nightly failure locally against Agent `master-py3-full`, then confirmed `adp-config-check-no-warn` and `adp-config-stream` pass with this change. > [!NOTE] > This addresses one of two independent regressions surfaced by the nightly. The remaining failures (`*-api-endpoints`, `adp-config-dynamic-basic`, `dogstatsd-forwarding`) are a separate issue — under the new Agent, ADP's API binds default ports instead of honoring `DD_DATA_PLANE__API_LISTEN_ADDRESS` — and will be handled separately. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: jesse.szwedko <jesse.szwedko@datadoghq.com> 94ecf81
## Summary Follow-up to #1958 (now merged), taking the alternative design proposed in review (#1958 (comment)): model `format: duration` as a first-class type and store **parsed** duration defaults in the generated classifier data, instead of carrying an `is_duration` side channel and reparsing a schema string on every classification. The base type is modeled correctly, and an invalid duration default now breaks the build rather than misbehaving at runtime. Key changes: - **New `go-duration` leaf crate** owns Go `time.ParseDuration` parsing. `saluki-config`'s `DurationString` delegates to it, and the classifier build script uses it too — so the algorithm has a single owner instead of being reimplemented (the parser #1958 added to the classifier is removed). An upstream crate (for example `go-parse-duration`) can replace this later after vetting; keeping it in-tree for now avoids taking an unvetted external dependency. - **`FieldType::Duration`** (plus a matching `ValueType::Duration` for the config-testing smoke tests) replaces the `is_duration` flag. `parse_value_type` folds `format: duration` in and panics on an illogical base type. Smoke tests use a generic `"42s"` value so future duration keys need no bespoke `test_json`. - **Typed `DefaultValue`** (`Missing` / `Json(&'static str)` / `DurationNanos(u64)`) replaces `Option<&str>` + `is_duration` on `ClassifierEntry`. Codegen parses duration defaults to nanoseconds via `go-duration` and **fails the build** if a `format: duration` default isn't a valid Go duration. The runtime default check simply switches on the typed default; it never parses a schema default. Behavior matches #1958 for the `tls_handshake_timeout` bug (a default-valued duration is recognized as default whether sent as nanoseconds or a Go duration string), but the model is cleaner and the failure mode moves to compile time. ## Test plan - [x] `go-duration` unit tests (Go-style units, signs/fractions/zero, largest value, invalid/overflow, error messages). - [x] `saluki-config` duration tests pass against the delegated parser. - [x] Classifier unit tests updated for `DefaultValue` (per-variant `is_default_value` coverage; duration matched both as nanoseconds and as a Go duration string). - [x] `cargo check --workspace --tests` and `cargo clippy` clean; config-testing smoke tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: jesse.szwedko <jesse.szwedko@datadoghq.com>
## Summary Follow-up to #1958 (now merged), taking the alternative design proposed in review (#1958 (comment)): model `format: duration` as a first-class type and store **parsed** duration defaults in the generated classifier data, instead of carrying an `is_duration` side channel and reparsing a schema string on every classification. The base type is modeled correctly, and an invalid duration default now breaks the build rather than misbehaving at runtime. Key changes: - **New `go-duration` leaf crate** owns Go `time.ParseDuration` parsing. `saluki-config`'s `DurationString` delegates to it, and the classifier build script uses it too — so the algorithm has a single owner instead of being reimplemented (the parser #1958 added to the classifier is removed). An upstream crate (for example `go-parse-duration`) can replace this later after vetting; keeping it in-tree for now avoids taking an unvetted external dependency. - **`FieldType::Duration`** (plus a matching `ValueType::Duration` for the config-testing smoke tests) replaces the `is_duration` flag. `parse_value_type` folds `format: duration` in and panics on an illogical base type. Smoke tests use a generic `"42s"` value so future duration keys need no bespoke `test_json`. - **Typed `DefaultValue`** (`Missing` / `Json(&'static str)` / `DurationNanos(u64)`) replaces `Option<&str>` + `is_duration` on `ClassifierEntry`. Codegen parses duration defaults to nanoseconds via `go-duration` and **fails the build** if a `format: duration` default isn't a valid Go duration. The runtime default check simply switches on the typed default; it never parses a schema default. Behavior matches #1958 for the `tls_handshake_timeout` bug (a default-valued duration is recognized as default whether sent as nanoseconds or a Go duration string), but the model is cleaner and the failure mode moves to compile time. ## Test plan - [x] `go-duration` unit tests (Go-style units, signs/fractions/zero, largest value, invalid/overflow, error messages). - [x] `saluki-config` duration tests pass against the delegated parser. - [x] Classifier unit tests updated for `DefaultValue` (per-variant `is_default_value` coverage; duration matched both as nanoseconds and as a Go duration string). - [x] `cargo check --workspace --tests` and `cargo clippy` clean; config-testing smoke tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: jesse.szwedko <jesse.szwedko@datadoghq.com> 7b98c6f
2 participants
Summary
The nightly integration suite (which runs against the latest Datadog Agent master rather than the pinned release) started failing because ADP emitted a spurious
Unsupported configuration keywarning fortls_handshake_timeout. The key was at its default the whole time — the classifier just couldn't tell, because it compares the Agent-supplied value against the schema default with exact JSON equality, and duration keys don't survive that comparison: the schema expresses their default as a Go duration string (10s) while the Agent transmits durations over the config stream as integer nanoseconds (10000000000). The result is a false "override" for any duration key the Agent populates at its default. This restores the intended behavior — a key sitting at its default is never treated as an incompatible override — by teaching the classifier that duration defaults and duration values are the same quantity in two encodings.flowchart LR A["Agent config stream<br/>tls_handshake_timeout = 10000000000 (ns)"] --> C{is_default?} S["Schema default<br/>10s (Go duration string)"] --> C C -- "before: String != Number<br/>=> false => WARN" --> W["Unsupported configuration key"] C -- "after: normalize both to ns<br/>10e9 == 10e9 => true" --> OK["Recognized as default (no warning)"]The fix threads the schema's
format: durationmarker through codegen into eachClassifierEntry(is_duration), andis_default_valuenormalizes both sides to nanoseconds before comparing. It applies to every duration-typed key in the classifier, not justtls_handshake_timeout. No schema/overlay edits were required.Test plan
classifier.rs: a duration key sent as nanoseconds is recognized as its default; a non-default nanosecond value is not; andparse_go_duration_nanoscovers the common Go duration forms (10s,500ms,1h30m0s,1.5h,250µs,-5s, bare0, and malformed input).master-py3-full, then confirmedadp-config-check-no-warnandadp-config-streampass with this change.Note
This addresses one of two independent regressions surfaced by the nightly. The remaining failures (
*-api-endpoints,adp-config-dynamic-basic,dogstatsd-forwarding) are a separate issue — under the new Agent, ADP's API binds default ports instead of honoringDD_DATA_PLANE__API_LISTEN_ADDRESS— and will be handled separately.🤖 Generated with Claude Code