Skip to content

Add bounded feature flag evaluation EVP reporting - #4288

Open
leoromanovsky wants to merge 19 commits into
masterfrom
leo.romanovsky/php-agentless-20261008
Open

leoromanovsky wants to merge 19 commits into
masterfrom
leo.romanovsky/php-agentless-20261008

Conversation

@leoromanovsky

@leoromanovsky leoromanovsky commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

PHP already reports feature-flag exposures. This adds privacy-safe flagevaluation track EVP events for both the native PHP client and OpenFeature, so Datadog can report how often flags are evaluated without sending customer identity or context by default.

Changes and key decisions

  • Record the final result returned to the application, with the timestamp and privacy setting captured for that evaluation.
  • Without explicit observeFullEvaluationData: true, hash the targeting key and omit evaluation context. With consent, include bounded scalar context; oversized/degraded observations can omit identity and context.
  • Reuse libdatadog's existing sender and sidecar for aggregation, privacy, and delivery. No new PHP worker or transport. Evaluation does not reconnect or wait for HTTP delivery; unavailable/congested transport can drop telemetry without changing the result returned to the application.
  • Use DD_FLAGGING_EVALUATION_COUNTS_ENABLED independently of OTel metrics. Fixed-name rejection metrics report drops through ordinary telemetry, without per-evaluation logging.
  • Share an Agent-only EVP target between exposures and flagevaluation events, with the PHP SDK name/version on both writers; retain Remote Config's Agent EVP v2 route.
  • Include events in explicit/configured shutdown flushes, with forked and threaded-PHP lifecycle coverage. Also release the owned error message when a shutdown flush fails. Ordinary requests do not gain a flush wait.

Builds on merged libdatadog #2605 and #2614, and continues the intent of PHP #3984.

The earlier native integration draft is libdatadog #2645; this branch now uses the same libdatadog commit as master, rather than a separate dependency bump.

Agentless activation/CDN wiring remains separate, following settings #4195 and poller #4196.

This PR enables Agent-backed delivery, not agentless PHP.

flowchart LR
  N[Native PHP client] --> E[Final evaluation result + privacy setting]
  O[OpenFeature provider] --> E
  E --> B[Bounded native submission]
  B --> S[Existing sidecar: aggregate + protect PII]
  S --> A[Agent EVP v2]
  X[Existing exposures] --> A
Loading

Blast radius and tradeoffs

  • Changes PHP FFE recording, native bindings, shared EVP routing, and explicit/shutdown flush integration. Evaluation values, exposures, OTel metrics, and span enrichment retain their existing purpose; comparisons cover both native PHP and OpenFeature APIs.
  • FFE telemetry adds evaluation-time CPU work. It is best-effort under overload: accepted is not guaranteed delivery. Keep the existing libdatadog load-shedding policy; changing shared transport capacity/fairness is out of scope.
  • Performance: at 10,000 evaluations/sec on one shared CPU, protected median calls rose 26 → 41 µs native / 49 → 94 µs OpenFeature. OpenFeature fell short of the requested rate and dropped 1.48% of telemetry; diagnostics account for losses. At 1,000/sec, all observations arrived in the measured runs. These are SDK-call measurements, not whole-application latency or customer workload guarantees.
  • Explicit/shutdown flushes retain a broader deadline limitation tracked in FFL-3400.

Testing

External comparisons below used 8d7488806 versus master 401174500. The only later commit, ce0ad1f61, fixes shutdown error cleanup and adds its regression test; CI and local leak checks cover that final commit.

  • System-tests: PR 50 passed / 3 failed, master 40 passed / 13 failed, no skips; every baseline pass preserved and all three consent cases pass. The remaining configuration/context/cardinality cases are documented in FFL-3407; the suite is not fully green.
  • Dogfooding: 36 matching results across native/OpenFeature, 36 OTel metrics on both builds, and 36 expected flagevaluation events from this branch. Live consent was true; absent/false consent is covered separately by system-tests.
  • Final-commit validation: 664 local extension tests passed (93 skips, 3 expected failures). The new regression reproduced the leak before the fix and passed with full Valgrind leak checking afterward; both it and the original flush test pass in sanitizer CI.
  • General CI benchmarks: 194 unchanged metrics, zero regressions or unstable metrics at ce0ad1f61. This is separate from the FFE-specific overhead measured above.

