chore(frontend): use explicit cache for admin analytics#64077
Conversation
| // Cache is optional. If nil, caching is disabled. | ||
| cache redispool.KeyValue |
There was a problem hiding this comment.
That's the core of the change. We replace cache bool with an explicit cache. Most of the other changes follow from that.
| if cache != nil { | ||
| err = setArrayToCache(cache, cacheKey, items) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This is a slightly different behavior. Before we always set the cache, even if it was disabled.
I am a bit torn whether we should move the check for a nil cache inside setArrayToCache (and return nil) to make the call sites prettier. Opinions?
There was a problem hiding this comment.
Left a comment above if we even need this, but if we do it might be nice to either make setArrayToCache do the nil check like you said, or more explicitly, to pass a NoopKeyValue into the analytics 🤔
There was a problem hiding this comment.
I like your idea with NoopKeyValue. It's the least surprising and there is no strange coupling between the feature flag and a nil check somewhere else.
eseliger
left a comment
There was a problem hiding this comment.
nothing blocking, thanks for reworking this :)
| if useCache := !featureflag.FromContext(ctx).GetBoolOr("admin-analytics-cache-disabled", false); useCache { | ||
| cache = redispool.Store | ||
| } |
There was a problem hiding this comment.
Q: Does anyone really ever use this? Or could we simplify a lot here and not have this != nil thing everywhere? What would be a reason to disable caching, which will inherently increase database load by a lot?
There was a problem hiding this comment.
It could be helpful for debugging and testing to disable the cache but I can only guess why this was added.
| if cache != nil { | ||
| err = setArrayToCache(cache, cacheKey, items) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Left a comment above if we even need this, but if we do it might be nice to either make setArrayToCache do the nil check like you said, or more explicitly, to pass a NoopKeyValue into the analytics 🤔
Relates to #64041
This refactors the
adminanalyticspackage to set the cache explicitly instead of implicitly relying on the globalredispool.Store. The global store obfuscated the dependency and also made testing a bit awkward.Test plan: