From fc3061f86ca15ebbdea96d56557fb2e404658f8a Mon Sep 17 00:00:00 2001 From: Leo Romanovsky Date: Mon, 27 Jul 2026 10:35:58 -0600 Subject: [PATCH 1/5] fix(openfeature): isolate provider warning failures --- src/DDTrace/OpenFeature/DataDogProvider.php | 7 +++- tests/OpenFeature/DataDogProviderTest.php | 46 ++++++++++++++++++++- 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/src/DDTrace/OpenFeature/DataDogProvider.php b/src/DDTrace/OpenFeature/DataDogProvider.php index 05c7e771f6d..1d328b078f7 100644 --- a/src/DDTrace/OpenFeature/DataDogProvider.php +++ b/src/DDTrace/OpenFeature/DataDogProvider.php @@ -230,8 +230,13 @@ private function warnIfNonProductionRuntime(EvaluationDetails $details): void $message = 'Datadog-backed PHP OpenFeature evaluation is not fully enabled yet.'; } - $this->warningLogger->warning($message); $this->warnedAboutNonProductionRuntime = true; + + try { + $this->warningLogger->warning($message); + } catch (\Throwable $ignored) { + // Diagnostic logging must not change the feature flag result. + } } private function mapReason(string $reason): string diff --git a/tests/OpenFeature/DataDogProviderTest.php b/tests/OpenFeature/DataDogProviderTest.php index 45334ca32fc..3b44d046c10 100644 --- a/tests/OpenFeature/DataDogProviderTest.php +++ b/tests/OpenFeature/DataDogProviderTest.php @@ -133,6 +133,42 @@ public function testProviderWarningIsEmittedOncePerProvider(): void self::assertSame(['temporary unavailable'], $logger->warnings()); } + public function testProviderWarningExceptionDoesNotChangeSuccessfulEvaluation(): void + { + $evaluator = new OpenFeatureTestEvaluator(); + $evaluator->setSuccess( + 'preview.flag', + true, + EvaluationReason::STATIC_REASON, + 'on', + ['productionRuntime' => false] + ); + $client = $this->openFeatureClientFor(DataDogProvider::createWithDependencies($evaluator)); + $warningCount = 0; + + set_error_handler( + static function ($severity, $message, $file, $line) use (&$warningCount): void { + ++$warningCount; + throw new \ErrorException($message, 0, $severity, $file, $line); + }, + E_USER_WARNING + ); + + try { + $firstDetails = $client->getBooleanDetails('preview.flag', false); + $secondDetails = $client->getBooleanDetails('preview.flag', false); + } finally { + restore_error_handler(); + } + + self::assertTrue($firstDetails->getValue()); + self::assertSame(EvaluationReason::STATIC_REASON, $firstDetails->getReason()); + self::assertSame('on', $firstDetails->getVariant()); + self::assertNull($firstDetails->getError()); + self::assertTrue($secondDetails->getValue()); + self::assertSame(1, $warningCount); + } + public function testProviderErrorsMapToOpenFeatureDetails(): void { $evaluator = new OpenFeatureTestEvaluator(); @@ -186,13 +222,19 @@ public function setSuccess( string $flagKey, mixed $value, string $reason = EvaluationReason::STATIC_REASON, - ?string $variant = null + ?string $variant = null, + array $providerState = [] ): self { $this->details[$flagKey] = new EvaluationDetails( $value, $this->typeForValue($value), $reason, - $variant + $variant, + null, + null, + [], + [], + $providerState ); return $this; From 7e3255ce980ee1ce2406a434bd2a13b61b76e4f4 Mon Sep 17 00:00:00 2001 From: Leo Romanovsky Date: Mon, 27 Jul 2026 10:43:58 -0600 Subject: [PATCH 2/5] fix(feature-flags): make logging non-throwing --- src/DDTrace/OpenFeature/DataDogProvider.php | 12 +-- src/api/FeatureFlags/Client.php | 6 +- src/api/Log/NonThrowingLogger.php | 52 +++++++++++ src/bridge/_files_tracer.php | 1 + tests/api/Unit/FeatureFlags/ClientTest.php | 40 ++++++++- tests/api/Unit/Log/NonThrowingLoggerTest.php | 92 ++++++++++++++++++++ 6 files changed, 192 insertions(+), 11 deletions(-) create mode 100644 src/api/Log/NonThrowingLogger.php create mode 100644 tests/api/Unit/Log/NonThrowingLoggerTest.php diff --git a/src/DDTrace/OpenFeature/DataDogProvider.php b/src/DDTrace/OpenFeature/DataDogProvider.php index 1d328b078f7..72388b8ba48 100644 --- a/src/DDTrace/OpenFeature/DataDogProvider.php +++ b/src/DDTrace/OpenFeature/DataDogProvider.php @@ -13,6 +13,7 @@ use DDTrace\FeatureFlags\Internal\Metric\EvaluationMetricRecorder; use DDTrace\FeatureFlags\Internal\NativeEvaluator; use DDTrace\Log\LoggerInterface; +use DDTrace\Log\NonThrowingLogger; use DDTrace\Log\TriggerErrorLogger; use OpenFeature\implementation\provider\AbstractProvider; use OpenFeature\implementation\provider\ResolutionDetailsBuilder; @@ -30,7 +31,7 @@ final class DataDogProvider extends AbstractProvider protected static string $NAME = 'Datadog'; private Evaluator $evaluator; - private LoggerInterface $warningLogger; + private NonThrowingLogger $safeLogger; private bool $warnedAboutNonProductionRuntime = false; private EvaluationMetricRecorder $metricRecorder; @@ -39,7 +40,7 @@ public function __construct(?LoggerInterface $logger = null) // Native evaluation metrics are disabled here because OpenFeature owns // the final provider outcome, including OF-level type mismatch mapping. $this->evaluator = NativeEvaluator::create(false); - $this->warningLogger = $logger ?: new TriggerErrorLogger(); + $this->safeLogger = new NonThrowingLogger($logger ?: new TriggerErrorLogger()); $this->metricRecorder = EvaluationMetricRecorder::createDefault(); } @@ -231,12 +232,7 @@ private function warnIfNonProductionRuntime(EvaluationDetails $details): void } $this->warnedAboutNonProductionRuntime = true; - - try { - $this->warningLogger->warning($message); - } catch (\Throwable $ignored) { - // Diagnostic logging must not change the feature flag result. - } + $this->safeLogger->warning($message); } private function mapReason(string $reason): string diff --git a/src/api/FeatureFlags/Client.php b/src/api/FeatureFlags/Client.php index b42bbfcbcdd..2849ccc441a 100644 --- a/src/api/FeatureFlags/Client.php +++ b/src/api/FeatureFlags/Client.php @@ -4,11 +4,13 @@ use DDTrace\FeatureFlags\Internal\NativeEvaluator; use DDTrace\Log\LoggerInterface; +use DDTrace\Log\NonThrowingLogger; use DDTrace\Log\TriggerErrorLogger; final class Client { private $evaluator; + /** @var NonThrowingLogger */ private $logger; private $warnedAboutNonProductionRuntime = false; @@ -19,7 +21,7 @@ public function __construct($logger = null) } $this->evaluator = NativeEvaluator::create(); - $this->logger = $logger ?: new TriggerErrorLogger(); + $this->logger = new NonThrowingLogger($logger ?: new TriggerErrorLogger()); } /** @@ -155,8 +157,8 @@ private function warnIfNonProductionRuntime(EvaluationDetails $details) $message = 'Datadog-backed PHP feature flag evaluation is running without exposure and metric reporting in this milestone.'; } - $this->logger->warning($message); $this->warnedAboutNonProductionRuntime = true; + $this->logger->warning($message); } private function expectFlagKey($flagKey) diff --git a/src/api/Log/NonThrowingLogger.php b/src/api/Log/NonThrowingLogger.php new file mode 100644 index 00000000000..c51fd8c93c4 --- /dev/null +++ b/src/api/Log/NonThrowingLogger.php @@ -0,0 +1,52 @@ +logger = $logger; + } + + public function debug($message, array $context = array()) + { + try { + $this->logger->debug($message, $context); + } catch (\Throwable $ignored) { + } + } + + public function warning($message, array $context = array()) + { + try { + $this->logger->warning($message, $context); + } catch (\Throwable $ignored) { + } + } + + public function error($message, array $context = array()) + { + try { + $this->logger->error($message, $context); + } catch (\Throwable $ignored) { + } + } + + public function isLevelActive($level) + { + try { + return (bool) $this->logger->isLevelActive($level); + } catch (\Throwable $ignored) { + return false; + } + } +} diff --git a/src/bridge/_files_tracer.php b/src/bridge/_files_tracer.php index 017d07ca669..c6d30e49c7b 100644 --- a/src/bridge/_files_tracer.php +++ b/src/bridge/_files_tracer.php @@ -29,6 +29,7 @@ __DIR__ . '/../api/Log/ErrorLogLogger.php', __DIR__ . '/../api/Log/NullLogger.php', __DIR__ . '/../api/Log/TriggerErrorLogger.php', + __DIR__ . '/../api/Log/NonThrowingLogger.php', __DIR__ . '/../api/Log/LoggingTrait.php', __DIR__ . '/../api/StartSpanOptions.php', __DIR__ . '/../DDTrace/SpanContext.php', diff --git a/tests/api/Unit/FeatureFlags/ClientTest.php b/tests/api/Unit/FeatureFlags/ClientTest.php index b2bc83d8321..19c8a800ee4 100644 --- a/tests/api/Unit/FeatureFlags/ClientTest.php +++ b/tests/api/Unit/FeatureFlags/ClientTest.php @@ -129,6 +129,44 @@ public function testWarningIsEmittedOncePerClientNotOncePerEvaluation() $this->assertCount(1, $logger->warnings()); } + public function testWarningExceptionDoesNotChangeSuccessfulEvaluation() + { + $evaluator = new ClientTestEvaluator(); + $evaluator->setSuccess( + 'preview.flag', + true, + EvaluationReason::STATIC_REASON, + 'on', + array(), + array(), + array('productionRuntime' => false) + ); + $client = $this->clientForEvaluator($evaluator); + $warningCount = 0; + + set_error_handler( + static function ($severity, $message, $file, $line) use (&$warningCount) { + ++$warningCount; + throw new \ErrorException($message, 0, $severity, $file, $line); + }, + E_USER_WARNING + ); + + try { + $firstDetails = $client->getBooleanDetails('preview.flag', false); + $secondDetails = $client->getBooleanDetails('preview.flag', false); + } finally { + restore_error_handler(); + } + + $this->assertTrue($firstDetails->getValue()); + $this->assertSame(EvaluationReason::STATIC_REASON, $firstDetails->getReason()); + $this->assertSame('on', $firstDetails->getVariant()); + $this->assertNull($firstDetails->getErrorCode()); + $this->assertTrue($secondDetails->getValue()); + $this->assertSame(1, $warningCount); + } + /** * @dataProvider invalidDefaultProvider */ @@ -151,7 +189,7 @@ public function invalidDefaultProvider() ); } - private function clientForEvaluator(Evaluator $evaluator, LoggerInterface $logger) + private function clientForEvaluator(Evaluator $evaluator, $logger = null) { $client = new Client($logger); (function () use ($evaluator) { diff --git a/tests/api/Unit/Log/NonThrowingLoggerTest.php b/tests/api/Unit/Log/NonThrowingLoggerTest.php new file mode 100644 index 00000000000..44defad4d46 --- /dev/null +++ b/tests/api/Unit/Log/NonThrowingLoggerTest.php @@ -0,0 +1,92 @@ +debug('debug message', array('level' => 'debug')); + $logger->warning('warning message', array('level' => 'warning')); + $logger->error('error message', array('level' => 'error')); + + $this->assertSame(array( + array('debug', 'debug message', array('level' => 'debug')), + array('warning', 'warning message', array('level' => 'warning')), + array('error', 'error message', array('level' => 'error')), + ), $delegate->calls()); + $this->assertTrue($logger->isLevelActive(LogLevel::WARNING)); + $this->assertFalse($logger->isLevelActive(LogLevel::DEBUG)); + } + + public function testSuppressesFailuresFromEveryLoggerMethod() + { + $logger = new NonThrowingLogger(new AlwaysThrowingLogger()); + + $this->assertNull($logger->debug('debug message')); + $this->assertNull($logger->warning('warning message')); + $this->assertNull($logger->error('error message')); + $this->assertFalse($logger->isLevelActive(LogLevel::WARNING)); + } +} + +final class NonThrowingRecordingLogger implements LoggerInterface +{ + private $calls = array(); + + public function debug($message, array $context = array()) + { + $this->calls[] = array('debug', $message, $context); + } + + public function warning($message, array $context = array()) + { + $this->calls[] = array('warning', $message, $context); + } + + public function error($message, array $context = array()) + { + $this->calls[] = array('error', $message, $context); + } + + public function isLevelActive($level) + { + return $level === LogLevel::WARNING; + } + + public function calls() + { + return $this->calls; + } +} + +final class AlwaysThrowingLogger implements LoggerInterface +{ + public function debug($message, array $context = array()) + { + throw new \ErrorException('debug logger failed'); + } + + public function warning($message, array $context = array()) + { + throw new \ErrorException('warning logger failed'); + } + + public function error($message, array $context = array()) + { + throw new \ErrorException('error logger failed'); + } + + public function isLevelActive($level) + { + throw new \ErrorException('logger level check failed'); + } +} From 7f20b4a9b0b4211815d76e1519e0c1507d34481a Mon Sep 17 00:00:00 2001 From: Leo Romanovsky Date: Mon, 27 Jul 2026 16:42:21 -0600 Subject: [PATCH 3/5] test(ffe): load non-throwing logger fixture dependency --- tests/ext/ffe/system_test_data_evaluate.phpt | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/ext/ffe/system_test_data_evaluate.phpt b/tests/ext/ffe/system_test_data_evaluate.phpt index 43398f7e5c6..877853c86be 100644 --- a/tests/ext/ffe/system_test_data_evaluate.phpt +++ b/tests/ext/ffe/system_test_data_evaluate.phpt @@ -102,6 +102,7 @@ function require_feature_flag_api($root) 'NullLogger', 'InterpolateTrait', 'TriggerErrorLogger', + 'NonThrowingLogger', ) as $classFile) { require_once $logRoot . '/' . $classFile . '.php'; } From 51ed2b1164b27cb121f8e535cbf19b4532a9db65 Mon Sep 17 00:00:00 2001 From: Leo Romanovsky Date: Mon, 27 Jul 2026 17:28:07 -0600 Subject: [PATCH 4/5] fix(feature-flags): use the standard logger by default --- src/DDTrace/OpenFeature/DataDogProvider.php | 8 +-- src/api/FeatureFlags/Client.php | 6 +-- src/api/Log/TriggerErrorLogger.php | 37 -------------- src/bridge/_files_tracer.php | 1 - tests/OpenFeature/DataDogProviderTest.php | 53 +++++++++++++------- tests/api/Unit/FeatureFlags/ClientTest.php | 53 +++++++++++++------- tests/ext/ffe/system_test_data_evaluate.phpt | 3 +- 7 files changed, 79 insertions(+), 82 deletions(-) delete mode 100644 src/api/Log/TriggerErrorLogger.php diff --git a/src/DDTrace/OpenFeature/DataDogProvider.php b/src/DDTrace/OpenFeature/DataDogProvider.php index 72388b8ba48..a6bcaaa2f06 100644 --- a/src/DDTrace/OpenFeature/DataDogProvider.php +++ b/src/DDTrace/OpenFeature/DataDogProvider.php @@ -12,9 +12,9 @@ use DDTrace\FeatureFlags\Internal\Metric\EvaluationMetric; use DDTrace\FeatureFlags\Internal\Metric\EvaluationMetricRecorder; use DDTrace\FeatureFlags\Internal\NativeEvaluator; +use DDTrace\Log\Logger as GlobalLogger; use DDTrace\Log\LoggerInterface; use DDTrace\Log\NonThrowingLogger; -use DDTrace\Log\TriggerErrorLogger; use OpenFeature\implementation\provider\AbstractProvider; use OpenFeature\implementation\provider\ResolutionDetailsBuilder; use OpenFeature\implementation\provider\ResolutionError; @@ -31,7 +31,7 @@ final class DataDogProvider extends AbstractProvider protected static string $NAME = 'Datadog'; private Evaluator $evaluator; - private NonThrowingLogger $safeLogger; + private LoggerInterface $datadogLogger; private bool $warnedAboutNonProductionRuntime = false; private EvaluationMetricRecorder $metricRecorder; @@ -40,7 +40,7 @@ public function __construct(?LoggerInterface $logger = null) // Native evaluation metrics are disabled here because OpenFeature owns // the final provider outcome, including OF-level type mismatch mapping. $this->evaluator = NativeEvaluator::create(false); - $this->safeLogger = new NonThrowingLogger($logger ?: new TriggerErrorLogger()); + $this->datadogLogger = new NonThrowingLogger($logger ?: GlobalLogger::get()); $this->metricRecorder = EvaluationMetricRecorder::createDefault(); } @@ -232,7 +232,7 @@ private function warnIfNonProductionRuntime(EvaluationDetails $details): void } $this->warnedAboutNonProductionRuntime = true; - $this->safeLogger->warning($message); + $this->datadogLogger->warning($message); } private function mapReason(string $reason): string diff --git a/src/api/FeatureFlags/Client.php b/src/api/FeatureFlags/Client.php index 2849ccc441a..82d20a1b999 100644 --- a/src/api/FeatureFlags/Client.php +++ b/src/api/FeatureFlags/Client.php @@ -3,14 +3,14 @@ namespace DDTrace\FeatureFlags; use DDTrace\FeatureFlags\Internal\NativeEvaluator; +use DDTrace\Log\Logger as GlobalLogger; use DDTrace\Log\LoggerInterface; use DDTrace\Log\NonThrowingLogger; -use DDTrace\Log\TriggerErrorLogger; final class Client { private $evaluator; - /** @var NonThrowingLogger */ + /** @var LoggerInterface */ private $logger; private $warnedAboutNonProductionRuntime = false; @@ -21,7 +21,7 @@ public function __construct($logger = null) } $this->evaluator = NativeEvaluator::create(); - $this->logger = new NonThrowingLogger($logger ?: new TriggerErrorLogger()); + $this->logger = new NonThrowingLogger($logger ?: GlobalLogger::get()); } /** diff --git a/src/api/Log/TriggerErrorLogger.php b/src/api/Log/TriggerErrorLogger.php deleted file mode 100644 index 9e60f34502f..00000000000 --- a/src/api/Log/TriggerErrorLogger.php +++ /dev/null @@ -1,37 +0,0 @@ -emit(LogLevel::DEBUG, $message, $context, E_USER_NOTICE); - } - - public function warning($message, array $context = array()) - { - $this->emit(LogLevel::WARNING, $message, $context, E_USER_WARNING); - } - - public function error($message, array $context = array()) - { - $this->emit(LogLevel::ERROR, $message, $context, E_USER_WARNING); - } - - private function emit($level, $message, array $context, $severity) - { - if (!$this->isLevelActive($level)) { - return; - } - - trigger_error($this->interpolate($message, $context), $severity); - } -} diff --git a/src/bridge/_files_tracer.php b/src/bridge/_files_tracer.php index c6d30e49c7b..7d924b7fe7c 100644 --- a/src/bridge/_files_tracer.php +++ b/src/bridge/_files_tracer.php @@ -28,7 +28,6 @@ __DIR__ . '/../api/Log/DatadogLogger.php', __DIR__ . '/../api/Log/ErrorLogLogger.php', __DIR__ . '/../api/Log/NullLogger.php', - __DIR__ . '/../api/Log/TriggerErrorLogger.php', __DIR__ . '/../api/Log/NonThrowingLogger.php', __DIR__ . '/../api/Log/LoggingTrait.php', __DIR__ . '/../api/StartSpanOptions.php', diff --git a/tests/OpenFeature/DataDogProviderTest.php b/tests/OpenFeature/DataDogProviderTest.php index 3b44d046c10..0f1b50c981e 100644 --- a/tests/OpenFeature/DataDogProviderTest.php +++ b/tests/OpenFeature/DataDogProviderTest.php @@ -133,7 +133,7 @@ public function testProviderWarningIsEmittedOncePerProvider(): void self::assertSame(['temporary unavailable'], $logger->warnings()); } - public function testProviderWarningExceptionDoesNotChangeSuccessfulEvaluation(): void + public function testThrowingLoggerDoesNotChangeSuccessfulEvaluation(): void { $evaluator = new OpenFeatureTestEvaluator(); $evaluator->setSuccess( @@ -143,30 +143,18 @@ public function testProviderWarningExceptionDoesNotChangeSuccessfulEvaluation(): 'on', ['productionRuntime' => false] ); - $client = $this->openFeatureClientFor(DataDogProvider::createWithDependencies($evaluator)); - $warningCount = 0; - - set_error_handler( - static function ($severity, $message, $file, $line) use (&$warningCount): void { - ++$warningCount; - throw new \ErrorException($message, 0, $severity, $file, $line); - }, - E_USER_WARNING - ); + $logger = new OpenFeatureThrowingLogger(); + $client = $this->openFeatureClientFor($this->providerForEvaluator($evaluator, $logger)); - try { - $firstDetails = $client->getBooleanDetails('preview.flag', false); - $secondDetails = $client->getBooleanDetails('preview.flag', false); - } finally { - restore_error_handler(); - } + $firstDetails = $client->getBooleanDetails('preview.flag', false); + $secondDetails = $client->getBooleanDetails('preview.flag', false); self::assertTrue($firstDetails->getValue()); self::assertSame(EvaluationReason::STATIC_REASON, $firstDetails->getReason()); self::assertSame('on', $firstDetails->getVariant()); self::assertNull($firstDetails->getError()); self::assertTrue($secondDetails->getValue()); - self::assertSame(1, $warningCount); + self::assertSame(1, $logger->warningCount()); } public function testProviderErrorsMapToOpenFeatureDetails(): void @@ -378,4 +366,33 @@ public function warnings(): array return $this->warnings; } } + +final class OpenFeatureThrowingLogger implements LoggerInterface +{ + private int $warningCount = 0; + + public function debug($message, array $context = []) + { + } + + public function warning($message, array $context = []) + { + ++$this->warningCount; + throw new \ErrorException($message); + } + + public function error($message, array $context = []) + { + } + + public function isLevelActive($level) + { + return true; + } + + public function warningCount(): int + { + return $this->warningCount; + } +} } diff --git a/tests/api/Unit/FeatureFlags/ClientTest.php b/tests/api/Unit/FeatureFlags/ClientTest.php index 19c8a800ee4..1477ad94c8d 100644 --- a/tests/api/Unit/FeatureFlags/ClientTest.php +++ b/tests/api/Unit/FeatureFlags/ClientTest.php @@ -129,7 +129,7 @@ public function testWarningIsEmittedOncePerClientNotOncePerEvaluation() $this->assertCount(1, $logger->warnings()); } - public function testWarningExceptionDoesNotChangeSuccessfulEvaluation() + public function testThrowingLoggerDoesNotChangeSuccessfulEvaluation() { $evaluator = new ClientTestEvaluator(); $evaluator->setSuccess( @@ -141,30 +141,18 @@ public function testWarningExceptionDoesNotChangeSuccessfulEvaluation() array(), array('productionRuntime' => false) ); - $client = $this->clientForEvaluator($evaluator); - $warningCount = 0; - - set_error_handler( - static function ($severity, $message, $file, $line) use (&$warningCount) { - ++$warningCount; - throw new \ErrorException($message, 0, $severity, $file, $line); - }, - E_USER_WARNING - ); + $logger = new ThrowingLogger(); + $client = $this->clientForEvaluator($evaluator, $logger); - try { - $firstDetails = $client->getBooleanDetails('preview.flag', false); - $secondDetails = $client->getBooleanDetails('preview.flag', false); - } finally { - restore_error_handler(); - } + $firstDetails = $client->getBooleanDetails('preview.flag', false); + $secondDetails = $client->getBooleanDetails('preview.flag', false); $this->assertTrue($firstDetails->getValue()); $this->assertSame(EvaluationReason::STATIC_REASON, $firstDetails->getReason()); $this->assertSame('on', $firstDetails->getVariant()); $this->assertNull($firstDetails->getErrorCode()); $this->assertTrue($secondDetails->getValue()); - $this->assertSame(1, $warningCount); + $this->assertSame(1, $logger->warningCount()); } /** @@ -304,3 +292,32 @@ public function warnings() return $this->warnings; } } + +final class ThrowingLogger implements LoggerInterface +{ + private $warningCount = 0; + + public function debug($message, array $context = array()) + { + } + + public function warning($message, array $context = array()) + { + ++$this->warningCount; + throw new \ErrorException($message); + } + + public function error($message, array $context = array()) + { + } + + public function isLevelActive($level) + { + return true; + } + + public function warningCount() + { + return $this->warningCount; + } +} diff --git a/tests/ext/ffe/system_test_data_evaluate.phpt b/tests/ext/ffe/system_test_data_evaluate.phpt index 877853c86be..2cba71f68b6 100644 --- a/tests/ext/ffe/system_test_data_evaluate.phpt +++ b/tests/ext/ffe/system_test_data_evaluate.phpt @@ -101,7 +101,8 @@ function require_feature_flag_api($root) 'AbstractLogger', 'NullLogger', 'InterpolateTrait', - 'TriggerErrorLogger', + 'ErrorLogLogger', + 'Logger', 'NonThrowingLogger', ) as $classFile) { require_once $logRoot . '/' . $classFile . '.php'; From bf360ef80839f483179e37215536d1713116167c Mon Sep 17 00:00:00 2001 From: Leo Romanovsky Date: Mon, 27 Jul 2026 17:48:21 -0600 Subject: [PATCH 5/5] fix(ffe): mark the native evaluator production ready --- src/DDTrace/OpenFeature/DataDogProvider.php | 15 ++++---- src/api/FeatureFlags/Client.php | 15 ++++---- .../FeatureFlags/Internal/NativeEvaluator.php | 4 +-- tests/OpenFeature/DataDogProviderTest.php | 30 ++++++---------- tests/api/Unit/FeatureFlags/ClientTest.php | 35 +++++-------------- tests/ext/ffe/system_test_data_evaluate.phpt | 10 ++++++ 6 files changed, 46 insertions(+), 63 deletions(-) diff --git a/src/DDTrace/OpenFeature/DataDogProvider.php b/src/DDTrace/OpenFeature/DataDogProvider.php index a6bcaaa2f06..122eb2ed2c3 100644 --- a/src/DDTrace/OpenFeature/DataDogProvider.php +++ b/src/DDTrace/OpenFeature/DataDogProvider.php @@ -32,7 +32,7 @@ final class DataDogProvider extends AbstractProvider private Evaluator $evaluator; private LoggerInterface $datadogLogger; - private bool $warnedAboutNonProductionRuntime = false; + private bool $warnedAboutRuntimeNotReady = false; private EvaluationMetricRecorder $metricRecorder; public function __construct(?LoggerInterface $logger = null) @@ -114,7 +114,7 @@ private function resolve( ): ResolutionDetailsInterface { $normalizedContext = $this->normalizeContext($context); $details = $this->evaluate($flagKey, $expectedType, $defaultValue, $normalizedContext); - $this->warnIfNonProductionRuntime($details); + $this->warnIfRuntimeNotReady($details); // The PHP OpenFeature SDK does not pass ResolutionDetails to finally // hooks, so PHP records metrics here after native evaluation has the // final provider result. @@ -215,23 +215,22 @@ private function normalizeContext(?EvaluationContext $context): array ]; } - private function warnIfNonProductionRuntime(EvaluationDetails $details): void + private function warnIfRuntimeNotReady(EvaluationDetails $details): void { - if ($this->warnedAboutNonProductionRuntime) { + if ($this->warnedAboutRuntimeNotReady) { return; } - $providerState = $details->getProviderState(); - if (!array_key_exists('productionRuntime', $providerState) || $providerState['productionRuntime'] !== false) { + if ($details->getErrorCode() !== EvaluationErrorCode::PROVIDER_NOT_READY) { return; } $message = $details->getErrorMessage(); if (!is_string($message) || $message === '') { - $message = 'Datadog-backed PHP OpenFeature evaluation is not fully enabled yet.'; + $message = 'Datadog-backed PHP OpenFeature evaluation is not ready. Returning the default value.'; } - $this->warnedAboutNonProductionRuntime = true; + $this->warnedAboutRuntimeNotReady = true; $this->datadogLogger->warning($message); } diff --git a/src/api/FeatureFlags/Client.php b/src/api/FeatureFlags/Client.php index 82d20a1b999..7a8df017709 100644 --- a/src/api/FeatureFlags/Client.php +++ b/src/api/FeatureFlags/Client.php @@ -12,7 +12,7 @@ final class Client private $evaluator; /** @var LoggerInterface */ private $logger; - private $warnedAboutNonProductionRuntime = false; + private $warnedAboutRuntimeNotReady = false; public function __construct($logger = null) { @@ -117,7 +117,7 @@ private function evaluate($flagKey, $expectedType, $defaultValue, array $context $attributes ); - $this->warnIfNonProductionRuntime($details); + $this->warnIfRuntimeNotReady($details); return $details; } @@ -141,23 +141,22 @@ private function normalizeContext(array $context) return array($targetingKey, $attributes); } - private function warnIfNonProductionRuntime(EvaluationDetails $details) + private function warnIfRuntimeNotReady(EvaluationDetails $details) { - if ($this->warnedAboutNonProductionRuntime) { + if ($this->warnedAboutRuntimeNotReady) { return; } - $providerState = $details->getProviderState(); - if (!array_key_exists('productionRuntime', $providerState) || $providerState['productionRuntime'] !== false) { + if ($details->getErrorCode() !== EvaluationErrorCode::PROVIDER_NOT_READY) { return; } $message = $details->getErrorMessage(); if (!is_string($message) || $message === '') { - $message = 'Datadog-backed PHP feature flag evaluation is running without exposure and metric reporting in this milestone.'; + $message = 'Datadog-backed PHP feature flag evaluation is not ready. Returning the default value.'; } - $this->warnedAboutNonProductionRuntime = true; + $this->warnedAboutRuntimeNotReady = true; $this->logger->warning($message); } diff --git a/src/api/FeatureFlags/Internal/NativeEvaluator.php b/src/api/FeatureFlags/Internal/NativeEvaluator.php index 8588f694e8e..929f058f733 100644 --- a/src/api/FeatureFlags/Internal/NativeEvaluator.php +++ b/src/api/FeatureFlags/Internal/NativeEvaluator.php @@ -103,9 +103,9 @@ private function withProviderState($rawResult) 'ready' => $hasConfig, 'hasConfig' => $hasConfig, 'configVersion' => $configVersion, - 'productionRuntime' => false, + 'productionRuntime' => true, 'mode' => 'native_remote_config', - 'reason' => $hasConfig ? 'metrics_delivery_pending' : 'configuration_missing', + 'reason' => $hasConfig ? 'ready' : 'configuration_missing', ); if (is_array($rawResult)) { diff --git a/tests/OpenFeature/DataDogProviderTest.php b/tests/OpenFeature/DataDogProviderTest.php index 0f1b50c981e..1404395c075 100644 --- a/tests/OpenFeature/DataDogProviderTest.php +++ b/tests/OpenFeature/DataDogProviderTest.php @@ -9,7 +9,6 @@ use DDTrace\FeatureFlags\EvaluationReason; use DDTrace\FeatureFlags\EvaluationType; use DDTrace\FeatureFlags\Internal\Evaluator; -use DDTrace\FeatureFlags\Internal\NativeEvaluator; use DDTrace\FeatureFlags\Internal\UnavailableEvaluator; use DDTrace\Log\LoggerInterface; use DDTrace\Log\LogLevel; @@ -101,7 +100,7 @@ public function testEvaluationContextIsNormalizedForDatadogClient(): void public function testUnavailableRuntimeReturnsDefaultDetailsAndOneWarning(): void { $logger = new OpenFeatureRecordingLogger(); - $client = $this->openFeatureClientFor(new DataDogProvider($logger)); + $client = $this->openFeatureClientFor($this->providerForEvaluator(new UnavailableEvaluator(), $logger)); $value = $client->getBooleanValue('checkout.enabled', true); $details = $client->getStringDetails('checkout.copy', 'fallback'); @@ -110,10 +109,7 @@ public function testUnavailableRuntimeReturnsDefaultDetailsAndOneWarning(): void self::assertSame('fallback', $details->getValue()); self::assertSame(Reason::ERROR, $details->getReason()); self::assertSame(ErrorCode::PROVIDER_NOT_READY()->getValue(), $details->getError()->getResolutionErrorCode()->getValue()); - self::assertContains($details->getError()->getResolutionErrorMessage(), [ - NativeEvaluator::WARNING_MESSAGE, - UnavailableEvaluator::WARNING_MESSAGE, - ]); + self::assertSame(UnavailableEvaluator::WARNING_MESSAGE, $details->getError()->getResolutionErrorMessage()); self::assertSame([$details->getError()->getResolutionErrorMessage()], $logger->warnings()); } @@ -133,27 +129,23 @@ public function testProviderWarningIsEmittedOncePerProvider(): void self::assertSame(['temporary unavailable'], $logger->warnings()); } - public function testThrowingLoggerDoesNotChangeSuccessfulEvaluation(): void + public function testThrowingLoggerDoesNotChangeProviderNotReadyEvaluation(): void { $evaluator = new OpenFeatureTestEvaluator(); - $evaluator->setSuccess( - 'preview.flag', - true, - EvaluationReason::STATIC_REASON, - 'on', - ['productionRuntime' => false] - ); + $evaluator->setUnavailable('preview.flag', false, 'runtime unavailable'); $logger = new OpenFeatureThrowingLogger(); $client = $this->openFeatureClientFor($this->providerForEvaluator($evaluator, $logger)); $firstDetails = $client->getBooleanDetails('preview.flag', false); $secondDetails = $client->getBooleanDetails('preview.flag', false); - self::assertTrue($firstDetails->getValue()); - self::assertSame(EvaluationReason::STATIC_REASON, $firstDetails->getReason()); - self::assertSame('on', $firstDetails->getVariant()); - self::assertNull($firstDetails->getError()); - self::assertTrue($secondDetails->getValue()); + self::assertFalse($firstDetails->getValue()); + self::assertSame(Reason::ERROR, $firstDetails->getReason()); + self::assertSame( + ErrorCode::PROVIDER_NOT_READY()->getValue(), + $firstDetails->getError()->getResolutionErrorCode()->getValue() + ); + self::assertFalse($secondDetails->getValue()); self::assertSame(1, $logger->warningCount()); } diff --git a/tests/api/Unit/FeatureFlags/ClientTest.php b/tests/api/Unit/FeatureFlags/ClientTest.php index 1477ad94c8d..7ea2458f419 100644 --- a/tests/api/Unit/FeatureFlags/ClientTest.php +++ b/tests/api/Unit/FeatureFlags/ClientTest.php @@ -8,7 +8,6 @@ use DDTrace\FeatureFlags\EvaluationReason; use DDTrace\FeatureFlags\EvaluationType; use DDTrace\FeatureFlags\Internal\Evaluator; -use DDTrace\FeatureFlags\Internal\NativeEvaluator; use DDTrace\FeatureFlags\Internal\UnavailableEvaluator; use DDTrace\Log\LoggerInterface; use PHPUnit\Framework\TestCase; @@ -93,7 +92,7 @@ public function testContextNormalizesTargetingKeyAndPrimitiveAttributes() public function testUnavailableRuntimeReturnsDefaultWithProviderNotReadyDetailsAndWarning() { $logger = new RecordingLogger(); - $client = new Client($logger); + $client = $this->clientForEvaluator(new UnavailableEvaluator(), $logger); $value = $client->getBooleanValue('checkout-redesign', true); $details = $client->getStringDetails('checkout-copy', 'fallback'); @@ -102,25 +101,19 @@ public function testUnavailableRuntimeReturnsDefaultWithProviderNotReadyDetailsA $this->assertSame('fallback', $details->getValue()); $this->assertSame(EvaluationReason::ERROR, $details->getReason()); $this->assertSame(EvaluationErrorCode::PROVIDER_NOT_READY, $details->getErrorCode()); - $this->assertContains($details->getErrorMessage(), array( - NativeEvaluator::WARNING_MESSAGE, - UnavailableEvaluator::WARNING_MESSAGE, - )); + $this->assertSame(UnavailableEvaluator::WARNING_MESSAGE, $details->getErrorMessage()); $providerState = $details->getProviderState(); $this->assertSame(false, $providerState['ready']); $this->assertSame(false, $providerState['productionRuntime']); - $this->assertTrue(in_array($providerState['reason'], array( - 'configuration_missing', - 'runtime_unavailable', - ), true)); + $this->assertSame('runtime_unavailable', $providerState['reason']); $this->assertSame(array($details->getErrorMessage()), $logger->warnings()); } public function testWarningIsEmittedOncePerClientNotOncePerEvaluation() { $logger = new RecordingLogger(); - $client = new Client($logger); + $client = $this->clientForEvaluator(new ClientTestEvaluator(), $logger); $client->getBooleanValue('flag-1', false); $client->getBooleanValue('flag-2', false); @@ -129,29 +122,19 @@ public function testWarningIsEmittedOncePerClientNotOncePerEvaluation() $this->assertCount(1, $logger->warnings()); } - public function testThrowingLoggerDoesNotChangeSuccessfulEvaluation() + public function testThrowingLoggerDoesNotChangeProviderNotReadyEvaluation() { $evaluator = new ClientTestEvaluator(); - $evaluator->setSuccess( - 'preview.flag', - true, - EvaluationReason::STATIC_REASON, - 'on', - array(), - array(), - array('productionRuntime' => false) - ); $logger = new ThrowingLogger(); $client = $this->clientForEvaluator($evaluator, $logger); $firstDetails = $client->getBooleanDetails('preview.flag', false); $secondDetails = $client->getBooleanDetails('preview.flag', false); - $this->assertTrue($firstDetails->getValue()); - $this->assertSame(EvaluationReason::STATIC_REASON, $firstDetails->getReason()); - $this->assertSame('on', $firstDetails->getVariant()); - $this->assertNull($firstDetails->getErrorCode()); - $this->assertTrue($secondDetails->getValue()); + $this->assertFalse($firstDetails->getValue()); + $this->assertSame(EvaluationReason::ERROR, $firstDetails->getReason()); + $this->assertSame(EvaluationErrorCode::PROVIDER_NOT_READY, $firstDetails->getErrorCode()); + $this->assertFalse($secondDetails->getValue()); $this->assertSame(1, $logger->warningCount()); } diff --git a/tests/ext/ffe/system_test_data_evaluate.phpt b/tests/ext/ffe/system_test_data_evaluate.phpt index 2cba71f68b6..0354581a76c 100644 --- a/tests/ext/ffe/system_test_data_evaluate.phpt +++ b/tests/ext/ffe/system_test_data_evaluate.phpt @@ -155,6 +155,16 @@ function run_fixture_case($client, $fileName, $index, array $case, array &$failu $context ); + $providerState = $details->getProviderState(); + if (($providerState['productionRuntime'] ?? null) !== true) { + $failures[] = $fileName . '#' . $index . ': productionRuntime must be true'; + } + if (($providerState['reason'] ?? null) !== 'ready') { + $failures[] = $fileName . '#' . $index + . ': runtime reason got=' . encode_value($providerState['reason'] ?? null) + . ' want="ready"'; + } + if (!array_key_exists('value', $case['result'])) { $failures[] = $fileName . '#' . $index . ': result must include value'; return;