feat: implement simple Fetch by key for cache items #4032
Conversation
|
Done, it works |
|
Oh, can we replace this as well: https://github.com/grafana/tempo/blob/main/tempodb/backend/cache/cache.go#L118 |
|
Is there any user impact for this change like a config file option? |
No, this is internal |
|
Now we have 0 references from code to the Fetch method, we even have an internal fetchKeysBatched method in Memcached code, wich seems a bit overengineered Line 206 in 5e3e03a Do think this is something we are going to need? I would favor to delete at very least that batched code |
If you don't see an immediate future for all the batched code, I'm good on deleting it. |
There was a problem hiding this comment.
This LGTM. Two things:
- Let's remove the batching code as you suggested.
- I'd prefer to back out the event code, but I'm good if you want to leave it. I thought we were replacing a span creation, but this will just incur more cost. Let me know which direction you decide to go.
right, I think we should handle this with two new metric counters one for hits and another one for missed ones |
What this PR does:
It implements a new way to fetch by key. That way we can skip more expensive operations using batches of key lookups when we only want one key.
It replaces the existing Fetch in the sync_handler_cache middleware.
Tempo local example now can use memcache, I think this doesn't add too much overhead and it's good for testing
[X] Test it locally against both Memcached and Redis
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]