Skip to content
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

feat: add caching to survey sync #2011

Merged
merged 2 commits into from
Feb 16, 2024

Conversation

jonas-hoebenreich
Copy link
Contributor

What does this PR do?

This PR adds public caching of 10min to the app.formbricks.com/api/v1/client//in-app/sync endpoint. Since Formbricks caches the survey endpoint for 15min in the frontend I think it makes sense to also add it to the backend to avoid unnecessary server load.
The cache times are up for discussion, how do you see the balance between rapid updates and server load?

Fixes # (issue)

How should this be tested?

  • make a request to app.formbricks.com/api/v1/client//in-app/sync and check the Cache-Control header
  • subsequent request should include Age: 123 headers when using a proxy like Cloudflare or Cloudfront

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read How we Code at Formbricks
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand bits
  • Ran pnpm build
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Formbricks Docs if changes were necessary

Copy link

vercel bot commented Feb 5, 2024

@jonas-hoebenreich is attempting to deploy a commit to the formbricks Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

github-actions bot commented Feb 5, 2024

Thank you for following the naming conventions for pull request titles! 🙏

@mattinannt
Copy link
Member

@jonas-hoebenreich thank you for the PR 😊💪
Our current approach to caching is to cache as much as possible using the Nextjs cache ( https://nextjs.org/docs/app/api-reference/functions/unstable_cache ) to avoid database requests and server load. This approach has the advantage that we can easily revalidate the cache if necessary (e.g. a new survey was created, changed or user attributes on the server changed). With this, if nothing has changed in the database, the sync request should be very light and only if something has changed, the server recalculates or fetches new data from the database. We will continue to improve this approach in future updates to make sure we really only recalculate if necessary.

I'm not sure if caching the whole endpoint for a fixed amount of time is the best approach, since we also have cases where we resync faster (e.g. with identified users, when a survey is answered and we fetch the new list of surveys from the backend and calculate the surveys the user qualifies for based on the surveys he has seen and answered). This might fail with the proposed caching approach (at least as I understand it) since we get the (at most) 10 minutes old values that include the already answered survey.

But I will look at this approach in more detail and check for edge cases and how we can solve them.

@jonas-hoebenreich
Copy link
Contributor Author

@mattinannt, thanks for the quick response.
only app.formbricks.com/api/v1/client/[environmentId]/in-app/sync/ is cached, app.formbricks.com/api/v1/client/[environmentId]/in-app/sync/[userId] is not affected by this change.

If I understand correctly, the configuration in the frontend is only updated every 15 minutes (https://github.com/formbricks/formbricks/blob/main/packages/js/src/lib/config.ts#L28). This means that you have to assume that every (unidentified) user has a configuration that is 15 minutes old. This change would extend this span by 10 minutes (worst case scenario: after 15 minutes the user fetches 10min old config --> config is 25min old by the time the frontend cache expires). If that seems too long, how about a shorter Cache-Control time?

The fundamental problem with unstable_cache is that it does not allow multiple containers (as the cache is specific to each container and is only revalidated in this container. If several containers are running at the same time, the cache in some containers is only revalidated after 30 minutes).
Conversely, this means that a single container must handle the entire workload. Of course, this quickly leads to problems during peak traffic. I therefore believe that a cache for this endpoint would be very helpful and that the downsides are justified.

@mattinannt
Copy link
Member

mattinannt commented Feb 5, 2024

@jonas-hoebenreich Thanks for the answer; yes, that makes sense :-)
But the problem with caching between containers still exists with the cache control headers, right?
Since it depends on which container the request ends up in, the cached state might be different (assuming a survey has been created or updated in the meantime). This isn't necessarily a problem; it's just something I want to understand.

Next.js is also working on ways to better support complex caching strategies: https://nextjs.org/docs/app/building-your-application/deploying#configuring-caching
But this may take a while to implement on our side, and also requires setting up a global key-value store for caching.

@jonas-hoebenreich
Copy link
Contributor Author

Yes, this PR only reduces the load on the single container and improves the response time for users in remote regions (when using a CDN).
In the long term it would of course be ideal to use a cache, like Redis, but I can fully understand that this is more of a long-term topic and also introduces new complexities.

The biggest problem with multiple containers that we have noticed is that in the backend the data is no longer displayed consistently in the backend.

@mattinannt
Copy link
Member

mattinannt commented Feb 12, 2024

@jonas-hoebenreich Quick update: We will merge our advanced targeting today and afterwards I will test this PR again and merge it. Since the Advanced Targeting PR also changes a lot in terms of caching and sync endpoints, I want to make sure they work well together.

@mattinannt mattinannt added this to the v1.6.0 milestone Feb 13, 2024
@mattinannt mattinannt added this pull request to the merge queue Feb 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 16, 2024
@mattinannt mattinannt added this pull request to the merge queue Feb 16, 2024
Merged via the queue into formbricks:main with commit 4c1e688 Feb 16, 2024
8 of 10 checks passed
@jonas-hoebenreich jonas-hoebenreich deleted the sync-caching branch March 16, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants