Repository navigation
Add bounded feature flag evaluation EVP reporting - #4288
leoromanovsky wants to merge 19 commits into
Conversation
|
Benchmarks [ tracer ]Benchmark execution time: 2026-10-20 22:46:04 Comparing candidate commit 37e3d84 in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 191 metrics, 1 unstable metrics.
|
…agentless-20261008 # Conflicts: # components-rs/common.h # libdatadog
…agentless-20261008 # Conflicts: # libdatadog
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Note to self: performanceFresh local Median per-evaluation time, in microseconds (median of three runs; application restricted to CPU 0):
Measurement scopePHP 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 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.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…agentless-20261008
There was a problem hiding this comment.
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.
🤖 Bits Code Review · Commit ce0ad1f
There was a problem hiding this comment.
💡 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".
| .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()), |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
🤖 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.
|
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/. Accessing |
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.
…agentless-20261008
|
✅ lgtm |
Motivation
PHP already reports feature-flag exposures. This adds privacy-safe
flagevaluationtrack 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
observeFullEvaluationData: true, hash the targeting key and omit evaluation context. With consent, include bounded scalar context; oversized/degraded observations can omit identity and context.DD_FLAGGING_EVALUATION_COUNTS_ENABLEDindependently of OTel metrics. Fixed-name rejection metrics report drops through ordinary telemetry, without per-evaluation logging.flagevaluationevents, with the PHP SDK name/version on both writers; retain Remote Config's Agent EVP v2 route.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.
Blast radius and tradeoffs
Testing
External comparisons below used
8d7488806versusmaster401174500. The only later commit,ce0ad1f61, fixes shutdown error cleanup and adds its regression test; CI and local leak checks cover that final commit.flagevaluationevents from this branch. Live consent was true; absent/false consent is covered separately by system-tests.ce0ad1f61. This is separate from the FFE-specific overhead measured above.