@datadog-prod-us1-6

datadog-prod-us1-6 Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Tests

⚠️ Warnings

Your PR has warnings. Please review the issues below.

❄️ 1 New flaky test detected

tmp/build_extension/tests/ext/ffe/flag_evaluation_flush.phpt (FFE explicit and configured shutdown flushes wait for pending HTTP sends with either trace sender) from php.tmp.build_extension.tests.ext.ffe
863e9a70b70863 /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library/alloc/src/vec/mod.rs:2782:14
024+     #11 0xed9e9a8af150 in _$LT$alloc..vec..Vec$LT$T$C$A$GT$$u20$as$u20$alloc..vec..spec_extend..SpecExtend$LT$$RF$T$C$core..slice..iter..Iter$LT$T$GT$$GT$$GT$::spec_extend::hf65e0b90f8a41dd8 /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library/alloc/src/vec/spec_extend.rs:61:23
025+     #12 0xed9e9a8af150 in alloc::vec::Vec$LT$T$C$A$GT$::extend_from_slice::h403b746b6a5e1c9e /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library/alloc/src/vec/mod.rs:3312:14
026+     #13 0xed9e9a8af150 in alloc::string::String::push_str::hd672bb5ef525e70e /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library/alloc/src/string.rs:1113:18
027+     #14 0xed9e9a8af150 in _$LT$alloc..string..String$u20$as$u20$core..fmt..Write$GT$::write_str::h62ff503f916e17ff /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library/alloc/src/string.rs:3242:14
028+     #15 0xed9e9a8bd708 in core::fmt::builders::DebugTuple::field::_$u7b$$u7b$closure$u7d$$u7d$::hc1de5db291a839ec /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library/core/src/fmt/builders.rs:330:35
029+     #16 0xed9e9a8bd708 in core::fmt::builders::DebugTuple::field_with::_$u7b$$u7b$closure$u7d$$u7d$::hd3648975d04cbcc3 /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library/core/src/fmt/builders.rs:355:17
030+     #17 0xed9e9a8bd708 in core::result::Result$LT$T$C$E$GT$::and_then::hf1cebfa4feeb97ab /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library/core/src/result.rs:1486:22
031+     #18 0xed9e9a8bd708 in core::fmt::builders::DebugTuple::field_with::h9d79a0d052dde22f /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library/core/src/fmt/builders.rs:342:35
032+     #19 0xed9e9a8bd708 in core::fmt::builders::DebugTuple::field::hb189eac1dd05c63a /rustc/ed61e7d7e242494fb7057f2657300d9e77bb4fcb/library/core/src/fmt/builders.rs:330:14
...

View in Flaky Test Management

ℹ️ Info

No other issues found (see more)

🧪 All tests passed

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 68.39% (-0.04%)

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 37e3d84 | Docs | View more details | Give us feedback!

@pr-commenter

pr-commenter Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Benchmarks [ tracer ]

Benchmark execution time: 2026-10-20 22:46:04

Comparing candidate commit 37e3d84 in PR branch leo.romanovsky/php-agentless-20261008 with baseline commit 77c6cf6 in branch master.

📊 Benchmarking dashboard

Found 2 performance improvements and 0 performance regressions! Performance is the same for 191 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:ComposerTelemetryBench/benchTelemetryParsing

  • 🟩 execution_time [-1.906µs; -0.894µs] or [-10.830%; -5.079%]

scenario:SamplingRuleMatchingBench/benchGlobMatching1

  • 🟩 execution_time [-173.609ns; -58.191ns] or [-6.679%; -2.239%]

