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(redirects): handle grapher redirects in grapher worker #2826

Merged
merged 4 commits into from
Oct 24, 2023

Conversation

marcelgerber
Copy link
Member

@marcelgerber marcelgerber commented Oct 23, 2023

Work around Cloudflare's 2000 static redirects limit by handling redirects directly in the worker.

This works by baking a grapher/_grapherRedirects.json file as part of the baking process, where all grapher redirects always go.
The original _redirects file will then only contain WP redirects, plus some hardcoded ones.
I checked and we're currently down to just 506 redirects in the file now, so quite some room to spare.

I tried out the following variations and they all work:

  • /grapher/life-expectancy goes through
  • /grapher/life-EXPECTANCY redirects to lowercase
  • /grapher/life-expectancy/ removes trailing slash
  • /grapher/life-expectancy-globally-since-1770 redirects
    • ^ this one also works with uppercase and a trailing slash mixed in
  • /grapher/number-affected-by-natural-disasters redirects to an explorer using WP redirect

Caveats:

  • The thumbnails worker doesn't really take redirects into account, so if you went to /grapher/thumbnail/life-expectancy-globally-since-1770 for example, it would just fail instead of resolving the redirect itself.
  • In the very rare case where we have a grapher redirect and a WP redirect for the same URL, the grapher redirect now takes precedence. This is not a problem at all.

The /grapher/_grapherRedirects.json file looks like this:

{
  "correlation-between-child-mortality-and-mean-years-of-schooling-for-those-aged-15-and-older-2000": "correlation-between-child-mortality-and-mean-years-of-schooling-for-those-aged-15-and-older",
  "us-education-expenditure-as-share-of-gdp-public-and-private": "us-education-expenditure-as-share-of-gdp-public-and-private-institutions",
  "revenues-for-public-schools-by-source-us": "revenues-for-public-schools-by-source-gdp-us",
  // ...
}

Comment on lines +16 to +19
const redirects = await env.ASSETS.fetch(
new URL("/grapher/_grapherRedirects.json", url),
{ cf: { cacheTtl: 2 * 60 } }
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really my biggest question in working on this, what is the best way to efficiently fetch the redirects file, which we're gonna have to await on every request now.
Other options include:

  • Storing redirects in Workers KV, somehow
  • Uploading the redirects.json as part of the worker (ugh, but that's probably actually the fastest way)

I'm also not entirely sure whether the CF caching header will actually do something meaningful here, but it probably can't hurt also, and maybe it can actually speed things up?

Copy link
Contributor

@Marigold Marigold Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised by how fast (uncached) fetch from R2 is, so I'd say that fetching redirects from assets explicitly should be light speed. It could be worth logging latency just to be sure.

@marcelgerber marcelgerber marked this pull request as ready for review October 23, 2023 13:50
@Marigold
Copy link
Contributor

(I'm still getting 404 from http://staging-site-master/grapher/life-expectancy-of-women-vs-life-expectancy-of-women that is shown in life expectancy. It could be just that workers don't work on staging?)

@marcelgerber
Copy link
Member Author

(I'm still getting 404 from http://staging-site-master/grapher/life-expectancy-of-women-vs-life-expectancy-of-women that is shown in life expectancy. It could be just that workers don't work on staging?)

Wrong server, http://staging-site-redirects-handling-worker/grapher/life-expectancy-of-women-vs-life-expectancy-of-women works fine.

@Marigold
Copy link
Contributor

Wrong server, http://staging-site-redirects-handling-worker/grapher/life-expectancy-of-women-vs-life-expectancy-of-women works fine.

You're right; it was a staging server URL issue (fixed by now).

@marcelgerber marcelgerber merged commit 7eade46 into master Oct 24, 2023
23 checks passed
@marcelgerber marcelgerber deleted the redirects-handling-worker branch October 24, 2023 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants