feat: Create a Growthbook server side provider #938
Conversation
msamper
changed the title
|
Hey @msamper, great addition, I will have a look tomorrow! |
|
Hi @msamper, could you please rebase this PR? It should resolve the E2E issue. |
Signed-off-by: Michael Samper <[email protected]>
Signed-off-by: Michael Samper <[email protected]>
Signed-off-by: Michael Samper <[email protected]>
Signed-off-by: Michael Samper <[email protected]>
msamper
force-pushed
the
msamper/growthbook-node-provider
branch
from
b0201e3 to
52b9510
Compare
@beeme1mr I rebased on the main branch of my fork and it seems like there's still CI issues with the e2e and the lint-test-build. Should I be rebasing on open-feature:main instead? |
There was a problem hiding this comment.
Thanks @msamper this looks good!
I just have two comments regarding the EvaluationContext -> Attributes.
When these are resolved this looks great!
| if (!isEmpty(context)) { | ||
| // Add flag evaluation context and merge with existing context | ||
| await this.client.setAttributes({ ...this.client.getAttributes(), ...context }); | ||
| } |
There was a problem hiding this comment.
The context is is not reset after the evaluations.
If am am not overseeing anything this would mean that if a first request with a context is done, and a second one is done with both, both would have the context set from the first.
There was a problem hiding this comment.
That's right. The context would remain the same across both requests unless it was overwritten. I wasn't sure if a merge was needed to support the Dynamic Context Paradigm that is expected for server providers, but realizing that maybe the OpenFeature server SDK handles the different evaluation context levels before calling these functions.
There was a problem hiding this comment.
That's true, you do not need to merge here, the SDK does it.
But this still has an issue.
If you do an evaluation with a context, the attributes of the GrowthBook object are set.
If another evaluation with absolutely no context is done after that, the attributes from the last evaluation will be used as they are not overwritten again.
This also relates to the concurrency problem I wrote in the other comment.
| if (!isEmpty(context)) { | ||
| // Add flag evaluation context and merge with existing context | ||
| await this.client.setAttributes({ ...this.client.getAttributes(), ...context }); | ||
| } |
There was a problem hiding this comment.
This could be problematic if setAttributes does network calls or so. Which I assume from here: https://github.com/growthbook/growthbook/blob/main/packages/sdk-js/src/GrowthBook.ts#L467
It could happen that we have a race condition here, if two evaluations with different context are done close to each other. In that case it might happen that the internal await this.refreshStickyBuckets();of client.setAttributes would take time, but the next evaluation is already started, where the attributes would be overwritten before the evaluation with that context has been done.
There was a problem hiding this comment.
I agree. That pattern looks more like our static context paradigm we use for client.
Is there a way to set attributes without mutating the client in any way? @msamper ?
There was a problem hiding this comment.
As of right now our SDK doesn't have a way of doing that. We could consider making some changes to our SDK to support passing in some attributes that take precedence over ones within the GrowthBook context. I'm looking into it to see what the level of effort is and if there's some better alternatives
Yes, open-feature:main should be your branch to rebase on @msamper. |
Signed-off-by: Michael Samper <[email protected]>
Signed-off-by: Michael Samper <[email protected]>
This PR
How to test
Integrate OpenFeature into a test node server and set it up with this new GrowthBook provider. Ensure that GrowthBook flag evaluation works as expected using the OpenFeature API
How to set up OpenFeature with node
OpenFeature.setProvider(new GrowthbookProvider(gbContext, initOptions))