feat(ffe): add runtime-backed PHP feature flag evaluation#3906
feat(ffe): add runtime-backed PHP feature flag evaluation#3906leoromanovsky wants to merge 19 commits into
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 782f622 | Docs | Datadog PR Page | Give us feedback! |
|
Benchmarks [ tracer ]Benchmark execution time: 2026-05-27 16:28:55 Comparing candidate commit d82e50d in PR branch Found 2 performance improvements and 6 performance regressions! Performance is the same for 186 metrics, 0 unstable metrics. scenario:ComposerTelemetryBench/benchTelemetryParsing-opcache
scenario:ContextPropagationBench/benchInject128Bit-opcache
scenario:ContextPropagationBench/benchInject64Bit-opcache
scenario:EmptyFileBench/benchEmptyFileBaseline
scenario:LogsInjectionBench/benchLogsInfoBaseline-opcache
scenario:LogsInjectionBench/benchLogsNullBaseline-opcache
scenario:PHPRedisBench/benchRedisBaseline
scenario:PHPRedisBench/benchRedisOverhead
|
…stone-1-runtime-evaluation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e55ebb976b
ℹ️ 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".
Brings the PHP FFE diagram convention to the M1 PR. Each subsequent PR in the stack (#3909, #3910, #3911) already carried its own stack + system diagram; #3906 was missing them. Mirrors the format used by the rest of the stack: - `stack-pr3906.mmd` — the 4-PR stack with #3906 badged as current and the downstream layers shown as "future". - `system-pr3906.mmd` — the target end-to-end architecture with M1's scope (UserCode, OpenFeature Client, DataDogProvider, DDTrace FeatureFlags Client, NativeEvaluator, Remote Config client) highlighted, and everything from the Hook layer onward dashed. All conventions match the other branches: quoted YAML titles (to keep `#PR-number` out of the YAML comment parser), `flowchart TD` orientation, rendered with `-w 2400 -H 2400 --scale 3 -b white`.
…stone-1-runtime-evaluation
|
Feedback from @bwoebi addressed; retest with local build against ffe-dogfooding server with satisfactory results. |
| variant: Some(string_to_zend_string( | ||
| assignment.variation_key.as_str().to_string(), | ||
| )), |
There was a problem hiding this comment.
| variant: Some(string_to_zend_string( | |
| assignment.variation_key.as_str().to_string(), | |
| )), | |
| variant: Some(assignment.variation_key.as_str().into()), |
should work, same for allocation_key and value_json in error branch
| } | ||
| } | ||
|
|
||
| void ddtrace_process_remote_config_now(void) { |
| { | ||
| $this->client = $client ?: FeatureFlagsClient::createWithDependencies( | ||
| null, | ||
| new NullLogger(LogLevel::EMERGENCY) |
There was a problem hiding this comment.
Is this intentional that it doesn't pass $this->warningLogger here?
| /** | ||
| * @internal Tests and Datadog-owned bridge adapters only. | ||
| */ | ||
| public static function createWithDependencies( |
There was a problem hiding this comment.
redundant with constructor.
What you probably want is:
public function __construct(?LoggerInterface $logger = null) // the public API, only has $logger
{
$this->warningLogger = $logger ?: new TriggerErrorLogger();
$this->client = FeatureFlagsClient::createWithDependencies(
null,
$this->warningLogger
);
}
public static function createWithDependencies(?FeatureFlagsClient $client = null, ?LoggerInterface $logger = null): self { // the internal test function, can swap the FeatureFlagsClient impl.
$instance = new self($logger);
if ($logger) {
$instance->logger = $logger;
}
}
Or, honestly, it's only for testing, right? Strip createWithDependencies fully from the API and assign it in the test:
$provider = new DataDogProvider($logger);
(fn() => $this->client = new RecordingLogger)->call($provider); // bypass private state directly in the test.
That would be my favourite approach.
| /** | ||
| * @internal Tests and Datadog-owned bridge adapters only. | ||
| */ | ||
| public static function createWithDependencies( |
There was a problem hiding this comment.
Same as for DataDogProvider, you probably should make the __construct public API, remove Evaluator as arg (logger becomes optional arg), and overwrite it in tests via Closure::call().
|
|
||
| version: 2 | ||
| updates: | ||
| - package-ecosystem: "gitsubmodule" |
There was a problem hiding this comment.
This is global, I don't want that for other submodules. If you can restrict it to the ffe tests submodule, fine by me.
| if (targeting_key != NULL) { | ||
| targeting_key_slice = dd_zend_string_to_CharSlice(targeting_key); | ||
| } |
There was a problem hiding this comment.
nit: dd_zend_string_to_CharSlice handles NULL itself already, if is redundant.
| * | ||
| * @internal Used by the Datadog feature flag client. | ||
| */ | ||
| function ffe_evaluate(string $flagKey, int $expectedType, ?string $targetingKey, array $attributes): ?array {} |
There was a problem hiding this comment.
minor suggestion, but feel free to ignore:
add
class FfeResult {
public ?string $valueJson = null;
public ?string $variant = null;
public ?string $allocationKey = null;
public int $reason;
public int $errorCode;
public bool $doLog;
}
and return that instead of array. Constructing/accessing objects is faster than arrays too, and better automcomplete etc. But not necessary.
Motivation
This PR adds real server-side PHP feature flag evaluation backed by Remote Config and the
libdatadognative evaluator. PHP 7 gets a Datadog API, PHP 8 can use the optional OpenFeature bridge, and both paths use the same live runtime evaluator.This is intentionally the evaluation layer only. Exposure delivery, exposure caching, and evaluation metrics land in the later hook, exposure, and metrics PRs.
Shared planning doc: https://docs.google.com/document/d/1NvMfTpZWLBlFmEFNjdnlMyeVpy5l7KD8qujGFco6w2w/edit?tab=t.0
Decisions
productionRuntime=falseremains a temporary guardrail while the hook, exposure delivery, and evaluation metrics layers are incomplete. The final production-ready PHP FFE stack should not ship with that warning enabled.bool,int,float,string) into the native evaluator. Nested arrays, objects, and null attribute values are dropped at the boundary.DDTrace\Testing\ffe_load_configexists only for local/canonical fixture tests.Where this PR fits in the stack
This is the bottom layer of the 4-PR stack. #3909 adds the shared hook, while #3910 (EVP exposures) and #3911 (OTLP metrics) build on top of that hook.
Where this PR fits in the target system
This PR contributes the in-PHP evaluation surface:
UserCode->OpenFeature Client(PHP 8) /DDTrace FeatureFlags Client(PHP 7 + 8), theDDTrace OpenFeature DataDogProvider, theNativeEvaluatorFFI bridge into libdatadog, and the Remote Config client for theFFE_FLAGSproduct.Changes
libdatadogFFE evaluator path and keeps PHP user-facing APIs as thin adapters over the native runtime.DDTrace\FeatureFlagsevaluation through live Remote Config.DDTrace\OpenFeatureprovider bridge.DataDog/ffe-system-test-datasubmodule and a native-runtime PHPT loop overufc-config.jsonandevaluation-cases/*.json.Not Included
Validation
php vendor/bin/phpunit --config phpunit.xml tests/api/Unit/FeatureFlags tests/OpenFeature/DataDogProviderTest.php: 29 tests / 87 assertions.make test_featureflags: 8 tests / 29 assertions.MAX_TEST_PARALLELISM=1 TESTS=tests/ext/ffe/native_bridge_evaluate.phpt make test_c: 1/1 passed.MAX_TEST_PARALLELISM=1 TESTS=tests/ext/ffe/system_test_data_evaluate.phpt make test_c: 1/1 passed.make test_internal_api_randomized: passed.Dogfooding Validation
See DataDog/ffe-dogfooding#68.
Ran the PHP 7 and PHP 8 servers locally; observed correct evaluations.