FF-3143 feat: add caching logger for assignment and bandit events#68
FF-3143 feat: add caching logger for assignment and bandit events#68
Conversation
rasendubi
force-pushed
the
ff-3143-assignment-cache
branch
2 times, most recently
from
ff71c4f to
5682b87
Compare
|
run-tests-with-tox in CI seems to be broken because of https://github.com/Eppo-exp/python-sdk/pull/65/files#r1738745786 I've checked that it passes locally (on all available python versions): UPD: fixed in #69 |
|
|
||
| @staticmethod | ||
| def __bandit_cache_keyvalue(event: Dict) -> Tuple[Tuple, Tuple]: | ||
| key = (event["flagKey"], event["subject"]) |
There was a problem hiding this comment.
Noticed that our event format is somewhat inconsistent between SDKs.
In Python, Go, PHP, .NET, Rust, and Ruby:
- assignment event uses
"featureFlag" - bandit event uses
"flagKey"
Javascript and Java SDK use "featureFlag" for both events.
There was a problem hiding this comment.
Yeah, it's just a factor of evolution. We're trying to move (slowly) more towards explicit keys, such as flagKey. However, it's also nice to have fields be analogously named between the two events (hence JavaScript and Java maintaining featureFlag). I think you're just seeing the personal preferences of the authors manifest in the various SDKs.
BanditEvaluator.evaluate_bandit() always returns a result, so `is None` checks were unnecessary. Furthermore, `evaluation` fields are accessed without this guard a couple of lines below.
There is no reason for AssignmentLogger to be a pydantic model because it's not a model at all (it's behavior, not data). Remove inheritance from the base model for AssignmentLogger and allow arbitrary types on Config instead. Also exclude assignment_logger from pydantic serialization. This refactor makes it easier to work with AssignmentLogger. Especially when mocking it, as pydantic models are not easily mockable.
rasendubi
force-pushed
the
ff-3143-assignment-cache
branch
from
5682b87 to
9b3599a
Compare
schmit
left a comment
schmit
left a comment
There was a problem hiding this comment.
👌 very clean implementation, love it!
| # cache 1024 least recently used assignments | ||
| assignment_cache=cachetools.LRUCache(maxsize=1024), | ||
| # cache bandit assignment for no longer than 10 minutes | ||
| bandit_cache=cachetools.TTLCache(maxsize=2048, ttl=600), |
|
|
||
| @staticmethod | ||
| def __bandit_cache_keyvalue(event: Dict) -> Tuple[Tuple, Tuple]: | ||
| key = (event["flagKey"], event["subject"]) |
There was a problem hiding this comment.
Yeah, it's just a factor of evolution. We're trying to move (slowly) more towards explicit keys, such as flagKey. However, it's also nice to have fields be analogously named between the two events (hence JavaScript and Java maintaining featureFlag). I think you're just seeing the personal preferences of the authors manifest in the various SDKs.
| assert inner.log_bandit_action.call_count == 1 | ||
|
|
||
|
|
||
| def test_bandit_flip_flop(): |
test/cache_assignment_logger_test.py
Outdated
| assert inner.log_bandit_action.call_count == 3 | ||
|
|
||
|
|
||
| def mk_assignment_event( |
There was a problem hiding this comment.
super minor, but consider spelling out mock (instead of mk), I don't think the few characters saved is worth the extra cognitive step for developers reading the code to get what is going on
There was a problem hiding this comment.
mk here actually stands for "make" (as in mkdir). Thought it's quite widely understood but it's probably just me coming from C background and writing too much makefiles (.mk) in the past 😅
I'll spell it out
3 participants
labels: mergeable
Fixes: https://linear.app/eppo/issue/FF-3143/%5Bpython%5D-deduplication-cache-for-assignment-and-bandit-events
Motivation and Context
This allows deduplicating assignments and bandit actions before they are logged to client storage.
Description
This PR adds a caching logger adapter that optionally accepts caches for assignment and bandit. Cache implementation can be any
MutableMapping:Cachefromcachetools(with any configuration)dictfor non-evicting (unbound) cache (not recommended)If cache is not provided for assignment or bandits, that type of events is not cached.
This design decouples eppoclient completely from caching implementation and the user may use any of the great caching implementation out there that fit their needs.
How has this been tested?
Added new test cases.