Skip to content

feat: Add caching to GoFeatureFlag provider #386

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

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: Add caching to GoFeatureFlag provider #386

wants to merge 4 commits into from

Conversation

hoerup
Copy link

@hoerup hoerup commented Apr 29, 2025

This PR

Add caching to GoFeatureFlag provider

Related Issues

Fixes thomaspoignant/go-feature-flag#657

Notes

Follow-up Tasks

How to test

@hoerup hoerup requested review from a team as code owners April 29, 2025 21:38
@github-actions github-actions bot requested a review from thomaspoignant April 29, 2025 21:38
@hoerup hoerup force-pushed the hoerup/goFF-cache branch 3 times, most recently from c352d05 to e82db60 Compare April 30, 2025 14:30
@hoerup
Add caching to GoFeatureFlag provider
d1ca06b 
Signed-off-by: Torben Hørup <torben@t-hoerup.dk>
@hoerup hoerup force-pushed the hoerup/goFF-cache branch from e82db60 to d1ca06b Compare April 30, 2025 20:32
@hoerup
Copy link
Author

hoerup commented May 1, 2025

@thomaspoignant could you take a look ?

@github-actions github-actions bot mentioned this pull request May 1, 2025
Open
@askpt askpt mentioned this pull request May 2, 2025
Merged
thomaspoignant
thomaspoignant reviewed May 2, 2025
View reviewed changes
Copy link
Member

@thomaspoignant thomaspoignant left a comment

Choose a reason for hiding this comment

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

Hey @hoerup this looks really great, but I think this deserve some unit test to be sure that we this is working as expected.
Do you think you can add some?

Also, since I am not expert in .NET it will be great if @askpt can help me reviewing more in details the .NET part.

askpt
askpt reviewed May 2, 2025
View reviewed changes
Copy link
Member

@askpt askpt left a comment

Choose a reason for hiding this comment

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

I don't see any major issues from this PR.
For consistency I would apply the suggestions in the Options file.
Finally, the build is failing due to formatting issues. I would run dotnet format at the root of this repository in your local branch!

@hoerup
Copy link
Author

hoerup commented May 2, 2025

Ill look at your suggestions in the weekend. In the meantime, do you think the default values are reasonable/ too low / too high??

askpt
askpt reviewed May 2, 2025
View reviewed changes
...enFeature.Contrib.Providers.GOFeatureFlag/OpenFeature.Contrib.Providers.GOFeatureFlag.csproj Outdated
@@ -15,8 +15,12 @@
<PackageReference Include="System.Net.Http" Version="4.3.4" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="ZiggyCreatures.FusionCache" Version="2.2.0" />
Copy link
Member

Choose a reason for hiding this comment

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

Another suggestion, please lower the required version of this dependency. The reason is, if a consumer is using 2.1, if they want to upgrade GoFF, they would need to upgrade to 2.2. My advise is to keep as lower as possible so no consumer is forced to upgrade. Other possible solution is to use PrivateAssets but I'm not that familiar with the full consequences.

@askpt
Copy link
Member

askpt commented May 2, 2025

Ill look at your suggestions in the weekend. In the meantime, do you think the default values are reasonable/ too low / too high??

@hoerup they seem like reasonable values. Also, they are default so the consumer can tune the way they prefer.

@hoerup
Copy link
Author

hoerup commented May 3, 2025 •
edited
Loading

another thing: Currently I use the entire EvaluationContext as part of the cache key - is that how you'd expect it or should I only use the targetingKey bit ?

@hoerup
Copy link
Author

hoerup commented May 3, 2025

@askpt could you take another look ?

askpt
askpt reviewed May 4, 2025
View reviewed changes
Copy link
Member

@askpt askpt left a comment

Choose a reason for hiding this comment

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

Looks good to me, I am just concerned about the key size. Waiting on opinions from others.

src/OpenFeature.Contrib.Providers.GOFeatureFlag/GoFeatureFlagProvider.cs Outdated
@@ -311,6 +357,11 @@ private async Task<OfrepResponse> CallApi<T>(string flagKey, T defaultValue,
return ofrepResp;
}

private string GenerateCacheKey(string flagKey, EvaluationContext ctx)
{
return ctx != null ? flagKey + ":" + new OfrepRequest(ctx).AsJsonString() : flagKey;
Copy link
Member

Choose a reason for hiding this comment

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

Bringing the cache key discussion here. I am concerned about the key size (character number) and it's impacts on performance. Do you think you can use some sort of hash that represents the object instead of the json string itself? @thomaspoignant @kylejuliandev @toddbaert Any suggestions?

Copy link
Author

Choose a reason for hiding this comment

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

That is exactly why i asked

#386 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

Maybe @thomaspoignant could help us with this decision ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @askpt, a Hash (like SHA or MD5) would produce a sufficiently unique key for this use case. A base64 encoded sting might be fine too. You may need to order the EvaluationContext so the encoded string is the same every time.

The problem I see with the JSON string, aside from performance, is whether it is actually deterministic. If the EvaluationContext is created slightly differently then a new key is produced. If we cannot reliably reproduce the same key each time then it kind of defeats the point of using a cache.

Copy link
Author

@hoerup hoerup May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I just checked the java provider for openfeature/goFF, and it looks like they use key + hash(evaluationContextAsJson), so I think i will do the same

https://github.com/open-feature/java-sdk-contrib/blob/main/providers/go-feature-flag/src/main/java/dev/openfeature/contrib/providers/gofeatureflag/controller/CacheController.java#L48

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

I cannot see any hashing in this method. You're just returning the string JSON.

Copy link
Author

Choose a reason for hiding this comment

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

Weird - im certain that I pushed a change last night - ill look into it

Copy link
Author

Choose a reason for hiding this comment

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

Finally got around to look at it - I don't know why it wasn't commited, but it is now

@hoerup
PR comments and unit test
6214c3d 
Signed-off-by: Torben Hørup <torben@t-hoerup.dk>
@hoerup hoerup force-pushed the hoerup/goFF-cache branch from b784a2d to 6214c3d Compare May 6, 2025 17:55
@hoerup
Copy link
Author

hoerup commented May 7, 2025

@askpt I think I have addressed all comments now

@hoerup
Copy link
Author

hoerup commented May 12, 2025

@thomaspoignant just saw that you are working on a in-process evaluation thing - should we abort this cache PR in favor of the new feature ?

@hoerup
use evaluation context hashcode as part of cache key
533a4a1 
Signed-off-by: Torben Hørup <torben@t-hoerup.dk>
@hoerup hoerup force-pushed the hoerup/goFF-cache branch from 2d0dbc1 to 533a4a1 Compare May 15, 2025 07:59
@hoerup hoerup mentioned this pull request May 21, 2025
Open
@hoerup
Copy link
Author

hoerup commented May 26, 2025

@thomaspoignant just saw that you are working on a in-process evaluation thing - should we abort this cache PR in favor of the new feature ?

@thomaspoignant when ever you got the time, i'd really appreciate your input on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@askpt askpt askpt left review comments

@thomaspoignant thomaspoignant thomaspoignant left review comments

@kylejuliandev kylejuliandev kylejuliandev left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@thomaspoignant thomaspoignant

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(feature) Implement open-feature provider cache for the .NET provider
4 participants
@hoerup @askpt @thomaspoignant @kylejuliandev