Unstable benchmarks

These benchmarks have a confidence interval too wide to call a change; treat them as noise rather than signal.

scenario:LaravelBench/benchLaravelDdprof-opcache

  • unstable execution_time [-1028.860µs; +463.460µs] or [-7.704%; +3.470%]

@vjfridge

This comment was marked as outdated.

@vjfridge

This comment was marked as outdated.

@vjfridge

Copy link
Copy Markdown

Note to self: performance

Fresh local 8d7488806 vs master 401174500, using the same optimized artifacts as the external tests. Values and privacy checks passed in all 96 runs, but there is measurable evaluation-time overhead and telemetry loss at high load.

Median per-evaluation time, in microseconds (median of three runs; application restricted to CPU 0):

API / requested rate master PR, events disabled PR, protected PR, consented
Native / 1,000 per sec 22.7 30.4 43.3 45.1
OpenFeature / 1,000 per sec 53.4 45.6 77.9 80.3
Native / 10,000 per sec 25.9 22.3 41.2 44.5
OpenFeature / 10,000 per sec 48.6 47.5 94.2 99.2
  • At 1,000/sec, all protected/consented observations arrived in all runs, on both CPU settings. This is a representative paced scenario, not a measured customer-traffic distribution.
  • At 10,000/sec on one CPU, protected native calls added about 15 µs; protected OpenFeature calls added about 46 µs. OpenFeature achieved about 9,750/sec protected / 9,301/sec consented, below the requested rate. Protected OpenFeature p95/p99 rose from 57/64 µs to 121/520 µs.
  • Allowing PHP CPUs 0–1 helped OpenFeature: protected median 49.6 → 72.8 µs, sustaining approximately 10,000/sec. It did not eliminate overhead or all losses.
  • All missing observations reconcile to queue_overflow diagnostics. At 10,000/sec, across 90,000 measured evaluations per cell: protected native dropped 9 on one CPU / 0 on two; protected OpenFeature 1,333 (1.48%) / 2. Consented native dropped 28 / 58; consented OpenFeature 971 / 26. No warmup losses. No reruns were used to replace these results.
  • At 10,000/sec on one CPU, PHP-process CPU rose from 33.5% → 45.6% of one core for native protected, and 50.6% → 76.0% for OpenFeature protected. Separate sidecar CPU is not included. PHP peak RSS remained roughly 36–37 MiB; this does not measure sidecar memory or prove long-term memory bounds.
Measurement scope

PHP 8.2.34 NTS / Linux ARM64, JIT disabled, default sidecar connection mode. Four scalar context fields, ten repeated identities, 500 warmup evaluations and 3,000 or 30,000 measured evaluations per run. Native and OpenFeature × one/two-CPU application affinity × 1,000/10,000 requested evaluations/sec × baseline/disabled/protected/consented × three repeats = 96 runs.

The normal shared sidecar was a separate process pinned to CPU 0 and reused across runs; the mock Agent ran on CPU 14. Both SDKs use the same native commit 634af2c84. CPU settings are affinity, not cgroup quotas. OTel evaluation metrics and exposure logging were disabled to isolate this signal. Context construction, warmup and final flush are outside the per-evaluation latency measurement. Values, exact emitted totals, drop diagnostics, protected raw-byte canaries and consented context were checked independently of timing.

These are short SDK-call microbenchmarks, not full PHP-FPM request latency or statistical performance-equivalence evidence. Disabled-mode differences are shown without claiming an improvement. The general CI benchmark report is separate and was still running when this comment was posted.

Drop the owned error returned by the shutdown flush while preserving best-effort error handling. Add a broken-connection regression verified with Valgrind leak checking.
@vjfridge
vjfridge marked this pull request as ready for review October 19, 2026 15:12
@vjfridge
vjfridge requested review from a team as code owners October 19, 2026 15:12
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-19T15:21:30.015823Z ce0ad1f Draft marked ready
🔒 Security Review ✅ Completed 2026-10-19T15:18:25.432856Z ce0ad1f Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@datadog-prod-us1-6 datadog-prod-us1-6 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: PASS

