refactor(remote-config)!: consumer-owned product enum#1958
Draft
iunanua wants to merge 11 commits into
Draft
Conversation
…Data trait + ParserRegistry Replace the closed `RemoteConfigData` enum with an extensible trait-object model so product crates register their own parsers and `datadog-remote-config` has zero compile-time dependencies on product crates. Changes: - `datadog-remote-config/src/parse.rs`: removed `RemoteConfigData` enum; added `RemoteConfigParsedData` trait, `ProductParser` type alias, `ParserRegistry` struct, and `default_registry()` (pre-loads AgentConfig, AgentTask, ApmTracing parsers). Internal config types implement the trait. - `datadog-remote-config/src/file_storage.rs`: `ParseFile` is now instance- based with `type Parsed`; `RawBytesParser` and `RegistryParser` concrete types; `ParsedFileStorage` uses `RegistryParser(Arc<ParserRegistry>)`. - `datadog-remote-config/Cargo.toml`: removed `live-debugger` and `ffe` Cargo features and their optional deps. - `datadog-remote-config/src/path.rs`: `LiveDebugger` variant made unconditional (feature-gate removed together with the feature). - `datadog-remote-config/src/config/dynamic.rs`: added `Clone` to `DynamicConfigFile`, `DynamicConfigTarget`, `DynamicConfig`. - `datadog-live-debugger`: new `src/remote_config.rs` implements `RemoteConfigParsedData` for `LiveDebuggingData` and exports `live_debugger_parser()`; added `datadog-remote-config` dep. - `datadog-ffe`: new `src/remote_config.rs` implements `RemoteConfigParsedData` for `UniversalFlagConfig` and exports `ffe_parser()`; added `datadog-remote-config` dep. - `libdd-tracer-flare`: migrated to downcast pattern; `impl TryFrom<&dyn RemoteConfigParsedData> for FlareAction`; `handle_remote_config_data` takes `&dyn RemoteConfigParsedData`. - `datadog-sidecar`: `RemoteConfigManager` holds `Arc<ParserRegistry>`; `read_config()` takes registry; removed `features = ["live-debugger"]` from remote-config dep; downcast replaces `RemoteConfigData` match. - `examples/remote_config_fetch.rs`: updated to registry-based API. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ParserRegistry::parse was bailing on unregistered products, which differs from the old RemoteConfigData::Ignored(product) behavior. Consumers like RemoteConfigManager rely on getting a successful (Ignored) value to insert the config into active_configs, preventing repeated parse warnings on every fetch_update call. Restore the semantics by returning Ok(Box::new(IgnoredProduct(product))) instead of an error. IgnoredProduct implements RemoteConfigParsedData so callers can downcast to detect it if needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…igParsedData impls Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ChangesFetcher Move runtime-discovered extra services off `Target` (where they would force existing struct-literal callers to update and conflict with `Target`'s `Hash` / `Eq` / map-key role) onto the per-client mutable state owned by the fetcher. New API: - `ConfigClientState::set_extra_services(Vec<String>)` - `SingleFetcher::set_extra_services(Vec<String>)` - `SingleChangesFetcher::set_extra_services(Vec<String>)` `fetch_once` reads `extra_services` from `opaque_state` and forwards them on the wire via `ClientTracer.extra_services` exactly as before. Default is an empty vec, so the change is non-breaking for all existing callers. Replace-semantics: each set fully overrides the previous list. Test: `test_extra_services_forwarded_in_client_tracer` covers default empty state, forwarding after set, and replace semantics across three sequential polls. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…parsing Pre-refactor, datadog-sidecar enabled the `live-debugger` Cargo feature on `datadog-remote-config`, which made the closed-enum parser handle LiveDebugger payloads. After the dependency-inversion refactor that feature is gone — `RemoteConfigManager::new()` now defaulted to `default_registry()` (only AgentConfig / AgentTask / ApmTracing), so LiveDebugger configs flowing through the sidecar's manager were silently parsed as `IgnoredProduct`. Restore the old behavior by registering `live_debugger_parser()` explicitly in `RemoteConfigManager::new()`. Consumers building a custom registry can still call `new_with_registry` to opt out. Add `test_live_debugger_config_parsed`: a regression test that drives a `SERVICE_CONFIGURATION` LiveDebugger payload through the SHM round trip and asserts the downcast to `LiveDebuggingData` succeeds (rules out silent fallback to `IgnoredProduct`). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
Contributor
📚 Documentation Check Results📦
|
Contributor
🔒 Cargo Deny Results📦
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1958 +/- ##
==========================================
+ Coverage 71.85% 72.73% +0.88%
==========================================
Files 434 449 +15
Lines 70454 73749 +3295
==========================================
+ Hits 50624 53645 +3021
- Misses 19830 20104 +274
🚀 New features to boost your workflow:
|
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: c6b2735 | Docs | Datadog PR Page | Give us feedback! |
Contributor
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
…product enum Removes the dynamic-dispatch RemoteConfigParsedData trait + ParserRegistry + Box<dyn>+downcast pattern in favour of a consumer-owned, statically-typed product enum (option C). - datadog-remote-config: drop RemoteConfigParsedData, ProductParser, ParserRegistry, IgnoredProduct, default_registry, RegistryParser; replace with a generic RemoteConfigValue<T> and a BuiltinProducts enum (+ BuiltinProductsParser ZST) covering AgentConfig/AgentTask/ApmTracing. - datadog-live-debugger: replace live_debugger_parser() with an inherent LiveDebuggingData::parse(&[u8]) method; drop the trait impl. - datadog-ffe: drop the empty remote_config module entirely; consumers call UniversalFlagConfig::from_json directly. - datadog-sidecar: define SidecarProducts enum + SidecarParser ZST covering the four products the sidecar actually consumes plus Other; drop the Arc<ParserRegistry> from RemoteConfigManager and drive read_config via a typed match on RemoteConfigProduct. - libdd-tracer-flare: reuse BuiltinProducts; replace the TryFrom<&dyn RemoteConfigParsedData> impl with a TryFrom<&BuiltinProducts> match; drop downcast_ref calls in the listener loop. - examples/remote_config_fetch.rs: switch to BuiltinProducts. No new dependencies. Zero Box<dyn ...> remote-config types and zero downcast_ref calls in consumer code. cargo check --workspace is clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
iunanua
force-pushed
the
igor/rc/di-refactor-claude-no-registry
branch
from
df2d027 to
c6b2735
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Replaces the dynamic-dispatch
ParserRegistry+Box<dyn RemoteConfigParsedData>+as_any().downcast_ref::<T>()pattern indatadog-remote-configwith a consumer-owned, statically-typed product enum.datadog-remote-configexposes aBuiltinProductsenum covering the RC-internal products (AgentConfig,AgentTask,ApmTracing,Other) and aBuiltinProductsParserZST that consumers can plug intoRawFileStorage.RemoteConfigValuebecomes generic over the parsed payload (RemoteConfigValue<T>), so each consumer pins it to its own product enum.try_parse(product, data)function. The sidecar usesSidecarProducts(composesBuiltinProductsfor the shared variants and addsLiveDebugger); tracer-flare reusesBuiltinProductsdirectly.datadog-live-debuggerexposes an inherentLiveDebuggingData::parse(&[u8])— no trait impl, no parser factory.datadog-ffedrops its emptyremote_configmodule entirely; consumers callUniversalFlagConfig::from_jsondirectly.Motivation
The registry-based design pushed every consumer through
Box<dyn>allocation, runtimeHashMaplookup, andas_any().downcast_ref::<T>()at every use site. Three follow-on costs in practice:Ok(None)(parser not registered) plus a chain ofif let Some(x) = data.downcast_ref::<X>()arms — easy to silently miss a product when adding one.registry.with::<T>()call meant the product silently parsed asNone. We hit exactly this regression in4dbfab4ee(fix(sidecar): register live_debugger_parser to preserve pre-refactor parsing).Boxed and downcasted at consumption.The consumer-owned enum design makes the set of supported products explicit at the type level: forgetting a variant is a compile error, not a silent runtime miss; matches are exhaustive; there is zero
Box<dyn>and zero downcast in consumer code.