Conversation
|
✨ Fix all issues with BitsAI or with Cursor
|
leoromanovsky
force-pushed
the
feature/ffe-feature-flagging
branch
from
18f7f00 to
508a8c8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3630 +/- ##
==========================================
+ Coverage 62.32% 62.39% +0.07%
==========================================
Files 142 142
Lines 13586 13586
Branches 1775 1775
==========================================
+ Hits 8467 8477 +10
+ Misses 4311 4304 -7
+ Partials 808 805 -3 see 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
leoromanovsky
force-pushed
the
feature/ffe-feature-flagging
branch
3 times, most recently
from
ff84598 to
9f36eb7
Compare
leoromanovsky
force-pushed
the
feature/ffe-feature-flagging
branch
from
7c43198 to
7bbed08
Compare
Benchmarks [ tracer ]Benchmark execution time: 2026-03-12 20:09:41 Comparing candidate commit c77115f in PR branch Found 2 performance improvements and 38 performance regressions! Performance is the same for 153 metrics, 1 unstable metrics. scenario:ComposerTelemetryBench/benchTelemetryParsing
scenario:ContextPropagationBench/benchExtractHeaders128Bit
scenario:ContextPropagationBench/benchExtractHeaders128Bit-opcache
scenario:ContextPropagationBench/benchExtractHeaders64Bit
scenario:ContextPropagationBench/benchExtractTraceContext128Bit
scenario:ContextPropagationBench/benchExtractTraceContext128Bit-opcache
scenario:ContextPropagationBench/benchExtractTraceContext64Bit
scenario:ContextPropagationBench/benchInject128Bit
scenario:ContextPropagationBench/benchInject64Bit
scenario:EmptyFileBench/benchEmptyFileDdprof
scenario:HookBench/benchHookOverheadInstallHookOnFunction
scenario:HookBench/benchHookOverheadInstallHookOnMethod
scenario:HookBench/benchHookOverheadTraceFunction-opcache
scenario:HookBench/benchWithoutHook
scenario:LaravelBench/benchLaravelDdprof
scenario:LogsInjectionBench/benchLogsInfoBaseline-opcache
scenario:MessagePackSerializationBench/benchMessagePackSerialization
scenario:MessagePackSerializationBench/benchMessagePackSerialization-opcache
scenario:PHPRedisBench/benchRedisBaseline
scenario:PHPRedisBench/benchRedisOverhead-opcache
scenario:SamplingRuleMatchingBench/benchGlobMatching1
scenario:SamplingRuleMatchingBench/benchGlobMatching2
scenario:SamplingRuleMatchingBench/benchGlobMatching3
scenario:SamplingRuleMatchingBench/benchGlobMatching4
scenario:SamplingRuleMatchingBench/benchRegexMatching1
scenario:SamplingRuleMatchingBench/benchRegexMatching1-opcache
scenario:SamplingRuleMatchingBench/benchRegexMatching2
scenario:SamplingRuleMatchingBench/benchRegexMatching2-opcache
scenario:SamplingRuleMatchingBench/benchRegexMatching3
scenario:SamplingRuleMatchingBench/benchRegexMatching3-opcache
scenario:SamplingRuleMatchingBench/benchRegexMatching4
scenario:SamplingRuleMatchingBench/benchRegexMatching4-opcache
scenario:SpanBench/benchDatadogAPI
scenario:WordPressBench/benchWordPressBaseline
scenario:WordPressBench/benchWordPressDdprof
scenario:WordPressBench/benchWordPressOverhead
|
## Motivation Add Feature Flagging and Experimentation (FFE) support to the remote config infrastructure, enabling tracers to subscribe to FFE_FLAGS configurations via the sidecar. WIP: php tracer changes (DataDog/dd-trace-php#3630) ## Changes - Add `FfeFlags` variant to `RemoteConfigProduct` enum - Add `"FFE_FLAGS"` string mapping in Display and FromStr - Add `FfeFlagConfigurationRules = 46` to `RemoteConfigCapabilities` - Add `FfeFlags(Vec<u8>)` variant to `RemoteConfigData` to preserve raw config bytes ## Decisions - Raw bytes are preserved (not parsed) in `FfeFlags(Vec<u8>)` since each tracer handles evaluation with the `datadog-ffe` crate directly - Capability bit 46 matches the server-side FFE capability definition Co-authored-by: leo.romanovsky <leo.romanovsky@datadoghq.com>
leoromanovsky
force-pushed
the
feature/ffe-feature-flagging
branch
from
1990188 to
867337e
Compare
leoromanovsky
changed the title
leoromanovsky
force-pushed
the
feature/ffe-feature-flagging
branch
from
a24b1ae to
24e4304
Compare
leoromanovsky
force-pushed
the
feature/ffe-feature-flagging
branch
from
2a5d688 to
1ec323b
Compare
leoromanovsky
changed the title
Add FFE support to dd-trace-php. Flag evaluation is delegated to libdatadog's datadog-ffe Rust crate via FFI. PHP handles orchestration: config lifecycle, exposure dedup, and HTTP transport. Rust FFI layer (components-rs/ffe.rs): - C-callable bridge to datadog-ffe::rules_based - Global config store behind Mutex<FfeState> - Structured attribute passing (no JSON on hot path) C extension (ext/ddtrace.c): - ffe_evaluate, ffe_has_config, ffe_config_changed, ffe_load_config - Marshals PHP arrays to FfeAttribute structs Remote Config (components-rs/remote_config.rs): - Register FfeFlags product + FfeFlagConfigurationRules capability - Handle add/remove of FFE configs via sidecar PHP Provider (src/DDTrace/FeatureFlags/Provider.php): - Singleton checking RC config state - Calls native evaluate, parses JSON results - Reports exposures via LRU-deduplicated writer Exposure pipeline: - LRU cache (65K entries) with length-prefixed composite keys - Batched writer to /evp_proxy/v2/api/v2/exposures (1000 cap) - Auto-flush via register_shutdown_function OpenFeature adapter (src/DDTrace/OpenFeature/DataDogProvider.php): - Implements AbstractProvider for open-feature/sdk Build: - Add datadog-ffe to RUST_FILES in Makefile for PECL packaging - Cargo.lock: minimal additions only (73 new crates, no gratuitous bumps) - Bump libdatadog to ed316b638 (FFE RC support, libdatadog#1532) Tests: - LRU cache unit tests (11 tests) - Exposure cache unit tests (12 tests) - 220 evaluation correctness tests from JSON fixtures Config: DD_EXPERIMENTAL_FLAGGING_PROVIDER_ENABLED (default: false)
…OpenFeature layers - Add null check for flag_key in ddog_ffe_evaluate Rust FFI - Set variant in OpenFeature ResolutionDetails (was always null) - Replace magic numbers with named constants for reason/error codes - Fix json_decode null ambiguity in JSON type parsing with json_last_error() - Add dropped event tracking and curl failure logging (DD_TRACE_DEBUG) - Guard against missing curl extension in ExposureWriter flush - Fix buildEvent docblock parameter order
Add FlagEvalMetrics OpenFeature hook that records an OTel counter (feature_flag.evaluations) after every flag evaluation. Attributes: feature_flag.key, feature_flag.result.variant, feature_flag.result.reason (lowercase), error.type (on error). Enabled only when DD_METRICS_OTEL_ENABLED=true and open-telemetry/sdk is installed. Is a complete noop otherwise. Register hook in DataDogProvider constructor via setHooks(). Add FlagEvalMetrics.php to _files_tracer.php autoloader.
…-zero error codes - Fix errorCodeToTag() to match actual Rust constants: 1=ERROR_TYPE_MISMATCH→type_mismatch, 2=ERROR_CONFIG_PARSE→parse_error, 3=ERROR_FLAG_UNRECOGNIZED→flag_not_found (was transposed) - Per OpenFeature spec, force reason=ERROR for any non-zero error_code. FlagUnrecognizedOrDisabled returns REASON_DEFAULT from the Rust layer but must be reported as ERROR to callers.
leoromanovsky
force-pushed
the
feature/ffe-feature-flagging
branch
from
f624325 to
6b83643
Compare
- FlagEvalMetrics.php: class constant visibility (private const) requires PHP 7.1+; drop to bare const for PHP 7.0 compatibility. The class is already internal so visibility adds nothing. - metadata/supported-configurations.json: regenerate after adding DD_EXPERIMENTAL_FLAGGING_PROVIDER_ENABLED to configuration.h. Fixes the "Configuration Consistency" CI check.
…tements Windows MSVC compiles PHP extensions in C89 mode (no /std:c11), which requires all variable declarations to precede executable statements within a block scope. The ffe_evaluate handler had three violations: - c_attrs and attrs_count declared after the targeting_key if-statement - idx/key/val declared after the c_attrs = ecalloc() call - val/var/ak declared after array_init(return_value) Moves all declarations to the top of their respective blocks.
leoromanovsky
changed the title
- Add feature_flag.provider.name=datadog to OTel metric attributes - Propagate error codes to OpenFeature ResolutionDetails via withError() - Prefer dd_trace_env_config over getenv in isFeatureFlagEnabled() - Log non-2xx HTTP status codes in ExposureWriter debug mode - Clarify continue-in-switch comment in ddtrace.c
leoromanovsky
requested review from
a team,
dd-oleksii and
greghuels
and removed request for
a team
Adds EvaluationTest.php with a data-provider that scans all JSON fixture files under tests/FeatureFlags/fixtures/evaluation-cases/ and drives ffe_evaluate() for each case, asserting value and reason. Fixture files cover boolean, integer, numeric, string, JSON, kill-switch, disabled, regex, date, comparator, null-operator, and special-character flag types. JSON variation fixtures use float values (1.0/2.0) matching the raw JSON returned by the FFE engine (php json_decode preserves the decimal point from the UFC config). Also fixes a minor LRUCache put() return-value doc inconsistency.
dd-oleksii
left a comment
dd-oleksii
left a comment
There was a problem hiding this comment.
Looks good overall. I think the only major piece is that we shouldn't default targeting key to empty string. oh, and a suspicious const-cast that probably needs some comments/justification
Not sure if that would speed up reviews or not but this PR seems pretty splittable:
- general setup returning default values, OF integration, public API
- evaluation, rust pieces
- exposures, lru cache, etc.
- evaluation metrics
| /// contention is not a practical concern. Keeping a Mutex for simplicity. | ||
| struct FfeState { | ||
| config: Option<Configuration>, | ||
| changed: bool, |
There was a problem hiding this comment.
minor: not sure how changed is used by the other side but having a global changed flag seems error-prone. What do you think about replacing it with version: u64?
| let was_changed = state.changed; | ||
| state.changed = false; | ||
| was_changed |
There was a problem hiding this comment.
nitpick: not necessarily better but there's a function for this:
| let was_changed = state.changed; | |
| state.changed = false; | |
| was_changed | |
| std::mem::replace(&mut state.changed, false) |
| pub value_json: CString, | ||
| pub variant: Option<CString>, | ||
| pub allocation_key: Option<CString>, | ||
| pub reason: i32, | ||
| pub error_code: i32, | ||
| pub do_log: bool, |
There was a problem hiding this comment.
minor: shall fields be non-public if it's supposed to be opaque?
| pub value_json: CString, | |
| pub variant: Option<CString>, | |
| pub allocation_key: Option<CString>, | |
| pub reason: i32, | |
| pub error_code: i32, | |
| pub do_log: bool, | |
| value_json: CString, | |
| variant: Option<CString>, | |
| allocation_key: Option<CString>, | |
| reason: i32, | |
| error_code: i32, | |
| do_log: bool, |
| attributes_count: usize, | ||
| ) -> *mut FfeResult { | ||
| if flag_key.is_null() { | ||
| return std::ptr::null_mut(); |
There was a problem hiding this comment.
minor: consider returning an error result on errors?
| }; | ||
|
|
||
| // Build targeting key | ||
| let tk = if targeting_key.is_null() { |
There was a problem hiding this comment.
nitpick:
| let tk = if targeting_key.is_null() { | |
| let targeting_key = if targeting_key.is_null() { |
| return rtrim($agentUrl, '/'); | ||
| } | ||
|
|
||
| $host = $this->getConfigValue('DD_AGENT_HOST', 'localhost'); |
There was a problem hiding this comment.
just checking — is this name correct? The other two start with DD_TRACE_AGENT and this one is DD_AGENT
| private static $REASON_MAP = [ | ||
| 0 => 'STATIC', | ||
| 1 => 'DEFAULT', | ||
| 2 => 'TARGETING_MATCH', | ||
| 3 => 'SPLIT', | ||
| 4 => 'DISABLED', | ||
| 5 => 'ERROR', | ||
| ]; | ||
|
|
||
| private static $TYPE_MAP = [ | ||
| 'STRING' => 0, | ||
| 'INTEGER' => 1, | ||
| 'NUMERIC' => 2, | ||
| 'BOOLEAN' => 3, | ||
| 'JSON' => 4, | ||
| ]; |
There was a problem hiding this comment.
minor: does it make sense to keep these translations in C, so we can use enums vs. having to sync maps manually?
| { | ||
| $this->writer = new ExposureWriter(); | ||
| $this->exposureCache = new ExposureCache(65536); | ||
| $this->enabled = $this->isFeatureFlagEnabled(); |
There was a problem hiding this comment.
nitpick: misleading name for the function
| * Flag configurations are received via Remote Config through the sidecar. | ||
| * Reports exposure events to the EVP proxy for analytics. | ||
| */ | ||
| class Provider |
There was a problem hiding this comment.
Doesn't look like it actually implements openfeature provider, so the name is misleading
| $targetingKey = ''; | ||
| $attributes = []; | ||
|
|
||
| if ($context !== null) { | ||
| $targetingKey = $context->getTargetingKey() ?? ''; |
There was a problem hiding this comment.
major: don't default targeting key to empty string. Default value should be null
| } else if (FUNCTION_NAME_MATCHES("ffe_has_config")) { | ||
| RETVAL_BOOL(ddog_ffe_has_config()); | ||
| } else if (FUNCTION_NAME_MATCHES("ffe_config_changed")) { | ||
| RETVAL_BOOL(ddog_ffe_config_changed()); | ||
| } else if (params_count == 1 && FUNCTION_NAME_MATCHES("ffe_load_config")) { | ||
| zval *json_zv = ZVAL_VARARG_PARAM(params, 0); | ||
| if (Z_TYPE_P(json_zv) == IS_STRING) { | ||
| RETVAL_BOOL(ddog_ffe_load_config(Z_STRVAL_P(json_zv))); | ||
| } | ||
| } else if (FUNCTION_NAME_MATCHES("ffe_evaluate") && params_count >= 4) { |
There was a problem hiding this comment.
Please add proper documented functions on ddtrace.stub.php for this.
dd_trace_internal_fn is generally meant for testing purposes.
| pub value_json: CString, | ||
| pub variant: Option<CString>, | ||
| pub allocation_key: Option<CString>, |
There was a problem hiding this comment.
Store it as String, instead of as CString. This simply costs you an extra conversion for no gain.
There's libdd_common_ffi::CharSlice to return these (not-null-byte-delimited strings) to PHP then instead of *mut c_char.
| size_t idx = 0; | ||
| zend_string *key; | ||
| zval *val; | ||
| c_attrs = ecalloc(attrs_count, sizeof(struct FfeAttribute)); |
There was a problem hiding this comment.
| c_attrs = ecalloc(attrs_count, sizeof(struct FfeAttribute)); | |
| c_attrs = emalloc(attrs_count * sizeof(struct FfeAttribute)); |
No reason for zeroing here.
| const char *val; | ||
| const char *var; | ||
| const char *ak; | ||
| array_init(return_value); | ||
| val = ddog_ffe_result_value(result); | ||
| var = ddog_ffe_result_variant(result); | ||
| ak = ddog_ffe_result_allocation_key(result); |
There was a problem hiding this comment.
| const char *val; | |
| const char *var; | |
| const char *ak; | |
| array_init(return_value); | |
| val = ddog_ffe_result_value(result); | |
| var = ddog_ffe_result_variant(result); | |
| ak = ddog_ffe_result_allocation_key(result); | |
| array_init(return_value); | |
| const char *val = ddog_ffe_result_value(result); | |
| const char *var = ddog_ffe_result_variant(result); | |
| const char *ak = ddog_ffe_result_allocation_key(result); |
nit
| if (val) { add_assoc_string(return_value, "value_json", (char *)val); } | ||
| else { add_assoc_null(return_value, "value_json"); } | ||
| if (var) { add_assoc_string(return_value, "variant", (char *)var); } | ||
| else { add_assoc_null(return_value, "variant"); } | ||
| if (ak) { add_assoc_string(return_value, "allocation_key", (char *)ak); } | ||
| else { add_assoc_null(return_value, "allocation_key"); } |
There was a problem hiding this comment.
We don't write our ifs here this way, could you please write it as:
if () {
...
} else {
...
}
| zval *targeting_key_zv = ZVAL_VARARG_PARAM(params, 2); | ||
| zval *attrs_zv = ZVAL_VARARG_PARAM(params, 3); | ||
| if (Z_TYPE_P(flag_key_zv) == IS_STRING) { | ||
| /* Declare all variables at top of block for C89/MSVC compatibility */ |
There was a problem hiding this comment.
We're at C99. (and MSVC supports that). No need.
| } | ||
|
|
||
| /** | ||
| * Send all buffered exposure events as a batch to the EVP proxy. |
There was a problem hiding this comment.
Can you route this through the extension all the way back to the sidecar, which shall take care of sending this?
The PHP tracer does not do requests by itself on the main thread (especially not "fire-and-forget" requests).
This will manifest as pure latency on every request on the user side. Very bad thing :-D
Then test_cross_request_dedup would also succeed...
There was a problem hiding this comment.
I.e. have ffe_evaluate handle this directly in the rust code. This whole ExposureWriter has no business being in the PHP code.
| } | ||
| self::$initialized = true; | ||
|
|
||
| if (!function_exists('dd_trace_env_config') || !\dd_trace_env_config('DD_METRICS_OTEL_ENABLED')) { |
There was a problem hiding this comment.
DD_METRICS_OTEL_ENABLED governs the default exporter or the datadog-configured exporter is used. It does not have a fundamental influence on the presence of otel or such.
I don't think this check belongs here.
| $envVal = getenv('DD_EXPERIMENTAL_FLAGGING_PROVIDER_ENABLED'); | ||
| if ($envVal === false || $envVal === '') { | ||
| return false; | ||
| } | ||
| return strtolower($envVal) === 'true' || $envVal === '1'; |
There was a problem hiding this comment.
I think it should default to false here, if dd_trace_env_config is not present. Because that means you also cannot eval it.
| </testsuite> | ||
| <testsuite name="featureflags"> | ||
| <directory>./FeatureFlags/</directory> | ||
| </testsuite> |
There was a problem hiding this comment.
You also need to ensure this is run in CI (see .gitlab/generate-tracer.php)
|
Also please don't split this PR. I want to ensure that the whole functionality works properly and shall be included in a single step. |
| /// `config` updated but `changed` still false (or vice-versa). | ||
| /// | ||
| /// A `RwLock` would be more appropriate here (many readers via `ddog_ffe_evaluate`, | ||
| /// rare writer via `store_config`), but PHP is single-threaded per process so |
There was a problem hiding this comment.
PHP is single-threaded per process
Thats only true for NTS builds of PHP, not for ZTS builds of PHP
| } | ||
|
|
||
| lazy_static::lazy_static! { | ||
| static ref FFE_STATE: Mutex<FfeState> = Mutex::new(FfeState { |
There was a problem hiding this comment.
Is FFE config truly global across services/envs etc.? Or is it computed depending on the specific target tuple (service, env, version)?
Note that every thread has its own remote config receiver, with its own target tuple.
There was a problem hiding this comment.
If it's the latter, you will have to carry the state in DDTRACE_G() (i.e. the thread-local state managed by the PHP runtime).
|
I also question the OpenTelemetry metrics a bit. Like, how important are these? The vast majority of our users will not have OpenTelemetry installed or enabled. I would encourage aggregating these in tracers directly. |
5 participants
Motivation
Add FFE (Feature Flags & Experimentation) support to dd-trace-php. PHP applications can evaluate feature flags delivered via Remote Config using the same
datadog-ffeRust engine used by Ruby and Python. Per the RFC "Flag evaluations tracking for APM tracers", we also emit afeature_flag.evaluationsOTel counter metric on each evaluation so flag usage can be tracked in Datadog.Changes
Core FFE Implementation
components-rs/ffe.rs): C-callable bridge todatadog-ffe::rules_based— config store, evaluate, result accessorsext/ddtrace.c):ffe_evaluate,ffe_has_config,ffe_config_changed,ffe_load_configinternal functions that marshal PHP arrays toFfeAttributestructscomponents-rs/remote_config.rs): RegisterFfeFlagsproduct +FfeFlagConfigurationRulescapability; handle add/remove of FFE configssrc/DDTrace/FeatureFlags/Provider.php): Singleton that checks RC config state, calls native evaluate, parses JSON results, reports exposures; enforcesreason=ERRORfor any non-zero error code per OpenFeature spec/evp_proxy/v2/api/v2/exposures(1000 event buffer cap)src/DDTrace/OpenFeature/DataDogProvider.php): ImplementsAbstractProviderfor theopen-feature/sdkcomposer packagedatadog-ffetoRUST_FILESin Makefile for PECL packagingDD_EXPERIMENTAL_FLAGGING_PROVIDER_ENABLEDgating via X-macro inext/configuration.hOTel Flag Evaluation Metrics (new)
src/DDTrace/FeatureFlags/FlagEvalMetrics.php: OTel counter hook that emitsfeature_flag.evaluations(count=1) after each evaluation. Tags:feature_flag.key,feature_flag.provider.name=datadog,feature_flag.result.variant,feature_flag.result.reason,feature_flag.result.allocation_key, and on error:error.type(type_mismatch/parse_error/flag_not_found).errorCodeToTag()now maps Rust constants correctly:1=ERROR_TYPE_MISMATCH→type_mismatch,2=ERROR_CONFIG_PARSE→parse_error,3=ERROR_FLAG_UNRECOGNIZED→flag_not_found(was transposed in initial implementation).Provider.php: Forcesreason=ERRORwhenerror_code != 0. The Rust layer returnsREASON_DEFAULTforFlagUnrecognizedOrDisabledbut the OpenFeature spec requiresERRORto be surfaced to callers.Decisions
Evaluation in Rust, not PHP. All flag evaluation (UFC parsing, targeting rules, shard hashing, allocation resolution) happens in
libdatadog'sdatadog-ffecrate via FFI. PHP only handles orchestration (config lifecycle, exposure dedup, HTTP transport). This matches Ruby and Python — no language re-implements evaluation logic.Global config behind
Mutex<FfeState>. The Rust FFE config is stored in alazy_staticglobal with aMutex. PHP is single-threaded per process, soRwLockwould be unnecessary complexity.Reuses existing RC pipeline. FFE configs flow through the same sidecar →
ddog_process_remote_configs()path as APM Tracing and Live Debugger. No new polling mechanism.Structured attributes, not JSON blobs. The C extension converts PHP arrays into
FfeAttributestructs (typed: string/number/bool) before calling Rust, avoiding JSON encode/decode overhead on the hot path.ExposureWriter caps at 1000 events per request. Matches Ruby and Python. Flush via
register_shutdown_function.FlagEvalMetrics via OTel SDK. Uses
open-telemetry/api+ OTLP exporter. Recorded after the type-specific evaluation method returns (not inside coreevaluate()) so type mismatch errors are captured withreason=errorrather than the intermediatetargeting_match.ERROR reason forced for non-zero error codes.
FlagUnrecognizedOrDisabledreturnsREASON_DEFAULTfrom Rust (the default value was served) but the OpenFeature spec requiresreason=ERRORwhenever an error code is set.Provider.phpoverrides the Rust-reported reason in this case.Test Results
Unit tests
System tests — all FFE tests pass (PHP 8.0-apache)
The 1 xfail is
test_cross_request_dedup— expected because PHP's shared-nothing architecture prevents cross-request LRU state.OTel metrics tests (all 5 pass)
LOC (excluding Cargo.lock + JSON fixtures)
Companion PRs