More details

Evaluation reporting uses the final mapped outcome and consent captured with that evaluation. The completed static review identified no reportable defects; native transport and lifecycle behavior remain unverified at runtime.

Was this helpful? React 👍 or 👎

Open Bits AI session

🤖 Bits Code Review · Commit ce0ad1f

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ce0ad1f61c

ℹ️ 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".

Comment thread tracer/ffe.c Outdated
Comment on lines +98 to +100
.service = dd_zend_string_to_CharSlice(get_DD_SERVICE()),
.env = dd_zend_string_to_CharSlice(get_DD_ENV()),
.version = dd_zend_string_to_CharSlice(get_DD_VERSION()),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use the resolved service and environment for evaluations

When DD_SERVICE or DD_ENV is unset, this sends the raw empty configuration values even though request initialization has already resolved them to the application defaults (for example, web.request or the CLI script name and none) and stored them in DATADOG_G(last_service_name)/last_env_name. Consequently, evaluation events from normally configured applications are attributed or coalesced under empty service/environment values instead of the same application identity used by traces and telemetry; build this context from the resolved request metadata rather than get_DD_SERVICE()/get_DD_ENV().

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or better: don't submit it at all. The sidecar has knowledge of the current context via the application_data present there. Not this calls task to submit that

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Thanks for pointing out that the sidecar already owns this information.
In 986e07435, PHP now passes empty application context with each flagevaluation submission. The argument remains because the current FFI requires it.

The pinned libdatadog does not yet attach the registered application's identity to these events, so the remaining plan is:

  • FFL-3438: attach service, environment, and application version in the sidecar before aggregation, so later application changes cannot relabel earlier evaluations.
  • FFL-3439: update PHP's libdatadog dependency and validate the complete path, including system-tests and ffe-dogfooding.

Both tickets are assigned to Vickie and are required before releasing this functionality. Until then, these events lack application attribution and are not separated by application identity during aggregation. Exposures and SDK-origin headers are unchanged.

Fresh local validation: 659 extension tests passed (93 skipped, three declared expected failures), 160 API tests passed, and 49 OpenFeature tests passed; lint and generated-source checks also passed. The payload tests cover the temporary absence of evaluation identity while retaining exposure identity and privacy checks. Earlier external-test results are historical; they were not rerun for this change.

Comment thread tracer/ddtrace.c
Comment thread src/api/FeatureFlags/Internal/NativeEvaluator.php
@bwoebi

bwoebi commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Could you please split the testsuites? Use phpunit tests (you can use phpt files there if you want - but some tests could benefit from proper API/helper reuse, so maybe a proper class test makes for nicer code?) for stuff testing src/. tests/ext is exclusively for extension stuff. I.e. if you want to test against the API surface exposed by the extension directly.

Accessing src/ from the extension is problematic for tests run outside of the well-controlled docker containers we use in our CI / testing environments.

Comment thread tracer/ddtrace.c
Comment thread tracer/functions.c Outdated
Comment thread tracer/functions.c Outdated
Comment thread tracer/tracer_telemetry.c Outdated
Comment thread src/api/FeatureFlags/Internal/NativeEvaluator.php Outdated
Include flag evaluations in signal flushes and use resolved request identity. Capture OpenFeature evaluation metadata per invocation and submit the final hook outcome without changing immediate native reporting.

Cover signal delivery, default and updated identity, terminal hook failures, nested calls, fibers, and raw payload privacy.
Keep Valgrind runtime checks and all signal-flush assertions while avoiding the slow final scan of libdatadog shared-memory reservations left by intentional _exit. Other tests retain their existing leak-check settings.
@leoromanovsky

Copy link
Copy Markdown
Contributor Author

✅ lgtm

@vjfridge
vjfridge requested a review from bwoebi October 20, 2026 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants