Add FastAPI Redis Caching #4195

ThomasLaPiana · 2023-09-29T13:28:26Z

Closes #4171

Description Of Changes

This adds caching for FastAPI endpoints, via our existing Redis instance

Docs for the package here: https://github.com/long2ice/fastapi-cache

Code Changes

bump the redis version
add custom caching logic for the privacy-experience endpoint
add a new response header X-Endpoint-Cache that contains either HIT or MISS
support a header bust-endpoint-cache that clears the privacy-experience endpoint cache
automatically clear the cache every 30 minutes
update tests

Steps to Confirm

nox -s load_tests should show significantly improved results for the privacy-experience endpoints

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- documentation complete, PR opened in fidesdocs
- documentation issue created in fidesdocs
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md
For API changes, the Postman collection has been updated

…ndpoint

…i-redis-caching

cypress · 2023-10-11T09:27:44Z

Passing run #4616 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge `a8a45a2` into `06fe0c6`...
Project: fides	Commit: `64c215e2d7 ℹ️`
Status: Passed	Duration: 00:52 💡
Started: Oct 13, 2023 7:03 AM	Ended: Oct 13, 2023 7:04 AM

Review all test suite changes for PR #4195 ↗︎

codecov · 2023-10-11T10:00:49Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (ef52d05) 87.71% compared to head (161e271) 87.73%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4195      +/-   ##
==========================================
+ Coverage   87.71%   87.73%   +0.01%     
==========================================
  Files         331      331              
  Lines       20870    20878       +8     
  Branches     2708     2708              
==========================================
+ Hits        18307    18317      +10     
+ Misses       2096     2094       -2     
  Partials      467      467

Files	Coverage Δ
...i/api/v1/endpoints/privacy_experience_endpoints.py	`93.75% <100.00%> (+0.11%)`	⬆️
src/fides/api/app_setup.py	`69.65% <100.00%> (+1.53%)`	⬆️
src/fides/api/main.py	`77.14% <100.00%> (+0.16%)`	⬆️
src/fides/api/util/cache.py	`93.10% <100.00%> (-0.12%)`	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ThomasLaPiana · 2023-10-11T15:25:25Z

At this point I've got the cache wired up, but it isn't making a difference. It seems like something is misconfigured or the endpoint isn't set up in a way that is cache-friendly. Investigating...

ThomasLaPiana · 2023-10-12T07:23:15Z

the latest commit features a stripped down privacy-experience endpoint but with proper caching that works as expected. now the challenge is to keep that endpoint at feature parity while also adding the custom caching

This reverts commit 4d50fd9.

ThomasLaPiana · 2023-10-12T08:45:06Z

as it currently stands, the caching is working but failing some tests due to the fact that the tests are adding new data and the cache is not being busted

@pattisdr how often do we expect these tables to be updated? I'm not sure of the context here, but maybe it isn't actually very feasible to use a simple caching method here since it could be hard to know when to actually bust the cache.

pattisdr · 2023-10-12T13:21:03Z

@ThomasLaPiana we ran into a similar issue in the original draft for TCF Experiences trying to use lru caching and realized it would take some more care to get the arguments right to bust the cache. It wasn't a quickly solved thing at the time when we were trying to get the POC out the door so we pulled it out.

I would think these tables could get updated frequently: we'll have automated pipelines that update systems and privacy declarations at least on a weekly cadence. Big picture, additions or deletions and updates to certain fields on systems or privacy declarations can potentially cause big changes to what is returned in the Experience.

For non-TCF Experiences, in addition to the above, changes to Privacy Notices or Experience Config (Experience copy/language) can cause the Experience to need to be re-built.

ThomasLaPiana · 2023-10-12T13:31:47Z

@ThomasLaPiana we ran into a similar issue in the original draft for TCF Experiences trying to use lru caching and realized it would take some more care to get the arguments right to bust the cache. It wasn't a quickly solved thing at the time when we were trying to get the POC out the door so we pulled it out.

I would think these tables could get updated frequently: we'll have automated pipelines that update systems and privacy declarations at least on a weekly cadence. Big picture, additions or deletions and updates to certain fields on systems or privacy declarations can potentially cause big changes to what is returned in the Experience.

