Add frontend nginx container for logging and plausible proxying #3108
Conversation
github-actions
bot
added
🧱 stack: api
openverse-bot
added
the
🚦 status: awaiting triage
|
Size Change: +4.21 kB (0%) Total Size: 931 kB
ℹ️ View Unchanged
|
sarayourfriend
added
🟧 priority: high
sarayourfriend
changed the title
github-actions
bot
added
🧱 stack: api
sarayourfriend
force-pushed
the
add/frontend-request-logs
branch
from
223830e to
f50d2bd
Compare
|
Full-stack documentation: https://docs.openverse.org/_preview/3108 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. New files ➕: Changed files 🔄: |
sarayourfriend
force-pushed
the
add/frontend-request-logs
branch
from
f50d2bd to
a647836
Compare
When I tested this PR, I only saw the POST requests sent for analytics. I did not see the logs for requests of the frontend pages when I visited them. So, nothing for a GET request to "localhost:8443" or a server-rendered "localhost:8443/image/?q=cat", for instance. Are those requests supposed to be logged in the |
Yes, the idea is that in production the Nginx container would proxy the Nuxt container and provide us with access logs (and Plausible proxying). To test the logs in this PR you have to access the frontend through the I'm realising the testing instructions aren't clear about this, it just says "visit the |
|
Thank you for clarifications, @sarayourfriend ! Also, it seems that these logs will be pretty noisy if we log every request for every JS file... Edited to add: The default config is like this: I tried adding I only see the json logs like this now: I wonder if we should filter out the |
There was a problem hiding this comment.
The implementation looks good! +1 to all the points @obulat brings up, it would be good to emit just the JSON logs and to remove all of the _nuxt/ access calls as well.
|
Those are great changes, Olga. We should apply them to the API configuration as well. I guess it would just about cut the size of our Nginx logs by at least a third, if not closer to a half! Talk about a real cost saver, especially as we're adding new logging for the frontend. Filtering the |
I hadn't considered how the |
|
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @krysal Excluding weekend1 days, this PR was ready for review 5 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @sarayourfriend, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
| # Keeping this as a separate Dockerfile prevents the need to rely on additional | ||
| # contexts required for the frontend build (namely, `repo_root`). These are | ||
| # possible with compose as of https://github.com/docker/compose/pull/10369 | ||
| # and do appear to work, but it increases the complexity and these features | ||
| # are not yet all available in every containerd implementation or distribution | ||
| # (Debian, for example, still distributes an older version of compose without | ||
| # this new feature). For the sake of keeping the local setup simpler and not | ||
| # requiring contributors to have bleeding-edge Docker and compose plugin versions, | ||
| # we can simply use a separate file for the time being. Once/if we start to use | ||
| # nginx to serve static files for the frontend, we will need to integrate this | ||
| # target into the shared Dockerfile. |
There was a problem hiding this comment.
This is helpful documentation. Should we have it as an issue (potentially blocked) to ensure this is completed at some point?
There was a problem hiding this comment.
Yes 👍 Done here #3180
sarayourfriend
removed
🧱 stack: api
Fixes
Related to #536 by @sarayourfriend
Related to #2996 by @obulat
Related to #2993 (does not close because we also need to deploy the nginx container to live environments)
The issue will only be fixed, if we take this approach, after adding the new service to the live deployments.
Description
Adds a new Nginx image for us to publish and use as a reverse-proxy to the frontend service. This enables two things:
This PR is still a draft because I need to add building and publishing the image to CI.
Testing Instructions
Run
just frontend/upwhich will automatically build the newfrontend_nginxservice, thenjust frontend/run dev. Afterwards, to be able to test Plausible locally, runjust frontend/init. Runjust psand visit thefrontend_nginxURL and confirm you can visit the site and browse it as normal. Audit the nginx logs usingjust logs frontend_nginx. Next, test Plausible by visiting it using the instructions from the Openverse documentation. Confirm that you see the page views as expected.To test that upstream proxying works, modify
OPENVERSE_NGINX_PLAUSIBLE_EVENT_URLto point to upstream (https://plausible.io/api/event) and confirm the requests are forwarded by Nginx.Note that in the Plausible documentation for Nginx proxying they also have you proxy the script. However, we bundle the script into the app via the vue plugin, so this isn't necessary for us to do (as you can see from the original configuration, we were not proxying the script).
Edit: I (@obulat) added the issue that this PR fixes
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin