diff --git a/src/DDTrace/OpenFeature/DataDogProvider.php b/src/DDTrace/OpenFeature/DataDogProvider.php index 05c7e771f6d..122eb2ed2c3 100644 --- a/src/DDTrace/OpenFeature/DataDogProvider.php +++ b/src/DDTrace/OpenFeature/DataDogProvider.php @@ -12,8 +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\TriggerErrorLogger; +use DDTrace\Log\NonThrowingLogger; use OpenFeature\implementation\provider\AbstractProvider; use OpenFeature\implementation\provider\ResolutionDetailsBuilder; use OpenFeature\implementation\provider\ResolutionError; @@ -30,8 +31,8 @@ final class DataDogProvider extends AbstractProvider protected static string $NAME = 'Datadog'; private Evaluator $evaluator; - private LoggerInterface $warningLogger; - private bool $warnedAboutNonProductionRuntime = false; + private LoggerInterface $datadogLogger; + private bool $warnedAboutRuntimeNotReady = false; private EvaluationMetricRecorder $metricRecorder; public function __construct(?LoggerInterface $logger = null) @@ -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->datadogLogger = new NonThrowingLogger($logger ?: GlobalLogger::get()); $this->metricRecorder = EvaluationMetricRecorder::createDefault(); } @@ -113,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. @@ -214,24 +215,23 @@ 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->warningLogger->warning($message); - $this->warnedAboutNonProductionRuntime = true; + $this->warnedAboutRuntimeNotReady = true; + $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 b42bbfcbcdd..7a8df017709 100644 --- a/src/api/FeatureFlags/Client.php +++ b/src/api/FeatureFlags/Client.php @@ -3,14 +3,16 @@ namespace DDTrace\FeatureFlags; use DDTrace\FeatureFlags\Internal\NativeEvaluator; +use DDTrace\Log\Logger as GlobalLogger; use DDTrace\Log\LoggerInterface; -use DDTrace\Log\TriggerErrorLogger; +use DDTrace\Log\NonThrowingLogger; final class Client { private $evaluator; + /** @var LoggerInterface */ private $logger; - private $warnedAboutNonProductionRuntime = false; + private $warnedAboutRuntimeNotReady = false; public function __construct($logger = null) { @@ -19,7 +21,7 @@ public function __construct($logger = null) } $this->evaluator = NativeEvaluator::create(); - $this->logger = $logger ?: new TriggerErrorLogger(); + $this->logger = new NonThrowingLogger($logger ?: GlobalLogger::get()); } /** @@ -115,7 +117,7 @@ private function evaluate($flagKey, $expectedType, $defaultValue, array $context $attributes ); - $this->warnIfNonProductionRuntime($details); + $this->warnIfRuntimeNotReady($details); return $details; } @@ -139,24 +141,23 @@ 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->warnedAboutRuntimeNotReady = true; $this->logger->warning($message); - $this->warnedAboutNonProductionRuntime = true; } private function expectFlagKey($flagKey) 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/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/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 017d07ca669..7d924b7fe7c 100644 --- a/src/bridge/_files_tracer.php +++ b/src/bridge/_files_tracer.php @@ -28,7 +28,7 @@ __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', __DIR__ . '/../DDTrace/SpanContext.php', diff --git a/tests/OpenFeature/DataDogProviderTest.php b/tests/OpenFeature/DataDogProviderTest.php index 45334ca32fc..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,6 +129,26 @@ public function testProviderWarningIsEmittedOncePerProvider(): void self::assertSame(['temporary unavailable'], $logger->warnings()); } + public function testThrowingLoggerDoesNotChangeProviderNotReadyEvaluation(): void + { + $evaluator = new OpenFeatureTestEvaluator(); + $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::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()); + } + public function testProviderErrorsMapToOpenFeatureDetails(): void { $evaluator = new OpenFeatureTestEvaluator(); @@ -186,13 +202,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; @@ -336,4 +358,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 b2bc83d8321..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,6 +122,22 @@ public function testWarningIsEmittedOncePerClientNotOncePerEvaluation() $this->assertCount(1, $logger->warnings()); } + public function testThrowingLoggerDoesNotChangeProviderNotReadyEvaluation() + { + $evaluator = new ClientTestEvaluator(); + $logger = new ThrowingLogger(); + $client = $this->clientForEvaluator($evaluator, $logger); + + $firstDetails = $client->getBooleanDetails('preview.flag', false); + $secondDetails = $client->getBooleanDetails('preview.flag', false); + + $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()); + } + /** * @dataProvider invalidDefaultProvider */ @@ -151,7 +160,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) { @@ -266,3 +275,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/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'); + } +} diff --git a/tests/ext/ffe/system_test_data_evaluate.phpt b/tests/ext/ffe/system_test_data_evaluate.phpt index 43398f7e5c6..0354581a76c 100644 --- a/tests/ext/ffe/system_test_data_evaluate.phpt +++ b/tests/ext/ffe/system_test_data_evaluate.phpt @@ -101,7 +101,9 @@ function require_feature_flag_api($root) 'AbstractLogger', 'NullLogger', 'InterpolateTrait', - 'TriggerErrorLogger', + 'ErrorLogLogger', + 'Logger', + 'NonThrowingLogger', ) as $classFile) { require_once $logRoot . '/' . $classFile . '.php'; } @@ -153,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;