Add FFE evaluation metrics#3911
Conversation
|
Benchmarks [ tracer ]Benchmark execution time: 2026-05-24 05:55:43 Comparing candidate commit ccd81f1 in PR branch Found 1 performance improvements and 1 performance regressions! Performance is the same for 192 metrics, 0 unstable metrics. scenario:BM_TeaSapiSpindown
scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f021234c39
ℹ️ 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".
Adds a parallel pathway for PHP feature-flag evaluation metrics mirroring the FfeExposures forwarder. dd-trace-php encodes `feature_flag.evaluations` counters as OTLP/protobuf in PHP (via its existing PHP 7-safe `OtlpMetricEncoder`) and ships the encoded bytes to the sidecar, which POSTs them to the user-configured OTLP HTTP metrics intake. Why a sibling action instead of reusing FfeExposures: - The OTLP collector is not the Datadog Agent. It's user-configurable via OTEL_EXPORTER_OTLP_METRICS_ENDPOINT (default http://localhost:4318/v1/metrics), so the endpoint travels with the payload rather than being derived from the sidecar session's agent base URL. - Content type differs (application/x-protobuf vs application/json). - No EVP subdomain header. - The payload is binary protobuf, not a JSON string. dd-trace-php side (PR DataDog/dd-trace-php#3911) will refactor its existing `OtlpHttpMetricTransport` (which currently does PHP-side HTTP I/O, violating the architectural rule "no I/O outside the sidecar") to call this new FFI. Validation: - `cargo test -p datadog-sidecar ffe` passes 7 tests (3 exposures + 4 metrics). - `cargo check -p datadog-sidecar-ffi` clean.
Per Bob's PR review (2026-05-22), the tracer extension must perform no I/O outside the sidecar. Replaces the raw-socket `AgentExposureTransport` with `SidecarExposureTransport`, which forwards exposure batches to the libdatadog sidecar via a new native PHP function `\DDTrace\send_ffe_exposures` that calls the `ddog_sidecar_send_ffe_exposures` FFI added in DataDog/libdatadog#2026. PHP side: - Delete `Internal/Exposure/AgentExposureTransport.php` (raw socket POST to the Agent EVP proxy). - Add `Internal/Exposure/SidecarExposureTransport.php` that JSON-encodes the batch and calls `\DDTrace\send_ffe_exposures()`. Fire-and-forget; the sidecar handles retries. - Update `ExposureWriter::createDefault()` to instantiate the sidecar transport. - Drop the obsolete `testAgentTransportBuildsAgentEvpRequest` PHPUnit test (HTTP construction now lives in libdatadog, covered by `cargo test -p datadog-sidecar ffe_flusher`). - Add `Internal/DefaultEvaluationCompletedHook` and `Internal/CompositeEvaluationCompletedHook` so production callers go through a composite hook factory. In this PR the composite contains only `ExposureHook`; the metrics PR (#3911) contributes `EvaluationMetricHook` and the file conflict at merge resolves by combining both. Update `Client::create()` to call `DefaultEvaluationCompletedHook::create()`. C/Rust bridge: - Declare `ddog_ByteSlice` (and underlying `ddog_Slice_U8`) in `components-rs/common.h` for the metrics path; declare both `ddog_sidecar_send_ffe_exposures` and `ddog_sidecar_send_ffe_metrics` in `components-rs/sidecar.h`. - Add C wrappers `ddtrace_sidecar_send_ffe_exposures(zend_string *)` and `ddtrace_sidecar_send_ffe_metrics(zend_string *endpoint, zend_string *payload_bytes)` in `ext/sidecar.{h,c}` that call the FFI with the current sidecar transport + instance id + queue id. - Declare native PHP functions `\DDTrace\send_ffe_exposures(string): bool` and `\DDTrace\send_ffe_metrics(string, string): bool` in `ext/ddtrace.stub.php`; add corresponding arginfo entries and `ZEND_FUNCTION` registrations in `ext/ddtrace_arginfo.h`; implement `PHP_FUNCTION(DDTrace_send_ffe_exposures)` and `PHP_FUNCTION(DDTrace_send_ffe_metrics)` in `ext/ddtrace.c`. - Bump `libdatadog` submodule to FFE branch tip `29762335c` (which provides both FFIs). The submodule will be bumped to the libdatadog main commit once #2026 merges. Docs: - Add `docs/php-ffe-stack/{stack,system}-pr3910.{mmd,png}` for this PR. Validation: - `php vendor/bin/phpunit --config phpunit.xml tests/api/Unit/FeatureFlags` → 41 tests, 174 assertions, OK. - libdatadog sidecar tests (`cargo test -p datadog-sidecar ffe_flusher`) → 3 passed, on the pinned submodule commit. - Mermaid PNGs regenerate via `npx @mermaid-js/mermaid-cli`. `make test_featureflags` and `make test_c TESTS=tests/ext/ffe/...` will run in CI; running them locally requires rebuilding the extension which is gated behind libdatadog #2026 merging.
Adds the M3 evaluation-metrics layer on top of the hook PR (#3909) as a sibling of the EVP exposures PR (#3910). Records `feature_flag.evaluations` for both PHP 7 (DD Client hook) and PHP 8 (OpenFeature SDK hook); both paths share `EvaluationMetricHook::sharedWriter()` for unified aggregation. OTLP/protobuf payloads are encoded in PHP via the existing `OtlpMetricEncoder` and delivered to the user-configured OTLP HTTP metrics intake through the libdatadog sidecar (`ddog_sidecar_send_ffe_metrics` FFI added in DataDog/libdatadog#2026). This branch is force-pushed (user-authorized one-time exception to the no-force-push rule, 2026-05-23) to restructure history away from being linearly stacked on the M2 exposures PR (#3910). The PR now stacks directly on the hook PR (#3909) as a sibling of the EVP PR. PHP side: - Add `Internal/Metric/EvaluationMetricWriter` with bounded series aggregation, drop accounting, and shutdown flush. - Add `Internal/Metric/EvaluationMetricHook` (DD Client hook) and `OtlpMetricEncoder` (PHP 7-safe protobuf encoding). - Add `Internal/Metric/SidecarOtlpMetricsTransport` that calls `\DDTrace\send_ffe_metrics()` (FFI declared in #3910). Endpoint resolution: `OTEL_EXPORTER_OTLP_METRICS_ENDPOINT`, falling back to `OTEL_EXPORTER_OTLP_ENDPOINT + /v1/metrics`, default `http://localhost:4318/v1/metrics`. - Add `DDTrace\OpenFeature\EvalMetricsHook` implementing `OpenFeature\interfaces\hooks\Hook` (after + error stages), registered on `DataDogProvider` via `setHooks()`. - `DataDogProvider` constructs its internal DD `Client` with `DefaultEvaluationCompletedHook::createWithoutMetric()` so the OpenFeature path records the metric via the OpenFeature hook (PR 3911 scope) and NOT via the DD Client hook — preventing double-counting. PHP 7 path keeps recording via the DD Client hook. - Add `Internal/CompositeEvaluationCompletedHook` and `Internal/DefaultEvaluationCompletedHook` (metric-only composite). This is the merge-conflict point with PR #3910's `[ExposureHook]` composite — second merge resolves by combining both hooks. - Update `Client::create()` to call `DefaultEvaluationCompletedHook::create()`. - Drop the obsolete `testOtlpTransportBuildsHttpProtobufRequest` PHPUnit test (HTTP construction now lives in libdatadog, covered by `cargo test -p datadog-sidecar ffe_metrics_flusher`). - Add `_files_openfeature.php` entry for `EvalMetricsHook.php`. C/Rust bridge: the `\DDTrace\send_ffe_metrics()` native function, its C wrapper `ddtrace_sidecar_send_ffe_metrics()`, and the `ddog_sidecar_send_ffe_metrics` FFI declaration in `components-rs/sidecar.h` were already added in #3910. This PR's branch picks up those changes once #3910 merges (or via the same libdatadog submodule pin during review). For development locally the libdatadog submodule is pinned to the FFE branch tip (`29762335c`). Docs: - Add `docs/php-ffe-stack/{stack,system}-pr3911.{mmd,png}` per the 4-PR documentation convention. Validation: - `php vendor/bin/phpunit --config phpunit.xml tests/api/Unit/FeatureFlags` → 40 tests, 160 assertions, OK. - Mermaid PNGs regenerate via `npx @mermaid-js/mermaid-cli`. `make test_featureflags`, OpenFeature PHPUnit, and ffe-dogfooding end-to-end validation will run in CI / are validated separately by FOLLOW-05 Steps 4–5.
f021234 to
e74b050
Compare
The SidecarOtlpMetricsTransport::resolveEndpoint() default ("http://localhost:4318/v1/metrics")
doesn't match the system-tests parametric setup, where the PHP test
client receives DD_AGENT_HOST=<test_agent_container> but no
OTEL_EXPORTER_OTLP_METRICS_ENDPOINT. The previous OtlpHttpMetricTransport
(replaced by this transport) derived the OTLP endpoint from DD_AGENT_HOST
+ port 4318. Restore that fallback so system-tests
test_php_ffe_evaluation_metric finds the test-agent OTLP intake.
Resolution order (now matches the old transport):
1. OTEL_EXPORTER_OTLP_METRICS_ENDPOINT (explicit)
2. OTEL_EXPORTER_OTLP_ENDPOINT + /v1/metrics
3. DD_AGENT_HOST + :4318/v1/metrics
4. localhost:4318/v1/metrics
The C/Rust bridge for the new native PHP functions \DDTrace\send_ffe_exposures() and \DDTrace\send_ffe_metrics() lives on the M2 EVP exposures PR (#3910) because that PR introduced the bridge when refactoring the exposure transport. PR #3911 (this PR) needs the same bridge for its OTLP metrics transport — without it the SidecarOtlpMetricsTransport silently drops batches because function_exists('\\DDTrace\\send_ffe_metrics') is false. Adds the same bridge files here so the M3 branch is independently compilable. At merge time the two PRs will conflict at the file level on these bridge files; resolution is deduplication (the bridge is identical in both PRs by design). Files added/modified: - components-rs/sidecar.h: declares ddog_sidecar_send_ffe_exposures and ddog_sidecar_send_ffe_metrics FFIs. - components-rs/common.h: declares ddog_ByteSlice typedef for the metrics payload. - ext/sidecar.h, ext/sidecar.c: C wrappers ddtrace_sidecar_send_ffe_exposures() and ddtrace_sidecar_send_ffe_metrics(). - ext/ddtrace.stub.php, ext/ddtrace_arginfo.h, ext/ddtrace.c: declares the native PHP functions and the PHP_FUNCTION implementations.
Pulls in libdatadog commit `875ec8f0e` ("fix(sidecar): dispatch FFE
actions before application-entry check"). Without this fix, the
`SidecarOtlpMetricsTransport::send()` call from PHP would silently
no-op for short-lived processes: the sidecar received the
`FfeMetrics` action but dropped it because the `Entry::Occupied`
gate on the application metadata had not yet fired.
This unblocks the parametric system-test
`Test_Feature_Flag_Parametric_Evaluation_Metrics::test_php_ffe_evaluation_metric`
which exercises the full PHP -> sidecar -> OTLP-HTTP-intake path
end-to-end. Local result: 26/27 FFE-scoped parametric tests pass
(remaining failure is the EVP exposure test, which lives on the
M2 PR #3910 branch).
Pulls in libdatadog commit `875ec8f0e` ("fix(sidecar): dispatch FFE
actions before application-entry check") so the EVP exposure batch sent
via `ddog_sidecar_send_ffe_exposures` is no longer silently dropped
when the PHP runtime hasn't yet registered the application against the
sidecar's `QueueId`. Same fix is on the sibling PR #3911 (`2a48c4987")
for the OTLP metric path.
Without this submodule bump, `Test_Feature_Flag_Parametric_Exposures::test_php_ffe_exposure_event`
sees zero EVP POSTs at the test-agent because the sidecar's
`enqueue_actions` handler discards the `FfeExposures` action under
the `Entry::Occupied` gate.
…macOS+colima
`build-debug-artifact` produces a `/output` bind mount via `-v
${TMP_OUT}:/output`. On macOS+colima only paths under $HOME are
mounted into the Linux VM; the macOS default `/var/folders/...` temp
dir is not, so writes from inside the container to `/output` land in
the VM and never propagate back to the host. The script exits without
error and the user's binaries dir is left with whatever stale artifact
was there before.
Pin TMP_OUT/TMP_PKG under $OUTPUT_DIR (which the user just passed as
an absolute, usable destination) so the bind mount is always on a
host-visible path. Inherits cleanup via the existing EXIT trap.
The same fix is independently on the M3 branch (#3911) as part of
e74b050; bringing it to the M2 branch (#3910) so both branches are
buildable on macOS without a `TMPDIR=$HOME/...` override.
…gh rez Three fixes to the per-PR FFE diagrams: 1. **Titles were truncated to "PHP FFE 4-PR stack — current =" with the PR number missing.** Mermaid uses the YAML frontmatter for the title block, and YAML treats unquoted `#` as the start of a comment, so `title: PHP FFE 4-PR stack — current = #3911 (OTLP metrics)` got parsed as just `PHP FFE 4-PR stack — current =`. Quoting the title keeps the `#PR-number` portion intact. 2. **System diagrams switched from `flowchart LR` to `flowchart TD`.** LR forced the PHP-process / host / backend lanes into a single very-wide row that rendered as an unreadable horizontal strip on PR pages. TD stacks them vertically and keeps the per-lane subgraphs readable. 3. **Re-rendered at 2400×2400 with `--scale 3`** (~1800×2000 stack, ~3000×4500 system) instead of the default ~600px width. PR-page thumbnails render legibly and zoomed-in detail stays sharp. README's regeneration recipe updated with all three knobs and a note on why the `title:` quoting matters.
…gh rez Same three fixes as the parallel commit on the M3 (PR #3911) branch: 1. Quote the YAML `title:` so the `#PR-number` is not parsed as a comment (previous render had the title truncated at "current ="). 2. `flowchart LR` → `flowchart TD` on system diagrams so vertical PHP-process → host → backend lanes stack vertically instead of getting squeezed into one wide row. 3. Render with `-w 2400 -H 2400 --scale 3 -b white` (~1900×2000 stack, ~3000×4600 system) instead of ~600px default.
…gh rez Same three fixes as on the M2 (#3910) and M3 (#3911) sibling branches: 1. Quote the YAML `title:` so the `#PR-number` survives parsing (otherwise YAML treats the `#` as a comment and the title renders as "PHP FFE 4-PR stack — current =" with the rest missing). 2. `flowchart LR` → `flowchart TD` on the system diagram so the PHP-process / host-sidecar / backend lanes stack vertically. 3. Render at 2400×2400 `--scale 3` instead of ~600px default.
Brings the PHP FFE diagram convention to the M1 PR. Each subsequent PR in the stack (#3909, #3910, #3911) already carried its own stack + system diagram; #3906 was missing them. Mirrors the format used by the rest of the stack: - `stack-pr3906.mmd` — the 4-PR stack with #3906 badged as current and the downstream layers shown as "future". - `system-pr3906.mmd` — the target end-to-end architecture with M1's scope (UserCode, OpenFeature Client, DataDogProvider, DDTrace FeatureFlags Client, NativeEvaluator, Remote Config client) highlighted, and everything from the Hook layer onward dashed. All conventions match the other branches: quoted YAML titles (to keep `#PR-number` out of the YAML comment parser), `flowchart TD` orientation, rendered with `-w 2400 -H 2400 --scale 3 -b white`.
`AbstractProvider::setHooks(array $hooks)` REPLACES the hook list, so registering our `EvalMetricsHook` via `setHooks([$metricsHook])` in the constructor would silently drop our metric emission as soon as a user configures their own provider-level hooks. Override `getHooks()` to always prepend the Datadog metric hook to the caller-supplied list. The user can register their own provider hooks freely (`$provider->setHooks($theirHooks)`) and we still record `feature_flag.evaluations` on every OpenFeature evaluation. Adds a unit test that constructs a DataDogProvider, calls `$provider->setHooks([$userHook])`, then asserts `$provider->getHooks()` returns `[EvalMetricsHook, $userHook]`.
`resolveEndpoint()` previously fell back to `http://$DD_AGENT_HOST:4318/v1/metrics` when neither OTel env var was set, and also peeled off `unix://` prefixes and wrapped IPv6 hosts in brackets. That extra path was Datadog-specific and undocumented — it existed to make the parametric system-tests pass without configuring the standard OTel env, and to be "helpful" for callers who set DD_AGENT_HOST but not OTEL_EXPORTER_OTLP_METRICS_ENDPOINT. Strip it. Follow the OpenTelemetry environment-variable spec only: 1. OTEL_EXPORTER_OTLP_METRICS_ENDPOINT — used as-is 2. OTEL_EXPORTER_OTLP_ENDPOINT — append /v1/metrics 3. http://localhost:4318/v1/metrics — OTel spec default If a Datadog Agent's OTLP intake is the target, the caller sets OTEL_EXPORTER_OTLP_METRICS_ENDPOINT explicitly. System-tests parametric metric test still passes: the parametric test client now injects OTEL_EXPORTER_OTLP_METRICS_ENDPOINT pointing at the test-agent OTLP listener (companion change in DataDog/system-tests:leo.romanovsky/pr-g-php-ffe-scaffold). FFE-scoped parametric: 26/27 (only the expected exposure_event failure remains — M3 branch has no exposure code path).
…iners Mirrors the existing DD_AGENT_HOST / DD_TRACE_AGENT_URL injection so OTel-aware exporters in the system-under-test can reach the test agent's OTLP/HTTP listener without each tracer needing to special-case the parametric framework. Concretely: the PHP FFE OTLP-metrics path (DataDog/dd-trace-php#3911) resolves its endpoint per the OpenTelemetry spec — OTEL_EXPORTER_OTLP_METRICS_ENDPOINT, then OTEL_EXPORTER_OTLP_ENDPOINT, then localhost:4318. Without this injection, the parametric metric test had no way to reach the test agent.
…silently drop When the series map reaches `seriesLimit` (default 1000 unique attribute-sets), the writer was returning false and incrementing `dropped` for every new key. In PHP-FPM/Apache that was harmless because `register_shutdown_function` fires per request and the map empties at the end of each request. In long-running PHP runtimes — Swoole, RoadRunner, FrankenPHP/Octane, CLI worker loops — the shutdown function only fires when the worker process exits, so the series map filled once and then every new unique attribute-set was silently dropped for the rest of the worker's lifetime (hours or days). Flush inline when the cap is hit so the new key fits. Same fix as the parallel commit on the M2 (PR #3910) ExposureWriter. Test `testSeriesOverflowDropsNewSeriesButKeepsExistingSeries` renamed and rewritten to assert the new auto-flush behavior: `droppedCount() === 0` after three records that would have dropped one under the old code; transport receives three batches instead of one. Caught by Codex review on the parallel M2 commit.
…-ffe-evaluation-metrics
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccd81f147f
ℹ️ 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".
| return rtrim($baseEndpoint, '/') . '/v1/metrics'; | ||
| } | ||
|
|
||
| return 'http://localhost:4318/v1/metrics'; |
There was a problem hiding this comment.
Honor the configured Datadog agent endpoint
When DD_METRICS_OTEL_ENABLED is used with the usual Datadog agent config (DD_AGENT_HOST or DD_TRACE_AGENT_URL) but no explicit OTEL_EXPORTER_OTLP*_ENDPOINT, this hardcoded localhost fallback sends FFE metrics to the application container instead of the agent. The existing OTel resolver in src/DDTrace/OpenTelemetry/CompositeResolver.php derives metrics endpoints from those Datadog settings, so in containerized deployments where DD_AGENT_HOST=datadog-agent makes normal OTel metrics work, these new feature_flag.evaluations batches are posted to localhost:4318 and get dropped.
Useful? React with 👍 / 👎.
Motivation
This PR emits
feature_flag.evaluationscounters for PHP feature-flag evaluations. The metric matches the Go/JS/Java/.NET reference SDK shape: one count per completed evaluation result, grouped by flag key, variant, reason, optional allocation key, and optional error type.Metrics are emitted only when
DD_METRICS_OTEL_ENABLED=trueis configured. Delivery goes through the libdatadog sidecar; PHP performs no HTTP I/O.Planning/reference doc: https://docs.google.com/document/d/1NvMfTpZWLBlFmEFNjdnlMyeVpy5l7KD8qujGFco6w2w/edit?tab=t.0
Decisions
DataDogProvider.createWithoutMetric()for the internal DD Client so only the OpenFeature hook records the metric; this prevents double-counting.after+error, notfinally, because PHP OpenFeature SDK'sHook::finallysignature does not carryResolutionDetails.OtlpMetricEncoderencodes OTLP/protobuf in PHP. The sidecar only POSTs the payload bytes.OTEL_EXPORTER_OTLP_METRICS_ENDPOINT, thenOTEL_EXPORTER_OTLP_ENDPOINT + /v1/metrics, thenDD_AGENT_HOST:4318/v1/metrics, thenhttp://localhost:4318/v1/metrics.service.name; env/version attribution depends on the configured OTLP/Agent pipeline unless reviewers ask us to encode more resource attributes directly in PHP.Where this PR fits in the stack
This is the OTLP metrics layer. It is a sibling of #3910 (EVP exposures) under the hook PR #3909.
Where this PR fits in the target system
This PR contributes the
EvaluationMetricWriter, the OpenFeature SDKEvalMetricsHook, theddog_sidecar_send_ffe_metricsFFI path, and the OTLP HTTP intake leg. The EVP/Agent exposure path is handled by #3910.Changes
EvaluationMetricWriterwith bounded aggregation by attribute set, drop accounting, inline flush on series-limit pressure, and shutdown flush.EvaluationMetricHookfor the Datadog client path andEvalMetricsHookfor the PHP 8 OpenFeature path.OtlpMetricEncoderandSidecarOtlpMetricsTransport.DataDogProviderso user calls tosetHooks()do not accidentally remove Datadog metric emission.CompositeEvaluationCompletedHookandDefaultEvaluationCompletedHookreturning[EvaluationMetricHook]. This file conflicts with Add FFE exposure emission #3910's[ExposureHook]composite; the second merge combines both hooks.\DDTrace\send_ffe_metrics()FFI surface and uses thelibdatadogsubmodule pin875ec8f0e, which contains both FFE sidecar FFIs.tooling/bin/build-debug-artifactto create its Docker-mounted staging directories under the requested output directory instead of the host default temp path.Dependencies
Questions for reviewers
deployment.environment/service.versionresource attributes directly, or isservice.nameplus collector/Agent enrichment the desired parity point with the other SDKs?