Skip to content

feat(flags): send the split serial ID on exposure events (EX-3422) #1409

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
danyal002 wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

feat(flags): send the split serial ID on exposure events (EX-3422) #1409

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
24 changes: 12 additions & 12 deletions packages/core/android/src/main/kotlin/com/datadog/reactnative/DdFlagsImplementation.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,19 +217,19 @@ private fun convertUnparsedFlagToMap(
}

// Return a [Map] as an intermediate because it is easier to use; we can convert it to WritableMap right before sending to React Native.
return mapOf(
"key" to flagKey,
"value" to (parsedValue ?: flag.variationValue),
"allocationKey" to flag.allocationKey,
"variationKey" to flag.variationKey,
"variationType" to flag.variationType,
"variationValue" to flag.variationValue,
"reason" to flag.reason,
"doLog" to flag.doLog,
"extraLogging" to flag.extraLogging.toMap(),
return buildMap {
put("key", flagKey)
put("value", parsedValue ?: flag.variationValue)
put("allocationKey", flag.allocationKey)
put("variationKey", flag.variationKey)
put("variationType", flag.variationType)
put("variationValue", flag.variationValue)
put("reason", flag.reason)
put("doLog", flag.doLog)
put("extraLogging", flag.extraLogging.toMap())
// Serialized as a String because the React Native bridge converts Long values to Double
SERIAL_ID_KEY to flag.serialId?.toString()
)
flag.serialId?.let { put(SERIAL_ID_KEY, it.toString()) }

@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 Kotlin unit tests for the serialId write on this line and for the read-back in convertMapToUnparsedFlag.

No Kotlin test calls convertUnparsedFlagToMap or convertMapToUnparsedFlag: git grep for either name, or for serialId, under packages/core/android/src/test finds nothing. DdFlagsImplementationTest.kt has three tests, and none of them covers flag snapshots or tracking. The JS tests mock NativeModules.DdFlags, so they never run this code. The direct offline path on Android now depends on the string-to-Long parse at lines 247-249.

Regressions that no current test would catch
  • Line 231 changes to put the Long itself instead of its string. toWritableMap turns a Long into a Double: in DdSdkBridgeExt.kt, its is Long branch calls map.putDouble(key, v.toDouble()). An ID above 2^53 then comes back through the as? Number fallback as a different value.
  • The toLongOrNull() branch at line 248 is removed. The JS offline path sends the serial ID as a string (entry.serialId = String(serialId) in precomputed.ts), so offline exposures on Android lose it.
  • The key name changes on one side only.
Suggested tests — a sketch: I could not run gradle here, so please compile it

Make both converters internal, as buildEvaluationContext (line 172) already is:

internal fun convertUnparsedFlagToMap(
internal fun convertMapToUnparsedFlag(map: Map<String, Any>): UnparsedFlag =

Then add to DdFlagsImplementationTest.kt:

import com.datadog.android.flags.model.UnparsedFlag
import com.facebook.react.bridge.JavaOnlyArray
import com.facebook.react.bridge.JavaOnlyMap
import org.json.JSONObject

    private fun unparsedFlag(serialId: Long?): UnparsedFlag = object : UnparsedFlag {
        override val variationType: String = "boolean"
        override val variationValue: String = "true"
        override val doLog: Boolean = true
        override val allocationKey: String = "alloc"
        override val variationKey: String = "on"
        override val extraLogging: JSONObject = JSONObject()
        override val reason: String = "TARGETING_MATCH"
        override val serialId: Long? = serialId
    }

    private fun rawFlag(vararg extra: Pair<String, Any>): Map<String, Any> = mapOf(
        "key" to "flag",
        "value" to true,
        "allocationKey" to "alloc",
        "variationKey" to "on",
        "variationType" to "boolean",
        "variationValue" to "true",
        "reason" to "TARGETING_MATCH",
        "doLog" to true,
        "extraLogging" to emptyMap<String, Any>(),
        *extra
    )

    @Test
    fun `M send serialId as a String W convertUnparsedFlagToMap()`() {
        assertThat(convertUnparsedFlagToMap("flag", unparsedFlag(340132L))["serialId"])
            .isEqualTo("340132")
        assertThat(convertUnparsedFlagToMap("flag", unparsedFlag(0L))["serialId"])
            .isEqualTo("0")
    }

    @Test
    fun `M omit the serialId key W convertUnparsedFlagToMap() { no serial id }`() {
        assertThat(convertUnparsedFlagToMap("flag", unparsedFlag(null)))
            .doesNotContainKey("serialId")
    }

    @Test
    fun `M parse serialId W convertMapToUnparsedFlag()`() {
        assertThat(convertMapToUnparsedFlag(rawFlag("serialId" to "340132")).serialId)
            .isEqualTo(340132L)
        assertThat(convertMapToUnparsedFlag(rawFlag("serialId" to "1.5")).serialId).isNull()
        assertThat(convertMapToUnparsedFlag(rawFlag()).serialId).isNull()
    }

    @Test
    fun `M keep full Long precision W serialId round-trips through the bridge maps`() {
        // 2^53 + 1 has no exact Double, so this fails if the ID ever crosses as a number.
        val serialId = 9_007_199_254_740_993L
        val writable = convertUnparsedFlagToMap("flag", unparsedFlag(serialId))
            .toWritableMap({ JavaOnlyMap() }, { JavaOnlyArray() })

        assertThat(convertMapToUnparsedFlag(writable.toMap()).serialId).isEqualTo(serialId)
    }

The anonymous UnparsedFlag overrides the same eight members that convertMapToUnparsedFlag overrides. JavaOnlyMap / JavaOnlyArray are what DdSdkBridgeExtTest already passes to toWritableMap.

}
}

