-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use redis to cache results #972
Conversation
|
||
people = _calculate_people(events=filtered_events) | ||
return Response([people]) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved everything out of the class because I needed the logic to be easily reusable by the worker
CACHES = { | ||
"default": { | ||
"BACKEND": "django_redis.cache.RedisCache", | ||
"LOCATION": REDIS_URL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reusing the redis we use for everything else. may consider having a dedicated instance because we are likely going to end up caching 100s -> 1000s of dashboard items indefinitely
…into function-caching
posthog/decorators.py
Outdated
TRENDS_ENDPOINT = 'Trends' | ||
FUNNEL_ENDPOINT = 'Funnel' | ||
|
||
def cached_function(cache_type: str, expiry=1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
somewhat inflexible decorator for the moment. adding this decorator to an endpoint will require an explicit condition to handle
8cc7785
to
0312c5c
Compare
TRENDS_ENDPOINT = 'Trends' | ||
FUNNEL_ENDPOINT = 'Funnel' | ||
|
||
def cached_function(cache_type: str, expiry=30): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to note is that if we have a default expiry of 30 seconds this will be nice for very heavily trafficked instances but for new instances, it might seem like the data is not refreshing/stuck when you're on the trends page. I'm open to limiting the caching for dashboart items only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love it. Some small comments.
Changes
implements #765 and #940
Todo:
- [ ] dashboard items out of order after refreshChecklist