Cleanup assets serving #1969

zwn · 2014-02-01T13:19:34Z

First commit in this PR fixes https://app.getsentry.com/gittip/gittip/group/13299458/ where we have so far 7.4k of exceptions. The trouble there has been that we were trying to set last modified time even on responses other than 200 (404 for example).

Second commit unifies our http caching strategy (#1555). Before we had two kinds of assets - long term and short term (versioned). For the long term, where we would want the maximum caching, we were not setting expire time. That means that the browser cannot know if the cached version is current or not without a hit to the server. We try to respond with 304 (not modified) but it is still a hit. Moreover we respond 304 not often enough because the decision to do so is based on mtime of the file. With each deploy possibly a new git checkout is made which results in mtime of 'now'. To conclude, the files we would want to be cached the most, were not.

Now all assets have far future expire time that allows the most caching. Our versioned assets (served from assets/%version/*) are invalidated with each deploy automatically. The other assets need to have their own version embedded in url (which is the case with jquery already).

Third commit makes a run to move all assets to CDN. What is not done yet a references from css files where I was not sure how to do the url expansion for website.asset_url.

Fourth folds separate css files into our main gittip.scss file to reduce the number of http requests to make the site speedier.

I'd appreciate a full review and thorough TTW test before merge & release. Thanks.

cc: @seanlinsley @clone1018 @duckinator

zwn · 2014-02-01T13:34:18Z

I have found some more files (more than I thought there were) that reference /assets/ directly so the move to CDN is not complete yet. But we are closer than before 😄. I do not want to stall this on the totality of the move to CDN.

zwn · 2014-02-01T13:36:19Z

cc @galuszkak @Changaco @sim6 - just trying to remember who could be interested in this PR. Anyone feel free to ping anyone else.

zwn · 2014-02-01T13:38:12Z

I was aiming to get a better score at
https://developers.google.com/speed/pagespeed/insights/?url=www.gittip.com&tab=desktop
http://www.webpagetest.org/result/140131_C7_STD/1/details/
We should be better in both after we deploy this.

galuszkak · 2014-02-01T14:35:19Z

Looking onto this.

galuszkak · 2014-02-01T14:48:02Z

Ok I have only one proposal. Wouldn't be better to write template tag/extension to jinja to create assets url (which will do "{base_url}{asset_url}".format(base_url=website.asset_url, asset_url=url) )? it will look something like that:

"{% url '/icons/twitter.16.png' %}"

Than this:

"{{ website.asset_url }}/icons/twitter.16.png"

this will remove also typo problems if we do something accidentally in variable. What do you think @zwn ?
I think this will be more clear and consistent than remember whole variable name.

zwn · 2014-02-01T15:05:43Z

I probably wouldn't mind having something like that. However the added value for me does not outweigh the time I'd spent learning how to do it. Typo problems will be caught by tests anyway and I use auto completion of my editor. So as I said before, maybe nice to have but for me definitely not worth the effort (since I have almost no idea how jinja does its magic).

zwn · 2014-02-01T15:55:20Z

https://developers.google.com/speed/docs/best-practices/rtt some more motivation.

seanlinsley · 2014-02-09T20:15:45Z

This has been sitting for 8 days. I'm going to review this and try and get it merged today.

chadwhitacre · 2014-02-10T22:17:56Z

!m @seanlinsley

seanlinsley · 2014-02-12T05:38:37Z

gittip/utils/cache_static.py

+
+        response.headers['Expires'] = 'Sun, 17 Jan 2038 19:14:07 GMT'
+        last_modified = get_last_modified(request.fs)
+        response.headers['Last-Modified'] = format_date_time(last_modified)


This approach worries me because it sets the Expires header to the far future for all assets, not just the ones in the %version directory. That means that if any of those files change content but not name, we're very likely to break the website for some users.

Is there a reason we're not versioning all of our assets? If it means that when we deploy everyone has a 100% cache miss on their first page load, that doesn't sound so bad.

I tried to address this in the description of this PR (second and mainly third paragraph). Can you reread it and come back to me if still does not make sense? Thanks.

Our versioned assets (served from assets/%version/*) are invalidated with each deploy automatically. The other assets need to have their own version embedded in url (which is the case with jquery already).

That works for jQuery, but there are many other files there that don't have a version number in the file name.

What is stopping us from adding one when we update them? Or do you feel that putting all of them under the %version header is better?

You're suggesting that, for example, if Open Street Map makes a new logo, we'd add it as www/assets/openstreetmap-2.png? A year from now, I doubt we're going to remember to do that.

I think I'd prefer for everything to be under %version. It means a 100% cache miss on the first page after a new deploy, but every page after that has a 100% cache hit (for assets at least).

CC @clone1018

@zwn I'd say go for it.

Ok, I will. Is there anything else standing in the way of merging this?

No, we should be good to go.

Looking at the timestamps, it's now been 6 days since we last talked. Is this still on your radar? If not, I'd be happy to do the last bit of work.

Still on the radar (I would eventually get to it). But feel free to jump in, I'll try to help with the big 'elsewhere' and 'coinbase' PRs.

chadwhitacre · 2014-03-13T17:25:55Z

@zwn Update on this? You are busy, or burned out on Gittip, or something. Haven't seen you for a week or two. :-) Everything okay?

chadwhitacre · 2014-03-14T21:04:46Z

We did a little bit of assets rearranging in #2142 ff.

clone1018 · 2014-03-15T01:49:32Z

I'll be taking over this PR in @zwn's absence.

zwn · 2014-03-15T11:53:33Z

@clone1018 Go ahead. Yeah, I've been busy with other stuff. Hope to be back soon.

chadwhitacre · 2014-03-25T21:56:34Z

scss/chosen.css

@@ -24,7 +24,7 @@
 /* @group Single Chosen */
 .chzn-container-single .chzn-single {
  background-color: #ffffff;
-  filter: progid:DXImageTransform.Microsoft.gradient( startColorstr='#ffffff', endColorstr='#eeeeee', GradientType=0 );   
+  filter: unquote("progid:DXImageTransform.Microsoft.gradient( startColorstr='#ffffff', endColorstr='#eeeeee', GradientType=0 )");


What's the purpose of the unquote here?

Presumably to support importation into the SCSS file?

Yeah, unquote is commonly used to bypass the SCSS parser for IE-specific hacks.

seanlinsley · 2014-04-05T19:43:39Z

@clone1018 as frustrating as the recent downtime was for me, I can't let this PR sit stale any longer. I'm going to try and get this finished up today.

seanlinsley · 2014-04-05T19:44:57Z

Rebased on master (2a26dc9 -> 24ad470)

seanlinsley · 2014-04-05T19:50:16Z

gittip/elsewhere/__init__.py

@@ -99,6 +98,9 @@ def __init__(self, db, api_key, api_secret, callback_url, api_url=None, auth_url
        elif api_format:
            raise ValueError('unknown API format: '+str(api_format))

+        # TODO: make this use website.asset_url!
+        self.icon = '/assets/icons/%s.16.png' % self.name
+


@whit537 thoughts?

Got this sorted with @pjz's help in IRC

seanlinsley · 2014-04-05T21:47:25Z

Besides the failing tests and gittip/elsewhere/__init__.py, this is complete 🍺

@whit537 IRT #1767, the first widget relies on an old version of jQuery that hasn't been provided for a long time :-/

@seanlinsley

Note that while rebasing this commit, I (@seanlinsley) added a good deal of extra changes since we were still using the static "/assets/foo" URL in a lot of other places.

This doesn't version the old widgets because people may still be using them. (ref: #1767) Except... the first widget refers to an old version of jQuery that we haven't provided in a long time :-/ This commit also fixes reset.css and chosen.css to correctly load.

and start using that name for `asset_version_url`

seanlinsley · 2014-04-06T16:46:28Z

Reabsed on master after #2242 (7bebe6b -> 8e3defc)

seanlinsley · 2014-04-08T02:19:05Z

So, the tests are failing because they don't have access to website. How do I fix that?

chadwhitacre · 2014-04-08T13:14:51Z

@seanlinsley The hacky way to fix that is to change get_avatar_url to take a second parameter, and pass website in every place we call get_avatar_url. A cleaner solution would require more refactoring than I think we want to get into here.

seanlinsley · 2014-04-08T16:51:12Z

What sort of refactoring would make it so that's not required?

chadwhitacre · 2014-04-08T19:49:42Z

@seanlinsley This is an old PR. I don't think we should block on any refactoring here.

seanlinsley · 2014-04-08T19:57:01Z

You didn't answer my question :-P. I'm genuinely curious what you had in mind.

that way it has access to the website object by default

Cleanup assets serving

chadwhitacre · 2014-04-14T01:36:04Z

Very happy this is out, thank you! 💃

!m @zwn @seanlinsley @clone1018

seanlinsley reviewed Feb 12, 2014
View reviewed changes

clone1018 added 3 - Work in Progress and removed 4 - Review labels Mar 15, 2014

clone1018 self-assigned this Mar 15, 2014

chadwhitacre reviewed Mar 25, 2014
View reviewed changes

seanlinsley assigned seanlinsley and unassigned clone1018 Apr 5, 2014

seanlinsley reviewed Apr 5, 2014
View reviewed changes

zwn and others added 6 commits April 6, 2014 11:41

Cache only 200 results in /assets/*.

3c6dff8

Cache all assets.

5f0b8d2

Move all assets to CDN.

58c271f

Note that while rebasing this commit, I (@seanlinsley) added a good deal of extra changes since we were still using the static "/assets/foo" URL in a lot of other places.

Fold chosen.css and reset.css into gittip.scss.

860a9f8

remove unused assets

d80adb3

version all assets

39ff7ac

This doesn't version the old widgets because people may still be using them. (ref: #1767) Except... the first widget refers to an old version of jQuery that we haven't provided in a long time :-/ This commit also fixes reset.css and chosen.css to correctly load.

seanlinsley added 2 commits April 6, 2014 11:44

remove now-unused asset_url

4509546

and start using that name for `asset_version_url`

use asset_url for elsewhere icons

8e3defc

seanlinsley mentioned this pull request Apr 9, 2014

Deal with site load better #2266

Closed

turn get_avatar_url into a Jinja macro

587e763

that way it has access to the website object by default

clone1018 added a commit that referenced this pull request Apr 12, 2014

Merge pull request #1969 from gittip/fix-cache

9f6e573

Cleanup assets serving

clone1018 merged commit 9f6e573 into master Apr 12, 2014

clone1018 deleted the fix-cache branch April 12, 2014 22:12

seanlinsley mentioned this pull request Apr 13, 2014

KeyError: 'Vary' #2272

Closed

seanlinsley removed the 3 - Work in Progress label Apr 13, 2014

seanlinsley mentioned this pull request Apr 27, 2014

#2041 revisited #2332

Merged

Cleanup assets serving #1969

Cleanup assets serving #1969

Conversation

zwn commented Feb 1, 2014

zwn commented Feb 1, 2014

zwn commented Feb 1, 2014

zwn commented Feb 1, 2014

galuszkak commented Feb 1, 2014

galuszkak commented Feb 1, 2014

zwn commented Feb 1, 2014

zwn commented Feb 1, 2014

seanlinsley commented Feb 9, 2014

chadwhitacre commented Feb 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadwhitacre commented Mar 13, 2014

chadwhitacre commented Mar 14, 2014

clone1018 commented Mar 15, 2014

zwn commented Mar 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlinsley commented Apr 5, 2014

seanlinsley commented Apr 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlinsley commented Apr 5, 2014

seanlinsley commented Apr 6, 2014

seanlinsley commented Apr 8, 2014

chadwhitacre commented Apr 8, 2014

seanlinsley commented Apr 8, 2014

chadwhitacre commented Apr 8, 2014

seanlinsley commented Apr 8, 2014

chadwhitacre commented Apr 14, 2014