chore(config): migrate HostTagsConfiguration to typed config#1996
Conversation
There was a problem hiding this comment.
Pull request overview
Migrates the HostTagsConfiguration component off the raw GenericConfiguration map and onto the typed SalukiConfiguration model (specifically shared.tags.expected_tags_duration), continuing the ADP config cutover work.
Changes:
- Updated
HostTagsConfiguration::from_configurationto accept a pre-builtRemoteAgentClientConfigurationplus typedGlobalTags, removing direct parsing ofexpected_tags_duration. - Updated the CLI pipeline wiring to build the remote-agent client config at the call site and pass typed tag settings into
HostTagsConfiguration.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| bin/agent-data-plane/src/components/host_tags/mod.rs | Drops GenericConfiguration/DurationString parsing and reads expected_tags_duration from typed GlobalTags. |
| bin/agent-data-plane/src/cli/run.rs | Builds RemoteAgentClientConfiguration locally and passes saluki.shared.tags into the host-tags transform config. |
Comments suppressed due to low confidence (1)
bin/agent-data-plane/src/cli/run.rs:491
config_system.config()is loaded inside thestandalone_modeconditional and then immediately loaded again afterwards. This adds an extra ArcSwap load and increases the chance that host-tags and metrics-encoder get built from different config snapshots. Consider loading the config once and reusing the same guard for both.
if !dp_config.standalone_mode() {
let client_config = RemoteAgentClientConfiguration::from_configuration(config)?;
let saluki = config_system.config();
let host_tags_config = HostTagsConfiguration::from_configuration(client_config, &saluki.shared.tags);
if host_tags_config.enabled() {
metrics_enrich_config = metrics_enrich_config.with_transform_builder("host_tags", host_tags_config);
}
}
let saluki = config_system.config();
let dd_metrics_config =
DatadogMetricsConfiguration::from_configuration(&saluki.shared.metrics_encoding, &saluki.shared.endpoints)
.error_context("Failed to configure Datadog Metrics encoder.")?;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Binary Size Analysis (Agent Data Plane)Baseline: 7b98c6f · Comparison: ed62b57 · 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 ( |
webern
left a comment
There was a problem hiding this comment.
Claude Code operating on behalf of webern. Clean-room review completed. I found one input-sanitization/behavioral regression that should be fixed before merging; config reachability and the zero default otherwise look preserved. The existing Copilot review contained no actionable inline finding, and the performance/binary-size bot reports are not relevant to this config cutover.
| client_config, | ||
| expected_tags_duration, | ||
| }) | ||
| expected_tags_duration: tags.expected_tags_duration, |
There was a problem hiding this comment.
[P1] Preserve rejection of negative numeric durations. Before this migration, GenericConfiguration::try_get_typed::<DurationString> rejected negative numeric values and HostTagsConfiguration::from_configuration propagated that error. The typed DatadogConfiguration path uses deserialize_go_duration, whose numeric visitor clamps negative values to zero. Consequently, expected_tags_duration: -1 now boots with host-tag enrichment silently disabled instead of rejecting the invalid configuration. Please preserve the old validation in the typed layer (including its DurationString overflow bounds) or otherwise make this cutover behavior-equivalent.
Build HostTagsConfiguration from the typed SalukiConfiguration model rather than GenericConfiguration. The expected_tags_duration key is witnessed and already modeled at shared.tags.expected_tags_duration, so the component now reads it from the typed slice. The remote-agent client configuration (a shared, not-yet-migrated struct) is built at the call site and passed in, removing GenericConfiguration from the component.
The typed Datadog config path deserializes duration fields via deserialize_go_duration. Its numeric visitors clamped negative values to zero and accepted nanosecond counts beyond Go's time.Duration bound, diverging from the saluki_config::DurationString behavior it replaced. For example, migrating HostTagsConfiguration to the typed model meant expected_tags_duration: -1 silently became 0s (disabling host-tag enrichment) instead of failing startup. Reject negative numeric and string durations and enforce the i64::MAX nanosecond overflow bound in the deserializer, matching DurationString. Go duration strings, numeric nanoseconds, and schema defaults (0s) are unchanged.
webern
left a comment
There was a problem hiding this comment.
Claude Code operating on behalf of webern. The negative-number and numeric-overflow fix is correct, but the follow-up review found one remaining DurationString compatibility gap in the same typed boundary.
| /// numeric values clamp to zero, since `Duration` cannot represent them. | ||
| /// `duration_value_as_nanos`); a string is parsed with the shared `go-duration` parser. | ||
| /// | ||
| /// Acceptance and rejection match `saluki_config::DurationString`, the type this replaced on the |
There was a problem hiding this comment.
[P1] Preserve the legacy string coercion too. DurationString does more than call parse_duration: it trims whitespace and, if Go-duration parsing fails, accepts a bare integer string as nanoseconds. Thus expected_tags_duration: "5" (and env-var input DD_EXPECTED_TAGS_DURATION=5) previously produced 5 ns, while this typed path rejects it; padded input such as " 5s" also previously worked. That leaves the cutover behaviorally incompatible despite this statement. Please use equivalent string parsing here (preferably shared rather than duplicated) and add coverage for bare-integer and whitespace-padded strings, including overflow/negative fallback cases.
The typed config deserializer (deserialize_go_duration) and
saluki_config::DurationString each need to accept exactly what the
Datadog Agent's viper/cast configuration coercion accepts. That contract
was implemented independently in both places and drifted: the typed path
called go_duration::parse_duration directly, so unlike DurationString it
rejected whitespace-padded strings, bare-integer nanosecond strings such
as "5" (how DD_EXPECTED_TAGS_DURATION=5 arrives), and applied the
negative/overflow rules inconsistently.
Move the coercion into the go-duration crate, its single owner:
parse_viper_duration (trim + Go grammar + bare-integer-nanoseconds
fallback) plus checked_duration_from_nanos_{i128,u128,f64} for the shared
negative/overflow guards, with MAX_NANOS_U64 now public. parse_duration
stays the strict Go grammar primitive.
DurationString and deserialize_go_duration are both reimplemented on top
of these functions, so the typed path and DurationString are provably the
same coercion. No new dependencies (go-duration stays std-only) and no
saluki-config dependency is added to datadog-agent-config.
Tests: go-duration gains coverage for the coercion function and numeric
guards; the typed path gains bare-integer-string, whitespace-padding, and
negative/overflow-via-fallback cases. DurationString's existing suite is
unchanged and still passes.
## Human Summary It thrashed a bit on preserving the exact behavior that `DurationString` was providing and on viper `time.Duration` parsing correctness. Overall the changes it made seem fine to me. ## AI Summary Migrates `HostTagsConfiguration` from the raw `GenericConfiguration` map to the typed `SalukiConfiguration` model, continuing the ADP config cutover. `expected_tags_duration` is witnessed and already modeled at `shared.tags.expected_tags_duration`, so the component now reads it directly from the typed slice instead of parsing a `DurationString` off `GenericConfiguration`. The component's hand-written zero default is dropped; the generated schema default (`0s`) is authoritative. `RemoteAgentClientConfiguration` is a shared struct still consumed by several not-yet-migrated remote-agent components, so it stays on `GenericConfiguration` for now. It is built at the call site and passed into `HostTagsConfiguration`, which removes `GenericConfiguration` from the component entirely. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? `make check-all`, `make test`, and `make check-docs` all pass. Translation of `expected_tags_duration` into the typed model is already covered by existing translator and config-system tests. ## References - Progresses #1788 - Merges into #1987
## Human Summary It thrashed a bit on preserving the exact behavior that `DurationString` was providing and on viper `time.Duration` parsing correctness. Overall the changes it made seem fine to me. ## AI Summary Migrates `HostTagsConfiguration` from the raw `GenericConfiguration` map to the typed `SalukiConfiguration` model, continuing the ADP config cutover. `expected_tags_duration` is witnessed and already modeled at `shared.tags.expected_tags_duration`, so the component now reads it directly from the typed slice instead of parsing a `DurationString` off `GenericConfiguration`. The component's hand-written zero default is dropped; the generated schema default (`0s`) is authoritative. `RemoteAgentClientConfiguration` is a shared struct still consumed by several not-yet-migrated remote-agent components, so it stays on `GenericConfiguration` for now. It is built at the call site and passed into `HostTagsConfiguration`, which removes `GenericConfiguration` from the component entirely. ## Change Type - [x] Non-functional (chore, refactoring, docs) ## How did you test this PR? `make check-all`, `make test`, and `make check-docs` all pass. Translation of `expected_tags_duration` into the typed model is already covered by existing translator and config-system tests. ## References - Progresses #1788 - Merges into #1987
It thrashed a bit on preserving the exact behavior that `DurationString` was providing and on viper `time.Duration` parsing correctness. Overall the changes it made seem fine to me. Migrates `HostTagsConfiguration` from the raw `GenericConfiguration` map to the typed `SalukiConfiguration` model, continuing the ADP config cutover. `expected_tags_duration` is witnessed and already modeled at `shared.tags.expected_tags_duration`, so the component now reads it directly from the typed slice instead of parsing a `DurationString` off `GenericConfiguration`. The component's hand-written zero default is dropped; the generated schema default (`0s`) is authoritative. `RemoteAgentClientConfiguration` is a shared struct still consumed by several not-yet-migrated remote-agent components, so it stays on `GenericConfiguration` for now. It is built at the call site and passed into `HostTagsConfiguration`, which removes `GenericConfiguration` from the component entirely. - [x] Non-functional (chore, refactoring, docs) `make check-all`, `make test`, and `make check-docs` all pass. Translation of `expected_tags_duration` into the typed model is already covered by existing translator and config-system tests. - Progresses #1788 - Merges into #1987
It thrashed a bit on preserving the exact behavior that `DurationString` was providing and on viper `time.Duration` parsing correctness. Overall the changes it made seem fine to me. Migrates `HostTagsConfiguration` from the raw `GenericConfiguration` map to the typed `SalukiConfiguration` model, continuing the ADP config cutover. `expected_tags_duration` is witnessed and already modeled at `shared.tags.expected_tags_duration`, so the component now reads it directly from the typed slice instead of parsing a `DurationString` off `GenericConfiguration`. The component's hand-written zero default is dropped; the generated schema default (`0s`) is authoritative. `RemoteAgentClientConfiguration` is a shared struct still consumed by several not-yet-migrated remote-agent components, so it stays on `GenericConfiguration` for now. It is built at the call site and passed into `HostTagsConfiguration`, which removes `GenericConfiguration` from the component entirely. - [x] Non-functional (chore, refactoring, docs) `make check-all`, `make test`, and `make check-docs` all pass. Translation of `expected_tags_duration` into the typed model is already covered by existing translator and config-system tests. - Progresses #1788 - Merges into #1987
It thrashed a bit on preserving the exact behavior that `DurationString` was providing and on viper `time.Duration` parsing correctness. Overall the changes it made seem fine to me. Migrates `HostTagsConfiguration` from the raw `GenericConfiguration` map to the typed `SalukiConfiguration` model, continuing the ADP config cutover. `expected_tags_duration` is witnessed and already modeled at `shared.tags.expected_tags_duration`, so the component now reads it directly from the typed slice instead of parsing a `DurationString` off `GenericConfiguration`. The component's hand-written zero default is dropped; the generated schema default (`0s`) is authoritative. `RemoteAgentClientConfiguration` is a shared struct still consumed by several not-yet-migrated remote-agent components, so it stays on `GenericConfiguration` for now. It is built at the call site and passed into `HostTagsConfiguration`, which removes `GenericConfiguration` from the component entirely. - [x] Non-functional (chore, refactoring, docs) `make check-all`, `make test`, and `make check-docs` all pass. Translation of `expected_tags_duration` into the typed model is already covered by existing translator and config-system tests. - Progresses #1788 - Merges into #1987
It thrashed a bit on preserving the exact behavior that `DurationString` was providing and on viper `time.Duration` parsing correctness. Overall the changes it made seem fine to me. Migrates `HostTagsConfiguration` from the raw `GenericConfiguration` map to the typed `SalukiConfiguration` model, continuing the ADP config cutover. `expected_tags_duration` is witnessed and already modeled at `shared.tags.expected_tags_duration`, so the component now reads it directly from the typed slice instead of parsing a `DurationString` off `GenericConfiguration`. The component's hand-written zero default is dropped; the generated schema default (`0s`) is authoritative. `RemoteAgentClientConfiguration` is a shared struct still consumed by several not-yet-migrated remote-agent components, so it stays on `GenericConfiguration` for now. It is built at the call site and passed into `HostTagsConfiguration`, which removes `GenericConfiguration` from the component entirely. - [x] Non-functional (chore, refactoring, docs) `make check-all`, `make test`, and `make check-docs` all pass. Translation of `expected_tags_duration` into the typed model is already covered by existing translator and config-system tests. - Progresses #1788 - Merges into #1987
It thrashed a bit on preserving the exact behavior that `DurationString` was providing and on viper `time.Duration` parsing correctness. Overall the changes it made seem fine to me. Migrates `HostTagsConfiguration` from the raw `GenericConfiguration` map to the typed `SalukiConfiguration` model, continuing the ADP config cutover. `expected_tags_duration` is witnessed and already modeled at `shared.tags.expected_tags_duration`, so the component now reads it directly from the typed slice instead of parsing a `DurationString` off `GenericConfiguration`. The component's hand-written zero default is dropped; the generated schema default (`0s`) is authoritative. `RemoteAgentClientConfiguration` is a shared struct still consumed by several not-yet-migrated remote-agent components, so it stays on `GenericConfiguration` for now. It is built at the call site and passed into `HostTagsConfiguration`, which removes `GenericConfiguration` from the component entirely. - [x] Non-functional (chore, refactoring, docs) `make check-all`, `make test`, and `make check-docs` all pass. Translation of `expected_tags_duration` into the typed model is already covered by existing translator and config-system tests. - Progresses #1788 - Merges into #1987
It thrashed a bit on preserving the exact behavior that `DurationString` was providing and on viper `time.Duration` parsing correctness. Overall the changes it made seem fine to me. Migrates `HostTagsConfiguration` from the raw `GenericConfiguration` map to the typed `SalukiConfiguration` model, continuing the ADP config cutover. `expected_tags_duration` is witnessed and already modeled at `shared.tags.expected_tags_duration`, so the component now reads it directly from the typed slice instead of parsing a `DurationString` off `GenericConfiguration`. The component's hand-written zero default is dropped; the generated schema default (`0s`) is authoritative. `RemoteAgentClientConfiguration` is a shared struct still consumed by several not-yet-migrated remote-agent components, so it stays on `GenericConfiguration` for now. It is built at the call site and passed into `HostTagsConfiguration`, which removes `GenericConfiguration` from the component entirely. - [x] Non-functional (chore, refactoring, docs) `make check-all`, `make test`, and `make check-docs` all pass. Translation of `expected_tags_duration` into the typed model is already covered by existing translator and config-system tests. - Progresses #1788 - Merges into #1987
It thrashed a bit on preserving the exact behavior that `DurationString` was providing and on viper `time.Duration` parsing correctness. Overall the changes it made seem fine to me. Migrates `HostTagsConfiguration` from the raw `GenericConfiguration` map to the typed `SalukiConfiguration` model, continuing the ADP config cutover. `expected_tags_duration` is witnessed and already modeled at `shared.tags.expected_tags_duration`, so the component now reads it directly from the typed slice instead of parsing a `DurationString` off `GenericConfiguration`. The component's hand-written zero default is dropped; the generated schema default (`0s`) is authoritative. `RemoteAgentClientConfiguration` is a shared struct still consumed by several not-yet-migrated remote-agent components, so it stays on `GenericConfiguration` for now. It is built at the call site and passed into `HostTagsConfiguration`, which removes `GenericConfiguration` from the component entirely. - [x] Non-functional (chore, refactoring, docs) `make check-all`, `make test`, and `make check-docs` all pass. Translation of `expected_tags_duration` into the typed model is already covered by existing translator and config-system tests. - Progresses #1788 - Merges into #1987
Human Summary
It thrashed a bit on preserving the exact behavior that
DurationStringwas providing and on vipertime.Durationparsing correctness. Overall the changes it made seem fine to me.AI Summary
Migrates
HostTagsConfigurationfrom the rawGenericConfigurationmap to the typedSalukiConfigurationmodel, continuing the ADP config cutover.expected_tags_durationis witnessed and already modeled atshared.tags.expected_tags_duration, so the component now reads it directly from the typedslice instead of parsing a
DurationStringoffGenericConfiguration. The component'shand-written zero default is dropped; the generated schema default (
0s) is authoritative.RemoteAgentClientConfigurationis a shared struct still consumed by several not-yet-migratedremote-agent components, so it stays on
GenericConfigurationfor now. It is built at the callsite and passed into
HostTagsConfiguration, which removesGenericConfigurationfrom thecomponent entirely.
Change Type
How did you test this PR?
make check-all,make test, andmake check-docsall pass. Translation ofexpected_tags_durationinto the typed model is already covered by existing translator andconfig-system tests.
References