Skip to content
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.

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

feat: implemented LRU caching for flagd provider #47

Merged
merged 18 commits into from
Apr 3, 2023

Conversation

bacherfl
Copy link
Contributor

@bacherfl bacherfl commented Mar 27, 2023

Closes #37

This PR implements standard caching functionality for the flagd provider. The configuration is done via the following env vars:

| Caching                      | FLAGD_CACHE                    | string  |           |     LRU       |
| Maximum cache size           | FLAGD_MAX_CACHE_SIZE           | number  | 10        |               |
| Maximum event stream retries | FLAGD_MAX_EVENT_STREAM_RETRIES | number  | 3         |               |

When caching is enabled, the flagd provider will establish an EventStream with the flagd server via grpc, and listen for configuration_change events. When such an event is received for a flag that has already been cached previously, that flag will be removed from the cache. When retrieving the value of a feature flag, the value will be cached if the Reason of the resolution is set to STATIC.

Note: The imported version of the OF dotnet-sdk is set to 0.5 - Since this version does not yet include the configuration_change event type, or the STATIC reason type for the resolutionDetails, it might be worth to consider bumping the version of the imported dotnet-sdk as a follow up task.

Note: Since the Event Stream is opened in a background task, I was not quite sure how to best verify in the Unit tests that things like the removal of a cached item were properly called by the provider. Coming from Golang, I was missing something like require.Eventually(t, len(mockCache.DeleteCalls) == 1). I tried to resemble that behaviour by creating an AutoResetEvent that gets set in the mock implementation of the method I would like to ensure it was called. Then, using Assert.True(_autoResetEvent.WaitOne()) I waited for the event to be triggered. This solution does seem to work, but if there is a better approach to this in C# I appreciate every input from the reviewers of this PR

@bacherfl bacherfl marked this pull request as ready for review March 28, 2023 06:54
@bacherfl bacherfl requested a review from a team as a code owner March 28, 2023 06:54
@toddbaert
Copy link
Member

toddbaert commented Mar 29, 2023

Note: Since the Event Stream is opened in a background task, I was not quite sure how to best verify in the Unit tests that things like the removal of a cached item were properly called by the provider. Coming from Golang, I was missing something like require.Eventually(t, len(mockCache.DeleteCalls) == 1). I tried to resemble that behaviour by creating an AutoResetEvent that gets set in the mock implementation of the method I would like to ensure it was called. Then, using Assert.True(_autoResetEvent.WaitOne()) I waited for the event to be triggered. This solution does seem to work, but if there is a better approach to this in C# I appreciate every input from the reviewers of this PR

I think your testing strategy is fine. I find the tests around caching about as readable as possible given the complexity. They look similar to the ones in other contribs.

I think my only request with them is that you add timeouts to every WaitOne() to prevent the tests from hanging forever in cases where they break because the awaited function is not called. I purposely broke some tests locally and it seems like Xunit happily runs with my CPU screaming until I manually kill the process 😂

It would be even better to add a Timeout to the Fact annotation, but apparently this doesn't work in Xunit (I tried).

@toddbaert toddbaert self-requested a review March 29, 2023 19:17
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Hey @bacherfl , IMO this all looks really great. I love your cache impl. I've left mostly optional/suggested changes except this one, which seems pretty small but will prevent CI/tests from hanging if people break something when making changes.

@bacherfl
Copy link
Contributor Author

Hey @bacherfl , IMO this all looks really great. I love your cache impl. I've left mostly optional/suggested changes except this one, which seems pretty small but will prevent CI/tests from hanging if people break something when making changes.

Thanks @toddbaert for your review and suggestions - I agree with all of them, and will try to include them into this PR

@bacherfl
Copy link
Contributor Author

alright, I have submitted all changes/suggestions, so the PR should be ready for another review :)

Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

lgtm!

@toddbaert toddbaert merged commit f4d2142 into open-feature:main Apr 3, 2023
@github-actions github-actions bot mentioned this pull request Apr 3, 2023
vpetrusevici pushed a commit to vpetrusevici/open-feature-dotnet-sdk-contrib that referenced this pull request Oct 18, 2023
Signed-off-by: Florian Bacher <florian.bacher@dynatrace.com>
Signed-off-by: Vladimir Petrusevici <vladimir.petrusevici@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[flagd-provider] Implement standard caching, with LRU cache
2 participants