feat(openfeature): add explicit RUM context enrichment - #1363
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enriches the online Datadog OpenFeature provider’s evaluation context with the current RUM user (id/name/email + flat primitive extraInfo), aligning React Native behavior with the browser SDK and ensuring the same effective context is used for both assignment fetching and evaluation tracking.
Changes:
- Add a core helper to enrich OpenFeature-shaped contexts with the current RUM user (and expose it for the OpenFeature package while keeping compatibility with older core versions).
- Update the online OpenFeature provider to apply RUM enrichment during
initializeandonContextChange(context reconciliation). - Add unit + integration coverage and document RUM-user context behavior and reconciliation guidance.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/react-native-openfeature/src/provider.ts | Applies optional core-provided RUM context enrichment before mapping to Datadog evaluation context. |
| packages/react-native-openfeature/src/tests/provider.test.ts | Adds unit tests verifying enrichment is applied on initialize and on context change. |
| packages/react-native-openfeature/src/tests/provider.integration.test.ts | Adds integration tests validating enriched context is used for fetch + tracking and respects rumIntegrationEnabled: false. |
| packages/react-native-openfeature/src/tests/provider.compatibility.test.ts | Ensures behavior is preserved when running against older core versions without the enrichment helper. |
| packages/react-native-openfeature/README.md | Documents how RUM user defaults affect OpenFeature context and how to reconcile after user changes. |
| packages/core/src/index.tsx | Exposes the enrichment helper via an internal __ddEnrichEvaluationContextWithRumUser export. |
| packages/core/src/flags/types.ts | Updates rumIntegrationEnabled documentation to include OpenFeature context enrichment behavior. |
| packages/core/src/flags/rumIntegration.ts | Implements RUM-user-to-context enrichment logic (id → targetingKey; name/email/flat primitive extraInfo → attributes). |
| packages/core/src/flags/DdFlags.ts | Wires rumIntegrationEnabled configuration into the enrichment helper’s runtime behavior. |
| packages/core/src/flags/tests/rumIntegration.test.ts | Adds unit tests for enrichment semantics and opt-out behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b46ce00 to
b3e773f
Compare
vjfridge
left a comment
There was a problem hiding this comment.
LTGM! Love more evaluation context fields for rich data 😍
🎉 All green!🧪 All tests passed 🔗 Commit SHA: f38accb | Docs | View more details | Give us feedback! |
sbarrio
left a comment
There was a problem hiding this comment.
Just left a question, but looks ok to me 👍
aarsilv
left a comment
There was a problem hiding this comment.
Thanks for authoring the ability to enrich context with RUM data, this is something many customers have asked us about! 📈
I know I'm late to the party and this has already merged, but given some unresolved discussion I figured I'd try to help by pointing my "aaron-in-the-loop" agent review workflow at this. No huge blockers, but it found some edge cases, cleanup, and README changes that I'd love for somebody on Feature Flags to tackle with a follow up PR. Filed as https://datadoghq.atlassian.net/browse/FFL-3312.
| const entries: Array<[string, unknown]> = []; | ||
|
|
||
| for (const [key, value] of Object.entries(user.extraInfo ?? {})) { | ||
| if (isSupportedAttribute(value)) { |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] Consider skipping targetingKey, name and email while promoting extraInfo, so a custom user attribute cannot occupy a reserved key.
extraInfo is pushed first and the RUM user's own fields overwrite it, which is the right precedence — but targetingKey is only pushed when typeof user.id === 'string'. addUserExtraInfo can leave a user with extraInfo and no id, and DdSdkReactNative.tsx:268 documents that path, so in that state an extraInfo entry is the only source for the key.
What I measured
Every result quoted below is one I ran locally against this head, not something you can read off the diff.
addUserExtraInfo({ targetingKey:'FROM-EXTRA' })
enrichRumContext({}) => {"targetingKey":"FROM-EXTRA"}
setUserInfo({ id:'rum-123', extraInfo:{ name:'FROM-EXTRA', email:'e@x.com' } })
enrichRumContext({}) => {"name":"FROM-EXTRA","email":"e@x.com","targetingKey":"rum-123"}
addUserExtraInfo({ targetingKey: 42 })
toDdContext(...).targetingKey === 42 // typeof "number"
The last line stops at toDdContext; I did not exercise the bridge. It is worth flagging because specs/NativeDdFlags.ts:21 declares targetingKey: string and DdFlagsImplementation.kt:67 declares a non-null Kotlin String, and processEvaluationContext passes targetingKey through untouched — it validates only attributes.
There is a precedent for the type check specifically: configuration/context.ts:24-25 treats a non-string targeting key as absent because "the wire is untrusted". Reserving the key's source is a separate policy question.
Suggested change — a reserved-key list checked while promoting
import { InternalLog } from '../InternalLog';
import { SdkVerbosity } from '../config/types/SdkVerbosity';
import { UserInfoSingleton } from '../sdk/UserInfoSingleton/UserInfoSingleton';
import type { UserInfo } from '../sdk/UserInfoSingleton/types';and, at module scope:
// Keys the RUM user's own fields own. `targetingKey` must stay a string and must come from
// `user.id` alone, so an `extraInfo` entry never reaches any of them.
const RESERVED_RUM_KEYS = ['targetingKey', 'name', 'email'];then, inside the extraInfo loop in getRumContextEntries:
if (RESERVED_RUM_KEYS.includes(key)) {
InternalLog.log(
`RUM user property "${key}" is reserved and was not added to the evaluation context. Only the RUM user's own id, name, and email supply targetingKey, name, and email.`,
SdkVerbosity.WARN
);
continue;
}The wording deliberately does not promise a replacement value, because in the motivating case the corresponding user field is absent.
There was a problem hiding this comment.
Same issue as #1363 (comment). Had a long AI conversation about tradeoffs and I think consistent merge behavior and removing any surprises wins.
Here's the LLM comment on this after some pushback:
I think we should prefer consistent merge precedence: extraInfo → RUM identity fields → explicit application context over mixing responsibilities of merge and validation. Special-casing these keys would make enrichment less predictable. An invalid final targetingKey should be handled the same way regardless of whether it came from RUM or application context, rather than changing the merge rules.
| }; | ||
|
|
||
| const getRumContextEntries = (): Array<[string, unknown]> => { | ||
| try { |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P1] Read extraInfo and the RUM user's own fields in separate try blocks, and log on the degraded paths — today one unreadable custom property discards the whole RUM user, targeting key included, with no signal.
Object.entries(user.extraInfo ?? {}) invokes application getters, and it runs before name, email and targetingKey are read, so one throwing property returns [] and the subject is lost. Both catch blocks here are silent. By rg over packages/core/src/flags/** excluding __tests__, they are the only fully silent catches against 12 InternalLog.log sites — internal.ts:47 warns merely for dropping one non-primitive attribute.
The existing test cannot distinguish the two outcomes
Every result quoted below is one I ran locally against this head, not something you can read off the diff.
rumIntegration.test.ts:105-119 supplies targetingKey: 'explicit-user' in the application context, which overwrites the RUM id either way, so it passes whether or not the id survives. Against a single-try structure, the assertion that does distinguish them is the only failure I saw:
enrichEvaluationContextWithRumUser › keeps the RUM user fields when an extraInfo property cannot be read
The outer catch hands back an object that throws at the next hop
In the one case I could construct for it — a throwing getter on the application context — it returns the input by reference, still carrying undefined-valued keys and the getter, and toDdContext's rest-spread re-reads it:
same object as input? true
own keys = [ 'region', 'email', 'boom' ]
toDdContext threw: app getter threw
So in that case it defers the throw rather than preventing it. Worth a log line either way, so the degrade is visible.
One thing to watch if you take this
Splitting the try blocks the obvious way moves the name/email/id reads outside any try, and getRumContextEntries() is called before the outer try, so a throwing user.name getter would then escape enrichEvaluationContextWithRumUser entirely. I hit that while testing the patch below; the version quoted keeps both reads guarded. Measured before and after:
reads outside a try: threw = "name getter threw" result = undefined
reads inside a try: threw = no result = {"region":"us"}
Suggested change — complete, including imports
import { InternalLog } from '../InternalLog';
import { SdkVerbosity } from '../config/types/SdkVerbosity';
import { UserInfoSingleton } from '../sdk/UserInfoSingleton/UserInfoSingleton';
import type { UserInfo } from '../sdk/UserInfoSingleton/types';and, at module scope:
// Keys the RUM user's own fields own. `targetingKey` must stay a string and must come from
// `user.id` alone, so an `extraInfo` entry never reaches any of them.
const RESERVED_RUM_KEYS = ['targetingKey', 'name', 'email'];const getRumContextEntries = (): Array<[string, unknown]> => {
const user = readRumUser();
if (!user) {
return [];
}
const entries: Array<[string, unknown]> = [];
// `extraInfo` and the RUM user's own fields are read in separate try blocks, so an unreadable
// custom property cannot discard the targeting key with it. Within each group it is all or
// nothing: `Object.entries` materializes every value before the loop runs, and the three
// reserved fields are destructured together. Every read here can invoke an application getter.
try {
for (const [key, value] of Object.entries(user.extraInfo ?? {})) {
if (RESERVED_RUM_KEYS.includes(key)) {
InternalLog.log(
`RUM user property "${key}" is reserved and was not added to the evaluation context. Only the RUM user's own id, name, and email supply targetingKey, name, and email.`,
SdkVerbosity.WARN
);
continue;
}
if (!isSupportedAttribute(value)) {
InternalLog.log(
`RUM user property "${key}" is not a string, number, or boolean. Omitting it from the evaluation context.`,
SdkVerbosity.WARN
);
continue;
}
entries.push([key, value]);
}
} catch (error) {
InternalLog.log(
`Some RUM user properties could not be read (${errorMessage(
error
)}) and were not added to the evaluation context.`,
SdkVerbosity.WARN
);
}
try {
// Read each field once and insert the value that was validated. Reading the property
// again after the `typeof` check would let a getter return a string and then something
// else, putting a non-string into a reserved key.
const { name, email, id } = user;
if (typeof name === 'string') {
entries.push(['name', name]);
}
if (typeof email === 'string') {
entries.push(['email', email]);
}
if (typeof id === 'string') {
entries.push(['targetingKey', id]);
}
} catch (error) {
InternalLog.log(
`Some RUM user fields could not be read (${errorMessage(
error
)}) and were not added to the evaluation context.`,
SdkVerbosity.WARN
);
}
return entries;
};
const readRumUser = (): UserInfo | undefined => {
let user: UserInfo | undefined;
try {
user = UserInfoSingleton.getInstance().getUserInfo();
} catch (error) {
InternalLog.log(
`Could not read the RUM user (${errorMessage(
error
)}). No RUM values were added to the evaluation context.`,
SdkVerbosity.WARN
);
return undefined;
}
// Reported here, not by the caller, so a read failure does not also report "no user".
if (!user) {
InternalLog.log(
'No RUM user is set, so no RUM values were added to the evaluation context. Call DdSdkReactNative.setUserInfo() and await it before enriching.',
SdkVerbosity.WARN
);
}
return user;
};// Reading `error.message` can itself throw, and this runs inside a catch that must not throw.
const errorMessage = (error: unknown): string => {
try {
// `String(...)` inside the guard: reading `message` can throw, and so can coercing it.
return error instanceof Error ? String(error.message) : 'unknown error';
} catch {
return 'unknown error';
}
};and the log line on the outer catch:
} catch (error) {
InternalLog.log(
`Could not read the application evaluation context (${errorMessage(
error
)}). Returning it unchanged, without the RUM user.`,
SdkVerbosity.WARN
);
return context;
}Three details worth keeping if you take this. The "no RUM user" warning lives in readRumUser, not the caller, so a read failure is reported once rather than also as an absent user. The isolation is between the two groups, not inside them: Object.entries materializes every extraInfo value before the loop runs, so one throwing getter still costs the whole attribute bag — I measured a readable plan: 'pro' disappearing alongside it. What the split buys is that the targeting key, name and email survive it. Both warnings are worded for that: some properties could not be read and were not added. The three reserved fields are destructured once and the destructured values are inserted, so a getter cannot pass the typeof check and then return something else on a second read — that one is inherited from the current code and is worth closing while you are here. And errorMessage both reads and coerces inside its guard, because error.message can throw on access and on toString — and template interpolation would do that coercion outside the guard. Two earlier drafts of this patch leaked an exception that way, and getRumContextEntries() runs before the outer try, so it would have escaped enrichment entirely. With the whole set applied I measured 60 suites and 833 tests passing (1 skipped, 834 total), tsc --noEmit clean, eslint 0 errors and prettier clean.
There was a problem hiding this comment.
Yep, this is an edge case worth addressing. I'll update.
| * precedence, and an explicitly undefined application field removes the corresponding RUM value | ||
| * from the returned context. | ||
| */ | ||
| export const enrichRumContext = ( |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P1] Widen the public input type so the documented undefined tombstone can be written without a cast, and consider dropping the generic on the core helper.
EvaluationContextValue excludes undefined, so the behaviour described at README:80-81 needs a cast from TypeScript. Two things I checked that would let this through: the root tsconfig.json excludes packages/**/__tests__, and no package.json in the repo has a tsc --noEmit script.
What I measured — tsc 5.0.4, strict, against the installed @openfeature/core 1.10.0
Every result quoted below is one I ran locally against this head, not something you can read off the diff. The probe is four calls in one file; the diagnostics are verbatim.
probe.ts(4,30): TS2322: Type 'undefined' is not assignable to type 'EvaluationContextValue'.
// enrichRumContext({ email: undefined, plan: undefined })
probe.ts(8,28): TS2345: '{ email: undefined; }' … 'email' is incompatible with index signature.
probe.ts(11,28): TS2345: '{ targetingKey: undefined; }' … incompatible with index signature.
probe.ts(18,28): TS2345: '{ targetingKey: string | undefined; region: string; }' … incompatible
The last one is the shape an unresolved optional id takes.
The core signature is separate, and smaller
<T extends FlatEvaluationContext>(context: T): T describes a shape-preserving transform, and this one adds RUM keys and removes tombstoned ones. Nothing depends on the generic today — the consumer reaches the function through a structural cast — so dropping it costs nothing and stops the declaration claiming more than it does. Taking it means removing the as T at rumIntegration.ts:37 in the same edit.
Suggested change — the whole file, so no identifier is left dangling
packages/react-native-openfeature/src/rumContext.ts:
/*
* Unless explicitly stated otherwise all files in this repository are licensed under the Apache License Version 2.0.
* This product includes software developed at Datadog (https://www.datadoghq.com/).
* Copyright 2016-Present Datadog, Inc.
*/
import * as DatadogSdk from '@datadog/mobile-react-native';
import type {
EvaluationContext,
EvaluationContextValue
} from '@openfeature/web-sdk';
/**
* An evaluation context that may carry explicitly `undefined` attributes. An `undefined` attribute
* is a tombstone: it removes the corresponding RUM value instead of inheriting one. OpenFeature's
* `EvaluationContext` forbids `undefined` values, so the input is widened here. The returned
* context is normally tombstone-free, but not guaranteed to be: the compatibility fallback below
* returns the input as it came in, and core's own helper does the same if it cannot enumerate the
* application context. A surviving tombstone reaches `processEvaluationContext`, which drops
* `undefined` attributes.
*/
export type EnrichableEvaluationContext = {
targetingKey?: string | undefined;
} & Record<string, EvaluationContextValue | undefined>;
// Pinned to core's real signature so a rename, or a change incompatible with this call site,
// breaks this build rather than only the tests. It does not catch every signature change: an
// added optional parameter still compiles. The type reference is erased, so it adds no runtime
// dependency on the export and the namespace lookup below still degrades on an older core.
type RumContextEnricher = typeof DatadogSdk.__ddEnrichEvaluationContextWithRumUser;
/**
* Explicitly add the current RUM user to an OpenFeature evaluation context.
*
* The helper reads the RUM user each time it is called and returns a new context; it does not keep
* the OpenFeature context synchronized when the RUM user changes. The RUM user ID supplies the
* targeting key, and flat `string`, `number`, and `boolean` user properties (`name`, `email`, and
* `extraInfo` entries) supply attributes — nested objects, arrays, and `null` values are dropped.
* Application fields take precedence, and an explicitly `undefined` application attribute removes
* the corresponding RUM value from the returned context.
*
* Requires a `@datadog/mobile-react-native` at least as new as this package. With an older one it
* logs a warning and returns the application context unchanged, so flag evaluation keeps working
* on the application's own context.
*/
export const enrichRumContext = (
context: EnrichableEvaluationContext
): EvaluationContext => {
const enricher = (DatadogSdk as {
__ddEnrichEvaluationContextWithRumUser?: RumContextEnricher;
}).__ddEnrichEvaluationContextWithRumUser;
if (typeof enricher !== 'function') {
// Returning the context unchanged keeps flag evaluation working on the application's own
// context. A setup problem must not fail the host app's startup or login path.
// `console.warn`, not `InternalLog`: this branch fires when the core module object is
// incomplete, so it cannot assume `InternalLog` is on it. `InternalLog.log` is also a
// no-op until `verbosity` is configured, which would hide a setup error.
// eslint-disable-next-line no-console
console.warn(
'DATADOG: `enrichRumContext` could not find `__ddEnrichEvaluationContextWithRumUser` on @datadog/mobile-react-native, so the RUM user was not added and the application context is used unchanged. Update @datadog/mobile-react-native to at least the version of @datadog/mobile-react-native-openfeature, check for a duplicate install with `npm ls @datadog/mobile-react-native`, and make sure any test mock of the module preserves the real one (use `@datadog/mobile-react-native/jest`, or spread `jest.requireActual`).'
);
return context as EvaluationContext;
}
return enricher(context) as EvaluationContext;
};Note EvaluationContextValue is added to the existing type-only import, the parameter type changes, RumContextEnricher replaces the hand-written alias, and the fallback uses console.warn rather than InternalLog — see my comment on line 28 for why. Export EnrichableEvaluationContext from src/index.ts alongside DatadogOpenFeatureProviderOptions.
The pinned typeof alias is the part I would keep regardless: with it, a rename of the core export fails the build rather than only the tests. It catches a rename, and a change incompatible with this call site; it does not catch every signature change — an added optional parameter still compiles. Measured against a rename mutation:
rumContext.ts(27,45): error TS2339: Property '__ddEnrichEvaluationContextWithRumUser'
does not exist on type 'typeof import(".../core/src/index")'.
Today the same rename fails eight tests, all reporting a package-version mismatch. A root "typecheck": "tsc --noEmit -p tsconfig.json" with packages/**/__tests__ included would cover the rest.
There was a problem hiding this comment.
Yep, good call. EvaluationContext doesn't support this right now.
| __ddEnrichEvaluationContextWithRumUser?: RumContextEnricher; | ||
| }).__ddEnrichEvaluationContextWithRumUser; | ||
|
|
||
| if (typeof enricher !== 'function') { |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P1] Consider returning the application context unchanged with a warning instead of throwing, and describing what was observed rather than naming a cause.
The throw happens while the argument is evaluated, so in the README's own shape await OpenFeature.setContext(enrichRumContext(ctx)) it reaches application code before setContext runs, and the setProviderAndWait on the next line never executes. The declared peer floor is ^3.1.0, so an install that satisfies the manifest can reach it with no package-manager warning.
The peer range was already stale before this PR
configurationFromString, imported unconditionally by src/index.ts, first appears in core 3.6.0 — I checked tags 3.1.0 through 3.7.0. So cores 3.1.0–3.5.4 satisfy ^3.1.0 and already give a broken offline provider. Worth raising the floor in the same change.
One detail for the floor value: the enrichment export is not in 3.7.0 either. It is absent from tag 3.7.0, absent from the published @datadog/mobile-react-native@3.7.0 tarball, and af7995d1 Bump to version 3.7.0 is an ancestor of this merge — so the symbol first ships in the next release, and that is the floor.
Why the message should not name a cause
An old core is the expected cause, but not the only one that reaches this branch:
- A future core renames or removes the export. I ran that as a mutation: it fired this guard in 10 tests, every one of them reporting a version mismatch, while the core was the newer package.
- A consumer's own
jest.mock('@datadog/mobile-react-native', () => ({ … }))factory drops the export. This PR'srumContext.compatibility.test.ts:13-16does that deliberately to simulate the old core, so the pattern is a known one; a consumer doing it by accident gets the same error and no hint.
Naming the missing symbol and the checks keeps the message accurate under all three. A hard-coded version number would also have to be revised by hand whenever the real minimum changes, which is easy to miss.
Suggested change — degrade, and diagnose
if (typeof enricher !== 'function') {
// Returning the context unchanged keeps flag evaluation working on the application's own
// context. A setup problem must not fail the host app's startup or login path.
// `console.warn`, not `InternalLog`: this branch fires when the core module object is
// incomplete, so it cannot assume `InternalLog` is on it. `InternalLog.log` is also a
// no-op until `verbosity` is configured, which would hide a setup error.
// eslint-disable-next-line no-console
console.warn(
'DATADOG: `enrichRumContext` could not find `__ddEnrichEvaluationContextWithRumUser` on @datadog/mobile-react-native, so the RUM user was not added and the application context is used unchanged. Update @datadog/mobile-react-native to at least the version of @datadog/mobile-react-native-openfeature, check for a duplicate install with `npm ls @datadog/mobile-react-native`, and make sure any test mock of the module preserves the real one (use `@datadog/mobile-react-native/jest`, or spread `jest.requireActual`).'
);
return context as EvaluationContext;
}console.warn rather than InternalLog on purpose, and this is the part I got wrong first: this branch fires precisely when the core module object is incomplete, so it cannot assume InternalLog is on it — a consumer's jest.mock factory that omits it would turn the graceful path into a TypeError. InternalLog.log is also a no-op until verbosity is configured, which would hide a setup error. The full file is in my comment on line 21.
| region: 'us-east-1' | ||
| }; | ||
|
|
||
| await DdSdkReactNative.setUserInfo({ |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P1] Neither snippet compiles as written, and the global setContext they use is not Datadog-scoped.
Both blocks use DdSdkReactNative and OpenFeature without importing them, so neither runs at all if pasted into a fresh module. The prose names logout but shows only setUserInfo, which does nothing for an empty id; clearUserInfo() is the logout call and is not mentioned. Separately, OpenFeature.setContext(ctx) with no domain is not Datadog-scoped, which matters once a second provider is in play.
What I measured — tsc on the exact extracted blocks
Every result quoted below is one I ran locally against this head, not something you can read off the diff.
current1.tsx(10,7): TS2304: Cannot find name 'DdSdkReactNative'.
current1.tsx(16,7): TS2304: Cannot find name 'OpenFeature'.
current1.tsx(17,7): TS2304: Cannot find name 'OpenFeature'.
current2.tsx(1,7): TS2304: Cannot find name 'DdSdkReactNative'.
current2.tsx(1,36): TS2304: Cannot find name 'newUser'.
current2.tsx(2,30): TS2304: Cannot find name 'enrichRumContext'.
current2.tsx(2,47): TS2304: Cannot find name 'applicationContext'.
The replacements below both compile clean under the same config — I extracted them back out of the proposed README and ran that text, not my local copy of it. The first one adds a DdFlags.enable() call so the flags step is visible in the block; it still assumes the core SDK was initialized earlier, as at README:38-44.
The logout gap, measured
setUserInfo early-returns for a non-string or empty id (UserInfoSingleton.ts:15-17), so the shape the prose implies leaves the previous user in place:
setUserInfo({id:'user-123', email:'old@example.com'})
setUserInfo({id:''}) // no-op
enrichRumContext({region:'us'})
=> {"email":"old@example.com","targetingKey":"user-123","region":"us"}
clearUserInfo()
enrichRumContext({region:'us'}) => {"region":"us"} // no targetingKey; toDdContext then supplies ''
The global context reaches other providers — and the fix needs a consumer change
From the installed @openfeature/web-sdk@1.8.0, dist/cjs/index.js:1074: the no-domain branch of setContext builds unboundProviders as every domain-scoped provider not present in _domainScopedContext, then runs their context-change handlers. So an app running another vendor's provider on a domain with no context of its own receives the RUM user's attributes there too. The section mentions domains at line 121 but never shows the two-argument call.
I kept the main snippets on the global context, because that is what the rest of the README does and <OpenFeatureProvider> at line 148 is unqualified. The domain variant belongs in the warning instead, with the consumer line it requires — a domain-bound provider is invisible to a client with no domain. Both parts, which I compiled de-quoted:
const DATADOG_DOMAIN = 'datadog';
await OpenFeature.setContext(
DATADOG_DOMAIN,
enrichRumContext(applicationContext)
);
await OpenFeature.setProviderAndWait(
DATADOG_DOMAIN,
new DatadogOpenFeatureProvider()
);and at line 148, <OpenFeatureProvider domain={DATADOG_DOMAIN}> in place of <OpenFeatureProvider>, or OpenFeature.getClient(DATADOG_DOMAIN) for a direct client.
It has to be all of it, not just registration: every later setContext needs the domain too, including the login and logout calls in the second snippet and the one at line 138. A domain keeps its own context, so a global update would leave the Datadog domain on the previous user.
One more line worth adding: numeric attributes differ by platform
DdFlagsImplementation.kt:176-179 builds mutableMapOf<String, String>() and assigns parsed[key] = value.toString(); DdFlagsImplementation.swift:122-127 uses AnyValue.wrap and preserves the type. The Kotlin file's own comment at :227 says "the React Native bridge converts Long values to Double", and a boxed Double of 42 prints as "42.0" (I ran that on the JDK). No Kotlin test covers buildEvaluationContext. This is pre-existing for hand-authored contexts; what is new is that isSupportedAttribute promotes numbers the application did not pick for targeting.
Suggested replacement snippets — both compile clean as posted
import { DdFlags, DdSdkReactNative } from '@datadog/mobile-react-native';
import {
DatadogOpenFeatureProvider,
enrichRumContext
} from '@datadog/mobile-react-native-openfeature';
import { OpenFeature } from '@openfeature/react-sdk';
// The context your application owns. Keep this reference and enrich it; never overwrite it.
const applicationContext = {
region: 'us-east-1'
};
const setUpFlags = async (): Promise<void> => {
await DdFlags.enable();
// `await` this: the RUM user is only readable once `setUserInfo` resolves.
await DdSdkReactNative.setUserInfo({
id: 'user-123',
email: 'user@example.com',
extraInfo: { company_name: 'Example, Inc.' }
});
await OpenFeature.setContext(enrichRumContext(applicationContext));
await OpenFeature.setProviderAndWait(new DatadogOpenFeatureProvider());
};
void setUpFlags();and for login / logout:
import { DdSdkReactNative } from '@datadog/mobile-react-native';
import { enrichRumContext } from '@datadog/mobile-react-native-openfeature';
import { OpenFeature } from '@openfeature/react-sdk';
// The same application-owned context as above.
const applicationContext = { region: 'us-east-1' };
const onLogin = async (): Promise<void> => {
await DdSdkReactNative.setUserInfo({
id: 'user-456',
email: 'next@example.com'
});
await OpenFeature.setContext(enrichRumContext(applicationContext));
};
const onLogout = async (): Promise<void> => {
await DdSdkReactNative.clearUserInfo();
// With no RUM user the context carries no targeting key, and the provider maps that to the
// anonymous subject.
await OpenFeature.setContext(enrichRumContext(applicationContext));
};|
|
||
| `rumIntegrationEnabled` only controls whether feature flag evaluation events are sent to RUM. It | ||
| does not enable or disable `enrichRumContext()`. If you use OpenFeature domains or multiple providers, | ||
| you can apply the enriched context only to the intended domain. For the offline provider, continue to |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P1] Consider replacing this forward-reference with a direct warning about DatadogOfflineOpenFeatureProvider, and pinning the behaviour with a test.
An enriched context is servable offline only when the snapshot was precomputed for that exact context, because the comparison is an exact attribute key-set match. The guidance this sentence points at starts 80+ lines later at line 204, behind an intervening section, and the hybrid-app recommendation at 216 is to give that domain an explicitly empty context, which an enriched context usually is not once a RUM user is set.
What I measured — one failure shape is loud, the other is silent
Every result quoted below is one I ran locally against this head, not something you can read off the diff. Snapshot precomputed for { targetingKey: 'user-123' }.
Loud — enrich before registering, with a RUM email the snapshot does not carry:
enriched = {"email":"u@example.com","targetingKey":"user-123"}
setProviderAndWait rejected: InvalidContextError
status = ERROR | value = false | errorCode = INVALID_CONTEXT
Silent — a matching context at startup, then one unrelated addUserExtraInfo and a re-enrich:
status before = READY | value = true
addUserExtraInfo({ last_screen: 'checkout' })
enrichRumContext({}) => {"last_screen":"checkout","targetingKey":"user-123"}
setContext(domain, drifted) rejected? NO - it resolved normally
status after = ERROR | value = false | errorCode = INVALID_CONTEXT
The second shape is the one worth documenting: the await succeeds rather than rejecting, while every flag reverts to its coded default. The provider does move to ERROR and evaluations do carry INVALID_CONTEXT, so an app listening for ProviderEvents.Error or reading evaluation details can see it — what it will not see is a thrown error at the call it just made. Note the drift is not automatic — it needs the app to re-enrich and set the context again, which is exactly what the section tells it to do after a user change. addUserExtraInfo merges into extraInfo and removes nothing (UserInfoSingleton.ts:22-30), so a call with a new key widens the set; setUserInfo replaces the whole user and clearUserInfo drops it. None of those necessarily changes the key set — a replacement user with the same keys does not, and an application override can mask an addition — but any of them can, and the comparison needs an exact match, so a narrowing change breaks it just as a widening one does. The one exception is narrowing all the way to an empty effective context, which re-adopts the embedded context and recovers, per README:209.
Suggested wording
> **Warning:** using `enrichRumContext()` with `DatadogOfflineOpenFeatureProvider` is fragile. A
> precomputed configuration is a single-subject snapshot and the active context must match it
> exactly after normalization, including the full attribute key set, so an enriched context is only
> servable if the snapshot was computed for that exact context. It is also easy to drift out of:
> an `addUserExtraInfo()`, a `setUserInfo()` that replaces the user, or a `clearUserInfo()` can
> each change the normalized context, so a context that matched at startup can stop matching the
> next time you re-enrich after a user change. The provider then enters the OpenFeature `ERROR` state
> and every flag falls back to your coded default (`errorCode: INVALID_CONTEXT`). That transition
> does not reject the `setContext` call, so watch `ProviderEvents.Error` if you rely on it. The one
> effective context that always works is an empty one, which re-adopts the snapshot's embedded
> context and recovers the provider — so prefer giving the offline provider its own domain with an
> explicit empty context, as described in [Offline initialization](#offline-initialization).Both shapes above are worth a regression test; there is no test combining enrichRumContext with the offline provider today.
There was a problem hiding this comment.
Agreed that offline doesn't make sense with enrichRumContext, but there's also value in a simple README.md, and this is a pretty verbose explanation for something that most users won't ever do.
| DdBabelInteractionTracking, | ||
| __ddExtractText | ||
| __ddExtractText, | ||
| enrichEvaluationContextWithRumUser as __ddEnrichEvaluationContextWithRumUser |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] Answering @sbarrio's question, and suggesting a short comment here.
Underscore-prefixing to mean "internal" is conventional throughout this repo, and so is publishing such a symbol — DdSdkReactNative carries four _-prefixed statics reachable through the root export, three of them under a "FOR INTERNAL USE ONLY" doc block. What is new on this line is the alias: it is core's only root export whose name differs from its definition. That is the part worth recording, because nothing else does.
Where the repo already does this — seven patterns, with links
| Pattern | Meaning | Instances |
|---|---|---|
class _Foo + export const Foo = getGlobalInstance(…) |
only the instance leaves the module | _InternalLog → InternalLog, _GlobalState → GlobalState, _BufferSingleton, _DatadogProviderState, _DistributedTracingSampling |
_method on an exported class |
internal, and published | _initializeFromDatadogProvider, _enableFeaturesFromDatadogProvider, _enableFeaturesFromDatadogProviderAsync, _initializeFromDatadogProviderWithConfigurationAsync — on a class root-exported |
export const _foo |
exported for tests, kept off every entry point | _isDebugIdInBundle, _replaceDebugIdInBundle, _insertDebugIdCommentInBundle |
_foo |
module-private helper | _getErrorMessage, _getErrorStack |
_foo |
module-private state | _cachedSessionId |
__internal…ForTesting |
internal seam for a same-package consumer | __internalResetIsInitializedForTesting |
__internal_x |
Babel plugin option the CLI supplies, declared "not meant for end users" | __internal_saveSvgMapToDisk, __internal_reactNativeSVG on PluginOptions, passed in by generate-sr-assets |
The second row is the load-bearing one: three of those four carry the "FOR INTERNAL USE ONLY" block (_enableFeaturesFromDatadogProviderAsync has none), so they are internal by documentation alone, and they ship on the public surface through a root-exported class. So publishing an underscored internal is established practice here, not a departure.
Separately, _param marks an intentionally unused argument — _oldContext and _context in the OpenFeature package, _route and _event in the navigation packages. That one is partly mechanical: noUnusedParameters: true makes TypeScript exempt _-prefixed parameters, for the files in the program — tests and utils are excluded. Elsewhere the underscore itself enforces nothing — the module boundary is what actually hides rows 1, 4 and 5, since those declarations are never exported. There is no naming-convention rule, and argsIgnorePattern is commented out with args: 'none' in its place.
So the placement is normal; the alias is the new part
index.tsx:116-117 holds the only two top-level named underscore exports on core's entry point, and this is the only one renamed at the boundary:
__ddExtractText,
enrichEvaluationContextWithRumUser as __ddEnrichEvaluationContextWithRumUser
__ddExtractText carries the prefix at its definition and is exported unaliased, because insertRumActionImport writes that exact identifier into application source through getImportDeclaration + insertAtProgramTop, alongside DdBabelInteractionTracking. Every addNamed call in that file is React's jsx, jsxs or Fragment. Injecting a fixed name into a customer's module scope is what motivates an unusual one there; nothing injects this symbol, so here the prefix is a marker rather than a constraint. Either symbol could have been aliased at the export; this one was, and the local name stays descriptive as a result. Reasonable — just unrecorded.
The marker travels one subpath further: ./jest spreads the root surface via ...actualDatadog, so both top-level __dd helpers survive there — though it then replaces DdSdkReactNative, which drops the four underscored statics.
What the prefix does not buy
Emitted with the repo's own tsconfig:
export { …, DdBabelInteractionTracking, __ddExtractText,
enrichEvaluationContextWithRumUser as __ddEnrichEvaluationContextWithRumUser };
@internal is advisory — no api-extractor.json, no tsdoc.json, no stripInternal in any tsconfig. .gitlab-ci.yml runs test:lint, test:js and test:build, none of which notices a new root export.
And the channel itself looks right
Core has no public on-demand getter for the current RUM user: UserInfoSingleton is exported from its own module but not from core's entry point, and DdSdkReactNative has setUserInfo, clearUserInfo and addUserExtraInfo with no reader. (An event mapper does receive userInfo on each event, which is not the same thing.)
A subpath was the alternative, with history worth knowing: packages/core/internal/package.json still exists as a directory stub pointing at ../lib/*/internal.js, but packages/core/src/internal.tsx was removed in b72cd3e5 Remove internal export, and internal is not in core's files array — so the stub is dead either way. Core's exports map lists only ., ./metro, ./jest and ./package.json; under Node resolution that blocks unlisted subpaths, though Metro warns and falls back. ./metro shows the live pattern if you ever revive it.
Suggested comment
DdBabelInteractionTracking,
// Internal entry points for the Datadog companion packages, and for the identifier the Babel
// plugin injects into application code. Not part of the public API of
// `@datadog/mobile-react-native`: they may change or be removed in any release, and nothing
// enforces that beyond this comment. The `__dd` prefix is that marker. It is applied at the
// export for the enrichment helper so its local name stays descriptive; `__ddExtractText`
// carries it at the definition because the Babel plugin injects that literal identifier.
// Application code uses the documented wrappers instead — for RUM context enrichment,
// `enrichRumContext` from `@datadog/mobile-react-native-openfeature`.
__ddExtractText,
enrichEvaluationContextWithRumUser as __ddEnrichEvaluationContextWithRumUser
};It pairs with pinning the consumer's type to this export (my comment on rumContext.ts:21), which turns a rename into a build error naming the symbol instead of eight tests reporting a version mismatch.
There was a problem hiding this comment.
Read this over -- I don't think adding a comment is necessary and would just add noise that wouldn't provide meaningful context. If anything, we can just get rid of the alias since the original name is descriptive and preserves the "internal" __dd prefix. The rename itself likely was a stylistic choice by the agent.
There was a problem hiding this comment.
@greghuels Thanks, that's exactly why I asked.
I thought that keeping the original name was fine (and aligned with other modules) and thus could not understand the need for a rename.
There was a problem hiding this comment.
Yeah, good callout. I'll open a follow-up PR with some changes. Appreciate you flagging it.
| export { | ||
| DatadogOpenFeatureProvider, | ||
| DatadogOfflineOpenFeatureProvider, | ||
| enrichRumContext, |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] Consider renaming this export to enrichContextWithRumUser before it ships — it is free now and breaking later.
enrichRumContext reads as "enrich the RUM context", but what you pass in is your application's OpenFeature context, and the RUM user is the material merged into it. The file name reinforces the wrong reading of the argument. Core already calls the same operation enrichEvaluationContextWithRumUser, so the concept has three names across two packages.
Why now — the symbol is unreleased
packages/react-native-openfeature/src/rumContext.ts is absent from tag 3.7.0 and from the published @datadog/mobile-react-native-openfeature@3.7.0 tarball, and af7995d1 Bump to version 3.7.0 is an ancestor of this merge. Since 3.7.0 is the latest published version, the name has never shipped.
Measured — the rename is mechanical
I applied it end to end and ran it: src/rumContext.ts → src/enrichContextWithRumUser.ts, the export, src/index.ts, the five test files and their names, and the README. Result: 60 suites and 833 tests pass (the same as without it), tsc --noEmit clean, and both README snippets still compile after re-extraction. eslint --fix handles the three arca/import-ordering errors the new filename introduces. I have reverted it locally; it is not part of the other changes I suggested.
Alternatives I considered
enrichContextWithRumUser— says both halves, and matches core's internal name minus "Evaluation". My pick.withRumUser— shortest, reads well at the call site:setContext(domain, withRumUser(applicationContext)). Good if you prefer terse.enrichWithRumContext— better than today, but "RUM context" is still the wrong noun for what is merged in, which is the RUM user.
There was a problem hiding this comment.
Or enrichWithRumContext might strike the right balance of being succinct with a more accurate name
There was a problem hiding this comment.
I'm all for short names but "Context" is becoming a bit of an overloaded term, and in a lot of our docs/code this is the RUM "user". Perhaps the compromise is enrichWithRumUserContext. Longer, but more explicit.
There was a problem hiding this comment.
Oh whoops, I mean to say enrichWithRumUser, instead of enrichContextWithRumUser. I think any of those work though
There was a problem hiding this comment.
There probably also needs to be a follow-up exploration to see what's stopping us from having the same user-facing interface as the browser SDK.
| } | ||
| }; | ||
|
|
||
| const isSupportedAttribute = ( |
There was a problem hiding this comment.
Should null be allowed as value?
We have IS_NULL checks in our targeting rules
There was a problem hiding this comment.
Yep, it looks like custom RUM attributes can contain null, I'll update
There was a problem hiding this comment.
Following up on my earlier reply: null is valid for IS_NULL targeting, but I found an existing Android limitation that makes this larger than a helper change. The native flags context currently accepts only string attributes, and the React Native bridge serializes null as the literal string "null". I checked edge-assignments: IS_NULL recognizes actual null/missing attributes, not that string.
To keep #1428 scoped, I've removed the added null support from isSupportedAttribute and its runtime predicate. It remains string | number | boolean, retaining the existing omission of null-valued RUM properties. Application-supplied context values are unchanged, and the tests/docs now reflect that scope.
End-to-end null preservation needs a separate, coordinated native SDK/bridge change that accounts for public API compatibility. Deferring that existing platform issue rather than expanding this review-feedback PR.
There was a problem hiding this comment.
Additional context -- I had an initial native proposal (DataDog/dd-sdk-android#3869), but I closed it since it risks NullPointerException in client apps. Since this explodes scope, I'm deferring this issue rather than expanding this review-feedback PR.
Summary
Add an explicit
enrichRumContext(applicationContext)helper for applications that want to use the current RUM user in their OpenFeature evaluation context.targetingKeyname,email, and flat primitiveextraInfovalues to evaluation attributesundefinedapplication fields as tombstones that remove corresponding RUM valuesrumIntegrationEnabledscoped to RUM evaluation tracking; it does not control the helperThe helper is point-in-time rather than a live binding. After login, logout, or an account switch, applications update the RUM user and call
OpenFeature.setContext(enrichRumContext(applicationContext))again. They should retain the original application-owned context instead of enrichingOpenFeature.getContext(), which may contain values inherited from the previous RUM user.This is implemented entirely in JavaScript/TypeScript; no Android or iOS SDK changes are needed. The provider remains usable with older compatible core package versions. Calling the new helper with an older core version produces a clear package-version error.
Acceptance plan
enrichRumContext().undefinedapplication fields remove inherited RUM values.Validation
yarn exec jest --watchman=false --runInBand --projects packages/core packages/react-native-openfeatureyarn bob buildpassed for both affected packages, including TypeScript declaration generation.git diff --checkpassed.