fix(openfeature): address RUM context review feedback - #1428
Conversation
d1960db to
3bb065f
Compare
greghuels
changed the title
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate findings affect absent-user warnings and non-string targetingKey values; documentation nits also remain.
Pull request overview
This PR renames and hardens explicit RUM user enrichment for OpenFeature, updating types, diagnostics, documentation, and tests.
Changes:
- Renames public and internal enrichment helpers.
- Adds guarded enrichment and compatibility fallback behavior.
- Expands type, integration, and regression-test coverage.
- Updates setup and context documentation.
File summaries
| File | Summary | Final review comments |
|---|---|---|
packages/react-native-openfeature/src/rumContext.ts |
Public helper and enrichable context type | — |
packages/react-native-openfeature/src/index.ts |
Updated package exports | — |
packages/react-native-openfeature/src/__tests__/rumContext.types.test.ts |
Type-level API coverage | — |
packages/react-native-openfeature/src/__tests__/rumContext.test.ts |
Runtime enrichment tests | — |
packages/react-native-openfeature/src/__tests__/rumContext.integration.test.ts |
Provider integration tests | — |
packages/react-native-openfeature/src/__tests__/rumContext.compatibility.test.ts |
Compatibility tests | — |
packages/react-native-openfeature/src/__tests__/__utils__/tsconfig.json |
Type-test compiler configuration | — |
packages/react-native-openfeature/src/__tests__/__utils__/rumContext.types.ts |
Compile-time regression cases | — |
packages/react-native-openfeature/README.md |
Setup and context documentation | Nit (1 vote): Document targetingKey as string-only and align the domain-scoped update example. |
packages/core/src/index.tsx |
Internal helper export | — |
packages/core/src/flags/rumIntegration.ts |
RUM context enrichment and diagnostics | Moderate (1 vote): Treat an absent RUM user as a normal empty contribution rather than warning. Moderate (1 vote): Restrict targetingKey values from extraInfo to strings or omit invalid values. |
packages/core/src/flags/__tests__/rumIntegration.test.ts |
Core enrichment regression tests | — |
Review details
Suppressed comments (4)
packages/core/src/flags/rumIntegration.ts:125
- This warning is emitted on every enrichment after
clearUserInfo()(and before login), even though the README documents that exact no-user state as the supported anonymous flow. With SDK verbosity enabled, logout/re-enrichment will therefore produce a misleading warning telling the caller to callsetUserInfo(); treat an absent user as a normal empty RUM contribution and reserve the warning for actual read failures.
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
);
}
packages/core/src/flags/rumIntegration.ts:66
extraInfovalues are accepted here wheneverisSupportedAttributereturns true, including numbers and booleans for the reservedtargetingKeykey. The new test consequently permits results such astargetingKey: 42, but OpenFeature's targeting key andNativeDdFlags.setEvaluationContextboth require a string, andtoDdContextforwards this value to native. RestricttargetingKeyfromextraInfoto strings (or omit/rename invalid values) and update the corresponding test/documentation.
if (!isSupportedAttribute(value)) {
InternalLog.log(
`RUM user property "${key}" is not a string, number, or boolean. Omitting it from the evaluation context.`,
SdkVerbosity.WARN
);
packages/react-native-openfeature/README.md:80
- This statement also documents an invalid case: unlike ordinary attributes,
targetingKeyis a reserved OpenFeature field and must be a string. Saying it follows the same primitive rules asextraInfowould encourage numeric/boolean values that the provider forwards to the native string parameter. Document the string-only restriction (and the behavior for invalid values) instead.
or boolean `extraInfo` properties to evaluation attributes. Merge precedence is `extraInfo`,
then the RUM user's own identity fields, then the application context (highest precedence).
`targetingKey`, `name`, and `email` in `extraInfo` follow the same rules as other attributes.
packages/react-native-openfeature/README.md:224
- The new domain guidance says every subsequent update, including the React example, must call
OpenFeature.setContext(DATADOG_DOMAIN, enrichWithRumUser(applicationContext)), but the example below still calls the globalOpenFeature.setContext({...raw user...})and does not use the domain or helper. Following both snippets leaves Datadog's explicit domain on the previous RUM context while updating other providers instead. Update that example to use the domain and the retained application context, or remove this reference.
Pass `DATADOG_DOMAIN` to **every** subsequent context update as well:
`OpenFeature.setContext(DATADOG_DOMAIN, enrichWithRumUser(applicationContext))`. This includes both
login and logout handlers above and the `setContext()` call in the React example below. A global
update does not replace an explicit domain context, so omitting the domain would leave Datadog on
the previous user's context. On logout, set the re-enriched application context on the domain rather
- Files reviewed: 12/12 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
hestonhoffman
added
the
editorial review
aarsilv
left a comment
aarsilv
left a comment
There was a problem hiding this comment.
🤖 Review from Claude working with Aaron Silverman. Nothing here blocks the merge.
This PR answers all ten comments on #1363. Eight are adopted; two are settled by discussion, with the reasoning written into the code. The new tests are strong — rumIntegration.ts reaches 100% branch coverage, and the compile-time type test is real: I mutated it twice and it failed both times.
I applied the head diff to a clean worktree and ran everything. Every claim below is backed by executed code, and every suggested change compiles, lints, and passes the suites.
Worth a look
| Where | Point |
|---|---|
internal.ts:32 |
Offline, a non-string targeting key now resolves to the anonymous subject's snapshot (READY) where it previously fell back to your coded defaults (ERROR). Provider-reachable and untested — two tests, plus the one uncovered branch in the file. |
rumContext.ts:22 |
typeof DatadogSdk.__dd… makes the file that exists to degrade at runtime fail at compile time, on a core the ^3.1.0 peer range does not require. |
README.md:105 |
The jest mock this line recommends leaves the RUM user empty, so enrichment silently returns an un-enriched context. |
README.md:258 |
The React example sets the context twice, so a cold start makes two native context calls instead of one. |
README.md:32 |
The <DatadogProvider /> setup snippet does not run: undeclared identifier, and onInitialized is not a prop. |
rumIntegration.ts:87 |
A name/email of the wrong type is dropped silently while the same value in extraInfo is kept, and a non-string id loses the targeting key with no signal. |
Smaller
| Where | Point |
|---|---|
rumIntegration.ts:68 |
One WARN per rejected property per call; an aggregate line per call caps it without touching the deferred null decision. |
rumContext.ts:47 |
The shortened warning no longer names the export to check, which is the greppable part. |
rumIntegration.test.ts:150 |
Three toEqual still pass under a mutation the rest of the suite catches. |
internal.test.ts:34 |
null and undefined are interchangeable there but not downstream — isEmptyContext treats only undefined as absent. |
Verification. All suggestions applied together: core 814 passed, 1 skipped; OpenFeature 63 passed; Prettier and ESLint clean; tsc --noEmit at the root gives 36 diagnostics before and after (33 benchmarks, 3 example-new-architecture), none introduced. Every suggested change was mutation-tested: reverting it turns a test red.
| // Validate only the final targeting key, after any RUM/application merge. Its source must | ||
| // not affect validation or cause a fallback to a lower-precedence user's identity. | ||
| // Both online and offline clients share this boundary; native calls require a string. | ||
| if (typeof targetingKey !== 'string') { |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] This block changes a provider-visible offline outcome from ERROR to READY, and nothing tests it. Worth a conscious decision plus three tests.
No behaviour change suggested. internal.test.ts documents the anonymous-subject fallback for all seven non-string values, so it is a decision, and I checked our other SDKs before assuming otherwise — see the last fold.
What is untested
The new tests here exercise processEvaluationContext in isolation. What they do not reach is what the '' it produces then does in contextMatchesConfiguration, which is the offline precomputed path. Two behaviours depend on it and neither is pinned:
- an active context with no targeting key now matches an embedded context with none — before the change the two sides normalized asymmetrically and could never match
- a non-string active key is normalized to
''and therefore matches a snapshot precomputed for the anonymous subject
Both are consequences of this block, and both are invisible in context.test.ts and FlagsClient.test.ts today, where every targetingKey is a string literal.
The two differ in reach, which matters for how seriously to take each:
- The absent-key case is not provider-reachable.
mappers.ts:28appliestargetingKey ?? ''before core sees the context, so a provider never presents a context without a targeting key. It applies to directDdFlagscallers. - The non-string case is provider-reachable, because
??only substitutes fornullandundefined. A number passes straight through.
I drove the second one end to end through the real DatadogOfflineOpenFeatureProvider, with a snapshot precomputed for the anonymous subject and the targeting key arriving from typed addUserExtraInfo({ targetingKey: 42 }):
enriched = {"targetingKey":42}
with this block: status = READY (serves the anonymous snapshot)
without this block: status = ERROR "The evaluation context does not match the offline
precomputed configuration. Serving default values."
That is the part worth a decision: offline, a garbage targeting key used to fall back to your coded defaults and now resolves to the anonymous subject's precomputed flags. Both are defensible — the second is more available, the first fails louder — but it is a behaviour change that no test records.
Why a non-string key is worth pinning at all
It is reachable from typed code, not only from as any. isSupportedAttribute admits number, so a RUM extraInfo entry named targetingKey survives the merge when no string id overwrites it, and addUserExtraInfo(extraInfo: Record<string, unknown>) takes it without a cast:
addUserExtraInfo({ targetingKey: 42, plan: 'pro' })
enrich({}) -> {"targetingKey":42,"plan":"pro"} typeof = number, 0 InternalLog calls
That is intended — rumIntegration.ts:59-62 keeps merge and validation separate and rumIntegration.test.ts:284-300 pins it. It just means the non-string case is a real input to this block rather than a hypothetical.
Suggested tests
describe('contextMatchesConfiguration targeting key normalization', () => {
it('matches an embedded context with no targeting key against an active context with none', () => {
// Both sides normalize an absent key to the anonymous subject ''.
expect(
contextMatchesConfiguration(
{ country: 'US' },
processEvaluationContext({
attributes: { country: 'US' }
} as never)
)
).toBe(true);
});
it('treats a coerced non-string targeting key as the anonymous subject', () => {
// JavaScript callers can provide values outside the TypeScript contract.
// processEvaluationContext replaces a non-string key with the anonymous subject, so
// the active context then matches a snapshot precomputed for that same subject.
expect(
contextMatchesConfiguration(
{ targetingKey: '', country: 'US' },
processEvaluationContext({
targetingKey: 42 as never,
attributes: { country: 'US' }
})
)
).toBe(true);
});
});Add import { processEvaluationContext } from '../../internal'; above the existing ../context import — arca/import-ordering wants it first.
Both pass as written and both fail if the new internal.ts block is removed, so they pin the change rather than restating it.
One uncovered branch, 34 lines down
if (value === undefined) { continue; } at internal.ts:66-68 is the only line the coverage run never reached: 92.3% branch, line 67 uncovered. Deleting the three lines leaves the core flags suite fully passing.
Attributes are exercised — primitives and non-primitives at internal.test.ts:56, a null attribute at line 74. What is missing is an explicit undefined attribute. The one undefined in the file is at line 34 and it is a targetingKey.
Reaching it needs the application to supply an enumerable attribute whose value is undefined; nothing in the SDK creates one. The degraded path returns the supplied context unchanged (rumContext.ts:50) and the mapper copies its attributes (mappers.ts:19-29), so an app using tombstones without enrichment gets there. Without the guard the key survives Object.fromEntries and remains an own property in the object handed to the native module at FlagsClient.ts:154. I did not trace what the bridge does with it past that call.
it('drops attributes explicitly set to undefined and keeps null ones', () => {
const result = processEvaluationContext({
targetingKey: 'user-1',
// JavaScript callers can provide values outside the TypeScript contract.
attributes: {
kept: 'yes',
nulled: null,
removed: undefined as never
}
});
expect(result).toStrictEqual({
targetingKey: 'user-1',
attributes: { kept: 'yes', nulled: null }
});
// An undefined attribute must not survive as an own key past this boundary.
expect(Object.keys(result.attributes ?? {})).toStrictEqual([
'kept',
'nulled'
]);
// Dropping an undefined value is normal, not an error worth warning about.
expect(InternalLog.log).not.toHaveBeenCalled();
});undefined as never is deliberate: PrimitiveValue is null | boolean | string | number, so a plain removed: undefined is a TS2322. The cast matches how the existing targetingKey cases in this file spell "outside the TypeScript contract". With the test in place, the same deletion turns red.
Aside — I checked the other SDKs before assuming the fallback was wrong
My first draft of this comment argued '' was the wrong substitute and proposed String(value). Withdrawn: the test here settles the intent, and the comparison does not support it either.
The Datadog OpenFeature browser provider does not collapse non-strings to the anonymous subject. It uses || '' on the wire, which substitutes for every falsy value, and a = '' default at the exposure event, which applies only to undefined:
| value | wire (fetchConfiguration.ts:79) |
exposure event (exposureEvent.ts:20) |
this PR |
|---|---|---|---|
42 |
number 42 |
number 42 |
"" |
null |
"" |
null |
"" |
0 / false |
"" |
0 / false |
"" |
{} / [] |
passed through | passed through | "" |
So it leans on TypeScript and lets truthy non-strings reach the wire. This PR is the more defensive of the two, and it has a constraint the browser does not: the bridge here declares a non-null Kotlin String (packages/core/android/src/newarch/kotlin/com/datadog/reactnative/DdFlags.kt:43). Recording the divergence in case it matters for consistency later; it is not an argument against this PR.
Aggregate
These three tests plus the other suggestions in this review, applied together: core 814 passed, 1 skipped; OpenFeature 63 passed. Prettier and ESLint clean. tsc --noEmit at the root: 36 diagnostics before and after — 33 in benchmarks, 3 in example-new-architecture, none introduced.
| OpenFeature initialization and evaluation can continue using the application's context without RUM | ||
| values. Update the core SDK to at least the OpenFeature package's version, check for duplicate | ||
| installs with `npm ls @datadog/mobile-react-native`, and ensure test mocks preserve the real module | ||
| exports (use `@datadog/mobile-react-native/jest` or spread `jest.requireActual`). The warning is |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] The jest mock this line recommends leaves the RUM user empty, so enrichWithRumUser() returns an un-enriched context with no warning.
packages/core/jest/mock.js:24 spreads the real helper, so typeof enricher === 'function' holds and the graceful-degradation branch never runs. But setUserInfo at line 30 is a jest.fn() that only resolves a promise — it never writes UserInfoSingleton, which is what the real helper reads. The mock replaces the setter and keeps the reader.
What a consumer sees
const publishedMock = require('@datadog/mobile-react-native/jest');
const { DdSdkReactNative, __ddEnrichEvaluationContextWithRumUser } = publishedMock;
await DdSdkReactNative.setUserInfo({ id: 'user-1', name: 'Ada' });
const enriched = __ddEnrichEvaluationContextWithRumUser({ plan: 'pro' });HELPER TYPE: function
ENRICHED: {"plan":"pro"}
No targetingKey, no name, and no console warning — nothing is broken from the helper's point of view, the singleton is simply empty.
Harness disclosure: I loaded jest/mock.js by path, because this monorepo's jest moduleNameMapper rewrites @datadog/mobile-react-native to ../core/src and the unanchored pattern captures the /jest subpath too, which recurses. A consumer outside the monorepo resolves the specifier normally.
Nothing in this repo imports @datadog/mobile-react-native/jest, so no in-repo test covers it. I grepped unscoped across every file type; the only hit is README.md:105.
Suggested change — delegate, do not reimplement
My first draft reimplemented the merge inside the mock. That was wrong twice over: it read each identity field twice — validate, then insert — reintroducing exactly the getter race that rumIntegration.ts:85-86 was changed to close, and it dropped the unsupported-property warnings. A second copy of the merge is a second thing to keep in sync.
Pointing the mocked setters at the real singleton avoids all of it. The real helper is already spread from actualDatadog, so it just works:
// The real `setUserInfo` writes this singleton only after its native call resolves, and the
// mocks below replace that call. Delegating to the real singleton keeps this mock and the real
// `__ddEnrichEvaluationContextWithRumUser` (spread from `actualDatadog` below) in agreement,
// with no second copy of the merge logic to drift or to lose a guard.
// Resolve it beside whichever entry point `actualDatadog` came from: this package publishes
// both `src` (the "react-native" field) and `lib/commonjs` ("main"), and those are separate
// module instances with separate singletons.
const path = require('path');
const { UserInfoSingleton } = jest.requireActual(
path.join(
path.dirname(require.resolve('@datadog/mobile-react-native')),
'sdk/UserInfoSingleton/UserInfoSingleton'
)
); setUserInfo: jest.fn().mockImplementation(userInfo => {
UserInfoSingleton.getInstance().setUserInfo(userInfo);
return Promise.resolve();
}),
addUserExtraInfo: jest.fn().mockImplementation(extraUserInfo => {
UserInfoSingleton.getInstance().addUserExtraInfo(extraUserInfo);
return Promise.resolve();
}),
clearUserInfo: jest.fn().mockImplementation(() => {
UserInfoSingleton.getInstance().clearUserInfo();
return Promise.resolve();
}),Parity is then a property of the code rather than something to keep testing for. The id guard comes along free: setUserInfo({ id: 42 }) is ignored, because this is UserInfoProvider.setUserInfo (UserInfoSingleton.ts:14-17).
Executed against the published mock:
setUserInfo({id:'user-1', name:'Ada', extraInfo:{plan:'pro', nested:{}}})
-> {"plan":"pro","name":"Ada","targetingKey":"user-1","region":"us"}
addUserExtraInfo({tier:'gold'}) -> {"plan":"pro","tier":"gold","name":"Ada","targetingKey":"user-1"}
clearUserInfo() -> {"region":"us"}
setUserInfo({id: 42}) -> {}
nested: {} is dropped and id: 42 ignored without the mock knowing either rule.
The require.resolve dance is not decoration: this package publishes src under the react-native field and lib/commonjs under main, and those are separate module instances with separate singletons. A plain relative ../src/... happens to work under this repo's own source moduleNameMapper, and would silently miss wherever the package resolves through exports.require to lib/commonjs — which is what the React Native jest preset's ['require', 'react-native'] conditions select.
Verification. Prettier and ESLint clean on mock.js. With this change plus the other suggestions in this review applied together: core 816 passed, 1 skipped; OpenFeature 63 passed. The mock change alone breaks no existing test — mock.test.ts:79 only asserts the export is present.
The README line
Line 104-105 reads:
ensure test mocks preserve the real module exports (use
@datadog/mobile-react-native/jestor spreadjest.requireActual)
That sentence is about a missing or non-callable export, which is a different problem from an unpopulated user, so it is not wrong as written. Two notes rather than a rewrite:
jest.requireActualkeeps the realDdSdkReactNative.setUserInfo, which does write the singleton once its native call resolves — asrumContext.integration.test.ts:40and:288already demonstrate. That option works today.@datadog/mobile-react-native/jestis the one that silently does not, until the change above. A clause pointing that out would save someone an afternoon.
|
|
||
| // Wrap your app with OpenFeatureProvider to allow flag evaluations throughout the app. | ||
| // Enrich the retained application context, never OpenFeature.getContext(). | ||
| void OpenFeature.setContext( |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] This effect re-sets a context the bootstrap already set: a successful bootstrap followed by one mount-effect execution makes two native context calls instead of one.
The prose at line 242-244 tells the reader to await setUpDatadogDomain() before rendering, and that function already calls OpenFeature.setContext(DATADOG_DOMAIN, enrichWithRumUser(applicationContext)) at line 213-216. This effect does it again on mount.
Executed — two identical native calls
Driving the real DatadogOpenFeatureProvider through the README's own sequence — bootstrap, then the mount effect:
NATIVE CALLS after bootstrap: 1 | after mount effect: 2
CALL ARGS: [["c1","rum-user",{"plan":"pro"}],["c1","rum-user",{"plan":"pro"}]]
Identical arguments, twice. Harness disclosure: my own fixtures (clientName: 'c1', a one-key context), and NativeDdFlags/NativeDdSdk mocked exactly as rumContext.integration.test.ts:17-46 already does. So this measures the JavaScript call path down to the native module boundary, not a device.
The OpenFeature SDK does not dedupe. In @openfeature/web-sdk@1.8.0, when a provider is registered for the domain, setContext calls runProviderContextChangeHandler without comparing old and new, and that handler invokes provider.onContextChange(oldContext, newContext). On the successful path enrichWithRumUser also returns a fresh object via Object.fromEntries (rumIntegration.ts:38), so reference equality would not save it either.
To be clear about what this is not: the dependency array is [] (line 262) and applicationContext is module-scoped (line 209), so there is no re-render loop. This is a duplicate at startup.
Suggested change — same demonstration, one call
The block demonstrates two things: scoping a provider to a domain, and consuming flags from that domain. Neither needs the effect. The bootstrap owns the initial context, and the domain-scoped versions of the login/logout handlers (the global ones are at lines 161-173; line 231-236 tells the reader to add DATADOG_DOMAIN to them) own the updates.
import { OpenFeatureProvider, useFlag } from '@openfeature/react-sdk';
import { DATADOG_DOMAIN } from './featureFlags';
// `setUpDatadogDomain()` sets the initial context before the app renders, and the
// domain-scoped login and logout handlers update it. Do not re-set it on mount.
function AppWithProviders() {
// Use the same domain for flag evaluation and context updates.
return (
<OpenFeatureProvider domain={DATADOG_DOMAIN}>
<App />
</OpenFeatureProvider>
);
}This is a partial edit: App, the useFlag call and the default export are unchanged. Four imports become unused and should go with the effect — useEffect, OpenFeature, enrichWithRumUser, applicationContext.
One knock-on: line 233 currently says the domain must be passed to "the setContext() call in the React example below". With the effect gone there is no such call, so that clause needs dropping.
I am not claiming the block compiles standalone — it references View and NewFeatureComponent, and @openfeature/react-sdk is a consumer-side install. Those limitations are pre-existing and unchanged by this edit.
The other split does not work — I tried it first
My first instinct was that you could instead drop the setContext from setUpDatadogDomain() and let the effect own the initial context. That does not remove the second call, and it makes the first one worse:
ALTERNATIVE (no setContext in setup, keep the effect):
bootstrap: 1 after mount: 2
args: [["alt","",{}],["alt","rum-user",{"region":"us-east-1"}]]
RECOMMENDED (setContext in setup, no effect):
total: 1
args: [["rec","rum-user",{"region":"us-east-1"}]]
setProviderAndWait calls initialize, which requests a context regardless. With no explicit domain context it inherits the global one, and when that is empty toDdContext (mappers.ts:19-29) yields '' and {} — so the first context request goes out as the anonymous subject with no attributes, and the mount effect then corrects it. Removing the effect is the only split of the two that ends at one call.
|
|
||
| ```tsx | ||
| import { CoreConfiguration, DatadogProvider, DdFlags } from '@datadog/mobile-react-native'; | ||
| import { CoreConfiguration, DatadogProvider, DdFlags, DdSdkReactNative } from '@datadog/mobile-react-native'; |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] The <DatadogProvider /> half of this snippet does not run as written: coreConfiguration is never declared, and onInitialized is not a prop.
The real prop is onInitialization (DatadogProvider.tsx:33). onInitialized appears exactly once in the whole repo — README line 55. A reader who pastes this gets a callback that never fires, so the DdFlags.enable() inside it never runs and this alternative never registers the provider. (The IIFE above it is unaffected.)
Compiler check
I extracted the block into a fixture and compiled it under --strict against the real package sources. The two defects, at fixture coordinates that correspond to README lines 54 and 55:
broken.tsx(15,20): error TS2552: Cannot find name 'coreConfiguration'. Did you mean 'CoreConfiguration'?
broken.tsx(16,5): error TS2322: Type '{ ...; onInitialized: () => Promise<void>; }' is not assignable to
'... & { configuration: DatadogProviderConfiguration | FileBasedConfiguration | AutoInstrumentationConfiguration;
onInitialization?: (() => void) | undefined; } & { ...; }'
Disclosure, because the fixture is not the README verbatim: @openfeature/react-sdk is not installed in this repo, so I shimmed it; that makes the imported OpenFeature effectively any. The run also reports react/jsx-runtime resolution noise, and the block additionally produces TS2554: Expected 2-4 arguments, but got 0 for the new CoreConfiguration(// ...) at line 38. That last one is the README's established placeholder convention — CoreConfiguration takes two required arguments — so it is not what I am reporting.
Behind the first error there is a scoping point: the IIFE declares const config inside the closure (line 38), so it is not in scope here. And config is a CoreConfiguration, which the prop's TypeScript type does not accept — the union is DatadogProviderConfiguration | AutoInstrumentationConfiguration | FileBasedConfiguration. At runtime the provider does handle a plain CoreConfiguration, with a console.warn telling you to use DatadogProviderConfiguration (DatadogProvider.tsx:51-55), so hoisting the IIFE's config would work but warn. Declaring the right type is cleaner.
Both defects predate this PR. I am raising them because its stated scope includes "Fix setup snippets", and it edits this block's import line.
Suggested change
Add DatadogProviderConfiguration to the import:
import {
CoreConfiguration,
DatadogProvider,
DatadogProviderConfiguration,
DdFlags,
DdSdkReactNative
} from '@datadog/mobile-react-native';and give the alternative branch its own configuration and the right prop:
// Alternatively, if using `<DatadogProvider />` for core SDK initialization.
const coreConfiguration = new DatadogProviderConfiguration(
// ...
);
<DatadogProvider
configuration={coreConfiguration}
onInitialization={async () => {
await DdFlags.enable();
const provider = new DatadogOpenFeatureProvider();
OpenFeature.setProvider(provider);
}}
>
{/* ... */}
</DatadogProvider>onInitialization is typed () => void and an async arrow is assignable to it, so the body needs no change. Note the provider invokes it without awaiting (DatadogProvider.tsx:68), inside a synchronous try/catch that cannot catch a rejected promise, and returns children without waiting (:100). Pre-existing, and not something this edit changes.
Verification. With clientToken/env substituted for the // ... placeholder, both reported errors disappear. One diagnostic remains in that fixture — react/jsx-runtime resolution — which is a property of my harness, not the snippet. I left the placeholder in the posted version to match the rest of this README.
| try { | ||
| // Read once so a getter cannot change the value between validation and insertion. | ||
| const { name, email, id } = user; | ||
| if (typeof name === 'string') { |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] A name or email of the wrong type is dropped here silently, while the same value in extraInfo is kept — and a non-string id loses the targeting key with no signal at all.
extraInfo: { name: 42 } is kept, because isSupportedAttribute admits numbers. setUserInfo({ id: 'u1', name: 42 }) is dropped, with no warning. Same key, same value, opposite outcomes.
What should happen, per the spec and our own provider
name and email are not special in OpenFeature. They are ordinary context attributes of type EvaluationContextValue, which admits numbers and booleans — so a numeric name is a valid attribute, not an error. For extraInfo, this helper's isSupportedAttribute admits strings, numbers and booleans. (The later processEvaluationContext boundary is slightly wider — it also preserves null — but isSupportedAttribute is the rule these fields would join.) Our OpenFeature JS/browser provider applies no special casing to name or email either.
So the defensible rule is: identity fields follow the same rule as every other attribute. That is a policy choice, not something the spec forces — the narrower alternative is to keep the string-only restriction and just add the missing warning. I would take the wider one, because it makes user.name and extraInfo.name agree, and the inconsistency is the part a user will actually hit.
id is different and the current guard is right: it becomes the targetingKey, which the API contract types as a string in OpenFeature (@openfeature/core/dist/types.d.ts:30), in the JS provider, and in this repo's Android bridge (packages/core/android/src/newarch/kotlin/com/datadog/reactnative/DdFlags.kt:43, a non-null Kotlin String). What is missing is the diagnostic.
One caveat on that diagnostic, since rumIntegration.ts:59-62 deliberately stays quiet about values the merge overwrites: it runs before the application context is merged, so it reports what the RUM user contributed rather than the final outcome. The message is worded that way. If you would rather the merge said nothing at all, dropping the else is defensible — the cost is that this case stays silent, because an absent final targeting key is silent by design downstream.
Reachability — I got this wrong first, so here is the check
My first draft asserted a non-string id was unreachable and proposed no diagnostic for it. That was wrong. Executed:
setUserInfo({id: 42}) -> user is undefined (rejected outright)
setUserInfo({id:'ok', name:42, email:true}) -> {"id":"ok","name":42,"email":true}
addUserExtraInfo({id: 42}) -> {"extraInfo":{"id":42}}
So a non-string name/email is reachable and stored, and setUserInfo does reject a non-string id up front. But the singleton stores the caller's object by reference (UserInfoSingleton.ts:18), so the check does not hold afterwards:
stored by reference: true
after caller mutates id -> enrich: {"name":"Ada"}
singleton id is now: 42
The targeting key simply disappears, and targeting silently falls to extraInfo or the anonymous subject. That is worth a line of output. Each of these is a fresh-singleton example, not a method return value.
I also checked the present-but-wrong-type direction is untested today: changing all three typeof x === 'string' guards to x !== undefined leaves the core flags suite fully passing.
Suggested change
// `name` and `email` are ordinary evaluation-context attributes, so they follow the
// same type rule as extraInfo. Dropping them silently would also contradict
// `extraInfo: { name: 42 }`, which this function keeps.
for (const [key, value] of [
['name', name],
['email', email]
] as const) {
if (value === undefined) {
continue;
}
if (!isSupportedAttribute(value)) {
InternalLog.log(
`The RUM user ${key} is not a string, number, or boolean. Omitting it from the evaluation context.`,
SdkVerbosity.WARN
);
continue;
}
entries.push([key, value]);
}
// The targeting key must be a string: it identifies the subject at the API boundary
// (OpenFeature, the native bridge) and selects a bucket. `setUserInfo` rejects a
// non-string id, but the singleton stores the caller's object by reference, so a later
// mutation or a getter can still make it non-string by the time enrichment reads it.
// This reports only what the RUM user contributed; the application context may still
// supply its own targeting key, and the merge deliberately does not comment on that.
if (typeof id === 'string') {
entries.push(['targetingKey', id]);
} else if (id !== undefined) {
InternalLog.log(
`The RUM user id is a ${typeof id}, not a string, so the RUM user did not supply a targeting key. Pass a string id to setUserInfo().`,
SdkVerbosity.WARN
);
}Suggested tests
it('reports a non-string RUM user id rather than silently dropping the targeting key', () => {
// The singleton stores the caller's object by reference, so a mutation after
// setUserInfo's own id check can make the id non-string by enrichment time.
const user = {
id: 'rum-user',
extraInfo: { targetingKey: 'from-extra-info' }
};
UserInfoSingleton.getInstance().setUserInfo(user);
(user as { id: unknown }).id = 42;
expect(__ddEnrichEvaluationContextWithRumUser({})).toStrictEqual({
targetingKey: 'from-extra-info'
});
expect(InternalLog.log).toHaveBeenCalledWith(
'The RUM user id is a number, not a string, so the RUM user did not supply a targeting key. Pass a string id to setUserInfo().',
SdkVerbosity.WARN
);
});
it.each([
['name', 42, { targetingKey: 'rum-user', name: 42 }],
['email', true, { targetingKey: 'rum-user', email: true }]
])(
'keeps a %s of supported non-string type, matching how extraInfo is treated',
(field, value, expected) => {
// JavaScript callers can provide values outside the TypeScript contract.
UserInfoSingleton.getInstance().setUserInfo({
id: 'rum-user',
[field]: value
} as never);
expect(__ddEnrichEvaluationContextWithRumUser({})).toStrictEqual(
expected
);
expect(InternalLog.log).not.toHaveBeenCalled();
}
);
it.each(['name', 'email'])(
'omits a %s of unsupported type and says so',
field => {
UserInfoSingleton.getInstance().setUserInfo({
id: 'rum-user',
[field]: { nested: true }
} as never);
expect(__ddEnrichEvaluationContextWithRumUser({})).toStrictEqual({
targetingKey: 'rum-user'
});
expect(InternalLog.log).toHaveBeenCalledWith(
`The RUM user ${field} is not a string, number, or boolean. Omitting it from the evaluation context.`,
SdkVerbosity.WARN
);
}
);Verification. These ran with the rest of this review's suggestions applied together, so the totals cover the whole set, not this change alone: core 816 passed, 1 skipped; OpenFeature 63 passed. Prettier and ESLint clean on both files; tsc diagnostics identical to baseline. Mutation checks: removing the isSupportedAttribute guard turns both "omits a name/email of unsupported type" red, and removing the id branch turns the targeting-key test red.
|
|
||
| expect( | ||
| enrichEvaluationContextWithRumUser({ targetingKey: '' }) | ||
| __ddEnrichEvaluationContextWithRumUser({ targetingKey: '' }) |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P3] This assertion and the two at lines 173 and 181 still pass under a mutation the rest of the suite catches.
toEqual ignores properties whose value is undefined, which is exactly the shape a dropped guard produces here. The conversions elsewhere in this file were for that; these two were missed.
Mutation result
Making the typeof name === 'string' and typeof email === 'string' guards at rumIntegration.ts:87 and :90 push unconditionally:
Tests: 16 failed, 14 passed
✓ preserves an explicitly empty targeting key
✓ uses the latest RUM user each time it is called
Both survivors set a RUM user with no name and no email, so the mutant emits name: undefined and toEqual discards it. The suite still catches the mutation through 16 other tests, so this is hygiene rather than a hole.
There are four toEqual here in total — 82, 151, 173, 181 — and these three changes leave only line 82. That one is load-bearing: its RUM user carries nullable: null, which toEqual does compare. Leave it.
Suggested change — three assertions
expect(
__ddEnrichEvaluationContextWithRumUser({ targetingKey: '' })
).toStrictEqual({ targetingKey: '' }); expect(__ddEnrichEvaluationContextWithRumUser({})).toStrictEqual({
targetingKey: 'rum-user-a'
}); expect(__ddEnrichEvaluationContextWithRumUser({})).toStrictEqual({
targetingKey: 'rum-user-b',
plan: 'pro'
});Verification. Applied and passing. Re-ran the same mutant: both survivors now go red.
| // InternalLog may also be absent from the core module, or have verbosity disabled. | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| 'DATADOG: `enrichWithRumUser` could not access the core RUM enrichment helper. Returning the application context unchanged. Check SDK compatibility.' |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P3] The message no longer names the exact export to check in the installed core and in any module mock.
Shortening this in f89be25a was the right instinct — the original was a paragraph in a console. The current text still names enrichWithRumUser, so it is searchable; what went with the prose is __ddEnrichEvaluationContextWithRumUser. The three concrete checks still exist at README:102-106, but nothing in the message points a reader there.
Why it matters for module mocks
Two shapes reach this branch: the resolved core has no callable helper (an older core, or a duplicate install resolving to one — a duplicate where both copies export it does not trigger the guard), and a consumer jest.mock factory that replaced the module and dropped the __dd* exports. Both satisfy the same callability guard and produce the same warning, so the message cannot say which occurred, and I cannot say which is more common.
What the symbol does give you is the thing to look for: it is greppable, and it appears in the README section that covers the mock case — which "Check SDK compatibility" points away from.
Suggested change
console.warn(
'DATADOG: `enrichWithRumUser` could not find a callable `__ddEnrichEvaluationContextWithRumUser` on @datadog/mobile-react-native. Returning the application context unchanged, without RUM values. See the RUM user context section of the @datadog/mobile-react-native-openfeature README.'
);Two additions over the current text: the missing symbol, and a pointer to where the checks live. Still one line in a console.
The exact-string assertion in rumContext.compatibility.test.ts:67 has to move with it. Match the identifier alone, so the test binds to the thing that matters rather than to the surrounding prose:
expect.stringContaining('__ddEnrichEvaluationContextWithRumUser')Verification. Applied; OpenFeature suite 63 passed. Two mutations confirm the assertion binds to the identifier and not the prose: changing find to locate keeps all 10 passing, removing __ddEnrichEvaluationContextWithRumUser from the message turns it red.
| entries.push([key, value]); | ||
| if (!isSupportedAttribute(value)) { | ||
| InternalLog.log( | ||
| `RUM user property "${key}" is not a string, number, or boolean. Omitting it from the evaluation context.`, |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P3] One WARN per rejected property, per call. The count scales with the user's data rather than with the event, so a user with several unsupported extraInfo entries re-logs the same set on every enrichment.
The count is N rejected extraInfo entries times M enrichments. null counts as rejected, so nullable optional fields contribute. Enrichment is explicit — it runs when the application calls the helper, not automatically on context change — so M is however many times the app chooses to enrich. There is no enforced floor, and N varies with the user: after clearUserInfo() the no-user guard at line 53 returns early, so no rejected-extraInfo warnings are emitted at all (other paths in the function, such as the unreadable-context catch at line 40, are unaffected).
It is a new pattern, not a broken convention
To be accurate about precedent, because my first draft of this overstated it: attribute encoding already does both things. It warns per unsupported value (sdk/AttributesEncoding/helpers.ts:96), and it also has a log-once guard, limitReachedWarned, for its limit warning (helpers.ts:158). So per-value warnings are not new here, and a dedupe pattern does exist in the codebase.
What is worth doing is capping this particular path, which is cheap and stateless.
Note the two costs are separate. The message strings are built whether or not logging is enabled, since the argument is evaluated before the call. The console noise only affects applications whose verbosity permits WARN — ERROR verbosity suppresses these entirely (InternalLog.tsx:29).
Suggested change — one line per call instead of one per property
// Report the omitted properties once per call rather than once per property. Repeated
// explicit enrichment calls re-log the same rejected properties, so a RUM user with a few
// unsupported extraInfo entries would otherwise emit the same handful of lines each time.
const omitted: string[] = [];
try {
for (const [key, value] of Object.entries(user.extraInfo ?? {})) {
if (!isSupportedAttribute(value)) {
omitted.push(key);
continue;
}
entries.push([key, value]);
}
if (omitted.length > 0) {
InternalLog.log(
`RUM user properties are not a string, number, or boolean and were omitted from the evaluation context: ${omitted.join(
', '
)}.`,
SdkVerbosity.WARN
);
}Replace the existing try { and everything in it, stopping immediately before } catch (error) {, which is unchanged. That is why the fragment ends open — pasting it as a body replacement instead would leave an unmatched try.
Stateless, so it needs no module-level Set and cannot leak between tests. It caps this path at one line per enrichment regardless of how many properties are rejected, and still names every key. Other warning paths in the function are untouched — a call that also hits, say, the unreadable-application-context catch at line 40 still emits that one too.
I deliberately did not touch the null decision here — 960901a1 defers null support on purpose, and this keeps that signal while removing the repetition.
Two existing assertions move from per-key to the aggregate:
expect(InternalLog.log).toHaveBeenCalledTimes(1);
expect(InternalLog.log).toHaveBeenCalledWith(
'RUM user properties are not a string, number, or boolean and were omitted from the evaluation context: nullable, overridden, removed.',
SdkVerbosity.WARN
);Verification. Applied: core 814 passed, 1 skipped; OpenFeature 63 passed. Mutation: dropping the key collection turns both affected tests red.
| targetingKey?: string | undefined; | ||
| } & Record<string, EvaluationContextValue | undefined>; | ||
|
|
||
| type RumContextEnricher = typeof DatadogSdk.__ddEnrichEvaluationContextWithRumUser; |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P2] This typeof makes the file that exists to degrade at runtime fail at compile time instead, on a core version the peer range does not require.
The cast at line 39 is written structurally, precisely so the symbol's absence is a runtime condition. This line puts the nominal dependency back.
Concrete failure
The peer range is "@datadog/mobile-react-native": "^3.1.0" with no dependencies entry, and the enrichment symbol is not in any published core at or below 3.7.0. A consumer whose TypeScript configuration actually resolves or includes this package's source — rather than its types entry — and who has a satisfying but older core, gets:
src/rumContext.ts(22,45): error TS2339: Property '__ddEnrichEvaluationContextWithRumUser'
does not exist on type 'typeof import(".../mobile-react-native/src/index")'.
Blast radius is bounded: RumContextEnricher is module-local so it is erased from the emitted .d.ts, and Metro strips types at runtime. A plain app running tsc over its own code is unaffected. That is why this is should-fix rather than blocking — though it is blocking if ^3.1.0 is meant as a real promise.
Suggested change
// Declared structurally, not as `typeof DatadogSdk.__dd…`. This file's whole purpose is to
// degrade at runtime when the core export is absent, and a `typeof` reference would make it a
// compile error instead, on a core version the peer range (^3.1.0) does not require.
// The compile-time tie to the real core export lives in the type test instead.
type RumContextEnricher = (
context: EnrichableEvaluationContext
) => Record<string, EvaluationContextValue>;On its own this change loses a check, which I missed first time round: the structural type still rejects some incompatible core signatures, but it stops checking full input-domain compatibility, because the existing fixture only calls core with { email: undefined }. Narrowing core's parameter would no longer fail the build. Restoring it belongs in the fixture, not in shipped code:
// The core helper must keep accepting everything EnrichableEvaluationContext allows.
// `rumContext.ts` describes the enricher structurally, so an older core is a runtime
// condition rather than a compile error. That structural type still rejects some
// incompatible signatures, but it no longer checks the full input domain — this does.
const coreAcceptsEnrichable: (
context: EnrichableEvaluationContext
) => unknown = __ddEnrichEvaluationContextWithRumUser;
return { results, unchangedShape, coreAcceptsEnrichable };Note package.json's files includes src/**, so the fixture does ship — it is outside the production import graph, not absent from the tarball.
Verification. Applied: OpenFeature 63 passed, including the type test; core 814 passed; prettier and ESLint clean; tsc diagnostics unchanged at 36. Two mutations against the fixture: widening EnrichableEvaluationContext.targetingKey to unknown fails it with TS2578, and narrowing core's targetingKey to undefined fails it with TS2322 — the second is the regression the structural type would otherwise have let through.
| } | ||
| ); | ||
|
|
||
| it.each([42, true, false, null, undefined, {}, []])( |
There was a problem hiding this comment.
🤖 Comment from Claude working with Aaron Silverman:
[P3] null and undefined are interchangeable here but not downstream — an application that writes targetingKey: null instead of undefined can get a different offline outcome.
toDdContext maps both to '' via ?? ''. But isEmptyContext (mappers.ts:45-47) tests value === undefined, and the offline provider branches on that before mapping. So the two diverge.
Executed — through the real offline provider, against a user-123 snapshot
{} -> READY
{ targetingKey: undefined } -> READY
{ targetingKey: null } -> ERROR "The evaluation context does not match the offline
precomputed configuration. Serving default values."
undefined is treated as "no override" and re-adopts the embedded context; null is treated as a real context, gets mapped to the anonymous subject, and fails to match.
The divergence is conditional, because isEmptyContext inspects every value: it only appears when the rest of the context is empty. Adding one ordinary attribute collapses it, and against an anonymous snapshot both succeed:
{ targetingKey: null, region: 'us' } -> ERROR { targetingKey: undefined, region: 'us' } -> ERROR
{ targetingKey: null } vs '' snapshot -> READY { targetingKey: undefined } vs '' snapshot -> READY
So: same intent, different result in the narrow case, and nothing documents it.
This is the concrete reason the null row in this it.each is worth a second look: it is not simply "another non-string". null is the one value the two layers disagree about.
Scope
For this warning specifically, the nullish rows are unreachable from either provider — toDdContext applies ?? '' first, so neither null nor undefined arrives here. 42, true and false do reach it, and rumContext.integration.test.ts:184 already asserts that. So the rows are not dead as a group; the two nullish ones are documentation for direct DdFlags callers.
For reference — what the browser SDK does with null
Pinned to local commit cb896583, because upstream has since moved (the provider now enriches context with RUM user defaults by default, and the line numbers below have shifted):
targetingKey: null |
|
|---|---|
wire (fetchConfiguration.ts:79, || '') |
'' |
exposure event (exposureEvent.ts:20, = '') |
null — the default only applies to undefined |
aggregation key (flagEvaluationAggregator.ts:106, || '') |
'' |
So the browser SDK is inconsistent about null too, in its own way: the wire treats it as anonymous while the exposure event lets it through to subject.id. Neither SDK has a stated position. Worth one, if these are ever reconciled.
Suggested change — a note here; the divergence itself is a separate call
// `null` and `undefined` never reach this warning from either provider: toDdContext
// applies `targetingKey ?? ''` first. They are here for direct DdFlags callers.
// Note the two are not equivalent further out — isEmptyContext treats only `undefined`
// as absent, so `{ targetingKey: null }` is a real context to the offline provider.
it.each([42, true, false, null, undefined, {}, []])(Whether isEmptyContext should also treat null as absent is a real decision and bigger than this PR; I would rather flag it than smuggle it in. If you want it tracked, it is a one-line change with an offline test.
marco-saia-datadog
left a comment
marco-saia-datadog
left a comment
There was a problem hiding this comment.
Looks good, thank you for the improvements and additional test coverage! Most of Claude comments are ultra-nit suggestions IMO, I wouldn't let them block the PR
What does this PR do?
Follow-up to #1363 to address review feedback on explicit RUM user enrichment for OpenFeature.
Motivation
Addresses feedback on #1363, including:
enrichRumContexttoenrichWithRumUserto make clear that the RUM user enriches the application's OpenFeature context, rather than enriching RUM itself. Update the public export, diagnostics, documentation, and tests; the helper is unreleased, so no compatibility alias is needed.__dd-prefixed name rather than retaining the alias and adding explanatory comments.EnrichableEvaluationContext, remove the core helper's inaccurate shape-preserving generic, and derive the companion helper type from the core export, with compile-time regression coverage.InternalLogbeing present or SDK verbosity being configured.Tracking: FFL-3312.
Additional Notes
The public helper is now imported as:
enrichRumContexthas not been released, so renaming it toenrichWithRumUseris safe and does not affect users of a released SDK version. No compatibility alias is needed or retained. Enrichment remains explicit and point-in-time; providers do not automatically synchronize with RUM user changes.Validation
git diff --check: passed.Review checklist (to be filled by reviewers)