991955 - Put fireplace app API behind CDN

Status: RESOLVED WONTFIX

(Marketplace Graveyard :: API, defect, P2)

Description

[Wil Clouser [:clouserw]]

https://marketplace.firefox.com/api/v1/fireplace/app/404807/ is our regular URL but https://marketplace.cdn.mozilla.net/api/v1/fireplace/app/404807/ is a 403.  Any reason not to CDN this API as well?

Comment 1

[Christopher Van Wiemeersch [:cvan]]

The only reason I can see is that Krupa and some developers probably expect any changes to the app to surface immediately here. With our current max-age of 3 minutes, well, you'd have to wait at most 3 minutes to see your change. I think the benefit outweighs that annoyance. I'm sure we can start coming up with a list of URLs to flush upon modification (I still see old code in AMO that used to do that). But yeah that's my 2¢.

Comment 2

[Mathieu Pillard [:mat]]

Also, this API still uses user authentication, because we need to be able to display a private app if you are the developer or a reviewer. So users that are logged in wouldn't benefit from the CDN (it would actually probably make it slightly slower for them)

Comment 3

[Wil Clouser [:clouserw]]

Hmm, I wouldn't expect that use case a ton - maybe we could check if they are a logged in developer and if so do another request after to a non-CDN url.  I don't know if that's worth it, just thinking out loud.

Comment 4

[Mathieu Pillard [:mat]]

FYI : https://marketplace.firefox.com/api/v1/fireplace/collection/<slug>/  should also go through the CDN.

Comment 5

[Mathieu Pillard [:mat]]

Filed bug 1002471 for the collection API.

Comment 6

[Christopher Van Wiemeersch [:cvan]]

Right now we're using the CDN to cache featured (homepage)/search/category pages. This bug is for the API response for an app detail page. If a user navigates to a detail page from the homepage, search results, or a category landing page, we don't hit this API because we already have the app data from the prior API endpoints. If a user is deeplinked to an app detail page, then we *do* use the data from this endpoint (in which case it should be cached on the CDN).

+1 to doing this but not super important.

Comment 7

[Christopher Van Wiemeersch [:cvan]]

This is really important now that the feed can no longer uses model data from the homepage API response to populate the detail page.

Comment 8

[Christopher Van Wiemeersch [:cvan]]

This continues to be the slowest API response:
https://rpm.newrelic.com/accounts/315282/browser/2914756/bjax_requests#id=733240733&sort_by=response_time

––

> diff --git a/src/media/js/routes_api.js b/src/media/js/routes_api.js
> index 42d3994..1bfb0ab 100644
> --- a/src/media/js/routes_api.js
> +++ b/src/media/js/routes_api.js
> @@ -12,7 +12,7 @@ define('routes_api', [], function() {
>          'feed': '/api/v2/feed/get/?cache=21600&vary=0',
>          'feed-app': '/api/v2/fireplace/feed/apps/{0}/',
>          'feed-brand': '/api/v2/fireplace/feed/brands/{0}/',
> -        'feed-collection': '/api/v2/fireplace/feed/collections/{0}/',
> +        'feed-collection': '/api/v2/fireplace/feed/collections/{0}/?cache=21600&vary=0',
>          'feed-items': '/api/v2/feed/items/',
>          'feed-shelf': '/api/v2/fireplace/feed/shelves/{0}/',
>          'feedback': '/api/v2/account/feedback/',
> @@ -31,8 +31,8 @@ define('routes_api', [], function() {
>          'recommended': '/api/v2/apps/recommend/',
>          'record_free': '/api/v2/installs/record/',
>          'record_paid': '/api/v2/receipts/install/',
> -        'review': '/api/v2/apps/rating/{0}/',
> -        'reviews': '/api/v2/apps/rating/',
> +        'review': '/api/v2/apps/rating/{0}/?cache=1&vary=0',
> +        'reviews': '/api/v2/apps/rating/?cache=1&vary=0',
>          'search': '/api/v2/fireplace/search/?cache=1&vary=0',
>          'settings': '/api/v2/account/settings/mine/',
>          'site-config': '/api/v2/services/config/site/?cache=1&serializer=commonplace&vary=0',
> diff --git a/src/media/js/settings.js b/src/media/js/settings.js
> index 76116a6..d01d0de 100644
> --- a/src/media/js/settings.js
> +++ b/src/media/js/settings.js
> @@ -49,6 +49,8 @@ define('settings', ['l10n', 'settings_local', 'underscore'], function(l10n, sett
>          // These are the only API endpoints that should be served from the CDN
>          // (key: URL; value: max-age in seconds, but it's unused at the moment).
>          api_cdn_whitelist: {
> +            '/api/v2/fireplace/app/': 60 * 3,  // 3 minutes
> +            '/api/v2/fireplace/feed/collections/': 6 * 60 * 60,  // 6 hours
>              '/api/v2/fireplace/search/': 60 * 3,  // 3 minutes
>              '/api/v2/feed/get/': 6 * 60 * 60,  // 6 hours
>              '/api/v2/services/config/site/': 60 * 3  // 3 minutes
> @@ -76,8 +78,10 @@ define('settings', ['l10n', 'settings_local', 'underscore'], function(l10n, sett
>          // these TTLs apply to only when the app is first launched.
>          offline_cache_whitelist: {
>              '/api/v2/feed/get/': 60 * 60 * 24 * 7,  // 1 week
> -            '/api/v2/fireplace/consumer-info/': 60 * 60 * 24 * 7,  // 1 week
> -            '/api/v2/services/config/site/': 60 * 60 * 24 * 7  // 1 week
> +            '/api/v2/fireplace/app/': 60 * 60 * 24 * 7,  // 1 week
> +            '/api/v2/fireplace/feed/collections/': 60 * 60 * 24 * 7,  // 1 week
> +            '/api/v2/fireplace/search/': 60 * 60 * 24 * 7,  // 1 week
> +            '/api/v2/services/config/site/': 60 * 60 * 24 * 7,  // 1 week
>          },
>          offline_cache_enabled: offline_cache_enabled,
>          offline_cache_limit: 1024 * 1024 * 4,  // 4 MB
> 

––

Because of this line https://github.com/mozilla/marketplace-core-modules/blob/f5006da/urls.js#L107 the `baseurl` becomes `/api/v2/fireplace/app/quick-travel/`, which doesn't match the `/api/v2/fireplace/app/` in `settings.api_cdn_whitelist`. So we may need to go back to doing regexes. I'll leave that as an exercise to the assignee.

Comment 9

[Mathieu Pillard [:mat]]

It's not so trivial - As the code is currently, this shouldn't just be cached on the CDN as it's an authenticated call.

Comment 10

[Christopher Van Wiemeersch [:cvan]]

(In reply to Mathieu Pillard [:mat] from comment #9)
> It's not so trivial - As the code is currently, this shouldn't just be
> cached on the CDN as it's an authenticated call.

Oh, I didn't realise that. When I load https://marketplace.firefox.com/app/7-minutos I didn't see the `?_user` in the URL:

     https://marketplace.firefox.com/api/v2/fireplace/app/7-minutos/?cache=1&lang=en-US&limit=25&region=us&vary=0