feat(#1873/playground): Return detailed information on feature toggle evaluation #1839
Conversation
Note: this is very rough and just straight ripped from the nodejs client. It will need a lot of work, but is a good place to start
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
|
|
||
| const prefix = info | ||
| ? info.username | ||
| : `generated-${Math.round(Math.random() * 1000000)}-${process.pid}`; |
There was a problem hiding this comment.
opt.semgrep.node_insecure_random_generator: crypto.pseudoRandomBytes()/Math.random() is a cryptographically weak random number generator.
Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.
When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.
Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]
There was a problem hiding this comment.
@sonatype-lift ignore
There was a problem hiding this comment.
I've recorded this as ignored for this pull request. If you change your mind, just comment @sonatype-lift unignore.
|
|
||
| export default class FlexibleRolloutStrategy extends Strategy { | ||
| private randomGenerator: Function = () => | ||
| `${Math.round(Math.random() * 100) + 1}`; |
There was a problem hiding this comment.
opt.semgrep.node_insecure_random_generator: crypto.pseudoRandomBytes()/Math.random() is a cryptographically weak random number generator.
Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.
When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.
Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]
There was a problem hiding this comment.
@sonatype-lift ignore
There was a problem hiding this comment.
I've recorded this as ignored for this pull request. If you change your mind, just comment @sonatype-lift unignore.
|
|
||
| export default class GradualRolloutRandomStrategy extends Strategy { | ||
| private randomGenerator: Function = () => | ||
| Math.floor(Math.random() * 100) + 1; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
@sonatype-lift ignore
There was a problem hiding this comment.
I've recorded this as ignored for this pull request. If you change your mind, just comment @sonatype-lift unignore.
| } | ||
|
|
||
| function randomString() { | ||
| return String(Math.round(Math.random() * 100000)); |
There was a problem hiding this comment.
opt.semgrep.node_insecure_random_generator: crypto.pseudoRandomBytes()/Math.random() is a cryptographically weak random number generator.
Reply with "@sonatype-lift help" for info about LiftBot commands.
Reply with "@sonatype-lift ignore" to tell LiftBot to leave out the above finding from this PR.
Reply with "@sonatype-lift ignoreall" to tell LiftBot to leave out all the findings from this PR and from the status bar in Github.
When talking to LiftBot, you need to refresh the page to see its response. Click here to get to know more about LiftBot commands.
Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]
There was a problem hiding this comment.
@sonatype-lift ignore
There was a problem hiding this comment.
I've recorded this as ignored for this pull request. If you change your mind, just comment @sonatype-lift unignore.
Note: compared to regular clients, they still fail
This _should_ make it work with custom strategies too 🤞
thomasheartman
force-pushed
the
feat/playground-reasoning
branch
from
4c033e0
to
e071bda
Compare
src/lib/util/feature-evaluator/strategy/application-hostname-strategy.ts
Outdated
Show resolved
Hide resolved
|
@andreas-unleash Suggestion for the whole How about we make the type feature = {
// ... other props
strategies: {
result: boolean | 'unknown',
data: Strategy[]
}
}We keep Actually; doing it this way, we could leave |
This commit changes the format of a playground feature's `strategies`
properties from a list of strategies to an object with properties
`result` and `data`. It looks a bit like this:
```ts
type Strategies = {
result: boolean | "unknown",
data: Strategy[]
}
```
The reason is that this allows us to avoid the breaking change that
was previously suggested in the PR:
`feature.isEnabled` used to be a straight boolean. Then, when we found
out we couldn't necessarily evaluate all strategies (custom strats are
hard!) we changed it to `boolean | 'unevaluated'`. However, this is
confusing on a few levels as the playground results are no longer the
same as the SDK would be, nor are they strictly boolean anymore.
This change reverts the `isEnabled` functionality to what it was
before (so it's always a mirror of what the SDK would show).
The equivalent of `feature.isEnabled === 'unevaluated'` now becomes
`feature.isEnabled && strategy.result === 'unknown'`.
|
@andreas-unleash Further to the comment above, 440cf4c introduces the necessary changes to do that. We haven't discussed this fully, so I would be happy to revert that change if you disagree, but I do think we need an alternative to changing With these changes, the frontend equivalents would be:
Does this sound sensible to you? Let me know your thoughts. |
| result: { | ||
| description: 'Whether this was evaluated as true or false.', | ||
| type: 'boolean', | ||
| }, |
There was a problem hiding this comment.
I haven't followed this PR so feel free to ignore, but extending the constraint schema seems complicated, both here and on the frontend. Maybe we could add stuff next to the constraints and segments instead of extending?
{
constraints: [...], // Regular constraints.
segments: [...], // Regular segments.
results: [
{
constraintIndex: 1, // A constraint ID would be nice.
enabled: true,
},
{
constraintIndex: 1,
segmentId: 2,
enabled: true,
}
// ...
]
}There was a problem hiding this comment.
Hmm, yes and no? We could definitely do that, but I'm not sure I agree. This might just be a personal thing, but it strikes me as natural to put the result right next to what it's evaluating. But you do make a good point that it does add more data models. Without it, we could potentially reuse some of the types from before. That said, from an API consumer view, I think I'd prefer to have the result colocated 🤷🏼
There was a problem hiding this comment.
Yeah, makes sense. I do want to see if we can avoid the constraint: IConstraint | PlaygroundConstraintSchema stuff on the frontend though. Would be nice if the playground-related types were kept there.
There was a problem hiding this comment.
Oh, that's interesting. I haven't seen the frontend implementation, so I don't know. Why do you need that? Couldn't you just use IConstraint? A playgroundconstraintschema should fit into an IConstraint, doesn't it?
There was a problem hiding this comment.
Not sure, haven't looked at it in detail. Might just be because it's IConstraint and not ConstraintSchema. 🤷
| strategies: PlaygroundStrategySchema[]; | ||
| }; | ||
|
|
||
| export default class UnleashClient { |
There was a problem hiding this comment.
Could it make sense to extend the node SDK itself with the ability to explain its results, instead of duplicating the SDK? Sounds like a nice SDK feature. Not sure how much code has been copied over though.
There was a problem hiding this comment.
Ah, actually, yeah. Extending the node SDK might be a better way to go. I hadn't thought of that 🙈 Basically, all the code has been copied over, and then I've removed as much as possible. In theory, extending the node SDK instead could be a (glorious) refactor later. That said, there may be a few things that make it harder. It should be fine to override the isEnabled functionality as we've done here, but overriding strategy implementations might be harder (and necessary).
thomasheartman
changed the title
thomasheartman
changed the title
This change adds detailed information about why a feature evaluates the way it does to the playground response payload (relates to #1873). To achieve this, it:
To make this as useful as possible to the end user, we want to provide as much information as we can. That means that every strategy should be evaluated, regardless of whether any preceding strategies have resolved to
true.Review guide: what to look at and to look for
Woah, this is a massive PR! But it's not as much new code as it actually looks like. Here's a rough outline of what kind of changes are located where:
src/lib/...Practically all changes are under this directory, so let's dig a bit deeper:
dbAs described inline and under "other changes", this is a change that allows the playground client to get IDs on its strategies.
openapiThese are all the new OpenAPI schemas and their direct tests. Feel free to have a look through these. There shouldn't be any executing code here, just definitions and tests.
routes/admin-apiThis is just a minor code adjustment (remove a redundant type annotation).
servicesMinor changes related to adding new services to the playground service and the change allowing you to get strategies with IDs from the store.
changes in the playground service implementation to allow returning the new and more detailed response objects.
types/storesSame change regarding strategies with IDs as mentioned before.
util/feature-evaluatorThis is the ported node SDK client. It's a whooooole lotta code. Most of it is more or less the same, but it's been reworked to evaluate all toggles to return information about evaluation. Probably the largest actual code change in this PR.
Note that the custom client (the
feature-evaluator) is tested against the actual node client to ensure that its evaluations are accurate. There may of course be bugs in the tests, but so far, they seem catch whenever I do something wrong.util/offline-clientThis is the entry point for the feature evaluator.
testA fairly comprehensive test suite based on property based testing. I reckon there's about 50 or so tests mainly enforcing different invariants about the code. There's some unfortunate duplication here and probably some opportunities for refactoring / joining some test suites, so I'm happy to rework this. This is also about a third of all the changed lines in this PR.
Evaluate everything always
One of the main changes from the actual SDK implementation is that we always evaluate all strategies. The SDK will return as soon as it finds a strategy that resolves to
true. This new client, however, keeps going until it has gone over every strategy a feature has.API change
With the added payload, the API gets a significant change. It's further described in the next section, but the biggest takeaway is that each
featurenow gets an additionalstrategyproperty, which contains information about all its strategies.Further, the
isEnabledproperty changes a bit (more about this in the discussions section and in the next section), as it now indicates whether a strategy is enabled based on its strategies and does not necessarily mirror the SDK'sisEnabledproperty. Afeaturealso gains a newisEnabledInCurrentEnvironmentproperty, which tells you whether the feature is enabled in the current environment or not.Payload description
Hidden in the next details element is a full sample response
Full playground response
{ "input": { "environment": "development", "context": { "appName": "my-app", "currentTime": "2022-07-05T12:56:41+02:00", "properties": { "customContextField": "this is one!", "otherCustomField": "3" }, "remoteAddress": "196.0.0.5", "sessionId": "b65e7b23-fec0-4814-a129-0e9861ef18fc", "userId": "username@provider.com", "additionalProp1": "top-level custom context value", "additionalProp2": "top-level custom context value", "additionalProp3": "top-level custom context value" } }, "features": [ { "isEnabled": false, "isEnabledInCurrentEnvironment": true, "strategies": [ { "name": "flexibleRollout", "id": "423cacd9-6afd-4854-aa7b-74517a79c576", "parameters": { "groupId": "aeuohtns", "rollout": "63", "stickiness": "default" }, "result": { "enabled": false, "evaluationStatus": "complete" }, "constraints": [], "segments": [] }, { "name": "default", "id": "7696e9da-2d9d-45f2-bc00-8d6236f4e1bb", "parameters": {}, "result": { "enabled": false, "evaluationStatus": "complete" }, "constraints": [ { "inverted": false, "values": [], "value": "2022-07-29T13:24:53.079Z", "operator": "DATE_AFTER", "contextName": "currentTime", "caseInsensitive": false, "result": false }, { "inverted": false, "values": ["@unleash.ai"], "operator": "STR_ENDS_WITH", "contextName": "userId", "caseInsensitive": true, "result": false } ], "segments": [] } ], "projectId": "default", "variant": { "name": "disabled", "enabled": false }, "name": "aeuohtns", "variants": [] }, { "isEnabled": true, "isEnabledInCurrentEnvironment": true, "strategies": [ { "name": "default", "id": "86ec07d8-3ae0-41cb-91d5-8c23a5928b6c", "parameters": {}, "result": { "enabled": true, "evaluationStatus": "complete" }, "constraints": [ { "inverted": false, "values": ["my-app"], "operator": "IN", "contextName": "appName", "caseInsensitive": false, "result": true } ], "segments": [] } ], "projectId": "create-toggle", "variant": { "name": "a", "enabled": true }, "name": "demo-feature", "variants": [ { "name": "a", "weight": 500, "weightType": "variable", "stickiness": "default", "overrides": [] }, { "name": "b", "weight": 500, "weightType": "variable", "stickiness": "default", "overrides": [] } ] }, { "isEnabled": true, "isEnabledInCurrentEnvironment": true, "strategies": [ { "name": "default", "id": "d28f858f-4c85-4fa7-abcb-e85db9e98ca7", "parameters": {}, "result": { "enabled": true, "evaluationStatus": "complete" }, "constraints": [], "segments": [] } ], "projectId": "default", "variant": { "name": "disabled", "enabled": false }, "name": "eao", "variants": [] }, { "isEnabled": "unevaluated", "isEnabledInCurrentEnvironment": true, "strategies": [ { "name": "custom-strategy-1", "id": "bea60fd6-e1b6-4722-9e69-e1d726481833", "parameters": { "Info": "no" }, "result": { "enabled": "unknown", "reason": "strategy not found", "evaluationStatus": "incomplete" }, "constraints": [], "segments": [] }, { "name": "default", "id": "26f379cd-62ca-433c-afec-cfe030bcd40c", "parameters": {}, "result": { "enabled": false, "evaluationStatus": "complete" }, "constraints": [], "segments": [ { "name": "segment one!", "id": 2, "result": false, "constraints": [ { "values": ["appOne", "playground"], "inverted": false, "operator": "IN", "contextName": "appName", "caseInsensitive": false, "result": false } ] } ] } ], "projectId": "default", "variant": { "name": "one!", "enabled": true }, "name": "feature", "variants": [ { "name": "one!", "weight": 500, "weightType": "variable", "stickiness": "default", "overrides": [] }, { "name": "two!", "weight": 500, "weightType": "variable", "stickiness": "default", "overrides": [] } ] } ] }feature.strategiesEach feature now has a list of
strategiesattached to it. The strategy contains the following properties:nameidparametersresultconstraintssegmentsA sample response is show next. (Note: it's manually edited, so might not be consistent in enabled/disabled states)
{ "strategies": [ { "name": "default", "id": "86ec07d8-3ae0-41cb-91d5-8c23a5928b6c", "parameters": {}, "result": { "enabled": true, "evaluationStatus": "complete" }, "constraints": [ { "inverted": false, "values": ["my-app"], "operator": "IN", "contextName": "appName", "caseInsensitive": false, "result": true } ], "segments": [ { "name": "segment one!", "id": 2, "result": true, "constraints": [ { "values": ["appOne", "playground"], "inverted": false, "operator": "IN", "contextName": "appName", "caseInsensitive": false, "result": true } ] } ] } ] }We'll discuss constraints and segments more in the next section, but first, let's consider the
resultproperty.The
resultproperty tells you about how a strategy evaluates. It is an object containing two subproperties:enabledandevaluationStatus.The
evaluationStatusproperty can be either'complete'or'incomplete'. If it iscomplete, that means that Unleash has evaluated the strategy without issues and thatenabledwill be eithertrueorfalse.evaluationStatuswill (currently) only be'incomplete'if you use a custom strategy that Unleash doesn't recognize. When it is'incomplete',enabledwill be one offalseand'unknown'. Unleash can't know whether a strategy it doesn't know about would be resolve to true or false, so it can never guarantee that it would be true. However, because a custom strategy can have constraints and segments applied, Unleash can guarantee that the strategy will be false if at least one constraint/segment fails.Constraints and segments
A strategy's
constraintsproperty is a list ofconstraintswith an addedresultproperty. This property can be eithertrueorfalse, indicating whether the constraint passed validation.A strategy's
segmentsproperty contains each segment (in full, expanded form) that is applied to the strategy. Each segment has the sameconstraintsproperty as a strategy (that is, with an addedresulton each constraint). Additionally, each segment also has aresultproperty to indicate whether the segment on a whole is true or false.The
feature.isEnabledpropertyBecause we may have strategies that we can't fully evaluate, the
feature.isEnabledproperty is now one oftrue,false, and'unevaluated'. It will only be'unevaluated'if all its strategies are eitherfalseor'unknown'as described above.Further, because we want the playground to be able to distinguish between features that are enabled in the current environment and features that would be enabled in the current environment if you turned it on, the
isEnabledproperty now only takes the feature's strategies into account. In other words, it disregards whether a feature is enabled in the current environment or not. To account for this, I've also introduced thefeature.isEnabledInCurrentEnvironmentprop, which has that information. That means that what's considered enabled by a regular SDK is a combination ofisEnabledInCurrentEnvironmentandisEnabled:feature.isEnabled && feature.isEnabledInCurrentEnvironment === sdkClient.isEnabled(feature.name)More about ramifications of this in the discussions section.
Other changes
Adding
includeStrategyIdsoption insrc/lib/db/feature-toggle-client-store.tsThe feature toggle client store can return strategy ids under certain situations, but it's rare. Because we need strategy IDs for the payload (and because it makes a number of the operations significantly easier), I added it as an optional parameter.
Still left to do
There's still some minor things to do. It's mostly cleanup, but important nonetheless.
Discussions
Is changing
isEnableda breaking change?Technically, yes! There's two different aspects that change it, though, so let's look at both:
Adding the
'unevaluated'optionThis changes the type of the endpoint from strict
booleantoboolean | 'unevaluated'. This could break clients that expect it to be a boolean. Another solution could be to add another property to thefeatureobject, sayevaluationStatus: 'certain' | 'uncertain', which would be'uncertain'when the feature'sisEnabledis'unevaluated'. At the time of writing, this strikes me as a worthwhile change.Relying on
isEnabledInCurrentEnvironmentto get the same results as the SDKThis is a bit confusing and might break clients expecting it to be the same as the SDK. An option would be to flip it on its head and instead use a property
wouldBeEnabledInTheCurrentEnvironmentand leaveisEnabledas the same as the SDK uses. In this case,{ wouldBeEnabledInTheCurrentEnvironment: true, isEnabled: false }would indicate that the SDK says no, but that it could be enabled if you flipped the switch. However, this version introduces an 'impossible' variant:{ wouldBeEnabledInTheCurrentEnvironment: false, isEnabled: true }, so I think it needs a bit more thought.Adding custom strategies
As of today, you can't add custom strategy implementations to Unleash, which means that these can't be fully evaluated. It might be worth thinking about a way to add impls to the instance itself, but that would require a lot of thought.
A potential improvement on the current handling, however, might be to check whether a custom strategy is defined on the unleash instance and differentiating between custom strategies without implementations and custom strategies that it doesn't recognize at all.