fix(feature-flags): make evaluation logging non-throwing

[leoromanovsky]

Motivation

A customer found that successful OpenFeature evaluations returned the code default in Laravel. Laravel converts the Datadog warning into ErrorException. OpenFeature catches that exception as GENERAL and discards the evaluated value.

Existing FFE system tests called the native Datadog client. That path bypassed the OpenFeature provider, its logger boundary, and framework error handling. DataDog/system-tests#7393 makes OpenFeature the default /ffe path and adds that path to Laravel and Symfony. PHP 8 must return the configured value without an error. A runtime without the provider must return PROVIDER_NOT_READY. This contract makes future regressions fail in the framework path.

PR #3906 added this warning as a temporary guardrail. The reviewer asked the implementation to reuse LoggerInterface and suggested TriggerErrorLogger only if trigger_error() remained the default. Exposure delivery and evaluation metrics are now complete, so the guardrail and its logger are obsolete.

Changes and Decisions

Successful native evaluations now report productionRuntime=true and reason=ready. Only PROVIDER_NOT_READY results produce the not-ready warning.

The change removes TriggerErrorLogger and uses the existing tracer logger. Both feature-flag entry points wrap their logger with NonThrowingLogger. This boundary prevents current and future logger failures from changing evaluation results. The provider retains LoggerInterface and uses datadogLogger because the OpenFeature parent class owns $logger.

Validation

DataDog/system-tests#7393 passed locally with system-tests commit 0dad5d48bec63cffdd206b724013d26f497c0c8c and tracer commit bf360ef80839f483179e37215536d1713116167c:

• PHP-FPM 8.2: 8 passed in 52.33 seconds.
• Laravel 11: 1 passed in 34.21 seconds.
• Symfony 7: 1 passed in 34.15 seconds.

[datadog-datadog-prod-us1]

 

✨ Unblock PR with BitsAI

⚠️ Warnings

🚦 5 Pipeline jobs failed

DataDog/apm-reliability/dd-trace-php | min install tests     

DataDog/apm-reliability/dd-trace-php | verify alpine: [php php-fpm php-json, alpine:3.17, php_installer]     

DataDog/apm-reliability/dd-trace-php | verify alpine: [php7 php7-fpm php7-json, alpine:3.15, native_package]     

View all 5 failed jobs.

ℹ️ Info

No other issues found (see more)

🧪 All tests passed
❄️ No new flaky tests detected

🔄 Datadog auto-retried 1 job - 0 passed on retry

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 60.65% (-0.00%)

Useful? React with 👍 / 👎

_{This comment will be updated automatically if new data arrives.
🔗 Commit SHA: bf360ef | Docs | Datadog PR Page | Give us feedback!}

[pr-commenter]

Benchmarks [ tracer ]

Benchmark execution time: 2026-07-28 14:49:32

Comparing candidate commit bf360ef in PR branch leo.romanovsky/fix-openfeature-warning-exception with baseline commit 5bf45ce in branch master.

Found 2 performance improvements and 1 performance regressions! Performance is the same for 190 metrics, 1 unstable metrics.

Explanation

This is an A/B test comparing a candidate commit's performance against that of a baseline commit. Performance changes are noted in the tables below as:

• 🟩 = significantly better candidate vs. baseline
• 🟥 = significantly worse candidate vs. baseline

We compute a confidence interval (CI) over the relative difference of means between metrics from the candidate and baseline commits, considering the baseline as the reference.

If the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD), the change is considered significant.

Feel free to reach out to #apm-benchmarking-platform on Slack if you have any questions.

More details about the CI and significant changes

You can imagine this CI as a range of values that is likely to contain the true difference of means between the candidate and baseline commits.

CIs of the difference of means are often centered around 0%, because often changes are not that big:

---------------------------------(------|---^--------)-------------------------------->
                              -0.6%    0%  0.3%     +1.2%
                                 |          |        |
         lower bound of the CI --'          |        |
sample mean (center of the CI) -------------'        |
         upper bound of the CI ----------------------'

As described above, a change is considered significant if the CI is entirely outside the configured SIGNIFICANT_IMPACT_THRESHOLD (or the deprecated UNCONFIDENCE_THRESHOLD).

For instance, for an execution time metric, this confidence interval indicates a significantly worse performance:

----------------------------------------|---------|---(---------^---------)---------->
                                       0%        1%  1.3%      2.2%      3.1%
                                                  |   |         |         |
       significant impact threshold --------------'   |         |         |
                      lower bound of CI --------------'         |         |
       sample mean (center of the CI) --------------------------'         |
                      upper bound of CI ----------------------------------'

scenario:MessagePackSerializationBench/benchMessagePackSerialization

• 🟩 execution_time [-8.442µs; -5.858µs] or [-7.366%; -5.111%]

scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache

• 🟩 execution_time [-5.694µs; -2.646µs] or [-4.980%; -2.314%]

scenario:WordPressBench/benchWordPressBaseline

• 🟥 execution_time [+0.647ms; +2.008ms] or [+2.917%; +9.046%]

[pavlokhrebto]

Reviewed the diff — solid, well-tested fix for a real footgun (trigger_error -> ErrorException -> OpenFeature discards the value). A few non-blocking notes inline. The two I would most want confirmed before approval: (1) the not-ready warning is now effectively silent by default (NullLogger unless DD_TRACE_DEBUG), and (2) light end-to-end coverage of the native config-missing branch. No interaction with the merged span-enrichment work.

[pavlokhrebto]

Behavior change worth calling out: Logger::get() returns NullLogger(EMERGENCY) unless DD_TRACE_DEBUG is set, so the not-ready warning is now effectively silent by default (previously TriggerErrorLogger always emitted E_USER_WARNING).

As far as i remember this is intended, but please note in the PR that not-ready state is now surfaced via exposures/metrics rather than a userland warning. Same applies to the equivalent line in DataDogProvider.

[pavlokhrebto]

After this PR nothing reads productionRuntime for logic (the old warnIfNonProductionRuntime checks are removed); it is only surfaced as metadata / asserted in the .phpt. Two small things: (a) consider => $hasConfig instead of a constant true — a config-missing result (reason='configuration_missing' -> PROVIDER_NOT_READY) currently reports productionRuntime=true alongside a not-ready error, which is contradictory; (b) a one-line comment that it is informational-only would help the next reader.

[pavlokhrebto]

Nice boundary. Note it swallows all Throwables silently on every method, so a genuinely broken/misconfigured underlying logger becomes invisible. Acceptable trade-off here (and I would not add an error_log fallback — recursion risk), just flagging the blind spot.

[pavlokhrebto]

This asserts the hasConfig=true path (productionRuntime=true/reason=ready). The hasConfig=false native branch — config-missing -> PROVIDER_NOT_READY with a returned value, which is exactly the scenario the warning re-keying governs — is not covered end-to-end. Consider a case that exercises it to lock the contract (the unit tests cover UnavailableEvaluator's PROVIDER_NOT_READY, but not native config-missing).

[tabgok]

looks good from the IDM perspective

[chasdevs]

Missed the boat on approval, but that is pretty wild.