amplitude / experiment-php-server

Amplitude Experiment PHP Server SDK
MIT License
2 stars 4 forks source link

Make Cache in AssignmentFilter configurable #24

Open ruudk opened 2 months ago

ruudk commented 2 months ago

As opposed to other languages , PHP is stateless. That means that keeping things inside an in memory cache will be cleared at the end of the request. That means that the LRUCache implementation in AssignmentFilter has no value.

The Cache should be made configurable. While doing that, it's best to allow any PSR-6 cache.

The LRUCache can be dropped and replaced with an ArrayAdapter cache.

https://symfony.com/doc/current/components/cache.html https://github.com/amplitude/experiment-php-server/blob/main/src/Assignment/LRUCache.php https://github.com/amplitude/experiment-php-server/blob/main/src/Assignment/AssignmentFilter.php

Currently, we cannot use assignments because we need to make sure the cache is stateful. It's impossible to define a custom AssignmentFilter. It's not an interface, so we have to extend the existing AssignmentFilter. But then it's not possible to easily use it because this is initialized here: https://github.com/amplitude/experiment-php-server/blob/0aa016fee719fd25ac396cc329d5a3427d997832/src/Local/LocalEvaluationClient.php#L95

Another solution, as opposed to making the Cache configurable, is to make AssignmentFilter an interface, with a DefaultAssignmentFilter implementation that can be swapped.

/cc @bgiori

ruudk commented 2 months ago

I think one solution could be to implement the filtering in the AssignmentTrackingProvider. But I feel that's doing the work twice.

tyiuhc commented 2 months ago

@ruudk Many thanks for raising this issue. This has been prioritized and we are working to address it.

tyiuhc commented 1 month ago

@ruudk, you can now configure a custom AssignmentFilter in v1.2.0. Please let us know if this is not working as expected, thanks!

ruudk commented 1 month ago

Too bad I didn't have time to look at this and it's already tagged. But wouldn't it have been better to make the cache inside the default filter configurable? That way I don't have to create my own filter.

tyiuhc commented 1 month ago

@ruudk To use a custom cache with the default filter, you can create a DefaultAssignmentFilter with your custom cache, then use that as your filter in AssignmentConfig.

We wanted to allow additional flexibility in determining whether an assignment should be tracked. Hope that helps with your use case.

ruudk commented 1 month ago

I understand. But given that much can be configured using a ConfigBuilder, I figured that allowing me to configure the cache there, would make it even easier 😁

ruudk commented 1 month ago

@tyiuhc It's not possible for me to use 1.2.0 because it's locked to symfony/cache ^5.

Could you please widen this to allow all the supported versions? https://symfony.com/releases

tyiuhc commented 1 month ago

@ruudk Apologies for the inconvenience, v1.2.1 will allow additional versions of symfony/cache