Skip to content

fix(openfeature): address RUM context review feedback #1428

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
greghuels merged 11 commits into from
Sep 21, 2026
Merged

fix(openfeature): address RUM context review feedback #1428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
fb00ecd
refactor(flags): remove RUM context enricher export alias
greghuels Sep 18, 2026
1e9b39e
fix(flags): isolate custom-property and identity-field read failures
greghuels Sep 18, 2026
8464aa8
fix(openfeature): accept undefined RUM context attributes
greghuels Sep 18, 2026
5b34621
fix(openfeature): degrade gracefully when RUM enrichment is unavailable
greghuels Sep 18, 2026
fd0194b
docs(openfeature): clarify RUM context setup and isolation
greghuels Sep 18, 2026
bb8e2ea
fix(flags): preserve null RUM context attributes
greghuels Sep 18, 2026
3bb065f
refactor(openfeature): rename helper to enrichWithRumUser
greghuels Sep 18, 2026
960901a
FFL-3312: Defer null RUM attribute enrichment
greghuels Sep 18, 2026
f470ad7
fix(openfeature): address RUM enrichment review findings [FFL-3312]
greghuels Sep 18, 2026
58296e9
fix(openfeature): separate context merging from validation [FFL-3312]
greghuels Sep 18, 2026
f89be25
fix(openfeature): shorten RUM compatibility warning [FFL-3312]
greghuels Sep 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions packages/core/src/flags/__tests__/internal.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* Copyright 2016-Present Datadog, Inc.
*/

import { InternalLog } from '../../InternalLog';
import { SdkVerbosity } from '../../config/types/SdkVerbosity';
import { processEvaluationContext } from '../internal';

jest.mock('../../InternalLog', () => {
Expand All @@ -14,6 +16,43 @@ jest.mock('../../InternalLog', () => {
});

describe('processEvaluationContext', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it.each(['user-1', ''])(
'preserves the string targeting key %p without a warning',
targetingKey => {
expect(processEvaluationContext({ targetingKey })).toStrictEqual({
targetingKey,
attributes: {}
});
expect(InternalLog.log).not.toHaveBeenCalled();
}
);

it.each([42, true, false, null, undefined, {}, []])(

@aarsilv aarsilv Sep 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Comment from Claude working 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.

'uses the anonymous subject for a non-string final targeting key %p',
targetingKey => {
// JavaScript callers can provide values outside the TypeScript contract.
const context = {
targetingKey: targetingKey as never,
attributes: { plan: 'pro' }
};

expect(processEvaluationContext(context)).toStrictEqual({
targetingKey: '',
attributes: { plan: 'pro' }
});
expect(context.targetingKey).toBe(targetingKey);
expect(InternalLog.log).toHaveBeenCalledTimes(1);
expect(InternalLog.log).toHaveBeenCalledWith(
"The evaluation context targetingKey is not a string. Using the anonymous subject ('') instead.",
SdkVerbosity.WARN
);
}
);

it('keeps primitive attributes and drops non-primitive ones', () => {
expect(
processEvaluationContext({
Expand Down
Loading
Loading