Repository navigation
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 0423cac | Docs | View more details | Give us feedback! |
Benchmarks [ tracer ]Benchmark execution time: 2026-10-26 16:12:10 Comparing candidate commit 0423cac in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 192 metrics, 2 unstable metrics.
|
Preserve zero and omit absent serial IDs while retaining existing flag metadata. Environment: Datadog workspace Co-Authored-By: Codex GPT-5 <noreply@localhost>
e3226c4 to
0423cac
Compare
hhan2024
changed the title
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. |
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The serial-ID mapping preserves zero, omits missing/null IDs, and retains existing metadata and exposure data behind the enrichment gate. The completed static review found no reportable defect.
🤖 Bits Code Review · Commit 0423cac
| $rawResult, | ||
| $expectedType, | ||
| $defaultValue, | ||
| SpanEnrichmentRegistry::gateEnabled() |
There was a problem hiding this comment.
🤖 Comment from Claude working fully autonomously overnight:
[P2] Please add an end-to-end test for this gate argument. At the moment, no in-repo test checks it.
ResultMapperTest passes the gate to ResultMapper::map() directly. ClientTest uses a fake evaluator. The tests/OpenFeature/span_enrichment_*.phpt tests build EvaluationDetails by hand. All six .phpt tests in tests/api/Unit/FeatureFlags/ reach NativeEvaluator, directly or through a child process. But they run with the gate off and do not check flag metadata. The system test that checks this behavior (test_serial_id_metadata.py) is still missing_feature for PHP in the system-tests manifest.
I changed this argument to false, and the three FeatureFlags PHPUnit test classes stayed green (39 tests, 111 assertions). The test below passes on this head. It fails when the argument is false, true, or removed.
Suggested test: tests/api/Unit/FeatureFlags/flag_metadata_serial_id.phpt
--TEST--
FFE flag metadata carries the selected split serial ID only when span enrichment is enabled
--ENV--
DD_METRICS_OTEL_ENABLED=false
DD_TRACE_GENERATE_ROOT_SPAN=0
DD_EXPERIMENTAL_FLAGGING_PROVIDER_SPAN_ENRICHMENT_ENABLED=true
--FILE--
<?php
require __DIR__ . '/../../../Common/FeatureFlags/ffe_api_bootstrap.inc';
function ffe_serial_split($serialId)
{
$split = array('variationKey' => 'blue', 'shards' => array());
if ($serialId !== null) {
$split['serialId'] = $serialId;
}
return $split;
}
function ffe_serial_flag($key, array $allocations)
{
return array(
'key' => $key, 'enabled' => true, 'variationType' => 'STRING',
'variations' => array('blue' => array('key' => 'blue', 'value' => 'blue')),
'allocations' => $allocations,
);
}
function ffe_serial_allocation($key, $serialId, array $rules = array())
{
return array('key' => $key, 'rules' => $rules, 'doLog' => false, 'splits' => array(ffe_serial_split($serialId)));
}
function show($label, $details)
{
echo $label, ' metadata=', json_encode($details->getFlagMetadata()), "\n";
}
$vipRule = array('conditions' => array(array('attribute' => 'plan', 'operator' => 'ONE_OF', 'value' => array('vip'))));
$config = array(
'createdAt' => '2026-05-22T00:00:00Z',
'environment' => array('name' => 'test'),
'flags' => array(
'positive' => ffe_serial_flag('positive', array(ffe_serial_allocation('a-positive', 4242))),
'zero' => ffe_serial_flag('zero', array(ffe_serial_allocation('a-zero', 0))),
'missing' => ffe_serial_flag('missing', array(ffe_serial_allocation('a-missing', null))),
'targeted' => ffe_serial_flag('targeted', array(
ffe_serial_allocation('a-vip', 201, array($vipRule)),
ffe_serial_allocation('a-standard', 200),
)),
),
);
var_dump(\DDTrace\Testing\ffe_load_config(json_encode($config)));
$client = new \DDTrace\FeatureFlags\Client();
$context = array('targetingKey' => 'subject');
show('on positive', $client->getStringDetails('positive', 'fallback', $context));
show('on zero', $client->getStringDetails('zero', 'fallback', $context));
show('on missing', $client->getStringDetails('missing', 'fallback', $context));
show('on unknown', $client->getStringDetails('unknown', 'fallback', $context));
show('on vip', $client->getStringDetails('targeted', 'fallback', array('targetingKey' => 'subject', 'attributes' => array('plan' => 'vip'))));
show('on standard', $client->getStringDetails('targeted', 'fallback', array('targetingKey' => 'subject', 'attributes' => array('plan' => 'standard'))));
// Gate off: the serial ID stays in exposure data but is not added to flag metadata.
var_dump(ini_set('datadog.experimental_flagging_provider_span_enrichment_enabled', '0') !== false);
$off = $client->getStringDetails('positive', 'fallback', $context);
show('off positive', $off);
$exposure = $off->getExposureData();
echo 'off exposure serialId=', json_encode($exposure['serialId']), "\n";
?>
--EXPECT--
bool(true)
on positive metadata={"__dd_split_serial_id":4242}
on zero metadata={"__dd_split_serial_id":0}
on missing metadata=[]
on unknown metadata=[]
on vip metadata={"__dd_split_serial_id":201}
on standard metadata={"__dd_split_serial_id":200}
bool(true)
off positive metadata=[]
off exposure serialId=4242It follows flag_evaluation_final_outcome.phpt, so make test_api_unit runs it. It uses array() syntax, so it also parses on PHP 7. The targeting rows check that the ID comes from the split that targeting selected.
How I ran it
- Runtime: PHP 8.2 CLI with the ddtrace 1.26.2 release extension. This PR's PHP sources were loaded with
datadog.trace.sources_path. I did not build the extension from this head. - Runner: PHPUnit 9.6 with
--test-suffix=.phpt. - This head:
OK (1 test, 1 assertion). - Argument changed to
false: fails. Theon positive,on zero,on vipandon standardlines printmetadata=[]. - Argument removed: fails the same way, because
map()defaults the gate tofalse. - Argument changed to
true: fails. Theoff positiveline printsmetadata={"__dd_split_serial_id":4242}.
| /** | ||
| * @dataProvider serialIdMetadataProvider | ||
| */ | ||
| public function testSerialIdFlagMetadata($serialFields, $enabled, $expectedMetadata) |
There was a problem hiding this comment.
🤖 Comment from Claude working fully autonomously overnight:
[P2] Please re-run the 5 serial-ID system tests at this head, and add the receipt to this PR's Validation section.
The system-test result in this PR is for an earlier commit. The companion system-tests PR, DataDog/system-tests#7857, names SDK commit e3226c4f on master d98de1ed. This head is 0423cacd on master 52afc1d8. The changed lines are the same. But NativeEvaluator.php around this change is different: master added recordFlagEvaluation() and the targetingKeyInvalid argument after d98de1ed. So the earlier run is not evidence for this exact head.
The run also used the 1.25.1 release extension with patched PHP sources. That is a valid approach, but the receipt should say so, with the commit and the system-tests revision. I do not expect a different result. I ran a direct probe at this head (1.26.2 release extension plus this PR's sources), and it shows the expected metadata for a positive ID, zero, a missing ID, and the gate off.
Suggested receipt for the Validation section
Local system-test validation
- SDK under test: 0423cacde808246e96a960cc7dc0a98e3ba12f92 (clean tree)
- Loaded artifact: <release extension version> + PHP sources from the SDK commit above
(DD_TRACE_SOURCES_PATH, DD_AUTOLOAD_NO_COMPILE=true)
- System-tests revision: <full SHA of #7857 head>; PARAMETRIC; PHP <version>
- Command: TEST_LIBRARY=php venv/bin/pytest -S PARAMETRIC --skip-parametric-build -n=2 \
-p no:warnings tests/parametric/test_ffe/test_serial_id_metadata.py \
-F tests/parametric/test_ffe/test_serial_id_metadata.py
- Tests: Test_FFE_Serial_Id_Metadata (5 tests)
- Result: <passed / failed / skipped counts>
The command is the one from DataDog/system-tests#7857. Fill in the angle-bracket fields from the new run.
| null, | ||
| null, | ||
| $this->readArray($rawResult, array('flag_metadata', 'flagMetadata', 'metadata')), | ||
| $this->flagMetadata($rawResult, $spanEnrichmentEnabled), |
There was a problem hiding this comment.
🤖 Comment from Claude working fully autonomously overnight:
[P3] On this default-return path, flag metadata can report a split serial ID while the same evaluation returns the caller's default. I suggest keeping the base behavior here.
Native can select a split whose JSON value is null, with reason DEFAULT. isDefaultReturn() then sends the result to defaultDetails(). The caller gets the default value, variant is null, and exposure data is empty. So SpanEnrichmentRegistry::record() records a runtime default, and the flag-evaluation record sets runtimeDefaultUsed. But with the gate on, getFlagMetadata() on the same result now returns ['__dd_split_serial_id' => 9]. The two signals disagree for one evaluation.
The case is narrow: a JSON flag with a null variation value, served by an allocation that has startAt/endAt and one unsharded split. It is reachable with a release extension. If you want the serial ID here on purpose, please add a test that pins it instead.
Reproduction with the release extension
UFC flag (other fields as in the existing FFE tests):
'jsonnull' => array(
'key' => 'jsonnull', 'enabled' => true, 'variationType' => 'JSON',
'variations' => array('nullv' => array('key' => 'nullv', 'value' => null)),
'allocations' => array(array(
'key' => 'a-tb', 'rules' => array(), 'doLog' => true,
'startAt' => '2020-01-01T00:00:00Z', 'endAt' => '2099-01-01T00:00:00Z',
'splits' => array(array('variationKey' => 'nullv', 'shards' => array(), 'serialId' => 9)),
)),
),With ddtrace 1.26.2 and this PR's sources, gate on:
ffe_evaluate(): valueJson="null", variant="nullv", reason=1 (DEFAULT), serialId=9
getObjectDetails('jsonnull', ['fb' => true]):
value={"fb":true} reason=DEFAULT variant=null
exposure=[] meta={"__dd_split_serial_id":9}
With the patch below, meta is []. A flag with a non-null JSON value on the same kind of allocation still gets {"__dd_split_serial_id":11}.
Suggested patch and test
Pass only four arguments at line 74:
return $this->defaultDetails($defaultValue, $expectedType, $reason, $rawResult);Restore defaultDetails() to its base version:
private function defaultDetails($defaultValue, $expectedType, $reason, $rawResult)
{
return new EvaluationDetails(
$defaultValue,
$expectedType,
$reason,
null,
null,
null,
$this->readArray($rawResult, array('flag_metadata', 'flagMetadata', 'metadata')),
array(),
$this->providerState($rawResult)
);
}Add to ResultMapperTest:
public function testDefaultReturnOmitsSerialIdMetadata()
{
// Native can select a split whose JSON value is null. The mapper then
// returns the caller's default, so no split serial ID is reported.
$details = (new ResultMapper())->map(array(
'value_json' => 'null',
'variant' => 'null-variation',
'reason' => ResultMapper::BRIDGE_REASON_DEFAULT,
'error_code' => ResultMapper::BRIDGE_ERROR_NONE,
'serial_id' => 9,
), EvaluationType::OBJECT, array('fallback' => true), true);
$this->assertSame(array('fallback' => true), $details->getValue());
$this->assertNull($details->getVariant());
$this->assertSame(array(), $details->getExposureData());
$this->assertSame(array(), $details->getFlagMetadata());
}This test fails on this head and passes with the patch. With the patch, ResultMapperTest passes (27 tests, 77 assertions), and so do ClientTest and EvaluationMetricRecorderTest.
|
|
||
| final class ResultMapper | ||
| { | ||
| const SERIAL_ID_METADATA_KEY = '__dd_split_serial_id'; |
There was a problem hiding this comment.
🤖 Comment from Claude working fully autonomously overnight:
[P3] Optional nit: give this constant a name that is not already used. SpanEnrichmentRegistry::SERIAL_ID_METADATA_KEY exists with the value 'serialId', and it names an exposure-data key.
Two constants with one name and two values in the same feature are easy to confuse when you read the code. For example, exposureData() in this file writes the literal 'serialId', a few lines above flagMetadata(). The existing exposure-data tests would catch a mix-up there, so this is only about readability. A name such as SPLIT_SERIAL_ID_FLAG_METADATA_KEY removes the ambiguity. It is a two-line change (here and at line 300).
Suggested change and check
const SPLIT_SERIAL_ID_FLAG_METADATA_KEY = '__dd_split_serial_id'; $metadata[self::SPLIT_SERIAL_ID_FLAG_METADATA_KEY] = (int) $serialId;No other file uses ResultMapper::SERIAL_ID_METADATA_KEY. I applied only these two lines on this head. ResultMapperTest (26 tests), ClientTest (10 tests) and EvaluationMetricRecorderTest (3 tests) pass.
I also checked the mix-up case. When exposureData() uses self::SERIAL_ID_METADATA_KEY in place of 'serialId', testThreadsSerialIdIntoExposureDataWhenPresent and testThreadsCamelCaseSerialIdFromObjectResult fail.
| private function flagMetadata($rawResult, $spanEnrichmentEnabled) | ||
| { | ||
| $metadata = $this->readArray($rawResult, array('flag_metadata', 'flagMetadata', 'metadata')); | ||
| $serialId = $this->read($rawResult, array('serial_id', 'serialId'), null); |
There was a problem hiding this comment.
🤖 Comment from Claude working fully autonomously overnight:
[P3] Optional nit: check the gate before you read the serial ID. Today flagMetadata() reads serial_id/serialId on every successful evaluation, also when the gate is off, which is the default. Then it discards the value. The cost is very small, but a gate-first early return also makes the off path read the same as the base code.
Suggested change
private function flagMetadata($rawResult, $spanEnrichmentEnabled)
{
$metadata = $this->readArray($rawResult, array('flag_metadata', 'flagMetadata', 'metadata'));
if (!$spanEnrichmentEnabled) {
return $metadata;
}
$serialId = $this->read($rawResult, array('serial_id', 'serialId'), null);
if ($serialId !== null) {
$metadata[self::SERIAL_ID_METADATA_KEY] = (int) $serialId;
}
return $metadata;
}The behavior does not change: zero is kept, and null or missing IDs are left out. I applied only this method on this head. The three FeatureFlags PHPUnit test classes pass, including the new serialIdMetadataProvider rows. I did not measure the time saved.
| } | ||
|
|
||
| $details = $this->mapper->map($rawResult, $expectedType, $defaultValue); | ||
| $details = $this->mapper->map( |
There was a problem hiding this comment.
🤖 Comment from Claude working fully autonomously overnight:
Reminder: When the system-tests PHP manifest enables test_serial_id_metadata.py (today the entry is missing_feature), use the first dd-trace-php release that contains this commit. The latest release (1.26.2) and current master do not add __dd_split_serial_id.
The adapter change in DataDog/system-tests#7857 ((object) $details->getFlagMetadata()) has to merge before that. On system-tests main, the PHP adapter sends $details->getFlagMetadata() without the cast. An empty PHP array encodes as [], and the test requires a map or null, so test_serial_id_absent_when_split_has_no_serial_id would fail. The cast does not depend on this PR, because getFlagMetadata() returns an array before and after it. So #7857 and this PR can merge in either order. No code change is needed here.
| return $exposureData; | ||
| } | ||
|
|
||
| private function flagMetadata($rawResult, $spanEnrichmentEnabled) |
There was a problem hiding this comment.
🤖 Comment from Claude working fully autonomously overnight:
Heads up: OpenFeature users do not get this metadata key. The PR description says "Both the native client and OpenFeature provider use this evaluator", which is true of the evaluator. But DataDogProvider::resolveWithObservation() builds ResolutionDetails from the value, reason, variant and error only, and the provider comment says "PHP OpenFeature has no flagMetadata surface here". So only \DDTrace\FeatureFlags\Client::get*Details() callers see __dd_split_serial_id. OpenFeature users get the span tags only.
No action is needed for the system-test contract, because the PHP test adapter uses the native Client. You may want to say this in the PR description. The latest open-feature/sdk release (2.3.0) has no withMetadata() on ResolutionDetailsBuilder. Its main branch adds one, so a later follow-up could forward the metadata after a release includes it.
Possible follow-up, after open-feature/sdk releases withMetadata() (sketch, not run)
if (method_exists($builder, 'withMetadata') && $details->getFlagMetadata() !== array()) {
$builder->withMetadata($details->getFlagMetadata());
}
| null, | ||
| null, | ||
| $this->readArray($rawResult, array('flag_metadata', 'flagMetadata', 'metadata')), | ||
| $this->flagMetadata($rawResult, $spanEnrichmentEnabled), |
There was a problem hiding this comment.
🤖 Comment from Claude working fully autonomously overnight:
Heads up: When native selects a split but PHP then rejects the value, the serial ID is not added to flag metadata. decodeValue() returns an error, and errorDetails() sets flag metadata to array(). Exposure data is empty on that path too, so the result is consistent. This path is not new in this PR, and it is narrow.
The system-test contract covers evaluations that return a split's value, so I think the current behavior is fine. I mention it in case other SDKs keep the ID on these errors. No action is needed now.
What I observed
A JSON flag with a null variation value, served by a plain allocation (serialId: 10), evaluated with getObjectDetails(). ddtrace 1.26.2 release extension, this PR's sources, gate on:
ffe_evaluate(): valueJson="null", variant="nullv", reason=0 (STATIC), errorCode=0, serialId=10
getObjectDetails(): reason=ERROR err=TYPE_MISMATCH variant=null meta=[] exposure=[]
FFE reviewer selectionReviewers: @sameerank, @typotter Why: openfeature architecture — The change extends the shared PHP evaluation metadata contract used by both the native client and the OpenFeature provider. Reviewers come from the matched pod. 🤖 Automated selection. Selection detailsInitiative: fallback / openfeature-architecture Why this match: The change extends the shared PHP evaluation metadata contract used by both the native client and the OpenFeature provider. ResultMapper now adds the selected split's serial ID to flag metadata under __dd_split_serial_id, but only when span enrichment is enabled. It keeps zero values, leaves out missing or null IDs, and keeps existing metadata. NativeEvaluator passes the existing span-enrichment gate to the mapper. No registered project fits: nothing in the diff ties this to EVP telemetry, which flag-evaluation-evp excludes for generic metadata changes, and nothing ties it to bandits, holdouts or other project behavior. This matches the fallback's evaluation metadata contracts and serial IDs scope.
The PR author was excluded. Existing reviewers and teams were preserved. Debug or fix a problemWorkflow run · Bits Code session
Evidence and run details✅ Complete — reviewer coverage verified. The workflow checked pending requests or submitted reviews on the evaluated head. Generated asynchronously by the FFE reviewer-routing workflow. Changed paths: Evaluated head: Registry revision: Pinned skill source: Instructions: workflow-injected snapshot (no plugin loaded) Instruction snapshot SHA-256: |
Description
Include the selected UFC split's serial ID in evaluation flag metadata under
__dd_split_serial_idwhen span enrichment is enabled. Preserve zero, omitmissing/null serial IDs, and retain existing metadata and exposure data.
The native evaluator passes the existing span-enrichment gate to ResultMapper.
No native extension changes are needed. Both the native client and OpenFeature
provider use this evaluator.
Companion system-tests draft: DataDog/system-tests#7857
(stacked on #7820 in that repository). Its activation remains pending the SDK
release version; this PR does not predict a release number.
Validation
with the fix. Coverage also checks missing/null IDs, disabled enrichment,
array/object bridge results, and preservation of existing metadata.
composer ci-lint: passed.with this patched PHP source via
DD_TRACE_SOURCES_PATHandDD_AUTOLOAD_NO_COMPILE=true, plus the companion adapter's empty-map JSON fix.Reviewer checklist
FFE reviewer selection
Reviewers: @sameerank, @greghuels
Why: Global fallback — At the evaluated head, ResultMapper::flagMetadata adds the selected split's serial_id/serialId to flag metadata as __dd_split_serial_id, but only when the span-enrichment gate… Global reviewers were selected because no project or SME pod matched.
🤖 Automated selection.
Selection details
Initiative: global / global
Why this match: At the evaluated head, ResultMapper::flagMetadata adds the selected split's serial_id/serialId to flag metadata as __dd_split_serial_id, but only when the span-enrichment gate is on. NativeEvaluator passes SpanEnrichmentRegistry::gateEnabled() through to the mapper. The change is APM span-enrichment metadata for evaluations. It does not touch EVP evaluation telemetry or consent (flag-evaluation-evp), onboarding diagnostics (guided-onboarding), migration-blocking capabilities (sdk-migration-parity), or any other registered project. No registered project clearly owns span enrichment and the catalog has no fallback rules, so this routes to global.
The PR author was excluded. Existing reviewers and teams were preserved.
Debug or fix a problem
Workflow run · Bits Code session
dry_run=false. Unchanged completed runs are skipped. If the receipt saysneeds_reconciliationor a write outcome is uncertain, ask the workflow owner to reconcile GitHub and the receipt before retrying; do not delete the lock blindly.Evidence and run details
✅ Complete — reviewer coverage verified. The workflow checked pending requests or submitted reviews on the evaluated head.
Generated asynchronously by the FFE reviewer-routing workflow.
Changed paths:
src/api/FeatureFlags/Internal/ResultMapper.php,src/api/FeatureFlags/Internal/NativeEvaluator.php,tests/api/Unit/FeatureFlags/ResultMapperTest.phpEvaluated head:
0423cacde808246e96a960cc7dc0a98e3ba12f92Registry revision:
revision:1946aa2a-530e-487a-84a7-83aff44a1089Pinned skill source:
d214cfc65ae154b97d39af5cf32caf48aaec9bf0Instructions: workflow-injected snapshot (no plugin loaded)
Instruction snapshot SHA-256:
f23b2a88c13c0da90e2dd58246a14f257af91e2195c555039bd555b52e6f3516