@Suppress("UNCHECKED_CAST")
Expand Down
2 changes: 1 addition & 1 deletion packages/core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@
}
},
"dependencies": {
"@datadog/flagging-core": "~2.0.1",
"@datadog/flagging-core": "~2.1.0",
"big-integer": "^1.6.52"
}
}
78 changes: 78 additions & 0 deletions packages/core/src/flags/__tests__/FlagsClient.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1080,4 +1080,82 @@ describe('FlagsClient', () => {
).toEqual({ status: 'error', errorCode: 'PROVIDER_NOT_READY' });
});
});

describe('serial id', () => {
const trackedFlag = () =>
(NativeModules.DdFlags.trackEvaluation as jest.Mock).mock
.calls[0][2];

it('hands the serial id to native exposure tracking on the offline path', () => {
const flagsClient = DdFlags.getClient();

flagsClient.setConfiguration(
buildConfig({
'offline-bool': {
...offlineFlags['offline-bool'],
serialId: 340132
}
})
);
flagsClient.getBooleanValue('offline-bool', false);

expect(trackedFlag().serialId).toBe('340132');
});

it('hands serial id 0 to native exposure tracking', () => {
const flagsClient = DdFlags.getClient();

flagsClient.setConfiguration(
buildConfig({
'offline-bool': {
...offlineFlags['offline-bool'],
serialId: 0
}
})
);
flagsClient.getBooleanValue('offline-bool', false);

expect(trackedFlag().serialId).toBe('0');
});

it('sends no serial id key when the configuration carries none', () => {
const flagsClient = DdFlags.getClient();

flagsClient.setConfiguration(buildConfig(offlineFlags));
flagsClient.getBooleanValue('offline-bool', false);

expect(JSON.stringify(trackedFlag())).not.toContain('serialId');
});

it('passes through the serial id from a native snapshot on the online path', async () => {
// The native snapshot is cached verbatim, so a field the bridge adds reaches
// trackEvaluation without the JS layer naming it.
jest.spyOn(
NativeModules.DdFlags,
'setEvaluationContext'
).mockResolvedValueOnce({
'test-boolean-flag': {
key: 'test-boolean-flag',
value: true,
allocationKey: '',
variationKey: 'true',
reason: 'STATIC',
doLog: true,
variationType: '',
variationValue: '',
extraLogging: {},
serialId: '340132'
}
});

const flagsClient = DdFlags.getClient();
await flagsClient.setEvaluationContext({
targetingKey: 'user-1',
attributes: {}
});
flagsClient.getBooleanValue('test-boolean-flag', false);

expect(trackedFlag().serialId).toBe('340132');
});
});
});
73 changes: 69 additions & 4 deletions packages/core/src/flags/configuration/__tests__/precomputed.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,77 @@ describe('decodePrecomputedFlags', () => {
expect(cache.get('f')?.extraLogging).toEqual({});
});

