-
Notifications
You must be signed in to change notification settings - Fork 66
variation call doesn't return consistent result for flag presence #238
Comments
Short answer: no, that's not how any of the LaunchDarkly SDKs work. It's not a bug, it's the defined behavior. The SDKs do not store user attributes in the way you may be thinking. In your example, when you provided a user with |
The user data that LaunchDarkly accumulates on the service side and displays on the dashboard is a somewhat different story. There, the attributes do persist from previous SDK activity, and they have a behavior (currently— this may not always be the case) of merging together attributes from different calls: so, if you provided only But that is retrospective analytics data that is accumulated in a database on the service side; those stored properties are not available to the SDKs. And that is deliberate, because the overhead of trying to make the SDKs work that way would be massive, and also because it would make flag evaluation behavior non-deterministic (that is, you would not be able to know for sure that if you called |
If the the first call creates the user in LD if it doesn't exist, allows for the flag evaluation logic to run and then return whether the provided flag was assigned to the user, I would expect the second call see that a user with the I get that that's just not how it works and that there's plenty technical limitations on this, I just thinkg that it's a bit counterintuitive. Regardless, thank you for the insight! |
It's not exactly that the SDK "knows not to create it again." The SDK doesn't actually "create" users; it sends analytics data to LaunchDarkly, which includes user attributes. It does have some deduplication logic to avoid sending user data for the same user key every single time it does an evaluation, but that is more of a simple key-caching thing— like, if it has seen the same key within the last few minutes, it will not re-send the data. But that also means that in your example of setting different user attributes at different times for the same key, there is no guarantee that the SDK will actually send both sets of attributes to LD; if you did the one with just a But the other reason your idea doesn't really make sense in the LD model is that you used the phrase "whether it has the passed-in flag". LD does not have a notion of a user having a flag; that is not a property of a user (and, a flag is not necessarily a boolean anyway, it could have many possible values, so it's not just "have" vs. "not have"). Instead, there is just the answer to the question "what do you get if you evaluate this flag for such-and-such user properties." The evaluation logic is rerun each time, based only on the input parameters and the flag configuration— that's what I meant about the behavior being deterministic. If it also depended on some (possibly different) parameters that you had used in a previous Also, if you think about how many flags and users can exist in an LD environment, and the fact that flags can be updated, you can see why it would be very impractical for the SDK to try to remember "whether [this user] has the passed-in flag." There is not just one flag, there could be thousands, and a web application can be servicing requests for thousands of users at any given time. So, if we did not rerun the evaluation logic each time, we would have to be storing potentially millions of combinations of previous For all of those reasons, a "user" in the SDKs does not mean a persistent entity that lives in the SDK. It is a container for parameters to an evaluation, period. |
If the flags are evaluated every time from the given attributes, how can it be deterministic when you have percentage rollout configured? |
Rollouts don't use a random number generator; they compute a one-way hash value from the user key (or other attributes, depending on the flag configuration), and then if you have for instance a 30%-70% division in your rollout, that corresponds to a cutoff of (30% * the maximum hash value). The distribution of hash values is fairly even and unpredictable for any given large set of inputs, so the effect is that the users are assigned pseudo-randomly— but consistently, since the inputs are only the user properties and the flag configuration. |
Another thing to keep in mind, on this general subject of predictable evaluations and the SDKs not retaining user state, is that there is not just one SDK instance evaluating flags. A webapp or service will likely have many instances of the app running, each with its own SDK client— possibly including different services written in different languages— and, if there is any client-side usage of LD (e.g. in mobile apps or front-end JS code), flag evaluations for those are done by the LD service endpoints rather than by the client-side SDK. So if we did something like using a random-number generator for rollouts, or having SDKs remember user properties they had previously gotten from the app code, there would be no way to keep that state consistent across all of the different places where a particular flag might be getting evaluated for a particular user. |
Awesome, I got it! Thank you for taking the time to break things down. I understand the reasoning behind the implementation and I can see how the current approach is a good one. After interacting with the SDK as intended things are working as expected. Again, appreciate your thoroughness 🙏🏽 |
* add mention of singleton usage * update diagnostic event info for OS name, data store type, Node version * standardize linting * disallow window and document * fix null/undef checks * misc linting fixes * inlineUsersInEvents is not an unknown option * drop node-sha1 dependency * don't omit streamInits.failed when it's false * bump request dependency to get security patch; loosen some exact dependencies * remove request package; improve polling cache logic + add test * bump typescript version to fix build error in Node 6 * update @types/node to fix TypeScript check step * lint * make sure we keep polling regardless of whether we got new data * use launchdarkly-eventsource, make stream retry behavior consistent * stream retry delay option should be in seconds & should be included in diagnostics * minor test fix * fix: Throw an error on malformed user-supplied logger * don't call unref() on Redis client; ensure that database integration tests close the store * update Redis driver to major version 3 * add test case * allow redisOpts parameter to be omitted * add logger adapter shim + tests * minor cleanup and comments for ch74741 fix (logger wrapper) * fix proxy tunnel configuration and make sure it's used in streaming * change some string concatenation expressions to use interpolation * feat: upgrade winston (#189) * fix merge * remove support for indirect/patch and indirect/put (#182) * reuse same Promise and same event listeners for all waitForInitialization calls * better docs for waitForInitialization + misc doc cleanup (#184) * update js-eventsource to 1.3.1 for stream parsing bugfix (#185) * fix broken logger format (#186) * retroactively update changelog for bugfix in 5.13.2 release * allow get/getAll Redis queries to be queued if Redis client hasn't yet connected * set stream read timeout * adding the alias functionality (#190) * Removed the guides link * remove monkey-patching of setImmediate * Persist contextKind property during feature and custom event transformations (#194) * add inlineUsersInEvents option in TypeScript * Add support for seed to bucketUser * Add note for incorporating seed into evaluation * Send events when the evaluation is from an experiment * Use seed to evaluate. * Clean up test descriptions * Rename variable to be less confusing * Use ternary to eliminate mutation * Make return signature more consistent * Un-prettier the tests * redis lower bounds bump (#199) * update launchdarkly-js-test-helpers to fix TLS tests (#200) * update js-eventsource to remove vulnerability warning (#201) * add CI jobs for all compatible Node versions * CI fixes * more CI fixes * comment * use default value to simplify config * (6.0 - #1) stop saying we're compatible with Node <12 (#203) * add CI jobs for all compatible Node versions (#202) * (6.0 - #2) remove Redis integration (#204) * allow feature store to be specified as a factory (so it can get our logger) * (6.0 - #3) remove Winston (#205) * remove deprecated things for 6.0 (#206) * update node-cache to 5.x (drops old Node compat) * update semver to 7.x (drops old Node compat) * update uuid to 8.x (Node compat, perf improvements, bugfixes) * update dev dependencies * linter * replace lrucache package with lru-cache (#209) * make yaml dependency optional (#210) * update release metadata to include maintenance branch * remove package-lock.json (#211) * rm prerelease changelog * (big segments #1) add interfaces for big segments (#212) * (big segments #2) add all components for big segments except evaluation (#213) * (big segments #3) implement big segments in flag evaluation (#214) * (big segments #4) add standard test suite for big segment store tests + refactor feature store tests (#215) * move new interfaces to a module instead of a namespace (#216) * fix TS export of CachingStoreWrapper * use Releaser v2 config * fix overly specific test expectation that breaks in Node 17 * Initial work on FlagBuilder (#219) * Add TestData factory(with some dummy methods); Initial work on FlagBuilder * fixed indentation and linter errors; fixed an error in update; fixed incorrect test label * fixed typo in TestData store * converted boolean variation constants to be file variables instead of class variables Co-authored-by: charukiewicz <christian@foxhound.systems> Co-authored-by: belevy <ben@foxhound.systems> * implemented FlagRuleBuilder; added .build() methods to FlagBuilder/FlagRuleBuilder and changed tests to avoid using private interface * converted _targets to be Map instead of object literal; changed variationForBoolean to be a module-scoped function instead class-scoped * Implement stream processor(data source) interface for test data * Add TestData to index.js and write out the types for TestData and friends * added testdata documentation to index.d.ts; fix linter errors; changed flag default behavior to create boolean flag * Fix the interface file: reindented to 2 spaces, corrected definition of functions from properties to functions in interfaces; corrected issues in JSDoc comments * modify tests to fix capitalization and actually test the test datasource works as an LDClient updateProcessor. * Fix linter error on defaulted callback * explicitly enable JSDOM types in TypeScript build to avoid errors when jsdom is referenced for some reason * capitalize Big Segments in docs & logs * documentation comment fixes for TestData * pin TypeScript to 4.4.x * move TestData and FIleDataSource to integrations module * lint * rename types used by TestData for clarity (#229) * use varargs semantics for TestFlagBuilder.variations() and add it to the TS interface (#230) * don't ever use for...in (#232) * don't ever use for...in * add null guard * bump launchdarkly-eventsource dependency for sc-136154 fix * use TestData in our own tests (#231) * use TestData in our own tests * update TS interface * lint * typo * fix allFlagsState behavior regarding experimentation * lint * allow "secondary" to be referenced in clauses * don't throw an exception for non-string in semver comparison * correctly handle "client not ready" condition in allFlagsState * lint * Flags with a version of 0 reported as 'unknown' in summary events. (#239) * Initial draft of typescript types. (#236) * Implement attribute reference support. * implement contract test service, not including big segments (#242) Co-authored-by: Eli Bishop <eli@launchdarkly.com> * Implement Application tags for the node SDK. (#241) * update js-eventsource to 1.4.4 for security fix * remove package-lock.json * adjust test expectation about error message to work in recent Node versions * #3 Add context filtering and legacy to single kind conversion. (#238) Co-authored-by: Eli Bishop <eli@launchdarkly.com> * #4 Switch from user to context for events. (#244) Co-authored-by: Eli Bishop <eli@launchdarkly.com> * #5 Rlamb/sc 142950/implement u2c evaluation (#248) Co-authored-by: Eli Bishop <eli@launchdarkly.com> * #6 Rlamb/sc 145767/attribute reference improvements (#250) Co-authored-by: Eli Bishop <eli@launchdarkly.com> * #7 Rlamb/sc 146614/do not support bucketby for experiments (#251) Co-authored-by: Eli Bishop <eli@launchdarkly.com> * #8 Rlamb/sc 147263/treat cyclic segements as errors (#252) Co-authored-by: Eli Bishop <eli@launchdarkly.com> * Do not use the secondary key for experiments. (#256) * Resolve issues with V2 test harness. (#258) * Adds link to Relay Proxy docs * Update index.d.ts Co-authored-by: Eli Bishop <eli@launchdarkly.com> * ensure setTimeout task is cleared when polling is stopped * fix some flaky tests using async blocking logic * rm unused * simplify polling implementation using setInterval * Update the test data source for U2C. (#257) * use newer js-test-helpers for async tests * add request number to timeout message * Enforce 64 character limit for application tag values. (#263) * Changed transient back to anonymous. (#264) * Fixed operator field key name in TestDataRuleBuilder (#246) * Do not set `inExperiment` if there is not a context for the specified kind. (#266) * [sc-160948] Switch to partial URL encoding. (#265) * Update event schema version. (#267) * [sc-171125] Do now allow indexing into an array with an attribute reference. (#268) * [sc-174033] Remove support for secondary. (#269) * Treat 'kind' and '/kind' the same. (#270) * [sc-176598] Update node U2C with latest changes from main. (#272) * [sc-176599] Update documentation for privateAttributes _meta attribute of contexts. (#271) * Remove copy/paste error. (#274) * [sc-177983] Add support for executing old style user tests. (#275) * Update release metadata. * Do not generate events for bad contexts. (#277) Co-authored-by: Yusinto Ngadiman <yusinto@gmail.com> * Handle nested segment dependencies. (#278) Co-authored-by: LaunchDarklyCI <dev@launchdarkly.com> Co-authored-by: Eli Bishop <eli@launchdarkly.com> Co-authored-by: Maxwell Gerber <maxwell.gerber@mulesoft.com> Co-authored-by: Chris West <solo-github@goeswhere.com> Co-authored-by: Ben Woskow <48036130+bwoskow-ld@users.noreply.github.com> Co-authored-by: Mike Zorn <mike@launchdarkly.com> Co-authored-by: Ben Woskow <bwoskow@launchdarkly.com> Co-authored-by: Robert J. Neal <rneal@launchdarkly.com> Co-authored-by: Ben Levy <benjaminlevy007@gmail.com> Co-authored-by: charukiewicz <christian@foxhound.systems> Co-authored-by: belevy <ben@foxhound.systems> Co-authored-by: charukiewicz <charukiewicz@protonmail.com> Co-authored-by: LaunchDarklyReleaseBot <launchdarklyreleasebot@launchdarkly.com> Co-authored-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Co-authored-by: Ember Stevens <ember.stevens@launchdarkly.com> Co-authored-by: Ember Stevens <79482775+ember-stevens@users.noreply.github.com> Co-authored-by: Yusinto Ngadiman <yusinto@gmail.com>
* update diagnostic event info for OS name, data store type, Node version * standardize linting * disallow window and document * fix null/undef checks * misc linting fixes * inlineUsersInEvents is not an unknown option * drop node-sha1 dependency * don't omit streamInits.failed when it's false * bump request dependency to get security patch; loosen some exact dependencies * remove request package; improve polling cache logic + add test * bump typescript version to fix build error in Node 6 * update @types/node to fix TypeScript check step * lint * make sure we keep polling regardless of whether we got new data * use launchdarkly-eventsource, make stream retry behavior consistent * stream retry delay option should be in seconds & should be included in diagnostics * minor test fix * fix: Throw an error on malformed user-supplied logger * don't call unref() on Redis client; ensure that database integration tests close the store * update Redis driver to major version 3 * add test case * allow redisOpts parameter to be omitted * add logger adapter shim + tests * minor cleanup and comments for ch74741 fix (logger wrapper) * fix proxy tunnel configuration and make sure it's used in streaming * change some string concatenation expressions to use interpolation * feat: upgrade winston (#189) * fix merge * remove support for indirect/patch and indirect/put (#182) * reuse same Promise and same event listeners for all waitForInitialization calls * better docs for waitForInitialization + misc doc cleanup (#184) * update js-eventsource to 1.3.1 for stream parsing bugfix (#185) * fix broken logger format (#186) * retroactively update changelog for bugfix in 5.13.2 release * allow get/getAll Redis queries to be queued if Redis client hasn't yet connected * set stream read timeout * adding the alias functionality (#190) * Removed the guides link * remove monkey-patching of setImmediate * Persist contextKind property during feature and custom event transformations (#194) * add inlineUsersInEvents option in TypeScript * Add support for seed to bucketUser * Add note for incorporating seed into evaluation * Send events when the evaluation is from an experiment * Use seed to evaluate. * Clean up test descriptions * Rename variable to be less confusing * Use ternary to eliminate mutation * Make return signature more consistent * Un-prettier the tests * redis lower bounds bump (#199) * update launchdarkly-js-test-helpers to fix TLS tests (#200) * update js-eventsource to remove vulnerability warning (#201) * add CI jobs for all compatible Node versions * CI fixes * more CI fixes * comment * use default value to simplify config * (6.0 - #1) stop saying we're compatible with Node <12 (#203) * add CI jobs for all compatible Node versions (#202) * (6.0 - #2) remove Redis integration (#204) * allow feature store to be specified as a factory (so it can get our logger) * (6.0 - #3) remove Winston (#205) * remove deprecated things for 6.0 (#206) * update node-cache to 5.x (drops old Node compat) * update semver to 7.x (drops old Node compat) * update uuid to 8.x (Node compat, perf improvements, bugfixes) * update dev dependencies * linter * replace lrucache package with lru-cache (#209) * make yaml dependency optional (#210) * update release metadata to include maintenance branch * remove package-lock.json (#211) * rm prerelease changelog * (big segments #1) add interfaces for big segments (#212) * (big segments #2) add all components for big segments except evaluation (#213) * (big segments #3) implement big segments in flag evaluation (#214) * (big segments #4) add standard test suite for big segment store tests + refactor feature store tests (#215) * move new interfaces to a module instead of a namespace (#216) * fix TS export of CachingStoreWrapper * use Releaser v2 config * fix overly specific test expectation that breaks in Node 17 * Initial work on FlagBuilder (#219) * Add TestData factory(with some dummy methods); Initial work on FlagBuilder * fixed indentation and linter errors; fixed an error in update; fixed incorrect test label * fixed typo in TestData store * converted boolean variation constants to be file variables instead of class variables Co-authored-by: charukiewicz <christian@foxhound.systems> Co-authored-by: belevy <ben@foxhound.systems> * implemented FlagRuleBuilder; added .build() methods to FlagBuilder/FlagRuleBuilder and changed tests to avoid using private interface * converted _targets to be Map instead of object literal; changed variationForBoolean to be a module-scoped function instead class-scoped * Implement stream processor(data source) interface for test data * Add TestData to index.js and write out the types for TestData and friends * added testdata documentation to index.d.ts; fix linter errors; changed flag default behavior to create boolean flag * Fix the interface file: reindented to 2 spaces, corrected definition of functions from properties to functions in interfaces; corrected issues in JSDoc comments * modify tests to fix capitalization and actually test the test datasource works as an LDClient updateProcessor. * Fix linter error on defaulted callback * explicitly enable JSDOM types in TypeScript build to avoid errors when jsdom is referenced for some reason * capitalize Big Segments in docs & logs * documentation comment fixes for TestData * pin TypeScript to 4.4.x * move TestData and FIleDataSource to integrations module * lint * rename types used by TestData for clarity (#229) * use varargs semantics for TestFlagBuilder.variations() and add it to the TS interface (#230) * don't ever use for...in (#232) * don't ever use for...in * add null guard * bump launchdarkly-eventsource dependency for sc-136154 fix * use TestData in our own tests (#231) * use TestData in our own tests * update TS interface * lint * typo * fix allFlagsState behavior regarding experimentation * lint * allow "secondary" to be referenced in clauses * don't throw an exception for non-string in semver comparison * correctly handle "client not ready" condition in allFlagsState * lint * Flags with a version of 0 reported as 'unknown' in summary events. (#239) * Initial draft of typescript types. (#236) * Implement attribute reference support. * implement contract test service, not including big segments (#242) Co-authored-by: Eli Bishop <eli@launchdarkly.com> * Implement Application tags for the node SDK. (#241) * update js-eventsource to 1.4.4 for security fix * remove package-lock.json * adjust test expectation about error message to work in recent Node versions * #3 Add context filtering and legacy to single kind conversion. (#238) Co-authored-by: Eli Bishop <eli@launchdarkly.com> * #4 Switch from user to context for events. (#244) Co-authored-by: Eli Bishop <eli@launchdarkly.com> * #5 Rlamb/sc 142950/implement u2c evaluation (#248) Co-authored-by: Eli Bishop <eli@launchdarkly.com> * #6 Rlamb/sc 145767/attribute reference improvements (#250) Co-authored-by: Eli Bishop <eli@launchdarkly.com> * #7 Rlamb/sc 146614/do not support bucketby for experiments (#251) Co-authored-by: Eli Bishop <eli@launchdarkly.com> * #8 Rlamb/sc 147263/treat cyclic segements as errors (#252) Co-authored-by: Eli Bishop <eli@launchdarkly.com> * Do not use the secondary key for experiments. (#256) * Resolve issues with V2 test harness. (#258) * Adds link to Relay Proxy docs * Update index.d.ts Co-authored-by: Eli Bishop <eli@launchdarkly.com> * ensure setTimeout task is cleared when polling is stopped * fix some flaky tests using async blocking logic * rm unused * simplify polling implementation using setInterval * Update the test data source for U2C. (#257) * use newer js-test-helpers for async tests * add request number to timeout message * Enforce 64 character limit for application tag values. (#263) * Changed transient back to anonymous. (#264) * Fixed operator field key name in TestDataRuleBuilder (#246) * Do not set `inExperiment` if there is not a context for the specified kind. (#266) * [sc-160948] Switch to partial URL encoding. (#265) * Update event schema version. (#267) * [sc-171125] Do now allow indexing into an array with an attribute reference. (#268) * [sc-174033] Remove support for secondary. (#269) * Treat 'kind' and '/kind' the same. (#270) * [sc-176598] Update node U2C with latest changes from main. (#272) * [sc-176599] Update documentation for privateAttributes _meta attribute of contexts. (#271) * Remove copy/paste error. (#274) * [sc-177983] Add support for executing old style user tests. (#275) * Update release metadata. * Do not generate events for bad contexts. (#277) Co-authored-by: Yusinto Ngadiman <yusinto@gmail.com> * Handle nested segment dependencies. (#278) * fix: bump async dependencies --------- Co-authored-by: Eli Bishop <eli@launchdarkly.com> Co-authored-by: LaunchDarklyCI <dev@launchdarkly.com> Co-authored-by: Maxwell Gerber <maxwell.gerber@mulesoft.com> Co-authored-by: Chris West <solo-github@goeswhere.com> Co-authored-by: Ben Woskow <48036130+bwoskow-ld@users.noreply.github.com> Co-authored-by: Mike Zorn <mike@launchdarkly.com> Co-authored-by: Ben Woskow <bwoskow@launchdarkly.com> Co-authored-by: Robert J. Neal <rneal@launchdarkly.com> Co-authored-by: Ben Levy <benjaminlevy007@gmail.com> Co-authored-by: charukiewicz <christian@foxhound.systems> Co-authored-by: belevy <ben@foxhound.systems> Co-authored-by: charukiewicz <charukiewicz@protonmail.com> Co-authored-by: LaunchDarklyReleaseBot <launchdarklyreleasebot@launchdarkly.com> Co-authored-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Co-authored-by: Ember Stevens <ember.stevens@launchdarkly.com> Co-authored-by: Ember Stevens <79482775+ember-stevens@users.noreply.github.com> Co-authored-by: Yusinto Ngadiman <yusinto@gmail.com> Co-authored-by: Louis Chan <lchan@launchdarkly.com> Co-authored-by: Louis Chan <91093020+louis-launchdarkly@users.noreply.github.com>
Describe the bug
We have a flag with a
contains
rule in theemail
attribute. Whenclient.variation
is called the first time withkey
, andemail
, it properly evaluates whether the user should belong to that flag, and returnstrue
if it matches. Later on the same variation call is made with the samekey
but with withoutemail
, and it returnsfalse
for the same flag for whichtrue
was returned when bothkey
andemail
were provided.To reproduce
Call
client.variation
withuser.key
anduser.email
to evaluate a flag that should resolve totrue
,true
will be the response.Call
client.variation
with the sameuser.key
as before but withoutuser.email
,false
will be the response.Expected behavior
client.variation
to identify the same user if less attributes were provided but the samekey
was sent.SDK version
launchdarkly-node-server-sdk: 6.2.3
Language version, developer tools
Node v16.13.2
OS/platform
WSL2 | Ubuntu 18.04.6
The text was updated successfully, but these errors were encountered: