getMetricsTags cached using a 600s redis cache #92
Conversation
|
|
Awesome work! Since getMetricsChart is also using the tags cache, we want to make sure they are pulling the same source. Do you think its possible to reuse MetricsPropertyTypeMappingsModel ? Can also check out buildMetricsPropertyTypeMappingsModel and see how its used within getMetricsChart method |
|
Just looking at how that data is used and returned, I think I might actually go the other direction with that and refactor the metrics chart to use the fetch to grab the metrics and then subquery. There doesn't seem to be a lot of overlap, to my eye, that would make it worth unifying the caches, unless I'm missing something. It's kind of situational depending on how many metrics you're grabbing per chart, but I do tend to use separate caches for bulk metric names and individual metric values (like in the property map? I might not be reading that right). Cache is usually pretty cheap on the margin, so storing more is often better than trying to reuse a single cache, though I'm used to using a LRU eviction cache (e.g. memcached) so maybe the Redis instance doesn't behave that way with TTL keys, depending on the config. I also might split the cache key up with a timestamp in it (e.g. metric_names_${teamid}_${current_hour_timestamp}) and fetch multiple entries to build the datastructure, which allows you to construct the last-day chart with 23 redis hits and 1 new query for the latest hour's data, for example. You can also roll it up to daily caches or weekly caches for longer term storage, it depends on how frequently a given metric is looked at (if it's on the primary dashboard vs only looked at for debugging once in a blue moon). |
Good point. Its a bit confusing on the surface. 'tags' refers to the unique set of '_string_attributes' group by metric type and name. My only concern is 'getMetricsChart' sees the different set of tags (especially new ones). But I think we can refactor the caching stuff in next PRs. caching by timestamp is a very interesting optimization and would love to explore it |
| @@ -13,3 +13,5 @@ client.on('error', (err: any) => { | |||
| }); | |||
|
|
|||
| export default client; | |||
|
|
|||
| export { client as redisClient }; | |||
There was a problem hiding this comment.
I think this export is redundant since we already export client as default. So importing code is like
import redisClient from './utils/redis';
There was a problem hiding this comment.
Yeah I prefer the import { redisClient } version because it doesn't accidentally get renamed:
import clickhouseClient from './utils/redis'; // no errors here
I've been bitten by that before :)
closes #91
Add a first draft of a cache for getMetricsTags - this uses simple inline architecture but could be refactored into a separate model if necessary. Could also use unit and/or functional tests (no tests in the API package right now⁉️ )
Tested locally but definitely a candidate for further exploration. It also preserves the clickhouse / query metadata which is probably not correct.
Adds two new env configuration parameters:
CACHE_METRICS_TAGS // boolean, disables metric cache if set to 'false'
CACHE_METRICS_EXPIRATION // string, time description e.g. '1d', '1h', '600s' - duration of cache TTL
These could in theory be unified to take e.g. a cache expiration time or 'false'