For non-TCF Experiences, in addition to the above, changes to Privacy Notices or Experience Config (Experience copy/language) can cause the Experience to need to be re-built.

If we really want caching here, I think we could do the following:

Bust the cache every 30 minutes or something automatically
Support a request header that busts the cache and forces a full rebuild

What do you think about adding these two solutions? Caching is basically the hardest thing to get right so if we decide to forego this and instead focus on async database calls and potentially parallel calls we can go that route as well. Caching is a nice-to-have feature but also risks giving us major headaches so it's worth weighing the pros and cons

pattisdr · 2023-10-12T18:41:23Z

@ThomasLaPiana I really like the addition of that second idea, as part of the overall solution, a request header that forces a rebuild - that would work great for unit tests. A thirty minute limit on the cache too probably would catch most things

In theory I'd like to have caching because while this is something that can change frequently, it's likely not changing daily, and that's a lot of us re-querying the same thing. Caching was something we'd talked about having originally - it seems preferable, compared to the other extreme where we're doing complex change tracking to rebuild the experiences over time as multiple other resources change.

But if you thought you could get similar orders of magnitude performance improvements with things like async database calls, that might be worth exploring first given the risks here of showing a customer an outdated experience.

ThomasLaPiana · 2023-10-13T03:58:40Z

@ThomasLaPiana I really like the addition of that second idea, as part of the overall solution, a request header that forces a rebuild - that would work great for unit tests. A thirty minute limit on the cache too probably would catch most things

In theory I'd like to have caching because while this is something that can change frequently, it's likely not changing daily, and that's a lot of us re-querying the same thing. Caching was something we'd talked about having originally - it seems preferable, compared to the other extreme where we're doing complex change tracking to rebuild the experiences over time as multiple other resources change.

But if you thought you could get similar orders of magnitude performance improvements with things like async database calls, that might be worth exploring first given the risks here of showing a customer an outdated experience.

If you think these are good solutions, it's worth continuing with! Even with Async database calls it won't cut down on processing time, a long/complex query is still just as long/complex, it would only mean less blocking time.

In that case, I'll continue with the work here and clean it up to get it production-ready 🙂 thank you for your input! I'll tag you again when its ready for a review

…g whether or not the cache was a hit or miss

pattisdr · 2023-10-13T12:11:10Z

src/fides/api/api/v1/endpoints/privacy_experience_endpoints.py

+    # This is structured to look like a paginated result to minimize impact from
+    # the caching changes
+    api_result = {"items": results, "total": len(results)}
+    PRIVACY_EXPERIENCE_CACHE[cache_hash] = api_result


Just want to comment early here. I see you're caching the output of the entire endpoint: now the performance gains make more sense. This is a neat strategy, I'm just more concerned about accuracy now. I had been assuming we'd instead cache some time-consuming individual pieces that go into the experience like the output of get_tcf_contents.

For example, fides_user_device_id is a unique user identifier. If present, we supplement the experience with previously-saved preferences for that user. So if the user saves new preferences, this experience would still return their outdated preferences.

That makes sense! Without knowledge of what is actually going on here I can't make those fine-grained caching decisions.

So we can either handle this on the front-end using the header that resets the cache, or we can use a more fine-grained caching method in which I'd need some more guidance on what is safe to cache.

real quick, something like the output of get_tcf_contents which is the bulk of TCF experiences and PrivacyExperience.get_related_privacy_notices which is for non-TCF experiences but perhaps the latter refactored into a separate method that isn't taking fides_user_device_id in, that is added separately

Just a note here, I'm moving a great deal of privacy preferences / privacy experiences endpoints, supporting methods, TCF experiences, etc. to Fidesplus -
https://ethyca.atlassian.net/browse/PROD-1258

thank you for the heads up! In that case I'll move this work over there. I haven't been able to circle back to this yet but Adam already figured it out for some other endpoints we have :)

ThomasLaPiana · 2023-11-28T06:52:29Z

this is being worked on elsewhere by @adamsachs !

ThomasLaPiana and others added 9 commits September 29, 2023 16:58

feat: get logins working with drill

1849fd3

feat: add more endpoints

1d60191

feat: separate each endpoint into its own task

5229381

feat: optimize the Drill test

220f060

feat: make the privacy-experience endpoint async

eb0d7e7

