feat(openfeature): stateful merge of multi-environment UFC configs#17208
feat(openfeature): stateful merge of multi-environment UFC configs#17208
Conversation
Codeowners resolved as |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1745f1a99
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if "flags" in config: | ||
| merged_flags.update(config["flags"]) |
There was a problem hiding this comment.
Handle wrapped UFC configs when merging state
_merge_configurations() now only reads config["flags"] from each stored payload, but FFE payloads can be wrapped (for example {"someConfig": {"flags": ...}}) and were previously forwarded to process_ffe_configuration() intact. With this change, wrapped payloads contribute zero flags and the callback applies an empty merged config ({"flags": {}, "format": "SERVER"}), which effectively clears feature flag evaluations for those updates.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The wrapped shape in test_rc_callback_with_complex_config doesn't match production payloads. Every path that actually resolves flags uses flat {"flags": {...}}:
- The production fixture has
dd-trace-py/tests/openfeature/flags-v1.json
Lines 1 to 8 in 08b8a88
flagsat top level - create_config() (used by all integration tests that hit the native parser) produces
{"flags": {...}, "format": "SERVER"} - The RC client's _extract_target_file does
json.loads(raw)with no wrapping
The wrapped test only asserts no exception. It never verifies flag resolution. It was already effectively broken before this PR (the native parser accepts the JSON but finds zero flags since flags isn't at the top level).
c1745f1 to
061492e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 061492eeed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| def __init__(self): | ||
| self._config_state: dict[str, dict] = {} |
There was a problem hiding this comment.
Clear merged UFC state when RC client resets
FeatureFlagCallback now keeps _config_state for the lifetime of the process, but this state is never cleared when remote-config lifecycle resets occur. In the same codebase, RemoteConfigPoller.reset_at_fork() calls RemoteConfigClient.renew_id() (which clears applied configs), so after fork/restart the agent can replay only currently-active UFC files without deletion events for previously-known paths; those stale paths remain in _config_state and get merged back into evaluations. This causes deleted flags to persist until an explicit delete for the old path arrives, so the callback state should be reset (or callback instance replaced) on RC reset paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
real concern but not a practical one.
The fix would just be adding one line though:
forksafe.register(_featureflag_rc_callback._config_state.clear)
There was a problem hiding this comment.
The environments are configured at deploy time via DD_ENV and the LLMObs environment. They don't change between fork and first poll.
This comment has been minimized.
This comment has been minimized.
The FeatureFlagCallback replaced the entire FFE configuration on each RC payload, so when multiple UFC configs arrived (one per FFE environment), the last one won and the others' flags were lost. Config deletions (content=None) were also logged but never cleared. Maintain a dict[path -> parsed_config] in the callback and merge all UFC entries into a single unified configuration after each delta. This fixes both the multi-config overwrite and the deletion no-op.
061492e to
ffa8229
Compare
dd-oleksii
left a comment
There was a problem hiding this comment.
Hey. Do you have a tech design doc that we can discuss?
My first impression is that this PR violates our architecture, so I want to understand the issue better and chat through consequences, and other options before we continue
The main issue is that UFC is not designed to be merged on clients, they are all-or-nothing:
- merging makes UFC-level metadata invalid
- merging may override flags with the same names
- some identifiers are scoped to orgs, so want to make sure these will not clash
Another issue is that python layer is not supposed to know about the UFC format — only rust layer knows, so we have consistent behavior across tracers. So if we do decide to merge configs, it needs to be done there
|
Hey @dd-oleksii 👋
I don't have any formal design docs. I have investigation notes, but they're quite raw. I can share them to you, but they weren't meant to 😅 That's why I tried to make PR descriptions... descriptive. But basically, as explained in this Slack message (I've invited you to the channel), the problem is that for our use case, prompts need to reach all SDK clients in an org regardless of Hence, I needed a way to publish a flag config that targets all SDK clients in the org, regardless of their Considering that I had to look through many codebases i don't know the edge cases about, I do expect not to have thought about everything - tried my best to keep things moving forward and bring a ~minimal solution that would address my use case. About some of your concerns:
I do agree about this in theory, however, prompt flags would use the
What's the impact of it? id/createdAt/environment aren't used for evaluation, right?
HEnce, would a better approach bringing this feature (if it's the right approach to unblock our use case) to the rust-layer to ensure X-tracer consistency? Happy to continue the discussion on slack, as I believe it'll be much easier 👍 |
Description
The
FeatureFlagCallbackreplaced the entire FFE configuration on each RC payload, so when multiple UFC configs arrived (one per FFE environment), the last one won and the others' flags were lost. Config deletions (content=None) were also logged but never cleared.This PR maintains a
dict[path -> parsed_config]in the callback and merges all UFC entries into a single unified configuration after each delta. This is a prerequisite for the dedicated LLMObs environment work, where each SDK receives could two UFC configs via Remote Config: one for the customer'sDD_ENV(regular feature flags) and one for the LLMObs environment (prompt flags).How it works
Before (last-write-wins)
After (stateful merge)
Merge ordering
The merge uses
dict.updatewhich means if the same flag key appeared in multiple UFCs, one would silently overwrite the other. This is safe because prompt flags always use thellmobs.prompt.*key prefix, and regular feature flags never use that prefix. The two UFCs contain disjoint flag key sets by construction.Merge dependency
This PR must be merged before https://github.com/DataDog/dd-source/pull/380379 and #16854 - that dd-source PR introduces the dedicated LLMObs environment which will cause multiple UFC configs to be delivered via RC.
Testing
5 new tests in
tests/openfeature/test_remoteconfig.py:_set_ffe_config(None)is calledExisting tests preserved and refactored to use shared helpers.
Risks
None. The merge logic is a straightforward
dict.updateover 1-2 configs. The callback is not a hot path (async RC polling). The change is backward-compatible: a single UFC config works identically to before.Additional Notes