it('tolerates a null serialId', () => {
const cache = decodePrecomputedFlags(
responseWith({ f: flag({ serialId: null }) })
describe('serialId', () => {
it('propagates a serial id as its string form', () => {
const cache = decodePrecomputedFlags(
responseWith({ f: flag({ serialId: 340132 }) })
);

expect(cache.get('f')?.serialId).toBe('340132');
});

it('propagates serial id 0', () => {
// Serial ids are zero-based per org, so 0 is the most common value in the fleet
// and the one a truthiness check would silently drop.
const cache = decodePrecomputedFlags(
responseWith({ f: flag({ serialId: 0 }) })
);

expect(cache.get('f')?.serialId).toBe('0');
});

it.each([
['null', null],
['undefined', undefined],
['a string', '340132'],
['a boolean', true],
['NaN', NaN],
['Infinity', Infinity]
])(
'omits the key entirely and keeps the flag when the serial id is %s',
(_label, serialId) => {
const cache = decodePrecomputedFlags(
responseWith({
f: flag({
serialId: (serialId as unknown) as PrecomputedFlag['serialId']
})
})
);

const entry = cache.get('f');

// The flag itself must still decode: a bad serial id binds to the serial id,
// never to the flag (nor to its siblings).
expect(entry?.key).toBe('f');
expect(entry?.serialId).toBeUndefined();
expect('serialId' in (entry as object)).toBe(false);
Comment thread
danyal002 marked this conversation as resolved.
Resolved
}
);

expect(cache.get('f')?.key).toBe('f');
it('keeps a sibling flag decodable when one has a malformed serial id', () => {
const cache = decodePrecomputedFlags(
responseWith({
good: flag({ serialId: 7 }),
bad: flag({
serialId: ('x' as unknown) as PrecomputedFlag['serialId']
})
})
);

expect(cache.get('good')?.serialId).toBe('7');
expect(cache.get('bad')?.key).toBe('bad');
expect(cache.get('bad')?.serialId).toBeUndefined();
});

it('omits the key from the object sent across the bridge, not just the value', () => {
// The bridge serializes the entry; a present-but-undefined property and an absent
// one are indistinguishable on the object but not on the wire.
Comment on lines +199 to +200

@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] This comment states the reverse of what JSON.stringify does. Please reword it to match the PR description.

The comment says a present-but-undefined property and an absent one "are indistinguishable on the object but not on the wire". The PR description has it the right way round: "a property set to undefined is absent from JSON but present on the object". So the assertion on line 205 checks the encoded form only, and it passes for both shapes. The in check on line 179 is what covers the object.

What Node 24 prints
Expression Result
JSON.stringify({ a: undefined }) {}
'a' in { a: undefined } true
Suggested wording 
            // JSON.stringify drops a key whose value is undefined, so this checks the encoded
            // form only. The `in` check in the it.each above covers the object itself.

FlagsClient.test.ts line 1127 uses the same JSON.stringify(...) pattern. If you want the object-level check there too, expect('serialId' in trackedFlag()).toBe(false) is the form line 179 here already uses.

const cache = decodePrecomputedFlags(
responseWith({ f: flag({ serialId: null }) })
);

expect(JSON.stringify(cache.get('f'))).not.toContain('serialId');
});
});

it('omits flags with an unsupported variation type and logs a warning', () => {
Expand Down
13 changes: 9 additions & 4 deletions packages/core/src/flags/configuration/precomputed.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ const toFlagCacheEntry = (
allocationKey,
reason,
doLog,
extraLogging
extraLogging,
serialId
} = flag as Partial<PrecomputedFlag>;

if (
Expand Down Expand Up @@ -152,9 +153,7 @@ const toFlagCacheEntry = (
return null;
}

// The released decoder's payload omits serialId. The OpenFeature offline adapter adds it from
// the evaluator's metadata, for both precomputed and rules flags.
return {
const entry: FlagCacheEntry = {

@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] This PR removes the note here that the OpenFeature adapter supplies the serial ID. Two related comments now contradict the code. Please update them.

  • Line 100 of packages/react-native-openfeature/src/offlineEvaluation.ts: "The native decoder does not keep the split serial ID; read it from the wire." After this PR, decodePrecomputedFlags keeps it, as a string.
  • Line 33 of this file: "Two transforms are applied per flag". This PR adds a third.

The wire read in the adapter is still needed. flagging-core's PrecomputedFlag.serialId is a number, and this decoder stores a string. Also, the adapter's peer range @datadog/mobile-react-native ^3.1.0 admits core releases whose decoder drops the serial ID.

Suggested wording

offlineEvaluation.ts, line 100:

                            // flagging-core needs the numeric serial ID. The core decoder keeps only its
                            // string form, and core releases without it keep none, so read it from the wire.

precomputed.ts, lines 33-37:

 * The mapping is ~1:1. Three transforms are applied per flag:
 * - `value` is the typed `variationValue` as-is (`integer`/`float` are JS `number`s);
 * - a string `variationValue` is derived (JSON for objects, `"true"/"false"` for
 *   booleans, `String(...)` otherwise) because native Android exposure tracking
 *   rebuilds the flag from the string form;
 * - a numeric `serialId` is carried as its string form, because the bridge turns
 *   integers into doubles. An absent or invalid one omits the key and keeps the flag.
Optional: one adapter test no longer tells the two sources apart

offlineProvider.delegation.test.ts, "sends the precomputed split serial ID to native tracking" (lines 649-670), expects serialId: '42'. Before this PR, only the adapter's { ...precomputed, ...serial } merge (line 224 of offlineEvaluation.ts) could supply it. Now the decoded entry also carries '42', so the test passes if either source is removed. Both sources agree for JSON input, so this is not a defect today. A test where the evaluator metadata carries a different value would pin which one wins.

key,
value: variationValue,
allocationKey,
Expand All @@ -165,6 +164,12 @@ const toFlagCacheEntry = (
doLog,
extraLogging: extraLogging ?? {}
};

if (typeof serialId === 'number' && Number.isFinite(serialId)) {

@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] Consider accepting only a non-negative safe integer here, so a bad value drops the serial ID instead of sending a wrong one.

I saw the resolved Copilot thread on this line. The reply covers configurations that Datadog generates. setConfiguration also accepts a configuration that the app supplies, for example from configurationFromString. This decoder already treats that input as untrusted (lines 57 and 135-136). For the same native-parsing reason, it requires Number.isInteger for integer values (lines 189-191). The impact is low: only a malformed or hand-edited configuration carries these values.

What the current gate sends for each wire value — checked with JSON.parse and String in Node 24
Wire value Current gate sends Kotlin toLongOrNull() at lines 247-249 of DdFlagsImplementation.kt
340132 "340132" 340132
-1 "-1" -1, accepted
9007199254740993 "9007199254740992" (JSON.parse rounds it) 9007199254740992, a different integer that native accepts
1.5 "1.5" null, so the ID is dropped
1e21 "1e+21" null, so the ID is dropped

The last two rows are safe only because the Kotlin parser rejects them. The as? Number fallback does not apply, because the value is a String.

Suggested change — a sketch: I could not run the test suite here 
    if (
        typeof serialId === 'number' &&
        Number.isSafeInteger(serialId) &&
        serialId >= 0
    ) {
        entry.serialId = String(serialId);
    }

Then extend the it.each in precomputed.test.ts:

            ['NaN', NaN],
            ['Infinity', Infinity],
            ['a fraction', 1.5],
            ['a negative number', -1],
            ['an unsafe integer', 2 ** 53]

By reading, no existing test changes outcome. 340132, 0 and 7 still propagate. null, undefined, '340132', true, NaN and Infinity are still omitted. If you take this, the PR description's "It accepts a finite number" becomes "It accepts a non-negative safe integer".

The OpenFeature adapter has the same loose check at line 222 of packages/react-native-openfeature/src/offlineEvaluation.ts (typeof serialId === 'number'). That file is outside this diff. The same predicate there would keep both paths consistent.

Optional: log a rejected value

Every other rejection in toFlagCacheEntry logs a WARN. A present but malformed serial ID is dropped silently, so a lost holdout link leaves no trace. Silence stays right for null, which means "no serial ID".

    } else if (serialId !== null && serialId !== undefined) {
        InternalLog.log(
            `Flag "${key}" has a malformed serial ID. Its exposures will carry none.`,
            SdkVerbosity.WARN
        );
    }

entry.serialId = String(serialId);
}
Comment thread
danyal002 marked this conversation as resolved.
Resolved

return entry;
};

const valueMatchesVariationType = (
Expand Down
18 changes: 5 additions & 13 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1973,20 +1973,12 @@ __metadata:
languageName: node
linkType: hard

"@datadog/flagging-core@npm:~2.0.1":
version: 2.0.1
resolution: "@datadog/flagging-core@npm:2.0.1"
"@datadog/flagging-core@npm:~2.1.0":
version: 2.1.0
resolution: "@datadog/flagging-core@npm:2.1.0"
dependencies:
"@datadog/js-core": 0.0.3
spark-md5: ^3.0.2
checksum: d4864a76be535acda9bbcdaa95718484f1b16842fba8591ba13308b988e043f7aafd002e240b2393349af616091b5cdb9a75325e24e82aeede8b34dc2bc7560b
languageName: node
linkType: hard

"@datadog/js-core@npm:0.0.3":
version: 0.0.3
resolution: "@datadog/js-core@npm:0.0.3"
checksum: c8d70cb21f0c5089fa9d3e8d54a9bc2fafde8927b9f9286721b5a12c82cf46b5ff2d39486ecd1c871e4d8a321d88ed5ad5d1cccc06659bce7b9c67c4eaac6695
checksum: 0bb9488ff29a059135804a688c1e929a3eea35a585510ce63480a84ad8d9f7e07eae6b84e18243b979fef79094cda71c94cec6492951a79f7a46aa03ec0e8739
languageName: node
linkType: hard

Expand Down Expand Up @@ -2132,7 +2124,7 @@ __metadata:
version: 0.0.0-use.local
resolution: "@datadog/mobile-react-native@workspace:packages/core"
dependencies:
"@datadog/flagging-core": ~2.0.1
"@datadog/flagging-core": ~2.1.0
"@testing-library/react-native": 7.0.2
big-integer: ^1.6.52
react-native-builder-bob: 0.26.0
Expand Down
Loading