Skip to content

[PHP] Expose FFE serial IDs in flag metadata with span enrichment #4247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hhan2024 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[PHP] Expose FFE serial IDs in flag metadata with span enrichment #4247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/api/FeatureFlags/Internal/NativeEvaluator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,12 @@ public function evaluate(
$rawResult = $this->withProviderState($rawResult);
}

$details = $this->mapper->map($rawResult, $expectedType, $defaultValue);
$details = $this->mapper->map(

@aarsilv aarsilv Oct 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 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.

$rawResult,
$expectedType,
$defaultValue,
SpanEnrichmentRegistry::gateEnabled()

@aarsilv aarsilv Oct 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 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=4242

It 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. The on positive, on zero, on vip and on standard lines print metadata=[].
  • Argument removed: fails the same way, because map() defaults the gate to false.
  • Argument changed to true: fails. The off positive line prints metadata={"__dd_split_serial_id":4242}.

);

// Native clients submit now. OpenFeature can retain this snapshot until
// its hooks finish, independently of native/OTLP metric recording.
Expand Down
23 changes: 18 additions & 5 deletions src/api/FeatureFlags/Internal/ResultMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

final class ResultMapper
{
const SERIAL_ID_METADATA_KEY = '__dd_split_serial_id';

@aarsilv aarsilv Oct 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 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.

const BRIDGE_REASON_STATIC = 0;
const BRIDGE_REASON_DEFAULT = 1;
const BRIDGE_REASON_TARGETING_MATCH = 2;
Expand All @@ -27,9 +28,10 @@ final class ResultMapper
* @param array<string, mixed>|object|null $rawResult
* @param string $expectedType One of EvaluationType::*.
* @param mixed $defaultValue
* @param bool $spanEnrichmentEnabled
* @return EvaluationDetails
*/
public function map($rawResult, $expectedType, $defaultValue)
public function map($rawResult, $expectedType, $defaultValue, $spanEnrichmentEnabled = false)
{
if (!EvaluationType::isValid($expectedType)) {
throw new \InvalidArgumentException('Unknown feature flag value type: ' . (string) $expectedType);
Expand Down Expand Up @@ -69,7 +71,7 @@ public function map($rawResult, $expectedType, $defaultValue)

$reason = $this->mapReason($this->read($rawResult, array('reason'), self::BRIDGE_REASON_DEFAULT));
if ($this->isDefaultReturn($rawResult, $reason)) {
return $this->defaultDetails($defaultValue, $expectedType, $reason, $rawResult);
return $this->defaultDetails($defaultValue, $expectedType, $reason, $rawResult, $spanEnrichmentEnabled);
}

$decoded = null;
Expand All @@ -93,13 +95,13 @@ public function map($rawResult, $expectedType, $defaultValue)
$this->read($rawResult, array('variant'), null),
null,
null,
$this->readArray($rawResult, array('flag_metadata', 'flagMetadata', 'metadata')),
$this->flagMetadata($rawResult, $spanEnrichmentEnabled),

@aarsilv aarsilv Oct 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 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=[]

$this->exposureData($rawResult),
$this->providerState($rawResult)
);
}

private function defaultDetails($defaultValue, $expectedType, $reason, $rawResult)
private function defaultDetails($defaultValue, $expectedType, $reason, $rawResult, $spanEnrichmentEnabled)
{
return new EvaluationDetails(
$defaultValue,
Expand All @@ -108,7 +110,7 @@ private function defaultDetails($defaultValue, $expectedType, $reason, $rawResul
null,
null,
null,
$this->readArray($rawResult, array('flag_metadata', 'flagMetadata', 'metadata')),
$this->flagMetadata($rawResult, $spanEnrichmentEnabled),

@aarsilv aarsilv Oct 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 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.

array(),
$this->providerState($rawResult)
);
Expand Down Expand Up @@ -290,6 +292,17 @@ private function exposureData($rawResult)
return $exposureData;
}

private function flagMetadata($rawResult, $spanEnrichmentEnabled)

@aarsilv aarsilv Oct 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 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());
        }

{
$metadata = $this->readArray($rawResult, array('flag_metadata', 'flagMetadata', 'metadata'));
$serialId = $this->read($rawResult, array('serial_id', 'serialId'), null);

@aarsilv aarsilv Oct 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 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.

if ($spanEnrichmentEnabled && $serialId !== null) {
$metadata[self::SERIAL_ID_METADATA_KEY] = (int) $serialId;
}

return $metadata;
}

private function providerState($rawResult)
{
$providerState = $this->readArray($rawResult, array('provider_state', 'providerState'));
Expand Down
31 changes: 31 additions & 0 deletions tests/api/Unit/FeatureFlags/ResultMapperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,37 @@

final class ResultMapperTest extends TestCase
{
/**
* @dataProvider serialIdMetadataProvider
*/
public function testSerialIdFlagMetadata($serialFields, $enabled, $expectedMetadata)

@aarsilv aarsilv Oct 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 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.

{
$rawResult = array_merge(array(
'value_json' => '"blue"',
'variant' => 'variant-a',
'reason' => ResultMapper::BRIDGE_REASON_SPLIT,
'error_code' => ResultMapper::BRIDGE_ERROR_NONE,
'flag_metadata' => array('owner' => 'ffe'),
), $serialFields);

foreach (array($rawResult, (object) $rawResult) as $result) {
$details = (new ResultMapper())->map($result, EvaluationType::STRING, 'red', $enabled);
$this->assertSame($expectedMetadata, $details->getFlagMetadata());
}
}

public function serialIdMetadataProvider()
{
return array(
'positive' => array(array('serial_id' => 4242), true, array('owner' => 'ffe', '__dd_split_serial_id' => 4242)),
'zero' => array(array('serial_id' => 0), true, array('owner' => 'ffe', '__dd_split_serial_id' => 0)),
'camel case' => array(array('serialId' => 7), true, array('owner' => 'ffe', '__dd_split_serial_id' => 7)),
'missing' => array(array(), true, array('owner' => 'ffe')),
'null' => array(array('serial_id' => null), true, array('owner' => 'ffe')),
'gate disabled' => array(array('serial_id' => 4242), false, array('owner' => 'ffe')),
);
}

public function testMapsSuccessfulBridgeResultToEvaluationDetails()
{
$details = (new ResultMapper())->map(array(
Expand Down
Loading