feat: sprinkle some async sleep magic into the privacy-experience e…

800a944

…ndpoint

feat: remove an unused requirement

abe119c

fix: static checks

2ae8afb

Add FastAPI Redis Caching

1f1f12b

ThomasLaPiana self-assigned this Sep 29, 2023

ThomasLaPiana changed the base branch from main to add-more-drill-endpoints September 30, 2023 05:56

Merge branch 'add-more-drill-endpoints' into ThomasLaPiana-add-fastap…

6b33377

…i-redis-caching

Base automatically changed from add-more-drill-endpoints to main September 30, 2023 08:32

ThomasLaPiana added 2 commits October 11, 2023 17:02

Merge branch 'main' into ThomasLaPiana-add-fastapi-redis-caching

ffe5a7e

fix: remove outdated redis import

1347246

feat: add caching to the privacy-experience endpoint

161e271

feat: rough POC of custom caching for the privacy experience endpoint

38f32bc

ThomasLaPiana added 3 commits October 12, 2023 15:56

feat: additional cleanup

4d50fd9

Revert "feat: additional cleanup"

fdf1d95

This reverts commit 4d50fd9.

feat: cleanup, passing more tests

cdb6e49

feat: add headers to the privacy-experience response object indicatin…

be70d0c

…g whether or not the cache was a hit or miss

ThomasLaPiana and others added 2 commits October 13, 2023 12:56

checkin: add headers and cache tests, but not passing

2b64527

fix: more tests and almost all static checks

a8a45a2

pattisdr reviewed Oct 13, 2023

View reviewed changes

ThomasLaPiana closed this Nov 28, 2023

ThomasLaPiana deleted the ThomasLaPiana-add-fastapi-redis-caching branch November 28, 2023 06:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FastAPI Redis Caching #4195

Add FastAPI Redis Caching #4195

ThomasLaPiana commented Sep 29, 2023 •

edited

Loading

cypress bot commented Oct 11, 2023 •

edited

Loading

codecov bot commented Oct 11, 2023 •

edited

Loading

ThomasLaPiana commented Oct 11, 2023

ThomasLaPiana commented Oct 12, 2023 •

edited

Loading

ThomasLaPiana commented Oct 12, 2023

pattisdr commented Oct 12, 2023

ThomasLaPiana commented Oct 12, 2023

pattisdr commented Oct 12, 2023 •

edited

Loading

ThomasLaPiana commented Oct 13, 2023

pattisdr Oct 13, 2023

ThomasLaPiana Oct 16, 2023

pattisdr Oct 17, 2023

pattisdr Oct 30, 2023

ThomasLaPiana Oct 31, 2023

ThomasLaPiana commented Nov 28, 2023

Add FastAPI Redis Caching #4195

Add FastAPI Redis Caching #4195

Conversation

ThomasLaPiana commented Sep 29, 2023 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

cypress bot commented Oct 11, 2023 • edited Loading

Passing run #4616 ↗︎

Review all test suite changes for PR #4195 ↗︎

codecov bot commented Oct 11, 2023 • edited Loading

Codecov Report

ThomasLaPiana commented Oct 11, 2023

ThomasLaPiana commented Oct 12, 2023 • edited Loading

ThomasLaPiana commented Oct 12, 2023

pattisdr commented Oct 12, 2023

ThomasLaPiana commented Oct 12, 2023

pattisdr commented Oct 12, 2023 • edited Loading

ThomasLaPiana commented Oct 13, 2023

pattisdr Oct 13, 2023

Choose a reason for hiding this comment

ThomasLaPiana Oct 16, 2023

Choose a reason for hiding this comment

pattisdr Oct 17, 2023

Choose a reason for hiding this comment

pattisdr Oct 30, 2023

Choose a reason for hiding this comment

ThomasLaPiana Oct 31, 2023

Choose a reason for hiding this comment

ThomasLaPiana commented Nov 28, 2023

ThomasLaPiana commented Sep 29, 2023 •

edited

Loading

cypress bot commented Oct 11, 2023 •

edited

Loading

codecov bot commented Oct 11, 2023 •

edited

Loading

ThomasLaPiana commented Oct 12, 2023 •

edited

Loading

pattisdr commented Oct 12, 2023 •

edited

Loading