frappe icon indicating copy to clipboard operation
frappe copied to clipboard

feat: add cache control

Open blaggacao opened this issue 1 year ago • 10 comments

Context

  • Site local server
  • Cloud proxy

Due to latency issues, there needs to be a way to serve static content from the edge

Proposed Solution

  • Add Cache-Control together with Etag
  • The proxy needs to independently predict the effectively rendered language in order to serve the right cache key
  • Therefore: set a user_lang cookie which records the effectively delivered language, this removes too much ambiguity from headers like Accept-Language, already after the first roundtrip

Usage Nginx

At the beginning of the server (?) / location block:

http {

  map $cookie_preferred_language $selected_lang {
    default        $cookie_user_lang;
  }

  server {
    proxy_cache frappe;
    proxy_cache_key $scheme$host$request_uri|$selected_lang;
    proxy_cache_valid 200 302 1d;
    proxy_cache_valid 404 1m;
    proxy_cache_lock on;

    add_header X-Cache-Status $upstream_cache_status;
  } 
}

Considerations

Nginx is really quite limited in terms of header and cookie manipulation in relation to its cache subsystem. Better to use Varnish on the edge. rules & example: tbd

no-docs

blaggacao avatar Jun 10 '24 12:06 blaggacao

@ankush with this machinery in place and validated, we could start to Cache-Control: private,max-age=60 some regular GET / POST resources on the hot path, which should make the backend still quite significantly more snappy.

Some candidates:

/api/method/erpnext.accounts.utils.get_fiscal_year  # prior to DOM load for some reason
/api/method/frappe.desk.doctype.event.event.get_events # ok to be 1m stale
/api/method/frappe.desk.desktop.get_workspace_sidebar_items  # quasi immutable on the private cache

Just private caching these three would take of 250ms overall (~3 resources loaded in parallel blocking subsequent loads) in our case.

There's also a redundand request to get_fiscal_year with added entropy at all:

image

Oh and /app/home is just a perfectly cachable minimal envelope DOM clocking in at ~700 ms:

image

blaggacao avatar Jun 12 '24 08:06 blaggacao

see also: https://github.com/frappe/erpnext/pull/41871

blaggacao avatar Jun 12 '24 09:06 blaggacao

Go for it now. The future is promised to no one. - Wayne Dyer


Kindly help move this PR forward. Many thanks from Yours Sincerely!

blaggacao avatar Jun 28 '24 08:06 blaggacao

Fears are nothing more than a state of mind. - Napoleon Hill


Kindly help move this PR forward. Many thanks from Yours Sincerely!

blaggacao avatar Jul 04 '24 09:07 blaggacao

Patience is the mother of all sciences - Unknown ;)

(thanks for your unvaluable work, @blaggacao )

git-avc avatar Jul 04 '24 09:07 git-avc

This wisdome stoically spourts Tstalebot — one week :smile:

blaggacao avatar Jul 04 '24 10:07 blaggacao

The important thing is this: to be able at any moment to sacrifice what we are for what we could become. - Charles Dubois


Kindly help move this PR forward. Many thanks from Yours Sincerely!

blaggacao avatar Aug 02 '24 07:08 blaggacao

An invincible determination can accomplish almost anything and in this lies the great distinction between great men and little men. - Thomas Fuller


Kindly help move this PR forward. Many thanks from Yours Sincerely!

blaggacao avatar Aug 25 '24 06:08 blaggacao

@akhilnarang @surajshetty3416 I'd want to proceed to merge and own this, if you havn't got any objections. Maybe there's someone on the team who should have a look at this, before I do?

Things to maybe review:

  • from_cache flag still useful?
  • No inadverted significant (performance?) consequences of adding the user language to the LoginManager slots
  • Relation to the design of after request hooks

Otherwise, we've been running this to great effect in production behind a varnish edge cache and static pages, but also delivery of website_script.js is notably sped up, usually from the edge and without any special proxy config.

blaggacao avatar Aug 27 '24 11:08 blaggacao

Note to self, before moving ahead: Flip the switch on the client side for dev servers according to this comment (last paragraph) so that Cache-Control is made ineffective (yet still present / visible) during development for these requests.

blaggacao avatar Aug 28 '24 06:08 blaggacao

@akhilnarang Since no notification about potential problems with https://github.com/frappe/frappe/pull/26752 have reached me in the last 2 weeks since its merge, I'd proceed cautiously with this one, next.

I'm running this successful in production for ever since this PR was opened, so I'm more than ready to stand by and address any potential issue asap should they come up.

blaggacao avatar Sep 07 '24 20:09 blaggacao

@blaggacao this is micro-optimization at best that doesn't work as well as you think.

  1. You're adding etag computation overhead on EVERY request. >99% of requests that hit python workers are not cacheable.
  2. You're suggesting that this helps on frequent requests like erpnext.accounts.utils.get_fiscal_year but does it really? All Etags do is tell the client to use cached response if possible. Client still has to wait for a response... you saved what? {"fiscal_year": "2024-25"} 26 bytes of transfer? That's less than the MTU size, it's basically free. The real fix would be fix the godawful code in ERPNExt that needs to make this request frequently in the first place.

Easy fix for framework: This should be opt-in, only functions that explicitly enable caching should have this overhead.

cc @akhilnarang @surajshetty3416

ankush avatar Oct 28 '24 11:10 ankush