-
Notifications
You must be signed in to change notification settings - Fork 64
feat(openfeature): add explicit RUM context enrichment #1363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ee54d86
14606c5
5799d9e
007073a
b3e773f
f38accb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| /* | ||
| * 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 { UserInfoSingleton } from '../../sdk/UserInfoSingleton/UserInfoSingleton'; | ||
| import { enrichEvaluationContextWithRumUser } from '../rumIntegration'; | ||
|
|
||
| describe('enrichEvaluationContextWithRumUser', () => { | ||
| beforeEach(() => { | ||
| UserInfoSingleton.reset(); | ||
| }); | ||
|
|
||
| it('normalizes the application context when no RUM user is available', () => { | ||
| const context = { | ||
| targetingKey: 'explicit-user', | ||
| email: undefined | ||
| }; | ||
|
|
||
| expect(enrichEvaluationContextWithRumUser(context)).toStrictEqual({ | ||
| targetingKey: 'explicit-user' | ||
| }); | ||
| expect(context).toStrictEqual({ | ||
| targetingKey: 'explicit-user', | ||
| email: undefined | ||
| }); | ||
| }); | ||
|
|
||
| it('adds flat primitive RUM user properties and lets explicit context win', () => { | ||
| UserInfoSingleton.getInstance().setUserInfo({ | ||
| id: 'rum-user', | ||
| name: 'RUM Name', | ||
| email: 'rum@example.com', | ||
| extraInfo: { | ||
| company_name: 'Example, Inc.', | ||
| age: 42, | ||
| active: true, | ||
| nullable: null, | ||
| profile: { plan: 'enterprise' }, | ||
| roles: ['admin'] | ||
| } | ||
| }); | ||
|
|
||
| expect( | ||
| enrichEvaluationContextWithRumUser({ | ||
| targetingKey: 'explicit-user', | ||
| email: 'explicit@example.com', | ||
| request_attribute: 'request-value' | ||
| }) | ||
| ).toEqual({ | ||
| targetingKey: 'explicit-user', | ||
| name: 'RUM Name', | ||
| email: 'explicit@example.com', | ||
| company_name: 'Example, Inc.', | ||
| age: 42, | ||
| active: true, | ||
| request_attribute: 'request-value' | ||
| }); | ||
| }); | ||
|
|
||
| it('preserves an explicitly empty targeting key', () => { | ||
| UserInfoSingleton.getInstance().setUserInfo({ id: 'rum-user' }); | ||
|
|
||
| expect( | ||
| enrichEvaluationContextWithRumUser({ targetingKey: '' }) | ||
| ).toEqual({ targetingKey: '' }); | ||
| }); | ||
|
|
||
| it('uses explicitly undefined fields to remove RUM defaults', () => { | ||
| UserInfoSingleton.getInstance().setUserInfo({ | ||
| id: 'rum-user', | ||
| email: 'rum@example.com', | ||
| extraInfo: { plan: 'pro' } | ||
| }); | ||
|
|
||
| expect( | ||
| enrichEvaluationContextWithRumUser({ | ||
| targetingKey: undefined, | ||
| email: undefined, | ||
| plan: undefined, | ||
| request_attribute: 'request-value' | ||
| }) | ||
| ).toStrictEqual({ request_attribute: 'request-value' }); | ||
| }); | ||
|
|
||
| it('uses the latest RUM user each time it is called', () => { | ||
| UserInfoSingleton.getInstance().setUserInfo({ id: 'rum-user-a' }); | ||
| expect(enrichEvaluationContextWithRumUser({})).toEqual({ | ||
| targetingKey: 'rum-user-a' | ||
| }); | ||
|
|
||
| UserInfoSingleton.getInstance().setUserInfo({ | ||
| id: 'rum-user-b', | ||
| extraInfo: { plan: 'pro' } | ||
| }); | ||
| expect(enrichEvaluationContextWithRumUser({})).toEqual({ | ||
| targetingKey: 'rum-user-b', | ||
| plan: 'pro' | ||
| }); | ||
| }); | ||
|
|
||
| it('uses application context when RUM user properties cannot be read', () => { | ||
| const extraInfo = Object.defineProperty({}, 'broken', { | ||
| enumerable: true, | ||
| get: () => { | ||
| throw new Error('cannot read user property'); | ||
| } | ||
| }); | ||
| UserInfoSingleton.getInstance().setUserInfo({ | ||
| id: 'rum-user', | ||
| extraInfo | ||
| }); | ||
| const context = { targetingKey: 'explicit-user' }; | ||
|
|
||
| expect(enrichEvaluationContextWithRumUser(context)).toStrictEqual( | ||
| context | ||
| ); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| /* | ||
| * 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 { UserInfoSingleton } from '../sdk/UserInfoSingleton/UserInfoSingleton'; | ||
|
|
||
| type FlatEvaluationContext = Record<string, unknown> & { | ||
| targetingKey?: string; | ||
| }; | ||
|
|
||
| /** | ||
| * Add the current RUM user to an OpenFeature-shaped evaluation context. | ||
| * | ||
| * @internal Used by the explicit helper in the Datadog OpenFeature package. This is a point-in-time | ||
| * read; it does not synchronize OpenFeature when the RUM user changes. RUM values provide defaults; | ||
| * fields explicitly supplied by the application remain authoritative. An explicitly undefined | ||
| * field removes the corresponding RUM default and is omitted from the effective context. | ||
| */ | ||
| export const enrichEvaluationContextWithRumUser = < | ||
| T extends FlatEvaluationContext | ||
| >( | ||
| context: T | ||
| ): T => { | ||
| const effectiveContext = new Map(getRumContextEntries()); | ||
|
|
||
| try { | ||
| for (const [key, value] of Object.entries(context)) { | ||
| if (value === undefined) { | ||
| effectiveContext.delete(key); | ||
|
greghuels marked this conversation as resolved.
|
||
| } else { | ||
| effectiveContext.set(key, value); | ||
| } | ||
| } | ||
|
|
||
| return Object.fromEntries(effectiveContext) as T; | ||
| } catch { | ||
| return context; | ||
| } | ||
| }; | ||
|
|
||
| const getRumContextEntries = (): Array<[string, unknown]> => { | ||
| try { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Comment from Claude working with Aaron Silverman: [P1] Read
The existing test cannot distinguish the two outcomesEvery result quoted below is one I ran locally against this head, not something you can read off the diff.
The outer catch hands back an object that throws at the next hopIn the one case I could construct for it — a throwing getter on the application context — it returns the input by reference, still carrying 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 thisSplitting the try blocks the obvious way moves the Suggested change — complete, including importsimport { 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, this is an edge case worth addressing. I'll update. |
||
| const user = UserInfoSingleton.getInstance().getUserInfo(); | ||
| if (!user) { | ||
| return []; | ||
| } | ||
|
|
||
| const entries: Array<[string, unknown]> = []; | ||
|
|
||
| for (const [key, value] of Object.entries(user.extraInfo ?? {})) { | ||
| if (isSupportedAttribute(value)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Comment from Claude working with Aaron Silverman: [P2] Consider skipping
What I measuredEvery result quoted below is one I ran locally against this head, not something you can read off the diff. The last line stops at There is a precedent for the type check specifically: Suggested change — a reserved-key list checked while promotingimport { 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 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: |
||
| entries.push([key, value]); | ||
| } | ||
| } | ||
|
|
||
| if (typeof user.name === 'string') { | ||
| entries.push(['name', user.name]); | ||
| } | ||
| if (typeof user.email === 'string') { | ||
| entries.push(['email', user.email]); | ||
| } | ||
| if (typeof user.id === 'string') { | ||
| entries.push(['targetingKey', user.id]); | ||
| } | ||
|
|
||
| return entries; | ||
| } catch { | ||
| return []; | ||
| } | ||
| }; | ||
|
|
||
| const isSupportedAttribute = ( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, it looks like custom RUM attributes can contain null, I'll update
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following up on my earlier reply: To keep #1428 scoped, I've removed the added null support from 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| value: unknown | ||
| ): value is string | number | boolean => { | ||
| return ( | ||
| typeof value === 'string' || | ||
| typeof value === 'number' || | ||
| typeof value === 'boolean' | ||
| ); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -37,6 +37,7 @@ import { | |||||||||||||||||||||||||
| configurationToString | ||||||||||||||||||||||||||
| } from './flags/configuration'; | ||||||||||||||||||||||||||
| import type { ParsedFlagsConfiguration } from './flags/configuration'; | ||||||||||||||||||||||||||
| import { enrichEvaluationContextWithRumUser } from './flags/rumIntegration'; | ||||||||||||||||||||||||||
| import type { | ||||||||||||||||||||||||||
| FlagsConfiguration, | ||||||||||||||||||||||||||
| FlagDetails, | ||||||||||||||||||||||||||
|
|
@@ -112,7 +113,8 @@ export { | |||||||||||||||||||||||||
| DatadogTracingIdentifier, | ||||||||||||||||||||||||||
| DatadogTracingContext, | ||||||||||||||||||||||||||
| DdBabelInteractionTracking, | ||||||||||||||||||||||||||
| __ddExtractText | ||||||||||||||||||||||||||
| __ddExtractText, | ||||||||||||||||||||||||||
| enrichEvaluationContextWithRumUser as __ddEnrichEvaluationContextWithRumUser | ||||||||||||||||||||||||||
|
btthomas marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 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 — Where the repo already does this — seven patterns, with links
The second row is the load-bearing one: three of those four carry the "FOR INTERNAL USE ONLY" block ( Separately, So the placement is normal; the alias is the new partindex.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:
The marker travels one subpath further: What the prefix does not buyEmitted with the repo's own tsconfig:
And the channel itself looks rightCore has no public on-demand getter for the current RUM user: A subpath was the alternative, with history worth knowing: 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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"
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, good callout. I'll open a follow-up PR with some changes. Appreciate you flagging it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is addressed in #1428 |
||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| export type { | ||||||||||||||||||||||||||
| Timestamp, | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,6 +67,60 @@ After completing this setup, your app is ready for flag evaluation with OpenFeat | |
|
|
||
| > **Note**: Sending flag evaluation data to Datadog is automatically enabled when using the Feature Flags SDK. Provide `rumIntegrationEnabled` and `trackExposures` parameters to the `DdFlags.enable()` call to configure. | ||
|
|
||
| ### RUM user context | ||
|
|
||
| Use `enrichRumContext()` when you explicitly want to use the current RUM user as part of an | ||
| OpenFeature evaluation context. Neither Datadog OpenFeature provider enriches context automatically. | ||
| This keeps context changes visible through OpenFeature and avoids changing flag assignments unless | ||
| your application opts in. | ||
|
|
||
| The helper maps the RUM user ID to `targetingKey`. It maps `name`, `email`, and flat string, number, | ||
| or boolean `extraInfo` properties to evaluation attributes. Values in the application context take | ||
| precedence over RUM values, so you can use a different targeting key (for example, a device or session | ||
| ID). An application field set to `undefined` removes the corresponding RUM value and is omitted from | ||
| the returned context. Nested RUM user properties are not included. | ||
|
|
||
| Keep the original application-owned context and enrich it before passing it to OpenFeature: | ||
|
|
||
| ```tsx | ||
| import { | ||
| DatadogOpenFeatureProvider, | ||
| enrichRumContext | ||
| } from '@datadog/mobile-react-native-openfeature'; | ||
|
|
||
| const applicationContext = { | ||
| region: 'us-east-1' | ||
| }; | ||
|
|
||
| await DdSdkReactNative.setUserInfo({ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Comment from Claude working with Aaron Silverman: [P1] Neither snippet compiles as written, and the global Both blocks use What I measured — tsc on the exact extracted blocksEvery result quoted below is one I ran locally against this head, not something you can read off the diff. 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 The logout gap, measured
The global context reaches other providers — and the fix needs a consumer changeFrom the installed I kept the main snippets on the global context, because that is what the rest of the README does and const DATADOG_DOMAIN = 'datadog';
await OpenFeature.setContext(
DATADOG_DOMAIN,
enrichRumContext(applicationContext)
);
await OpenFeature.setProviderAndWait(
DATADOG_DOMAIN,
new DatadogOpenFeatureProvider()
);and at line 148, It has to be all of it, not just registration: every later One more line worth adding: numeric attributes differ by platform
Suggested replacement snippets — both compile clean as postedimport { 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));
}; |
||
| id: 'user-123', | ||
| email: 'user@example.com', | ||
| extraInfo: { company_name: 'Example, Inc.' } | ||
| }); | ||
|
|
||
| await OpenFeature.setContext(enrichRumContext(applicationContext)); | ||
| await OpenFeature.setProviderAndWait(new DatadogOpenFeatureProvider()); | ||
| ``` | ||
|
|
||
| `enrichRumContext()` reads the RUM user when it is called; it does not establish a live connection | ||
| between RUM and OpenFeature. After a login, logout, or account switch, update the RUM user and enrich | ||
| the original application-owned context again: | ||
|
|
||
| ```tsx | ||
| await DdSdkReactNative.setUserInfo(newUser); | ||
| await OpenFeature.setContext(enrichRumContext(applicationContext)); | ||
| ``` | ||
|
|
||
| Do not pass `OpenFeature.getContext()` back to `enrichRumContext()`. That context already contains | ||
| values from the previous RUM user, so those values would be treated as application-owned overrides | ||
| and could prevent the new RUM user from replacing them. Retain the original application context | ||
| separately, as shown above. | ||
|
|
||
| `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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 Comment from Claude working with Aaron Silverman: [P1] Consider replacing this forward-reference with a direct warning about 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 silentEvery result quoted below is one I ran locally against this head, not something you can read off the diff. Snapshot precomputed for Loud — enrich before registering, with a RUM email the snapshot does not carry: Silent — a matching context at startup, then one unrelated The second shape is the one worth documenting: the 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| follow the precomputed configuration context requirements below. | ||
|
|
||
| ### Using the OpenFeature React SDK | ||
|
|
||
| For complete details on using the OpenFeature React SDK, including flag evaluation, evaluation context management, and advanced setup options, see the OpenFeature React SDK [documentation][1]. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| /* | ||
| * 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 { DatadogOpenFeatureProvider, enrichRumContext } from '../index'; | ||
|
|
||
| const mockFlagsClient = { | ||
| setEvaluationContext: jest.fn(() => Promise.resolve()) | ||
| }; | ||
|
|
||
| jest.mock('@datadog/mobile-react-native', () => ({ | ||
| DdFlags: { getClient: jest.fn(() => mockFlagsClient) }, | ||
| configurationFromString: jest.fn() | ||
| })); | ||
|
|
||
| describe('RUM context core compatibility', () => { | ||
| it('keeps the provider usable with a core version that predates enrichment', async () => { | ||
| const provider = new DatadogOpenFeatureProvider(); | ||
|
|
||
| await provider.initialize({ | ||
| targetingKey: 'explicit-user', | ||
| plan: 'pro' | ||
| }); | ||
|
|
||
| expect(mockFlagsClient.setEvaluationContext).toHaveBeenCalledWith({ | ||
| targetingKey: 'explicit-user', | ||
| attributes: { plan: 'pro' } | ||
| }); | ||
| }); | ||
|
|
||
| it('reports incompatible package versions when enrichment is requested', () => { | ||
| expect(() => enrichRumContext({})).toThrow( | ||
| 'requires compatible versions of @datadog/mobile-react-native and @datadog/mobile-react-native-openfeature' | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 Comment from Claude working with Aaron Silverman:
[P1] Consider
toStrictEqualfor these four assertions, and characterization tests for three branches that currently survive mutation.toEqualignores properties whose value isundefined, which is the shape a droppedtypeofguard here would produce — so core's own suite cannot see that regression today. The suite is otherwise sharp: a positive control I ran reddened 14 of the 15 new tests. The four calls sit at lines 51, 67, 89 and 97, across three tests.What I measured — mutations over both projects, 58 suites / 813 tests baseline
Every result quoted below is one I ran locally against this head, not something you can read off the diff. "After" means with the
toStrictEqualchange and the tests below added.typeof user.email === 'string'guardtry/catchtypeof user.id === 'string'guardextraInfoafter the top-level fieldsTo be precise about what these tests do: all four are characterization tests. They pass against this head and detect the mutations listed above — not one-to-one, though: the missing-id test also fires on the email-guard mutation, because that one leaves an unexpected
undefined-valued property behind. They pin current behaviour rather than assert a fix. That is the point; the three GREEN rows are branches nothing currently holds in place.A smaller note on the compatibility suite
rumContext.compatibility.test.ts:19-31does assert something real — that the provider still initializes and forwards context against a reduced core mock. It just does not cover anything this PR changes:provider.tsis byte-identical to the base commit. It also stayed green under the positive control that reddened 14 of the other 15 new tests, so it will not move if enrichment regresses. Worth retitling so it does not read as coverage for the new helper.Suggested tests
Both imports already exist in the file;
contextis declared locally.The last one matters because the README documents a logout flow and no test exercises
clearUserInfotoday — the existing tests useUserInfoSingleton.reset().There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I'll update!