From 1369d39a4d16e59334f30f30c6b634fc7bfa7386 Mon Sep 17 00:00:00 2001 From: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com> Date: Wed, 14 Dec 2022 08:59:01 +1100 Subject: [PATCH] Auto-format markdown; switch to double quotes for JS (#349) * Add markdown to pre-commit prettier * Prettify markdown; use single quotes --- .github/GITHUB.md | 14 +- .github/ISSUE_TEMPLATE/bug_report.md | 17 +- .github/ISSUE_TEMPLATE/discussion.md | 3 + .github/ISSUE_TEMPLATE/feature_request.md | 4 + .github/PULL_REQUEST_TEMPLATE.md | 19 +- .pre-commit-config.yaml | 3 +- .pre-commit-config.yaml.jinja | 5 +- CONTRIBUTING.md | 291 +++++-- README.md | 23 +- automations/README.md | 6 +- automations/python/README.md | 2 +- brand/README.md | 4 +- justfile | 3 +- load_testing/README.md | 32 +- load_testing/k6/main.js | 42 +- load_testing/k6/search.js | 24 +- load_testing/k6/utils.js | 20 +- prettier.config.js | 9 +- prettier.config.js.jinja | 9 +- rfcs/20220217-visual_regression_testing.md | 481 +++++++---- ...8-introduce_ui_state_cookie_to_frontend.md | 40 +- rfcs/20220221-3d_model_support.md | 296 ++++--- ...23-convert_the_store_from_vuex_to_pinia.md | 238 ++++-- rfcs/20220307-monitoring_across_the_stack.md | 757 +++++++++++++----- rfcs/20220309-feature_flags.md | 178 ++-- rfcs/README.md | 55 +- 26 files changed, 1835 insertions(+), 740 deletions(-) diff --git a/.github/GITHUB.md b/.github/GITHUB.md index 168caad1db8..060af47fb48 100644 --- a/.github/GITHUB.md +++ b/.github/GITHUB.md @@ -16,8 +16,8 @@ to the "In progress" column. ### PR project automation This workflow archives PRs in the "Merged!" and "Closed" columns of the PR -project board, 15 minutes after the commencement of the weekly developer chat -at 15:00 UTC every Tuesday. +project board, 15 minutes after the commencement of the weekly developer chat at +15:00 UTC every Tuesday. **Cron:** [at 15:15 on Tuesday](https://crontab.guru/#15_15_*_*_2) @@ -40,9 +40,9 @@ week, outlining the closed issues and merged PRs of the preceding week. ### New discussion notification -This workflow makes a `POST` request to the Slack webhook when a new -discussion is created, sending a notification message to the -`#openverse-notifications` channel. +This workflow makes a `POST` request to the Slack webhook when a new discussion +is created, sending a notification message to the `#openverse-notifications` +channel. **Discussion:** created @@ -54,7 +54,7 @@ truth, it creates PRs to resolve any differences. **Cron:** [at 00:00](https://crontab.guru/#0_0_*_*_*) **Push:** Branch `main` -**Dispatch:** enabled +**Dispatch:** enabled **Config:** `.github/sync.yml` ## Synced workflows @@ -101,5 +101,5 @@ These workflows run only in the downstream synced repos and not in `openverse`. This workflow updates the draft release message when new commits are added to `main` so that there is a ready-to-go changelog when publishing a new release. -**Push:** Branch `main` +**Push:** Branch `main` **Config:** `.github/release_drafter.yml` diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index 980cda441f0..cffdd1d814f 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -6,28 +6,35 @@ title: "" --- ## Description + ## Reproduction + + 1. 2. 3. 4. See error. ## Screenshots + ## Environment + - - Device: - - OS: - - Browser: - - Version: - - Other info: + +- Device: +- OS: +- Browser: +- Version: +- Other info: ## Additional context + diff --git a/.github/ISSUE_TEMPLATE/discussion.md b/.github/ISSUE_TEMPLATE/discussion.md index 79344d2c4ec..ca91bba132d 100644 --- a/.github/ISSUE_TEMPLATE/discussion.md +++ b/.github/ISSUE_TEMPLATE/discussion.md @@ -6,13 +6,16 @@ title: "" --- + ## Due date: yyyy-mm-dd + ## Assigned reviewers - [ ] TBD - [ ] TBD + ## Description diff --git a/.github/ISSUE_TEMPLATE/feature_request.md b/.github/ISSUE_TEMPLATE/feature_request.md index 0acab425c22..ab70479470e 100644 --- a/.github/ISSUE_TEMPLATE/feature_request.md +++ b/.github/ISSUE_TEMPLATE/feature_request.md @@ -6,15 +6,19 @@ title: "" --- ## Problem + ## Description + ## Alternatives + ## Additional context + diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index accf40a74ca..dc5a5773b06 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,28 +1,39 @@ ## Fixes + + Fixes #[issue number] by @[issue author] ## Description + ## Testing Instructions + ## Checklist + -- [ ] My pull request has a descriptive title (not a vague title like `Update index.md`). -- [ ] My pull request targets the *default* branch of the repository (`main`) or a parent feature branch. + +- [ ] My pull request has a descriptive title (not a vague title like + `Update index.md`). +- [ ] My pull request targets the _default_ branch of the repository (`main`) or + a parent feature branch. - [ ] My commit messages follow [best practices][best_practices]. - [ ] My code follows the established code style of the repository. - [ ] I added or updated tests for the changes I made (if applicable). - [ ] I added or updated documentation (if applicable). -- [ ] I tried running the project locally and verified that there are no visible errors. +- [ ] I tried running the project locally and verified that there are no visible + errors. -[best_practices]:https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines +[best_practices]: + https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines ## Developer Certificate of Origin +
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 373c849d8ed..08273177619 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -68,7 +68,7 @@ repos: hooks: - id: eslint files: ^js/.*$ - 'types': [file] # ESLint only accepts [javascript] by default. + "types": [file] # ESLint only accepts [javascript] by default. args: - --ignore-path=.gitignore - --ignore-path=.eslintignore @@ -82,7 +82,6 @@ repos: rev: v2.6.0 hooks: - id: prettier - exclude: \.md$ # TODO: https://github.com/WordPress/openverse/issues/333 additional_dependencies: - prettier@2.6.0 diff --git a/.pre-commit-config.yaml.jinja b/.pre-commit-config.yaml.jinja index 8fafdbaa205..1941ff88dcb 100644 --- a/.pre-commit-config.yaml.jinja +++ b/.pre-commit-config.yaml.jinja @@ -76,11 +76,11 @@ repos: rev: v8.15.0 hooks: - id: eslint - files: {{ eslint_files | default('^js/.*$') }} + files: {{ eslint_files | default("^js/.*$") }} {% if eslint_exclude %} exclude: {{ eslint_exclude }} # ESLint raises warnings for ignored files. {% endif %} - 'types': [file] # ESLint only accepts [javascript] by default. + "types": [file] # ESLint only accepts [javascript] by default. args: - --ignore-path=.gitignore - --ignore-path=.eslintignore @@ -98,7 +98,6 @@ repos: rev: v2.6.0 hooks: - id: prettier - exclude: \.md$ # TODO: https://github.com/WordPress/openverse/issues/333 additional_dependencies: {% block prettier_dependencies %} - prettier@2.6.0 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5d2329f59bd..eca0d50d760 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,153 +1,286 @@ ## ✨ Welcome -Thank you for your interest in Openverse! We're so excited to bring new contributors into the fold in WordPress, Openverse and FOSS in general. +Thank you for your interest in Openverse! We're so excited to bring new +contributors into the fold in WordPress, Openverse and FOSS in general. ## ❤️ Code of Conduct -Please be aware that as a project of the WordPress Foundation, Openverse follows [the WordPress Etiquette](https://wordpress.org/about/etiquette/), a code of conduct that you should read and agree to abide by before contributing to WordPress projects. This applies to all Openverse respositories. +Please be aware that as a project of the WordPress Foundation, Openverse follows +[the WordPress Etiquette](https://wordpress.org/about/etiquette/), a code of +conduct that you should read and agree to abide by before contributing to +WordPress projects. This applies to all Openverse respositories. ## 🎨 Design contributions ### How to contribute with design -If you'd like to contribute to the design, feel free to propose a solution to an existing problem labeled with [Needs Design](https://github.com/WordPress/openverse-frontend/issues?q=is%3Aopen+is%3Aissue+label%3A%22needs+design%22), or share an idea if you think it meets Openverse's goals. - -The [WordPress Design team](http://make.wordpress.org/design/) uses [Figma](https://www.figma.com/) to collaborate and share work for all WordPress projects. If you are not familiar with designing for WordPress, please carefully read the [design handbook](https://make.wordpress.org/design/handbook/). Once you have a [WordPress Slack](https://make.wordpress.org/chat/) account, join the [#design channel](http://wordpress.slack.com/messages/design/) and ask the team to set you up with a free Figma account. - -This will give you access to [all projects and files](https://www.figma.com/files/team/642003996626140596/WordPress.org?fuid=968867265893371002) used in WordPress. - -Before designing a proposal, browse the [Design Library](https://www.figma.com/file/GIIQ4sDbaToCfFQyKMvzr8/Openverse-Design-Library?node-id=0%3A1) file to understand how Openverse has been built, and take a look at the [created work](https://www.figma.com/files/project/31962071/Openverse?fuid=968867265893371002) to get a glance at how design ideas are made. As the design onboarding section in the design library file is constantly being added to and improved, some documentation may be missing. If you have doubts, ask on [#design channel](http://wordpress.slack.com/messages/design/) for clarification. If you discover new information that is yet to be documented, contributing this information back to the documentation is very much appreciated. - -Once you are done and ready to share your idea, [create an issue with the `design` label and fill in the template](https://github.com/WordPress/openverse-frontend/issues/new?assignees=&labels=🖼️+aspect%3A+design%2C✨+goal%3A+improvement%2C+🚦+status%3A+awaiting+triage&template=feature_request.md&title=). Please be as specific and concise as possible and feel free to add mockups, prototypes, videos, sketches, and anything that makes your idea easier to understand. - -After creating the issue, it will be labeled with `aspect: design`. Please reference [existing design issues](https://github.com/WordPress/openverse-frontend/issues?q=is%3Aopen+is%3Aissue+label%3A%22%F0%9F%96%BC%EF%B8%8F+aspect%3A+design%22) as a guide for how to describe your solution and to understand how the discussion evolves before implementation begins. +If you'd like to contribute to the design, feel free to propose a solution to an +existing problem labeled with +[Needs Design](https://github.com/WordPress/openverse-frontend/issues?q=is%3Aopen+is%3Aissue+label%3A%22needs+design%22), +or share an idea if you think it meets Openverse's goals. + +The [WordPress Design team](http://make.wordpress.org/design/) uses +[Figma](https://www.figma.com/) to collaborate and share work for all WordPress +projects. If you are not familiar with designing for WordPress, please carefully +read the [design handbook](https://make.wordpress.org/design/handbook/). Once +you have a [WordPress Slack](https://make.wordpress.org/chat/) account, join the +[#design channel](http://wordpress.slack.com/messages/design/) and ask the team +to set you up with a free Figma account. + +This will give you access to +[all projects and files](https://www.figma.com/files/team/642003996626140596/WordPress.org?fuid=968867265893371002) +used in WordPress. + +Before designing a proposal, browse the +[Design Library](https://www.figma.com/file/GIIQ4sDbaToCfFQyKMvzr8/Openverse-Design-Library?node-id=0%3A1) +file to understand how Openverse has been built, and take a look at the +[created work](https://www.figma.com/files/project/31962071/Openverse?fuid=968867265893371002) +to get a glance at how design ideas are made. As the design onboarding section +in the design library file is constantly being added to and improved, some +documentation may be missing. If you have doubts, ask on +[#design channel](http://wordpress.slack.com/messages/design/) for +clarification. If you discover new information that is yet to be documented, +contributing this information back to the documentation is very much +appreciated. + +Once you are done and ready to share your idea, +[create an issue with the `design` label and fill in the template](https://github.com/WordPress/openverse-frontend/issues/new?assignees=&labels=🖼️+aspect%3A+design%2C✨+goal%3A+improvement%2C+🚦+status%3A+awaiting+triage&template=feature_request.md&title=). +Please be as specific and concise as possible and feel free to add mockups, +prototypes, videos, sketches, and anything that makes your idea easier to +understand. + +After creating the issue, it will be labeled with `aspect: design`. Please +reference +[existing design issues](https://github.com/WordPress/openverse-frontend/issues?q=is%3Aopen+is%3Aissue+label%3A%22%F0%9F%96%BC%EF%B8%8F+aspect%3A+design%22) +as a guide for how to describe your solution and to understand how the +discussion evolves before implementation begins. ## 📖 Translation contributions You can also contribute to Openverse by translating it. -An overview of Openverse translations is here: [https://translate.wordpress.org/projects/meta/openverse/](https://translate.wordpress.org/projects/meta/openverse/) +An overview of Openverse translations is here: +[https://translate.wordpress.org/projects/meta/openverse/](https://translate.wordpress.org/projects/meta/openverse/) -A getting started guide for translating on GlotPress (the software behind [translate.wordpress.org](http://translate.wordpress.org)) is here: [https://make.wordpress.org/polyglots/handbook/translating/glotpress-translate-wordpress-org/#getting-started](https://make.wordpress.org/polyglots/handbook/translating/glotpress-translate-wordpress-org/#getting-started) +A getting started guide for translating on GlotPress (the software behind +[translate.wordpress.org](http://translate.wordpress.org)) is here: +[https://make.wordpress.org/polyglots/handbook/translating/glotpress-translate-wordpress-org/#getting-started](https://make.wordpress.org/polyglots/handbook/translating/glotpress-translate-wordpress-org/#getting-started) -## ☁️ Provider contributions +## ☁️ Provider contributions -Openverse is powered by upstream providers of openly licensed media. You can help expand Openverse by identifying sources of Creative Commons licensed media - we're always looking to broaden our dataset. +Openverse is powered by upstream providers of openly licensed media. You can +help expand Openverse by identifying sources of Creative Commons licensed +media - we're always looking to broaden our dataset. [Our currently list of providers which have been identified but are not yet being ingested can be found here](https://github.com/WordPress/openverse-catalog/issues?q=is%3Aopen+is%3Aissue+label%3A%22%E2%98%81%EF%B8%8F+provider%3A+any%22%2C%22%E2%98%81%EF%B8%8F+provider%3A+audio%22%2C%22%E2%98%81%EF%B8%8F+provider%3A+images%22%2C%22%E2%98%81%EF%B8%8F+provider%3A+text%22%2C%22%E2%98%81%EF%B8%8F+provider%3A+video%22%2C%22%E2%98%81%EF%B8%8F+provider%3A+3D+models%22). -You can use the [New Source Suggestion for Openverse](https://github.com/WordPress/openverse-catalog/issues/new?assignees=&labels=%F0%9F%9A%A6+status%3A+awaiting+triage%2C%F0%9F%A7%B9+status%3A+ticket+work+required%2C%E2%98%81%EF%B8%8F+provider%3A+any&template=new_source_suggestion.yml&title=%3CSource+name+here%3E) issue template to submit whatever sources you find. +You can use the +[New Source Suggestion for Openverse](https://github.com/WordPress/openverse-catalog/issues/new?assignees=&labels=%F0%9F%9A%A6+status%3A+awaiting+triage%2C%F0%9F%A7%B9+status%3A+ticket+work+required%2C%E2%98%81%EF%B8%8F+provider%3A+any&template=new_source_suggestion.yml&title=%3CSource+name+here%3E) +issue template to submit whatever sources you find. ## 💻️ Code contributions ### ⛳️ How to contribute with code -Any issues labeled as "good first issue" or "help wanted" in our repositories are all up for grabs. Just add a comment with `@WordPress/openverse-maintainers` on the issue with questions or requesting the issue be assigned to you when you're ready to work on it. We also regularly discuss Openverse development issues outside the meeting times (like debugging problems we're having while working through an issue). +Any issues labeled as "good first issue" or "help wanted" in our repositories +are all up for grabs. Just add a comment with `@WordPress/openverse-maintainers` +on the issue with questions or requesting the issue be assigned to you when +you're ready to work on it. We also regularly discuss Openverse development +issues outside the meeting times (like debugging problems we're having while +working through an issue). ### ⛺ Keeping in Touch -For folks who'd like to be involved in our regular development conversations, please feel free to join us in the WordPress make chat ([https://make.wordpress.org/chat/](https://make.wordpress.org/chat/)). You'll find us in the #openverse channel, of course! We have weekly community meetings to discuss ongoing work and assign new issues and, every four weeks, a prioritization meeting to review our current projects and roadmap. The sessions are on Tuesdays at 15:00 UTC ([https://everytimezone.com/s/d1d42c7b](https://everytimezone.com/s/d1d42c7b)) and all are welcome! Attendance is not mandatory to contribute, however. - -You can also keep in touch with [progress](https://github.com/orgs/WordPress/projects/3) and the latest updates to the project with our [WordPress contributor group](https://make.wordpress.org/openverse/). +For folks who'd like to be involved in our regular development conversations, +please feel free to join us in the WordPress make chat +([https://make.wordpress.org/chat/](https://make.wordpress.org/chat/)). You'll +find us in the #openverse channel, of course! We have weekly community meetings +to discuss ongoing work and assign new issues and, every four weeks, a +prioritization meeting to review our current projects and roadmap. The sessions +are on Tuesdays at 15:00 UTC +([https://everytimezone.com/s/d1d42c7b](https://everytimezone.com/s/d1d42c7b)) +and all are welcome! Attendance is not mandatory to contribute, however. + +You can also keep in touch with +[progress](https://github.com/orgs/WordPress/projects/3) and the latest updates +to the project with our +[WordPress contributor group](https://make.wordpress.org/openverse/). ### 🎯 Choosing an issue -Below is a link to a list of issues that encompass a variety of different technologies, from Airflow to Django to GitHub Actions to Vue.js. You're welcome to take on any of the issues. Most require development environment set up, but some do not. Expect development environment set up to take 1 to 2 hours depending on experience and how much software you've already got installed. If you have git, a text editor/IDE, Python or Node and Docker set up, things will go a little more smoothly. If not, that's totally fine! We're here to help and our repositories all have README files that should include helpful instructions for self-guided environment set up. +Below is a link to a list of issues that encompass a variety of different +technologies, from Airflow to Django to GitHub Actions to Vue.js. You're welcome +to take on any of the issues. Most require development environment set up, but +some do not. Expect development environment set up to take 1 to 2 hours +depending on experience and how much software you've already got installed. If +you have git, a text editor/IDE, Python or Node and Docker set up, things will +go a little more smoothly. If not, that's totally fine! We're here to help and +our repositories all have README files that should include helpful instructions +for self-guided environment set up. -Once you've picked an issue to work on, please leave a comment saying so on the issue and include `@openverse-maintainers` in the comment. That way one of us will get a notification and can assign the issue to you. +Once you've picked an issue to work on, please leave a comment saying so on the +issue and include `@openverse-maintainers` in the comment. That way one of us +will get a notification and can assign the issue to you. ### 🤓 Initial set up #### 🪟 Note for Windows users -Windows Subsystem for Linux can be a much more versatile and familiar environment for software development on Windows. Everything from installing `git` and other dependencies to using command line tools that will be familiar to the wider community of software developers are more likely to be easier under WSL. While some parts of some Openverse projects may be able to be developed under native Windows, you will have a much smoother time with WSL as our command runners (`just` and `pnpm`) assume a unix-type environment (Linux and macOS). We would like to support native Windows development, but we do not currently have any active contributors who use Windows on a daily basis and do not have the time to spend supporting it. Likewise, many other OSS projects will not be configured to allow easy local development under Windows. WSL can help there as well. - -Installation instructions for WSL on Windows 10 and 11 can be found [here on Microsoft's official documentation website](https://docs.microsoft.com/en-us/windows/wsl/install). - -Of course, if you would like to contribute to Openverse by making our projects work natively on Windows, PRs for this are always welcome. +Windows Subsystem for Linux can be a much more versatile and familiar +environment for software development on Windows. Everything from installing +`git` and other dependencies to using command line tools that will be familiar +to the wider community of software developers are more likely to be easier under +WSL. While some parts of some Openverse projects may be able to be developed +under native Windows, you will have a much smoother time with WSL as our command +runners (`just` and `pnpm`) assume a unix-type environment (Linux and macOS). We +would like to support native Windows development, but we do not currently have +any active contributors who use Windows on a daily basis and do not have the +time to spend supporting it. Likewise, many other OSS projects will not be +configured to allow easy local development under Windows. WSL can help there as +well. + +Installation instructions for WSL on Windows 10 and 11 can be found +[here on Microsoft's official documentation website](https://docs.microsoft.com/en-us/windows/wsl/install). + +Of course, if you would like to contribute to Openverse by making our projects +work natively on Windows, PRs for this are always welcome. ### ✍️ General set up -Skip any steps you've already completed on your own. This is meant as an exhaustive list for brand-new devs who might not have these tools yet. +Skip any steps you've already completed on your own. This is meant as an +exhaustive list for brand-new devs who might not have these tools yet. 1. Install `git` - * Most Linux distributions and macOS will already have this installed. Open up your terminal app and type `which git`. If the response is anything other than `git not found` then you've already got it installed. - * If you need to install it, follow the official instructions here: https://git-scm.com/downloads + - Most Linux distributions and macOS will already have this installed. Open + up your terminal app and type `which git`. If the response is anything + other than `git not found` then you've already got it installed. + - If you need to install it, follow the official instructions here: + https://git-scm.com/downloads 1. Install a text editor. - * [VSCode](https://code.visualstudio.com/) is an option with good out-of-the-box support for our entire stack. - * [PyCharm](https://www.jetbrains.com/pycharm/) is another very popular option with lots of bells and whistles. - * [Sublime Text](https://www.sublimetext.com/) is a minimalistic option that can get you off the ground quickly with lots of room for expansion through it's package system. - * [vim](https://www.vim.org/) and [emacs](https://www.gnu.org/software/emacs/) are ubiquitous terminal-based options. + - [VSCode](https://code.visualstudio.com/) is an option with good + out-of-the-box support for our entire stack. + - [PyCharm](https://www.jetbrains.com/pycharm/) is another very popular + option with lots of bells and whistles. + - [Sublime Text](https://www.sublimetext.com/) is a minimalistic option that + can get you off the ground quickly with lots of room for expansion through + it's package system. + - [vim](https://www.vim.org/) and + [emacs](https://www.gnu.org/software/emacs/) are ubiquitous terminal-based + options. 1. Create a GitHub account 1. Install pyenv or Anaconda - * These tools make it simpler to manage multiple different versions of Python on a single machine. This is useful for contributing to multiple projects, each of which may be using a different specific version of Python. - * Skip this if you only want to do a JavaScript issue - * https://github.com/pyenv/pyenv - * https://www.anaconda.com/products/individual + - These tools make it simpler to manage multiple different versions of Python + on a single machine. This is useful for contributing to multiple projects, + each of which may be using a different specific version of Python. + - Skip this if you only want to do a JavaScript issue + - https://github.com/pyenv/pyenv + - https://www.anaconda.com/products/individual 1. Install volta - * Similar to pyenv/Anaconda but for Node.js - * Skip this if you only want to do a Python issue - * https://volta.sh/ + - Similar to pyenv/Anaconda but for Node.js + - Skip this if you only want to do a Python issue + - https://volta.sh/ 1. Install Docker - * Skip this if you only want to do a JavaScript issue - * Windows with WSL: https://docs.microsoft.com/en-us/windows/wsl/tutorials/wsl-containers#install-docker-desktop - * All other scenarios (macOS, Linux, Native Windows): https://docs.docker.com/engine/install/ -1. Choose an issue from the list below and add a comment as described in the second paragraph of the "Choosing an issue" section above + - Skip this if you only want to do a JavaScript issue + - Windows with WSL: + https://docs.microsoft.com/en-us/windows/wsl/tutorials/wsl-containers#install-docker-desktop + - All other scenarios (macOS, Linux, Native Windows): + https://docs.docker.com/engine/install/ +1. Choose an issue from the list below and add a comment as described in the + second paragraph of the "Choosing an issue" section above 1. Fork the repository for the issue you chose - * Go to the repository page on GitHub and find the "Fork" button in the upper corner of the page. Click this and GitHub will guide you through the process of forking the repository + - Go to the repository page on GitHub and find the "Fork" button in the upper + corner of the page. Click this and GitHub will guide you through the + process of forking the repository 1. Clone the repository into your computer - * On the page for your fork, find the "<> Code" dropdown button. Click it and copy the link provided under the "Local" tab of the dropdown. - * Open up your terminal app and type `git clone ` replacing `` with the URL you copied from the GitHub website. -1. Follow the environment set up instructions in the README or CONTRIBUTING document in the repository + - On the page for your fork, find the "<> Code" dropdown button. Click it and + copy the link provided under the "Local" tab of the dropdown. + - Open up your terminal app and type `git clone ` replacing + `` with the URL you copied from the GitHub website. +1. Follow the environment set up instructions in the README or CONTRIBUTING + document in the repository 1. Start working on the issue you chose 🎉 ### 🏃 Good first issues -Most of these issues are potentially able to be completed in less than 4 hours, development environment set up included. It may take significantly more or less than 4 hours depending on experience and how smoothly development environment set up goes. Unfortunately sometimes dev env set up can sometimes be tricky! In these cases it would be helpful to the Openverse project to share your experience in an issue so we can try to remove any roadblocks for future contributors. **You can make meaningful contributions to the project in the form of valuable feedback about the development experience!** +Most of these issues are potentially able to be completed in less than 4 hours, +development environment set up included. It may take significantly more or less +than 4 hours depending on experience and how smoothly development environment +set up goes. Unfortunately sometimes dev env set up can sometimes be tricky! In +these cases it would be helpful to the Openverse project to share your +experience in an issue so we can try to remove any roadblocks for future +contributors. **You can make meaningful contributions to the project in the form +of valuable feedback about the development experience!** [List of Good First Issues](https://github.com/issues?q=is%3Aopen+is%3Aissue+repo%3AWordPress%2Fopenverse-catalog+repo%3AWordPress%2Fopenverse+repo%3AWordPress%2Fopenverse-api+repo%3AWordPress%2Fopenverse-frontend+label%3A%22good+first+issue%22+label%3A%22help+wanted%22+-label%3A%22%E2%9B%94+status%3A+blocked%22+-label%3A%22%F0%9F%94%92+staff+only%22+) ### 🤗 Friendly note for new comers -1. It is totally ok and even encouraged to do more than one "[good first issue](https://github.com/issues?q=is%3Aopen+is%3Aissue+repo%3AWordPress%2Fopenverse-catalog+repo%3AWordPress%2Fopenverse+repo%3AWordPress%2Fopenverse-api+repo%3AWordPress%2Fopenverse-frontend+label%3A%22good+first+issue%22+label%3A%22help+wanted%22+-label%3A%22%E2%9B%94+status%3A+blocked%22+-label%3A%22%F0%9F%94%92+staff+only%22+)" -2. All are welcome to write issues and the Openverse maintainers have deep gratitude for those who do. If you see something that could be worked on, each repository has a set of issue templates to help you get started writing issues: - * [`WordPress/openverse-frontend`](https://github.com/wordpress/openverse-frontend/issues/new/choose) - * [`WordPress/openverse-api`](https://github.com/wordpress/openverse-api/issues/new/choose) - * [`WordPress/openverse-catalog`](https://github.com/wordpress/openverse-catalog/issues/new/choose) - * [`WordPress/openverse`](https://github.com/wordpress/openverse/issues/new/choose) -3. New contributors are welcome and invited to provide feedback on pull requests. You can start by just asking questions! It's great to get to know the project and helps PR authors by uncovering unspoken or undocumented assumptions that exist about the project. It is frequently folks who know the least about and are newest to a project that ask the most helpful questions in this regard. -4. Don't hesitate to ask for help! In [WordPress Make Slack](https://make.wordpress.org/chat/) or in a draft PR, if you're stuck, we're here for you. Everyone is learning and thinking things through all the time. Each Openverse repository has an `@` alias you can ping for help in any issue or pull request, draft or otherwise: - * `@WordPress/openverse-api` - * `@WordPress/openverse-catalog` - * `@WordPress/openverse-frontend` - * etc. -5. On the technical side, there are a ton of [`just`](https://github.com/casey/just) recipes to help you out. If you run `just` on its own in any Openverse repository, it will show you what recipes are available and what they do. `just lint`, `just test`, `just recreate`, we all run these so frequently! - ``` - # Run `just` in any of the Openverse repositories to see a list of available commands. - # This example is for the `WordPress/openverse` repository. - > just - Available recipes: - default # Show all available recipes - install # Install Python dependencies in Pipenv environments and JS dependencies - precommit # Setup pre-commit as a Git hook - lint # Run pre-commit to lint and reformat all files - ``` +1. It is totally ok and even encouraged to do more than one + "[good first issue](https://github.com/issues?q=is%3Aopen+is%3Aissue+repo%3AWordPress%2Fopenverse-catalog+repo%3AWordPress%2Fopenverse+repo%3AWordPress%2Fopenverse-api+repo%3AWordPress%2Fopenverse-frontend+label%3A%22good+first+issue%22+label%3A%22help+wanted%22+-label%3A%22%E2%9B%94+status%3A+blocked%22+-label%3A%22%F0%9F%94%92+staff+only%22+)" +2. All are welcome to write issues and the Openverse maintainers have deep + gratitude for those who do. If you see something that could be worked on, + each repository has a set of issue templates to help you get started writing + issues: + - [`WordPress/openverse-frontend`](https://github.com/wordpress/openverse-frontend/issues/new/choose) + - [`WordPress/openverse-api`](https://github.com/wordpress/openverse-api/issues/new/choose) + - [`WordPress/openverse-catalog`](https://github.com/wordpress/openverse-catalog/issues/new/choose) + - [`WordPress/openverse`](https://github.com/wordpress/openverse/issues/new/choose) +3. New contributors are welcome and invited to provide feedback on pull + requests. You can start by just asking questions! It's great to get to know + the project and helps PR authors by uncovering unspoken or undocumented + assumptions that exist about the project. It is frequently folks who know the + least about and are newest to a project that ask the most helpful questions + in this regard. +4. Don't hesitate to ask for help! In + [WordPress Make Slack](https://make.wordpress.org/chat/) or in a draft PR, if + you're stuck, we're here for you. Everyone is learning and thinking things + through all the time. Each Openverse repository has an `@` alias you can ping + for help in any issue or pull request, draft or otherwise: + - `@WordPress/openverse-api` + - `@WordPress/openverse-catalog` + - `@WordPress/openverse-frontend` + - etc. +5. On the technical side, there are a ton of + [`just`](https://github.com/casey/just) recipes to help you out. If you run + `just` on its own in any Openverse repository, it will show you what recipes + are available and what they do. `just lint`, `just test`, `just recreate`, we + all run these so frequently! + ``` + # Run `just` in any of the Openverse repositories to see a list of available commands. + # This example is for the `WordPress/openverse` repository. + > just + Available recipes: + default # Show all available recipes + install # Install Python dependencies in Pipenv environments and JS dependencies + precommit # Setup pre-commit as a Git hook + lint # Run pre-commit to lint and reformat all files + ``` ## 🏔️ Other non-code contributions -There are a number of other ways to contribute to the project that don't involve writing Python or Javascript. +There are a number of other ways to contribute to the project that don't involve +writing Python or Javascript. ### 📑 Text improvements -Our documentation and copy text could always use improvement! The following links can be useful for finding text issues to tackle: +Our documentation and copy text could always use improvement! The following +links can be useful for finding text issues to tackle: - [Good first issues with the label “aspect: text”](https://href.li/?https://github.com/issues?q=is%3Aopen+is%3Aissue+repo%3AWordPress%2Fopenverse-catalog+repo%3AWordPress%2Fopenverse+repo%3AWordPress%2Fopenverse-api+repo%3AWordPress%2Fopenverse-frontend+label%3A%22good+first+issue%22+label%3A%22help+wanted%22+-label%3A%22%E2%9B%94+status%3A+blocked%22+-label%3A%22%F0%9F%94%92+staff+only%22+label%3A%22%F0%9F%93%84+aspect%3A+text%22) - [All issues with the label “aspect: text”](https://href.li/?https://github.com/issues?q=is%3Aopen+is%3Aissue+repo%3AWordPress%2Fopenverse-catalog+repo%3AWordPress%2Fopenverse+repo%3AWordPress%2Fopenverse-api+repo%3AWordPress%2Fopenverse-frontend+-label%3A%22%E2%9B%94+status%3A+blocked%22+-label%3A%22%F0%9F%94%92+staff+only%22+label%3A%22%F0%9F%93%84+aspect%3A+text%22+) ### 🩹 Bug reproduction & triage -Openverse has a large list of open bugs: [all issues with the label "goal: fix"](https://github.com/issues?q=is%3Aopen+is%3Aissue+repo%3AWordPress%2Fopenverse-catalog+repo%3AWordPress%2Fopenverse+repo%3AWordPress%2Fopenverse-api+repo%3AWordPress%2Fopenverse-frontend+-label%3A%22%E2%9B%94+status%3A+blocked%22+-label%3A%22%F0%9F%94%92+staff+only%22+label%3A%22%F0%9F%9B%A0+goal%3A+fix%22). In many cases these bugs can be out of date, or their reproduction criteria may no longer be accurate. It's useful information for maintainers to know whether the issue mentioned can still be reproduced on recent versions of the code or if the issue can no longer be replicated. +Openverse has a large list of open bugs: +[all issues with the label "goal: fix"](https://github.com/issues?q=is%3Aopen+is%3Aissue+repo%3AWordPress%2Fopenverse-catalog+repo%3AWordPress%2Fopenverse+repo%3AWordPress%2Fopenverse-api+repo%3AWordPress%2Fopenverse-frontend+-label%3A%22%E2%9B%94+status%3A+blocked%22+-label%3A%22%F0%9F%94%92+staff+only%22+label%3A%22%F0%9F%9B%A0+goal%3A+fix%22). +In many cases these bugs can be out of date, or their reproduction criteria may +no longer be accurate. It's useful information for maintainers to know whether +the issue mentioned can still be reproduced on recent versions of the code or if +the issue can no longer be replicated. ### 🫱🏿‍🫲🏽 Recruit -If you know folks who have expertise in any of the above areas who you think might be interested in contributing to open source, send them our way! We're happy to help onboard folks to the project itself, as well as the tools and technologies we use. +If you know folks who have expertise in any of the above areas who you think +might be interested in contributing to open source, send them our way! We're +happy to help onboard folks to the project itself, as well as the tools and +technologies we use. diff --git a/README.md b/README.md index 0406bbc27e4..782c5b50b18 100644 --- a/README.md +++ b/README.md @@ -10,16 +10,24 @@ # Openverse -Openverse is a powerful search engine for GPL-compatible images, audio, and more. Openverse is live at wp.org/openverse. +Openverse is a powerful search engine for GPL-compatible images, audio, and +more. Openverse is live at +wp.org/openverse. This repository **does not** contain most of the codebase. The code is divided into individual repositories, and managed via a [GitHub Project Board](https://github.com/orgs/WordPress/projects/3): -- [Frontend](https://github.com/wordpress/openverse-frontend) | The public search engine at wp.org/openverse, built with Vue.js and Nuxt.js -- [Catalog](https://github.com/wordpress/openverse-catalog) | The Apache Airflow-powered system for downloading and storing Openverse's metadata -- [API](https://github.com/wordpress/openverse-api) | The Django REST API for querying the catalog data, used by the frontend -- [Browser extension](https://github.com/wordpress/openverse-browser-extension) | An extension to view Openverse images directly in your web browser +- [Frontend](https://github.com/wordpress/openverse-frontend) | The public + search engine at + wp.org/openverse, built with + Vue.js and Nuxt.js +- [Catalog](https://github.com/wordpress/openverse-catalog) | The Apache + Airflow-powered system for downloading and storing Openverse's metadata +- [API](https://github.com/wordpress/openverse-api) | The Django REST API for + querying the catalog data, used by the frontend +- [Browser extension](https://github.com/wordpress/openverse-browser-extension) + | An extension to view Openverse images directly in your web browser It is possible we will explore a monorepo structure in the future, but since all the repos are decoupled from each other and use different technologies, we've @@ -35,7 +43,7 @@ felt it best to keep them distinct. - [Openverse handbook](https://make.wordpress.org/openverse/handbook/openverse-handbook/) - [Make WordPress Design handbook](https://make.wordpress.org/design/handbook/) -## What *is* in this repo, then? +## What _is_ in this repo, then? - This repo contains automation scripts used for various workflows around Openverse repositories and processes. @@ -48,7 +56,8 @@ felt it best to keep them distinct. ## Repository structure -- **`automations/`:** This directory contains scripts related to project management or one-off tasks. +- **`automations/`:** This directory contains scripts related to project + management or one-off tasks. - **`python/`:** This directory contains scripts written in Python. - Use this as the working directory when executing Python scripts. - Requires [Pipenv](https://pipenv.pypa.io) as the package manager. diff --git a/automations/README.md b/automations/README.md index 687fc37ed1c..df24fd7b69b 100644 --- a/automations/README.md +++ b/automations/README.md @@ -1,11 +1,13 @@ # Automations -This directory contains automations for managing the Openverse repositories and Developer Experience. +This directory contains automations for managing the Openverse repositories and +Developer Experience. ## Requirements - Node v14 - - This should be kept in sync between the GitHub Actions `node-version` and the `volta.node` key in the `package.json` file. + - This should be kept in sync between the GitHub Actions `node-version` and + the `volta.node` key in the `package.json` file. - Python 3.10 - [Just](https://github.com/casey/just) command runner diff --git a/automations/python/README.md b/automations/python/README.md index a3bdd821823..4123c2943d8 100644 --- a/automations/python/README.md +++ b/automations/python/README.md @@ -1,6 +1,6 @@ # Python scripts | Name | Description | -|--------------------------------------------|---------------------------------------------------------------------| +| ------------------------------------------ | ------------------------------------------------------------------- | | [`issues_with_prs.py`](issues_with_prs.py) | Move issue cards with linked PRs from one project column to another | | [`sync_labels.py`](sync_labels.py) | Ensure the presence of a consistent set of labels on all repos | diff --git a/brand/README.md b/brand/README.md index 51bd794dc00..6bc9e778453 100644 --- a/brand/README.md +++ b/brand/README.md @@ -36,7 +36,7 @@ color them. The SVG paths for the icon are classed based on their position as ### Palette | Name | Hex code | -|--------|-------------------------------------------------------------------| +| ------ | ----------------------------------------------------------------- | | Black | ![000000](https://via.placeholder.com/16/000000?text=+) `#000000` | | White | ![ffffff](https://via.placeholder.com/16/ffffff?text=+) `#ffffff` | | Yellow | ![ffe033](https://via.placeholder.com/16/ffe033?text=+) `#ffe033` | @@ -44,7 +44,7 @@ color them. The SVG paths for the icon are classed based on their position as ### Acceptable combinations | Logo color | Background color | -|---------------------------------------------------------------|----------------------------------------------------------------| +| ------------------------------------------------------------- | -------------------------------------------------------------- | | ![000000](https://via.placeholder.com/16/000000?text=+) Black | ![ffe033](https://via.placeholder.com/16/ffe033?text=+) Yellow | | ![000000](https://via.placeholder.com/16/000000?text=+) Black | ![ffffff](https://via.placeholder.com/16/ffffff?text=+) White | | ![ffffff](https://via.placeholder.com/16/ffffff?text=+) White | ![000000](https://via.placeholder.com/16/000000?text=+) Black | diff --git a/justfile b/justfile index ddeaccf0a23..010b3838dbc 100644 --- a/justfile +++ b/justfile @@ -12,6 +12,7 @@ default: @install: just _py-install just _js-install + just precommit # Setup pre-commit as a Git hook precommit: @@ -35,7 +36,7 @@ _py-install: # Install dependencies for JavaScript _js-install: - cd automations/js && pnpm install + pnpm install # Run `render-jinja.js` with given input file, output file and context render in_file out_file ctx="{}": diff --git a/load_testing/README.md b/load_testing/README.md index f8280fb00fc..ecf853bd660 100644 --- a/load_testing/README.md +++ b/load_testing/README.md @@ -1,8 +1,14 @@ # Load testing -This directory contains a collection of locust files for load testing the Openverse application. Currently, only an API load test exists but in the future we can also create load tests for the frontend or any other service we stand up. These exist in this repository rather than one of the service specific ones for this reason. We can re-use the base docker image we create for all our load testing scripts. +This directory contains a collection of locust files for load testing the +Openverse application. Currently, only an API load test exists but in the future +we can also create load tests for the frontend or any other service we stand up. +These exist in this repository rather than one of the service specific ones for +this reason. We can re-use the base docker image we create for all our load +testing scripts. -**Note:** Production is throttled, so these scripts cannot be used as a shortcut to DDoS our production stack. +**Note:** Production is throttled, so these scripts cannot be used as a shortcut +to DDoS our production stack. ## Running the tests @@ -10,12 +16,26 @@ This directory contains a collection of locust files for load testing the Openve #### API -The API load tests assume that you will be using an API key as it is impossible to make unauthenticated requests large enough (`page_size=500`) to facilitate easy flooding. Likewise, unless throttling is temporarily disabled in the environment being tested, you'll probably want to exempt the API key from throttling by adding it to the [API Key throttle exemption](https://github.com/WordPress/openverse-api/blob/c09fd7e16a8eb104c311e8d4f0da08238570067c/api/catalog/api/utils/throttle.py#L77). Please ping `@WordPress/openverse-maintainers` for help doing this in live environments as it requires adding the key to a set in that environment's Redis cache. +The API load tests assume that you will be using an API key as it is impossible +to make unauthenticated requests large enough (`page_size=500`) to facilitate +easy flooding. Likewise, unless throttling is temporarily disabled in the +environment being tested, you'll probably want to exempt the API key from +throttling by adding it to the +[API Key throttle exemption](https://github.com/WordPress/openverse-api/blob/c09fd7e16a8eb104c311e8d4f0da08238570067c/api/catalog/api/utils/throttle.py#L77). +Please ping `@WordPress/openverse-maintainers` for help doing this in live +environments as it requires adding the key to a set in that environment's Redis +cache. -To make the API key accessible to the load testing script, copy the `.env.sh.template` file and name it `.env.sh` and fill in the `ACCESS_TOKEN` variable. +To make the API key accessible to the load testing script, copy the +`.env.sh.template` file and name it `.env.sh` and fill in the `ACCESS_TOKEN` +variable. ### Running -All load tests are accessible through `just` scripts in this directory's `justfile`. +All load tests are accessible through `just` scripts in this directory's +`justfile`. -To run API load tests against a local API instance, use `just api`. You can optionally specify a host (the default is to point to local). For example `just api https://api-dev.openverse.engineering` will run the load tests against the staging API. +To run API load tests against a local API instance, use `just api`. You can +optionally specify a host (the default is to point to local). For example +`just api https://api-dev.openverse.engineering` will run the load tests against +the staging API. diff --git a/load_testing/k6/main.js b/load_testing/k6/main.js index fdb7be2a47f..e9243f7aece 100644 --- a/load_testing/k6/main.js +++ b/load_testing/k6/main.js @@ -1,10 +1,10 @@ -import { group } from 'k6' -import { searchBy } from './search.js' -import { getProvider, getRandomWord } from './utils.js' +import { group } from "k6" +import { searchBy } from "./search.js" +import { getProvider, getRandomWord } from "./utils.js" const createScenario = (mediaType, pageSize, funcName) => { return { - executor: 'per-vu-iterations', + executor: "per-vu-iterations", env: { MEDIA_TYPE: mediaType, PAGE_SIZE: pageSize, @@ -18,29 +18,29 @@ const createScenario = (mediaType, pageSize, funcName) => { export const options = { scenarios: { random_word_image_page_20: createScenario( - 'images', - '20', - 'searchByRandomWord' + "images", + "20", + "searchByRandomWord" ), random_word_audio_page_20: createScenario( - 'audio', - '20', - 'searchByRandomWord' + "audio", + "20", + "searchByRandomWord" ), random_word_image_page_500: createScenario( - 'images', - '500', - 'searchByRandomWord' + "images", + "500", + "searchByRandomWord" ), random_word_audio_page_500: createScenario( - 'audio', - '500', - 'searchByRandomWord' + "audio", + "500", + "searchByRandomWord" ), - provider_image_page_20: createScenario('image', '20', 'searchByProvider'), - provider_image_page_500: createScenario('image', '500', 'searchByProvider'), - provider_audio_page_20: createScenario('audio', '20', 'searchByProvider'), - provider_audio_page_500: createScenario('audio', '500', 'searchByProvider'), + provider_image_page_20: createScenario("image", "20", "searchByProvider"), + provider_image_page_500: createScenario("image", "500", "searchByProvider"), + provider_audio_page_20: createScenario("audio", "20", "searchByProvider"), + provider_audio_page_500: createScenario("audio", "500", "searchByProvider"), }, } @@ -49,7 +49,7 @@ const searchByField = (paramFunc, followLinks = false) => { const PAGE_SIZE = __ENV.PAGE_SIZE console.log(`VU: ${__VU} - ITER: ${__ITER}`) const param = paramFunc(MEDIA_TYPE) - const depth = followLinks ? 'Deep' : 'Shallow' + const depth = followLinks ? "Deep" : "Shallow" group( `${depth} ${MEDIA_TYPE} search of ${PAGE_SIZE} items (using '${param}')`, diff --git a/load_testing/k6/search.js b/load_testing/k6/search.js index 6e014b8bbd6..bba009e2e4d 100644 --- a/load_testing/k6/search.js +++ b/load_testing/k6/search.js @@ -1,12 +1,12 @@ -import http from 'k6/http' -import { group, sleep } from 'k6' +import http from "k6/http" +import { group, sleep } from "k6" import { API_URL, getUrlBatch, makeResponseFailedCheck, REQUEST_HEADERS, SLEEP_DURATION, -} from './utils.js' +} from "./utils.js" export const searchBy = (param, page, media_type, page_size, followLinks) => { let url = `${API_URL}${media_type}/?${param}&page=${page}&page_size=${page_size}&filter_dead=false` @@ -14,37 +14,37 @@ export const searchBy = (param, page, media_type, page_size, followLinks) => { const checkResponseFailed = makeResponseFailedCheck(param, page) - if (checkResponseFailed(response, 'search')) { + if (checkResponseFailed(response, "search")) { console.error(`Failed URL: ${url}`) return 0 } const parsedResp = response.json() - const pageCount = parsedResp['page_count'] - const detailUrls = parsedResp['results'].map((i) => i.detail_url) - const relatedUrls = parsedResp['results'].map((i) => i.related_url) + const pageCount = parsedResp["page_count"] + const detailUrls = parsedResp["results"].map((i) => i.detail_url) + const relatedUrls = parsedResp["results"].map((i) => i.related_url) // Don't view details/related if not requested if (!followLinks) { return pageCount } - group('Details requests', () => { + group("Details requests", () => { console.info( `Requesting all ${media_type} details from "${param}" at page ${page}` ) const responses = http.batch(getUrlBatch(detailUrls)) - responses.map((r) => checkResponseFailed(r, 'details')) + responses.map((r) => checkResponseFailed(r, "details")) }) sleep(SLEEP_DURATION) - group('Related requests', () => { + group("Related requests", () => { console.info( `Requesting all ${media_type} related from "${param}" at page ${page}` ) - const responses = http.batch(getUrlBatch(relatedUrls, 'related_url')) - responses.map((r) => checkResponseFailed(r, 'related')) + const responses = http.batch(getUrlBatch(relatedUrls, "related_url")) + responses.map((r) => checkResponseFailed(r, "related")) }) sleep(SLEEP_DURATION) diff --git a/load_testing/k6/utils.js b/load_testing/k6/utils.js index 29088af64ac..f3595a49ea7 100644 --- a/load_testing/k6/utils.js +++ b/load_testing/k6/utils.js @@ -1,13 +1,13 @@ -import { check } from 'k6' -import http from 'k6/http' -import { randomItem } from 'https://jslib.k6.io/k6-utils/1.2.0/index.js' +import { check } from "k6" +import http from "k6/http" +import { randomItem } from "https://jslib.k6.io/k6-utils/1.2.0/index.js" export const API_URL = - __ENV.API_URL || 'https://api-dev.openverse.engineering/v1/' + __ENV.API_URL || "https://api-dev.openverse.engineering/v1/" export const SLEEP_DURATION = 0.1 // Use the random words list available locally, but filter any words that end with apostrophe-s -const WORDS = open('/usr/share/dict/words') - .split('\n') +const WORDS = open("/usr/share/dict/words") + .split("\n") .filter((w) => !w.endsWith("'s")) export const getRandomWord = () => randomItem(WORDS) @@ -21,19 +21,19 @@ export const getProvider = (media_type) => { export const REQUEST_HEADERS = { Authorization: `Bearer ${__ENV.API_TOKEN}`, - 'User-Agent': 'k6', + "User-Agent": "k6", } -export const getUrlBatch = (urls, type = 'detail_url') => { +export const getUrlBatch = (urls, type = "detail_url") => { return urls.map((u) => { const params = { headers: REQUEST_HEADERS, tags: { name: type } } - return ['GET', u, null, params] + return ["GET", u, null, params] }) } export const makeResponseFailedCheck = (param, page) => { return (response, action) => { - if (check(response, { 'status was 200': (r) => r.status === 200 })) { + if (check(response, { "status was 200": (r) => r.status === 200 })) { console.log( `Checked status 200 ✓ for param "${param}" at page ${page} for ${action}.` ) diff --git a/prettier.config.js b/prettier.config.js index b48775a1a90..7b0c36b4789 100644 --- a/prettier.config.js +++ b/prettier.config.js @@ -3,15 +3,16 @@ // - https://github.com/WordPress/openverse/blob/main/prettier.config.js.jinja module.exports = { - trailingComma: 'es5', + trailingComma: "es5", tabWidth: 2, semi: false, - singleQuote: true, + singleQuote: false, + proseWrap: "always", overrides: [ { - files: '*.yml', + files: "*.yml", options: { - singleQuote: false, + proseWrap: "preserve", }, }, ], diff --git a/prettier.config.js.jinja b/prettier.config.js.jinja index fd0f17724d0..ec63c829bb7 100644 --- a/prettier.config.js.jinja +++ b/prettier.config.js.jinja @@ -6,15 +6,16 @@ {# This comment adds a blank line. #} module.exports = { - trailingComma: 'es5', + trailingComma: "es5", tabWidth: 2, semi: false, - singleQuote: true, + singleQuote: false, + proseWrap: "always", overrides: [ { - files: '*.yml', + files: "*.yml", options: { - singleQuote: false, + proseWrap: "preserve", }, }, ], diff --git a/rfcs/20220217-visual_regression_testing.md b/rfcs/20220217-visual_regression_testing.md index 2cb96589c94..d51701b52b6 100644 --- a/rfcs/20220217-visual_regression_testing.md +++ b/rfcs/20220217-visual_regression_testing.md @@ -1,6 +1,7 @@ # RFC: Visual regression testing -A proposal to integrate visual regression testing into the Openverse frontend's automated testing suite. +A proposal to integrate visual regression testing into the Openverse frontend's +automated testing suite. ## Reviewers @@ -19,77 +20,172 @@ https://github.com/WordPress/openverse-frontend/pull/899 ### What is "visual regression testing"? -Visual regression testing is a testing strategy that diffs two screenshots reports a failure if there are significant differences between the two. +Visual regression testing is a testing strategy that diffs two screenshots +reports a failure if there are significant differences between the two. -Playwright is capable of generating and diffing screenshots of the running application via the [screenshot function](https://playwright.dev/docs/screenshots) available on the `Page` and `Locator` objects. These can be used with `toMatchSnapshot` like so: +Playwright is capable of generating and diffing screenshots of the running +application via the +[screenshot function](https://playwright.dev/docs/screenshots) available on the +`Page` and `Locator` objects. These can be used with `toMatchSnapshot` like so: ```ts expect(await page.screenshot()).toMatchSnapshot({ - name: 'fullpage', + name: "fullpage", }) ``` -Playwright uses the popular [pixelmatch](https://github.com/mapbox/pixelmatch) library to diff the screenshots. Example diffs [can be found here](https://github.com/mapbox/pixelmatch#example-output). Playwright on its own does not appear capable of outputting the visual-diff image. An alternative to this is covered below. +Playwright uses the popular [pixelmatch](https://github.com/mapbox/pixelmatch) +library to diff the screenshots. Example diffs +[can be found here](https://github.com/mapbox/pixelmatch#example-output). +Playwright on its own does not appear capable of outputting the visual-diff +image. An alternative to this is covered below. ### Rationale for this style of testing -A lot of the recent regressions that we've experienced on the frontend _on critical paths_ have been visual style regressions. It's difficult, but not impossible, to write "sensible" unit tests against the applied styles. Testing Library offers the [`toHaveStyle`](https://github.com/testing-library/jest-dom#tohavestyle) matcher and we _could_ start utilizing this more, but it would require tedious manual changes to tests that _most of the time_ would just be 1-1 confirmations of the applied Tailwind classes. It also doesn't necessarily test components in composition very well and it would be tedious to write individual specific style tests for each of the scenarios any given widespread component is used in (`VButton`, for example, is used everywhere). - -Visual regression testing could be a way of accomplishing more robust tests without the tedious practice of hand-writing tests for specific styles. +A lot of the recent regressions that we've experienced on the frontend _on +critical paths_ have been visual style regressions. It's difficult, but not +impossible, to write "sensible" unit tests against the applied styles. Testing +Library offers the +[`toHaveStyle`](https://github.com/testing-library/jest-dom#tohavestyle) matcher +and we _could_ start utilizing this more, but it would require tedious manual +changes to tests that _most of the time_ would just be 1-1 confirmations of the +applied Tailwind classes. It also doesn't necessarily test components in +composition very well and it would be tedious to write individual specific style +tests for each of the scenarios any given widespread component is used in +(`VButton`, for example, is used everywhere). + +Visual regression testing could be a way of accomplishing more robust tests +without the tedious practice of hand-writing tests for specific styles. ### Existing tooling -We already utilize Playwright for our end-to-end tests. Provided we wrote the requisite tests to generate the screenshots, we could stop here and already have visual regression testing. +We already utilize Playwright for our end-to-end tests. Provided we wrote the +requisite tests to generate the screenshots, we could stop here and already have +visual regression testing. -However, we also need a way of easily reviewing the diffed images. Luckily, [GitHub already has this functionality built into it's PR diff previews](https://docs.github.com/en/repositories/working-with-files/using-files/working-with-non-code-files#viewing-differences). This means when an existing screenshot is replaced we'd be able to review and approve this change as a normal part of our PR review process, just like we review code. +However, we also need a way of easily reviewing the diffed images. Luckily, +[GitHub already has this functionality built into it's PR diff previews](https://docs.github.com/en/repositories/working-with-files/using-files/working-with-non-code-files#viewing-differences). +This means when an existing screenshot is replaced we'd be able to review and +approve this change as a normal part of our PR review process, just like we +review code. ### Alternative/additional tooling -Everything in the previous section is basically the "bare minimum" of what we'd need to start utilizing visual regression testing. We could stop there and already be able to take advantage of this technology. However, there are additionally a whole world of visual regression testing tools. In this section I cover one popular external diff reviewing tool (which I ultimately recommend against, for now) and an enhanced image snapshot matcher. +Everything in the previous section is basically the "bare minimum" of what we'd +need to start utilizing visual regression testing. We could stop there and +already be able to take advantage of this technology. However, there are +additionally a whole world of visual regression testing tools. In this section I +cover one popular external diff reviewing tool (which I ultimately recommend +against, for now) and an enhanced image snapshot matcher. #### Percy -[Percy](https://percy.io/) seems to be far and away the most powerful tool for reviewing image diffs. It is owned by BrowserStack. While BrowserStack has a generous open source sponsorship (that we should definitely eventually utilize to run e2e tests and even potentially generate snapshots on real mobile devices), they do not appear to extend this offering to Percy[^1]. +[Percy](https://percy.io/) seems to be far and away the most powerful tool for +reviewing image diffs. It is owned by BrowserStack. While BrowserStack has a +generous open source sponsorship (that we should definitely eventually utilize +to run e2e tests and even potentially generate snapshots on real mobile +devices), they do not appear to extend this offering to Percy[^1]. -Percy also requires you to specifically upload the files through a GitHub action which would require managing additional secrets. It would also restrict reviewers to those with access to Percy which would unnecessarily exclude casual contributors. +Percy also requires you to specifically upload the files through a GitHub action +which would require managing additional secrets. It would also restrict +reviewers to those with access to Percy which would unnecessarily exclude casual +contributors. -Percy definitely looks very cool and I would recommend creating a BrowserStack account and checking out the Percy demo project. It's fun to poke around in and the UI mostly works very nicely. +Percy definitely looks very cool and I would recommend creating a BrowserStack +account and checking out the Percy demo project. It's fun to poke around in and +the UI mostly works very nicely. -However, if I'm being honest, when taking into account the fact that GitHub already has image diffing built into the PR review process, Percy seems like an unnecessary additional complication to the baseline of visual regression testing. Maybe it's something that we would come to realize the value of down the line, but it does not seem to me to be a requirement for the initial phase of visual regression testing. +However, if I'm being honest, when taking into account the fact that GitHub +already has image diffing built into the PR review process, Percy seems like an +unnecessary additional complication to the baseline of visual regression +testing. Maybe it's something that we would come to realize the value of down +the line, but it does not seem to me to be a requirement for the initial phase +of visual regression testing. #### Alternative image snapshot libraries -Playwrights's built in image snapshot-diffing capability works fine for the most part. In fact, it basically does everything you need short of one very useful feature: it throws away the actual generated diff. +Playwrights's built in image snapshot-diffing capability works fine for the most +part. In fact, it basically does everything you need short of one very useful +feature: it throws away the actual generated diff. -[Amex's `jest-image-snapshot`](https://github.com/americanexpress/jest-image-snapshot) fixes this. In addition to failing when snapshots do not match, it will also write the diff image that includes the "hot spots" where the visual differences exist. It also automatically generates the snapshot name, which Playwright's matcher doesn't seem to be capable of (this is usually a standard feature of Jest snapshot matchers otherwise). +[Amex's `jest-image-snapshot`](https://github.com/americanexpress/jest-image-snapshot) +fixes this. In addition to failing when snapshots do not match, it will also +write the diff image that includes the "hot spots" where the visual differences +exist. It also automatically generates the snapshot name, which Playwright's +matcher doesn't seem to be capable of (this is usually a standard feature of +Jest snapshot matchers otherwise). -`jest-image-snapshot` is also significantly more configurable than Playwright's matcher. Playwright's matcher has a single option, `threshold` which it passes directly to `pixelmatch`. Amex's library has a [ton of potentially useful options for configuring snapshots on an individual basis or project wide](https://github.com/americanexpress/jest-image-snapshot#%EF%B8%8F-api) and also still uses `pixelmatch` in the background anyway. +`jest-image-snapshot` is also significantly more configurable than Playwright's +matcher. Playwright's matcher has a single option, `threshold` which it passes +directly to `pixelmatch`. Amex's library has a +[ton of potentially useful options for configuring snapshots on an individual basis or project wide](https://github.com/americanexpress/jest-image-snapshot#%EF%B8%8F-api) +and also still uses `pixelmatch` in the background anyway. **Important caveat to this section!** -I ended up trying out `jest-image-snapshot` in https://github.com/WordPress/openverse-frontend/pull/899 but running the matcher fails on some error about `_counters` being undefined. It looks like Playwright, while it uses the jest `expect` library, does not support the same matcher context as Jest proper does. To use `jest-image-snapshot` we might have to switch to `jest-playwright` which would be less than ideal as it's [recommended against by the Playwright project](https://playwright.dev/docs/test-runners#jest--jasmine) (though for what reason exactly I'm really not sure). - -We can still _try_ to find a workaround for this, but if it ends up being too much trouble I'd say just leave it out of the final roadmap for this feature and we can use the built-in Playwright tool. Playwright will output the before and after images for failed snapshot comparisons into a `test-results` folder at the top level of the project, so I figure the worst case scenario is that we could write a little script to walk through that directory and run `pixelmatch` on them to get the diff image. +I ended up trying out `jest-image-snapshot` in +https://github.com/WordPress/openverse-frontend/pull/899 but running the matcher +fails on some error about `_counters` being undefined. It looks like Playwright, +while it uses the jest `expect` library, does not support the same matcher +context as Jest proper does. To use `jest-image-snapshot` we might have to +switch to `jest-playwright` which would be less than ideal as it's +[recommended against by the Playwright project](https://playwright.dev/docs/test-runners#jest--jasmine) +(though for what reason exactly I'm really not sure). + +We can still _try_ to find a workaround for this, but if it ends up being too +much trouble I'd say just leave it out of the final roadmap for this feature and +we can use the built-in Playwright tool. Playwright will output the before and +after images for failed snapshot comparisons into a `test-results` folder at the +top level of the project, so I figure the worst case scenario is that we could +write a little script to walk through that directory and run `pixelmatch` on +them to get the diff image. ## Repository size considerations -Storing many images in a Git repository can be problematic. Between using more of our contributor's bandwidth in uploading and downloading images to just generally butting up against the limit of GitHub's repository size limit. - -The repository size limit I don't think we'll ever need to worry about. GitHub has a [repository size limit](https://docs.github.com/en/repositories/working-with-files/managing-large-files/about-large-files-on-github#repository-size-limits) of 100GB. Playwright screenshots are small (in the kilobytes) so it would take a very long time and tons and tons of screenshots to start getting close to the repo size limit. - -However, if we're not careful we could significantly increase the size of our repository which would adversely effect contributors by eating their bandwidth when pushing or pull contributions. There are some strategies we could use if this becomes a problem. For example we could use git's `clean` and `smudge` features to losslessly compress and decompress the snapshot images automatically, though I'm not sure how well that would work[^2]. Before we invested too much in that, however, we'd want to ensure that more typical, though lossy, [git repository size reduction](https://stackoverflow.com/questions/2116778/reduce-git-repository-size#2116892) isn't a better option. - -Ultimately I think the best option would be to regularly (once a day) run a scheduled maintenance task to remove any historical snapshots older than a month that are not currently used by the main branch. That would mean we would always have a _reasonable_ historical view of snapshots that change frequently and I frankly doubt snapshot history extending more than a month back would be very useful anyway. +Storing many images in a Git repository can be problematic. Between using more +of our contributor's bandwidth in uploading and downloading images to just +generally butting up against the limit of GitHub's repository size limit. + +The repository size limit I don't think we'll ever need to worry about. GitHub +has a +[repository size limit](https://docs.github.com/en/repositories/working-with-files/managing-large-files/about-large-files-on-github#repository-size-limits) +of 100GB. Playwright screenshots are small (in the kilobytes) so it would take a +very long time and tons and tons of screenshots to start getting close to the +repo size limit. + +However, if we're not careful we could significantly increase the size of our +repository which would adversely effect contributors by eating their bandwidth +when pushing or pull contributions. There are some strategies we could use if +this becomes a problem. For example we could use git's `clean` and `smudge` +features to losslessly compress and decompress the snapshot images +automatically, though I'm not sure how well that would work[^2]. Before we +invested too much in that, however, we'd want to ensure that more typical, +though lossy, +[git repository size reduction](https://stackoverflow.com/questions/2116778/reduce-git-repository-size#2116892) +isn't a better option. + +Ultimately I think the best option would be to regularly (once a day) run a +scheduled maintenance task to remove any historical snapshots older than a month +that are not currently used by the main branch. That would mean we would always +have a _reasonable_ historical view of snapshots that change frequently and I +frankly doubt snapshot history extending more than a month back would be very +useful anyway. ## Cross platform rendering -Browsers will sometimes render minute differences between platforms, even with the exact same webpage. This means that contributors running Linux might generate different screenshots from those running macOS and again from those running Windows, even if they're all using the same browser and version. +Browsers will sometimes render minute differences between platforms, even with +the exact same webpage. This means that contributors running Linux might +generate different screenshots from those running macOS and again from those +running Windows, even if they're all using the same browser and version. -To avoid this, other visual regression testing libraries like BackstopJS support running tests inside a docker container and using the browser inside the container. We can do the same with Playwright (it's just not as easy). +To avoid this, other visual regression testing libraries like BackstopJS support +running tests inside a docker container and using the browser inside the +container. We can do the same with Playwright (it's just not as easy). I was about to figure out "a way" of doing it. Here's what I came up with: 1. Create a `./bin/visual-regression.js` file with the following: + ```js /** * Run this as a node script so that we can retrieve the `process.cwd` @@ -100,37 +196,40 @@ I was about to figure out "a way" of doing it. Here's what I came up with: * versions expected by the `playwright` binary installed locally * matches the versions pre-installed in the container. */ -const { spawnSync } = require('child_process') -const fs = require('fs') -const path = require('path') -const yaml = require('yaml') +const { spawnSync } = require("child_process") +const fs = require("fs") +const path = require("path") +const yaml = require("yaml") const pnpmLock = yaml.parse( - String(fs.readFileSync(path.resolve(process.cwd(), 'pnpm-lock.yaml'))) + String(fs.readFileSync(path.resolve(process.cwd(), "pnpm-lock.yaml"))) ) -const playwrightVersion = pnpmLock.devDependencies['@playwright/test'] +const playwrightVersion = pnpmLock.devDependencies["@playwright/test"] const args = [ - 'run', - '--rm', - '-it', - '--mount', + "run", + "--rm", + "-it", + "--mount", `type=bind,source=${process.cwd()},target=/src`, - '--add-host=host.docker.internal:host-gateway', // This is necessary to make `host.docker.internal` work on Linux. Does it break the command for other OSs? + "--add-host=host.docker.internal:host-gateway", // This is necessary to make `host.docker.internal` work on Linux. Does it break the command for other OSs? `mcr.microsoft.com/playwright:v${playwrightVersion}-focal`, - '/src/bin/visual-regression.sh', + "/src/bin/visual-regression.sh", ] -if (process.argv.includes('-u')) { - args.push('-u') +if (process.argv.includes("-u")) { + args.push("-u") } -spawnSync('docker', args, { - stdio: 'inherit', +spawnSync("docker", args, { + stdio: "inherit", }) ``` -2. Create a `./bin/visual-regression.sh` file containing the following (don't forget to `chmod +x` it): + +2. Create a `./bin/visual-regression.sh` file containing the following (don't + forget to `chmod +x` it): + ```sh #!/bin/bash # This script is meant to run inside the playwright docker container, hence the assumed `/src` directory @@ -139,51 +238,91 @@ cd /src # Use npm to avoid having to install pnpm inside the container, it doesn't matter in this case npm run test:visual-regression:local -- $1 ``` + 3. Add two new package.json scripts: + ```json "test:visual-regression": "node ./bin/visual-regression.js", "test:visual-regression:local": "playwright test -c ./test/visual-regression" ``` -All of this together should successfully allow us to run the snapshot tests inside a docker container. +All of this together should successfully allow us to run the snapshot tests +inside a docker container. -For local debugging purposes, `pnpm test:visual-regression:local --debug` can be used to run the snapshot tests headed in a browser on the host OS. This is useful for debugging the playwright scripts before they actually take a screenshot. Just keep in mind that the final screenshots submitting in a PR need to come from the docker container. +For local debugging purposes, `pnpm test:visual-regression:local --debug` can be +used to run the snapshot tests headed in a browser on the host OS. This is +useful for debugging the playwright scripts before they actually take a +screenshot. Just keep in mind that the final screenshots submitting in a PR need +to come from the docker container. ## Additional cross-platform considerations -Some versions of Windows [do not allow installing Docker](https://www.freecodecamp.org/news/how-to-run-docker-on-windows-10-home-edition/); additionally, Docker may not be available on someone's machine for licensing reasons and alternatives like containerd and Podman are not yet wide-spread enough to be reliably available on contributor machines. - -For that reason, it'd be good to consider ways for making sure snapshots are able to be updated without Docker being available locally (props to @obulat for mentioning this potential roadblock for contributors). I'd like to propose a new GitHub workflow that runs if the `ci:visual-regression` script fails. - -The workflow would create a new branch based on the PR with the changes using the pattern `__vr-snapshot-update`, run `ci:visual-regression:update` (we'd need a new script for this to pass the `--u` flag to our script specifically instead of to the e2e `run-server-and-test` utility), commit the changes in the new branch and open a PR targeting the parent PR with the changes. - -This PR could then be merged directly into the parent PR after the snapshot changes are reviewed. - -Additionally, subsequent pushes to the parent PR that include changes to the snapshots would re-use the same branch name and just force push; there'd only ever be a single commit in the companion branch this way and we wouldn't have to worry about a muddied commit history. Ideally this will simplify the implementation of the workflow rather than complicate it. - -Likewise, if we end up needing to run `pixelmatch` directly to generate the diff output, it'd be nice if we could just add those diff outputs as a comment in the parent PR whenever snapshot images are updated. +Some versions of Windows +[do not allow installing Docker](https://www.freecodecamp.org/news/how-to-run-docker-on-windows-10-home-edition/); +additionally, Docker may not be available on someone's machine for licensing +reasons and alternatives like containerd and Podman are not yet wide-spread +enough to be reliably available on contributor machines. + +For that reason, it'd be good to consider ways for making sure snapshots are +able to be updated without Docker being available locally (props to @obulat for +mentioning this potential roadblock for contributors). I'd like to propose a new +GitHub workflow that runs if the `ci:visual-regression` script fails. + +The workflow would create a new branch based on the PR with the changes using +the pattern `__vr-snapshot-update`, run +`ci:visual-regression:update` (we'd need a new script for this to pass the `--u` +flag to our script specifically instead of to the e2e `run-server-and-test` +utility), commit the changes in the new branch and open a PR targeting the +parent PR with the changes. + +This PR could then be merged directly into the parent PR after the snapshot +changes are reviewed. + +Additionally, subsequent pushes to the parent PR that include changes to the +snapshots would re-use the same branch name and just force push; there'd only +ever be a single commit in the companion branch this way and we wouldn't have to +worry about a muddied commit history. Ideally this will simplify the +implementation of the workflow rather than complicate it. + +Likewise, if we end up needing to run `pixelmatch` directly to generate the diff +output, it'd be nice if we could just add those diff outputs as a comment in the +parent PR whenever snapshot images are updated. ## Test variants -Most tests should be written against two parameters: language direction and breakpoint. +Most tests should be written against two parameters: language direction and +breakpoint. -For language direction, the best way to do this is to just test in the default locale which is LTR and to test in Arabic (`ar`) which is RTL. +For language direction, the best way to do this is to just test in the default +locale which is LTR and to test in Arabic (`ar`) which is RTL. -For breakpoints, it will be tedious to have to add the iteration for each test case, so I've written a utility that we can use to make this easier. This utility can also be used for our regular end-to-end tests: +For breakpoints, it will be tedious to have to add the iteration for each test +case, so I've written a utility that we can use to make this easier. This +utility can also be used for our regular end-to-end tests: ```ts -import { test, PlaywrightTestArgs, TestInfo } from '@playwright/test' - -import { Breakpoints, SCREEN_SIZES } from '../../../src/constants/screens' - -export const testEachBreakpoint = (title: string, testCb: (breakpoint: Breakpoints, args: PlaywrightTestArgs,testInfo: TestInfo) => Promise) => { +import { test, PlaywrightTestArgs, TestInfo } from "@playwright/test" + +import { Breakpoints, SCREEN_SIZES } from "../../../src/constants/screens" + +export const testEachBreakpoint = ( + title: string, + testCb: ( + breakpoint: Breakpoints, + args: PlaywrightTestArgs, + testInfo: TestInfo + ) => Promise +) => { SCREEN_SIZES.forEach((screenWidth, breakpoint) => { - test.describe(`screen at breakpoint ${breakpoint} with width ${screenWidth}`, () => { - test(title, async ({ page, context, request }, testInfo) => { - await page.setViewportSize({ width: screenWidth, height: 700 }) - await testCb(breakpoint, { page, context, request }, testInfo) - }) - }) + test.describe( + `screen at breakpoint ${breakpoint} with width ${screenWidth}`, + () => { + test(title, async ({ page, context, request }, testInfo) => { + await page.setViewportSize({ width: screenWidth, height: 700 }) + await testCb(breakpoint, { page, context, request }, testInfo) + }) + } + ) }) } ``` @@ -191,14 +330,14 @@ export const testEachBreakpoint = (title: string, testCb: (breakpoint: Breakpoin It's then used in the test like so: ```ts -test.describe('homepage snapshots', () => { +test.describe("homepage snapshots", () => { // Repeat below for `rtl` - test.describe('ltr', () => { + test.describe("ltr", () => { test.beforeEach(async ({ page }) => { - await page.goto('/') + await page.goto("/") }) - testEachBreakpoint('full page', async (breakpoint, { page }) => { + testEachBreakpoint("full page", async (breakpoint, { page }) => { await deleteImageCarousel(page) expect(await page.screenshot()).toMatchSnapshot({ @@ -211,77 +350,141 @@ test.describe('homepage snapshots', () => { ## Recommended tooling -* Continue using Playwright to manipulate the browser and use it's `screenshot` function to generate screenshots. -* _Try_ to use `jest-image-snapshot` to save and diff screenshots. -* Run snapshot tests inside a docker container to avoid cross-platform rendering differences. -* Write a GitHub action to be run [once daily using the `on.schedule` feature](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#onschedule) to automatically remove snapshots from the repository history older than a month old. -* Use GitHub's PR review UI to review visual diffs of snapshots. -* Write Playwright scripts using TypeScript - * Note: TypeScript linting and general infrastructure has already been set up in https://github.com/WordPress/openverse-frontend/issues/921 and https://github.com/WordPress/openverse-frontend/pull/917. +- Continue using Playwright to manipulate the browser and use it's `screenshot` + function to generate screenshots. +- _Try_ to use `jest-image-snapshot` to save and diff screenshots. +- Run snapshot tests inside a docker container to avoid cross-platform rendering + differences. +- Write a GitHub action to be run + [once daily using the `on.schedule` feature](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#onschedule) + to automatically remove snapshots from the repository history older than a + month old. +- Use GitHub's PR review UI to review visual diffs of snapshots. +- Write Playwright scripts using TypeScript + - Note: TypeScript linting and general infrastructure has already been set up + in https://github.com/WordPress/openverse-frontend/issues/921 and + https://github.com/WordPress/openverse-frontend/pull/917. ## Implementation plan -Once the final version of this RFC is approved, a milestone with issues for each individual checkbox listed below will be created. +Once the final version of this RFC is approved, a milestone with issues for each +individual checkbox listed below will be created. - [ ] Configure local visual regression testing: - - Create a `playwright.config.ts` file in the new directory with the following code: + + - Create a `playwright.config.ts` file in the new directory with the following + code: + + ```ts + // /test/visual-regression/playwright.config.ts + import { PlaywrightTestConfig } from "@playwright/test" + + export default { + testDir: ".", + use: { + baseURL: "http://localhost:8443", + }, + timeout: 60 * 1000, + } as PlaywrightTestConfig + ``` + + - Add the two `bin/visual-regression.{sh,js}` files described in the "Cross + platform rendering" section above. + - Add three new scripts to the root `package.json` (**Note: in light of + https://github.com/WordPress/openverse-frontend/pull/881 these scripts will + also need to be updated to use talkback to mock API calls during e2e!**): + + ``` + "test:visual-regression": "node ./bin/visual-regression.js", + "test:visual-regression:local": "playwright test -c test/visual-regression", + "ci:visual-regression": "start-server-and-test run-server http://localhost:8443 test:visual-regression" + ``` + + - Install and configure `jest-image-snapshot` and + `@types/jest-image-snapshot`: + - Note: It may be necessary to add additional TypeScript typings in the root + level `typings` folder for TypeScript to know that `expect()` includes the + `toMatchImageSnapshot` function. Follow the existing examples extending + `@types/nuxt` to extend the `@playwright/test` library's `Matchers` + interface. + - Additional note: As mentioned in the section about `jest-image-snapshot` + above it may not be possible to get this library to play nicely with + Playwright's specific `expect.extend` context. I'd say time box trying to + get this to work to an hour or so and then just move on to the next thing. + There's an additional task at the end of this list for writing the + `pixelmatch` diff generator script in case we need it. + - **Extra important note:** This library might not work with Playwright, in + which case we'll just leave this out; there's a contingency plan for this + case below. + - Write basic tests for the homepage; a full-page snapshot for `ltr` and `rtl` + versions of the page using `testEachBreakpoint` should do just as a start + and proof-of-concept. + - Note: It will be necessary to remove the featured images from the page + before taking the snapshot or the diff will fail 2/3 of the time due to + them being different. The following code can be used to accomplish this: ```ts - // /test/visual-regression/playwright.config.ts - import { PlaywrightTestConfig } from '@playwright/test' - - export default { - testDir: '.', - use: { - baseURL: 'http://localhost:8443', - }, - timeout: 60 * 1000, - } as PlaywrightTestConfig - ``` - - Add the two `bin/visual-regression.{sh,js}` files described in the "Cross platform rendering" section above. - - Add three new scripts to the root `package.json` (**Note: in light of https://github.com/WordPress/openverse-frontend/pull/881 these scripts will also need to be updated to use talkback to mock API calls during e2e!**): + const deleteImageCarousel = async (page: Page) => { + const element = await page.$('[data-testid="image-carousel"]') + await element.evaluate((node) => node.remove()) + element.dispose() + } ``` - "test:visual-regression": "node ./bin/visual-regression.js", - "test:visual-regression:local": "playwright test -c test/visual-regression", - "ci:visual-regression": "start-server-and-test run-server http://localhost:8443 test:visual-regression" - ``` - - Install and configure `jest-image-snapshot` and `@types/jest-image-snapshot`: - - Note: It may be necessary to add additional TypeScript typings in the root level `typings` folder for TypeScript to know that `expect()` includes the `toMatchImageSnapshot` function. Follow the existing examples extending `@types/nuxt` to extend the `@playwright/test` library's `Matchers` interface. - - Additional note: As mentioned in the section about `jest-image-snapshot` above it may not be possible to get this library to play nicely with Playwright's specific `expect.extend` context. I'd say time box trying to get this to work to an hour or so and then just move on to the next thing. There's an additional task at the end of this list for writing the `pixelmatch` diff generator script in case we need it. - - **Extra important note:** This library might not work with Playwright, in which case we'll just leave this out; there's a contingency plan for this case below. - - Write basic tests for the homepage; a full-page snapshot for `ltr` and `rtl` versions of the page using `testEachBreakpoint` should do just as a start and proof-of-concept. - - Note: It will be necessary to remove the featured images from the page before taking the snapshot or the diff will fail 2/3 of the time due to them being different. The following code can be used to accomplish this: - ```ts - const deleteImageCarousel = async (page: Page) => { - const element = await page.$('[data-testid="image-carousel"]') - await element.evaluate((node) => node.remove()) - element.dispose() - } - ``` - - Create a `README.md` in the `visual-regression` directory with basic instructions for how to run and write visual regression tests. In particular mention our usage of `jest-image-snapshot` instead of the built in snapshot matcher. -- [ ] Complete https://github.com/WordPress/openverse-frontend/issues/890 if it isn't already done. + - Create a `README.md` in the `visual-regression` directory with basic + instructions for how to run and write visual regression tests. In particular + mention our usage of `jest-image-snapshot` instead of the built in snapshot + matcher. + +- [ ] Complete https://github.com/WordPress/openverse-frontend/issues/890 if it + isn't already done. - [ ] Configure CI to run visual regression tests. - - Write a new workflow to run the visual regression tests. You should be able to copy the existing e2e workflow and replace the call to `ci:e2e` to `ci:visual-regression`. + - Write a new workflow to run the visual regression tests. You should be able + to copy the existing e2e workflow and replace the call to `ci:e2e` to + `ci:visual-regression`. - [ ] Configure Storybook visual regression tests. - - Copy the existing `visual-regression` folder's configuration and create a new test folder called `storybook-visual-regression`. - - Create corresponding local and CI scripts in `package.json` to run these tests, using `storybook` instead of `run-server` and use the correct port for Storybook - - Write basic state and variation tests for each button variant (or some other straight component with similar easy to configure props). - - This will also establish a pattern for writing e2e tests against Storybook. - - Copy the visual regression test CI and swap the appropriate scripts to run the Storybook ones instead. - - If possible, create a shared action that can be re-used between all the Playwright based test workflows. -- [ ] Write a script to generate `pixelmatch` diffs from failed test results in the `test-results` folder created my Playwright. - - **Contingent on `jest-image-snapshot` not being able to be used** -- [ ] Write a new GitHub workflow to generate supplemental PRs with updated snapshots whenever there are outdated snapshots. -- [ ] Write a new GitHub workflow to generate the snapshot diffs and upload them as a comment in the PR with snapshot changes. -- [ ] Create a scheduled GitHub action to clean the repository history of snapshot images older than a month in age outside of the `main` branch. - - I tried searching for prior art here and there isn't much aside from how to completely remove a directory or file from history (usually because something illegal or secret was committed). I think `git filter-branch` is perfect for this though: - ```bash - git filter-branch --prune-empty --tree-filter './bin/clean_historical_snapshots.sh' HEAD - ``` - But the devil's the details... The implementation of `clean_historical_snapshots.sh` will take some care and frankly there will need to be some stateful file hashes written to a temporary directory to be used by the script to skip current snapshots from deletion. We'll also want to establish a pattern for the snapshot directory names that can be easily followed for this purpose. - - **Low priority, can be delayed up to a month or longer; really it can be delayed until we notice a problem with the repository size.** + - Copy the existing `visual-regression` folder's configuration and create a + new test folder called `storybook-visual-regression`. + - Create corresponding local and CI scripts in `package.json` to run these + tests, using `storybook` instead of `run-server` and use the correct port + for Storybook + - Write basic state and variation tests for each button variant (or some other + straight component with similar easy to configure props). + - This will also establish a pattern for writing e2e tests against Storybook. + - Copy the visual regression test CI and swap the appropriate scripts to run + the Storybook ones instead. + - If possible, create a shared action that can be re-used between all the + Playwright based test workflows. +- [ ] Write a script to generate `pixelmatch` diffs from failed test results in + the `test-results` folder created my Playwright. + - **Contingent on `jest-image-snapshot` not being able to be used** +- [ ] Write a new GitHub workflow to generate supplemental PRs with updated + snapshots whenever there are outdated snapshots. +- [ ] Write a new GitHub workflow to generate the snapshot diffs and upload them + as a comment in the PR with snapshot changes. +- [ ] Create a scheduled GitHub action to clean the repository history of + snapshot images older than a month in age outside of the `main` branch. + - I tried searching for prior art here and there isn't much aside from how to + completely remove a directory or file from history (usually because + something illegal or secret was committed). I think `git filter-branch` is + perfect for this though: + ```bash + git filter-branch --prune-empty --tree-filter './bin/clean_historical_snapshots.sh' HEAD + ``` + But the devil's the details... The implementation of + `clean_historical_snapshots.sh` will take some care and frankly there will + need to be some stateful file hashes written to a temporary directory to be + used by the script to skip current snapshots from deletion. We'll also want to + establish a pattern for the snapshot directory names that can be easily + followed for this purpose. + - **Low priority, can be delayed up to a month or longer; really it can be + delayed until we notice a problem with the repository size.**
-[^1]: I could be wrong about this. I tried searching around for it but couldn't find it and it was not included in the default set of allowances we got with out initial application as an OSS project. +[^1]: + I could be wrong about this. I tried searching around for it but couldn't + find it and it was not included in the default set of allowances we got with + out initial application as an OSS project. -[^2]: There's very little information about this online so I suspect it's a _non-problem_ for the most part. +[^2]: + There's very little information about this online so I suspect it's a + _non-problem_ for the most part. diff --git a/rfcs/20220218-introduce_ui_state_cookie_to_frontend.md b/rfcs/20220218-introduce_ui_state_cookie_to_frontend.md index 052704e115d..328d8864a9b 100644 --- a/rfcs/20220218-introduce_ui_state_cookie_to_frontend.md +++ b/rfcs/20220218-introduce_ui_state_cookie_to_frontend.md @@ -7,7 +7,9 @@ ## Rationale -We removed all the cookies from the application. That's a good thing generally. But we _can_ use cookies without a banner and still be GDPR compliant as long as the cookies are necessary for providing a smoother user experience. +We removed all the cookies from the application. That's a good thing generally. +But we _can_ use cookies without a banner and still be GDPR compliant as long as +the cookies are necessary for providing a smoother user experience. Doing this would fix problems like the UI pop in/out in this video: @@ -17,11 +19,20 @@ https://user-images.githubusercontent.com/24264157/154730060-dd602b4c-52cf-441b- To quote from https://gdpr.eu/cookies: -> Strictly necessary cookies — These cookies are essential for you to browse the website and use its features, such as accessing secure areas of the site. Cookies that allow web shops to hold your items in your cart while you are shopping online are an example of strictly necessary cookies. These cookies will generally be first-party session cookies. While it is not required to obtain consent for these cookies, what they do and why they are necessary should be explained to the user. +> Strictly necessary cookies — These cookies are essential for you to browse the +> website and use its features, such as accessing secure areas of the site. +> Cookies that allow web shops to hold your items in your cart while you are +> shopping online are an example of strictly necessary cookies. These cookies +> will generally be first-party session cookies. While it is not required to +> obtain consent for these cookies, what they do and why they are necessary +> should be explained to the user. -I believe the cookie I am proposing we add in this RFC falls under this and we would not need a consent banner for it. +I believe the cookie I am proposing we add in this RFC falls under this and we +would not need a consent banner for it. -We should however add an explanatory page for our cookie usage; potentially this could just be a general "Privacy" page that links out to the WP.org privacy policy and explains Openverse specific things. +We should however add an explanatory page for our cookie usage; potentially this +could just be a general "Privacy" page that links out to the WP.org privacy +policy and explains Openverse specific things. ## Cookie shape @@ -55,9 +66,18 @@ interface UIStateCookie { ## Implementation plan -- [ ] `useUiStateCookie()` -> Returns an existing cookie or creates a new one and saves it. It should be a `reactive` object with custom `set` handlers to update the stored cookie value. Creating a new one should sniff the UA for the correct default value for `breakpoint`. - - Additionally: `useSyncUiStateCookieBreakpoint()` -> Called by `useUiStateCookie` to sync the breakpoint upon resizing. -- [ ] Update `useMediaQuery` to use the ui state cookie to deduce a default for `shouldPassInSSR` based on the breakpoint value. -- [ ] Update the translation banner dismissal to set the cookie property and read from it to determine whether to show. -- [ ] Update filter sidebar opening to set the cookie property and read from it to determine whether to show. -- [ ] Add new Privacy page that explains our use of cookies and links to WP.org's Privacy policy. Replace the page menu privacy link to use the internal one. +- [ ] `useUiStateCookie()` -> Returns an existing cookie or creates a new one + and saves it. It should be a `reactive` object with custom `set` handlers + to update the stored cookie value. Creating a new one should sniff the UA + for the correct default value for `breakpoint`. + - Additionally: `useSyncUiStateCookieBreakpoint()` -> Called by + `useUiStateCookie` to sync the breakpoint upon resizing. +- [ ] Update `useMediaQuery` to use the ui state cookie to deduce a default for + `shouldPassInSSR` based on the breakpoint value. +- [ ] Update the translation banner dismissal to set the cookie property and + read from it to determine whether to show. +- [ ] Update filter sidebar opening to set the cookie property and read from it + to determine whether to show. +- [ ] Add new Privacy page that explains our use of cookies and links to + WP.org's Privacy policy. Replace the page menu privacy link to use the + internal one. diff --git a/rfcs/20220221-3d_model_support.md b/rfcs/20220221-3d_model_support.md index e0eca50d486..d9e58596717 100644 --- a/rfcs/20220221-3d_model_support.md +++ b/rfcs/20220221-3d_model_support.md @@ -5,47 +5,76 @@ ## Rationale -The Openverse team and members of the community have identified a number of sources of CC-licensed 3D models. We would like to add these models to Openverse, as they can be useful for game development, art, 3D printing, etc. In order to incorporate them into Openverse, we need to add a new media type and provide infrastructure in the frontend for rendering the models. +The Openverse team and members of the community have identified a number of +sources of CC-licensed 3D models. We would like to add these models to +Openverse, as they can be useful for game development, art, 3D printing, etc. In +order to incorporate them into Openverse, we need to add a new media type and +provide infrastructure in the frontend for rendering the models. ## Overview -Openverse will add first-class support for 3D models, alongside audio and images. This means 3D models will have their own table in the Catalog, and their own API endpoints. Adding these to the frontend will require some new components and utilities, but should be more straightforward than the implementation of audio because we are not conducting a complete redesign of the site in parallel. +Openverse will add first-class support for 3D models, alongside audio and +images. This means 3D models will have their own table in the Catalog, and their +own API endpoints. Adding these to the frontend will require some new components +and utilities, but should be more straightforward than the implementation of +audio because we are not conducting a complete redesign of the site in parallel. -This outline of work is predicated on the idea that we've identified sources of 3D models and will seek to flesh out the details regarding metadata fields, database migrations, API development, and frontend work. +This outline of work is predicated on the idea that we've identified sources of +3D models and will seek to flesh out the details regarding metadata fields, +database migrations, API development, and frontend work. -This work describes 3D models as a specific entity, exclusive of 2D models. 2D models can be covered under our `image` media type and identified via a separate category. +This work describes 3D models as a specific entity, exclusive of 2D models. 2D +models can be covered under our `image` media type and identified via a separate +category. # 3D Models in the Catalog & API ## Some potential risks -* As it stands currently, each new media type means an additional data refresh. Data refreshes cannot be run simultaneously at present, so we may reach a saturation point where all of our data cannot be refreshed weekly. -* We do not have any standard database migration approach for the catalog, so all changes suggested here will likely be ad hoc unless #235 is addressed prior to this implementation. -* The API, while it does have a migration tool, does not have a standardized migration application process. We will need to run the migration manually when the time comes. +- As it stands currently, each new media type means an additional data refresh. + Data refreshes cannot be run simultaneously at present, so we may reach a + saturation point where all of our data cannot be refreshed weekly. +- We do not have any standard database migration approach for the catalog, so + all changes suggested here will likely be ad hoc unless #235 is addressed + prior to this implementation. +- The API, while it does have a migration tool, does not have a standardized + migration application process. We will need to run the migration manually when + the time comes. ## Some other considerations -* It would be ideal to have this under a feature flag in the API. We could perform deployments and testing on our staging setup with the flag enabled, but still make other deployments to production with the flag disabled. We should be able to run the API for images & audio successfully with the feature flag disabled AND without having run the migrations. -* As we work through this on both the catalog and the API, we should begin compiling information for a document describing the process for adding a **new media type**. This will be useful for future media types we are sure to add! +- It would be ideal to have this under a feature flag in the API. We could + perform deployments and testing on our staging setup with the flag enabled, + but still make other deployments to production with the flag disabled. We + should be able to run the API for images & audio successfully with the feature + flag disabled AND without having run the migrations. +- As we work through this on both the catalog and the API, we should begin + compiling information for a document describing the process for adding a **new + media type**. This will be useful for future media types we are sure to add! ## Prior work -We may be able to use previous work regarding the addition of audio to the catalog/API as a guide. These updates required a fair amount of generalization, which we may not need to do a second time around. +We may be able to use previous work regarding the addition of audio to the +catalog/API as a guide. These updates required a fair amount of generalization, +which we may not need to do a second time around. WordPress/openverse#19 ## Design requirements ### New content type + - [ ] Create a 3D model icon - [ ] Create an icon indicating that the image is a 3d model screenshot/preview ### Search results + - [ ] Add 3d model results in the All content results - [ ] Design a 3d model results page - [ ] Review and include filter components if necessary ### Single result view + - [ ] Design the 3d model single view ## Technical implementation @@ -53,75 +82,88 @@ WordPress/openverse#19 ### 1. Add 3D models to the catalog - [ ] Define necessary fields & schema for `model_3d` table & associated data -- [ ] Add DDL to local Postgres image: https://github.com/WordPress/openverse-catalog/tree/main/docker/local_postgres +- [ ] Add DDL to local Postgres image: + https://github.com/WordPress/openverse-catalog/tree/main/docker/local_postgres - [ ] Create a popularity calculation view for `model_3d` -- [ ] Add the database columns to `db_columns.py`: https://github.com/WordPress/openverse-catalog/blob/main/openverse_catalog/dags/common/storage/db_columns.py -- [ ] Add the TSV columns to `tsv_columns.py`: https://github.com/WordPress/openverse-catalog/blob/main/openverse_catalog/dags/common/storage/tsv_columns.py -- [ ] Create a `Model3DStore` subclass of [`MediaStore`](https://github.com/WordPress/openverse-catalog/blob/9538f38401e5428b32dc14c5d5ba7ef50af87a96/openverse_catalog/dags/common/storage/media.py#L39): -- [ ] Update the [new provider API script template](https://github.com/WordPress/openverse-catalog/tree/main/openverse_catalog/templates) with the `model_3d` provider +- [ ] Add the database columns to `db_columns.py`: + https://github.com/WordPress/openverse-catalog/blob/main/openverse_catalog/dags/common/storage/db_columns.py +- [ ] Add the TSV columns to `tsv_columns.py`: + https://github.com/WordPress/openverse-catalog/blob/main/openverse_catalog/dags/common/storage/tsv_columns.py +- [ ] Create a `Model3DStore` subclass of + [`MediaStore`](https://github.com/WordPress/openverse-catalog/blob/9538f38401e5428b32dc14c5d5ba7ef50af87a96/openverse_catalog/dags/common/storage/media.py#L39): +- [ ] Update the + [new provider API script template](https://github.com/WordPress/openverse-catalog/tree/main/openverse_catalog/templates) + with the `model_3d` provider - [ ] Create one/several provider API scripts & associated DAGs ### 2. Add 3D models to the API + _Note: Borrowed heavily from WordPress/openverse#19_ + - [ ] Django models for `model_3d` - [ ] Add DRF serializers for `model_3d` - [ ] Add DRF endpoints for `model_3d` - [ ] Make the thumbnail server work with model thumbnails ### 3. Add 3D models to the data refresh + - [ ] Add Elasticsearch `model_3d` models - [ ] Ensure the data refresh works for 3D models - [ ] Add 3D model sample data into the sample data CSV for testing - ## Open Questions -* What data is important to store in the catalog? Some of this is encapsulated in the proposed schema, but there may be other important attributes beyond those listed. -* Will it be important to differentiate the "type" of 3D model that a particular record refers to? (E.g. 3D printer model, game asset, artwork, etc.) -* Many model sites come with asset groups, should we have a table similar to `AudioSets` for these cases? +- What data is important to store in the catalog? Some of this is encapsulated + in the proposed schema, but there may be other important attributes beyond + those listed. +- Will it be important to differentiate the "type" of 3D model that a particular + record refers to? (E.g. 3D printer model, game asset, artwork, etc.) +- Many model sites come with asset groups, should we have a table similar to + `AudioSets` for these cases? ## Proposed schema -Below is the proposed schema for the `model_3d` table in the catalog database. It is subject to change based on discussions that happen here and elsewhere. - -Many of the fields towards the end are taken from examples on SketchFab (e.g. https://sketchfab.com/3d-models/charmanders-midnight-snack-57866c01da20414ba81885206acbcc85) - -| Field | Type | Description | -| --- | --- | --- | -| `identifier` | `UUID` | Identifier for the record | -| `created_on` | `timestamp` | Time the record was created | -| `updated_on` | `timestamp` | Time the record was updated | -| `ingestion_type` | `varchar` | What generated the record | -| `provider` | `varchar` | Provider API that the model comes from | -| `source` | `varchar` | Source within the provider (e.g. NASA on Flickr), typically the same as `provider` | -| `foreign_id` | `varchar` | Identifier that the provider uses for this model | -| `foreign_landing_url` | `varchar` | URL on the provider that displays the "single-result" page for this model | -| `url` | `varchar` | URL to the actual asset itself (e.g. the image URL) | -| `thumbnail` | `varchar` | Thumbnail URL | -| `filesize` | `integer` | Size of the file | -| `license` | `varchar` | License for the model | -| `license_version` | `varchar` | Version of the model's license | -| `creator` | `varchar` | Creator ID of the model from the provider | -| `creator_url` | `varchar` | URL to the creator's page on the provider's site | -| `title` | `varchar` | Title of the model | -| `meta_data` | `jsonb` | Uncategorized metadata for the model | -| `tags` | `jsonb` | Tags that describe the model, for use in searching | -| `watermarked` | `bool` | Whether the model is watermarked - is this needed for 3D models? | -| `last_synced_with_source` | `timestamp` | Time the record was last updated from the provider | -| `removed_from_source` | `bool` | Whether the model has been removed from the upstream source | -| `category` | `varchar` | Category for the model (is this where we'll make the distinction between 3D printers, game assets, etc.?) | -| `filetype` | `varchar` | File type of the primary file for the model | -| `alt_files` | `jsonb` | Additional file types for the model | -| `vertices` | `integer` | Number of vertices within the model | -| `textures` | `integer` | Number of [textures](https://en.wikibooks.org/wiki/Blender_3D:_Noob_to_Pro/Materials_and_Textures) the model uses | -| `materials` | `integer` | Number of [materials](https://en.wikibooks.org/wiki/Blender_3D:_Noob_to_Pro/Materials_and_Textures) the model uses | -| `uv_mapping` | `bool` | Whether the model contains [UV Mapping](https://en.wikipedia.org/wiki/UV_mapping) | -| `vertex_colors` | `bool` | Whether the model contains [vertex colors](https://help.sketchfab.com/hc/en-us/articles/210765623-Vertex-Colors) | -| `pbr` | `bool` | Whether the model supports [physically based rendering](https://en.wikipedia.org/wiki/Physically_based_rendering) | -| `rigged_geometries` | `bool` | Whether the model supports [rigging for animation](https://dreamfarmstudios.com/blog/what-is-3d-rigging/) | -| `animations` | `integer` | Number of animations the model has | -| `morph_geometries` | `integer` | Number of [morph targets for animation](https://en.wikipedia.org/wiki/Morph_target_animation) | - +Below is the proposed schema for the `model_3d` table in the catalog database. +It is subject to change based on discussions that happen here and elsewhere. + +Many of the fields towards the end are taken from examples on SketchFab (e.g. +https://sketchfab.com/3d-models/charmanders-midnight-snack-57866c01da20414ba81885206acbcc85) + +| Field | Type | Description | +| ------------------------- | ----------- | ------------------------------------------------------------------------------------------------------------------ | +| `identifier` | `UUID` | Identifier for the record | +| `created_on` | `timestamp` | Time the record was created | +| `updated_on` | `timestamp` | Time the record was updated | +| `ingestion_type` | `varchar` | What generated the record | +| `provider` | `varchar` | Provider API that the model comes from | +| `source` | `varchar` | Source within the provider (e.g. NASA on Flickr), typically the same as `provider` | +| `foreign_id` | `varchar` | Identifier that the provider uses for this model | +| `foreign_landing_url` | `varchar` | URL on the provider that displays the "single-result" page for this model | +| `url` | `varchar` | URL to the actual asset itself (e.g. the image URL) | +| `thumbnail` | `varchar` | Thumbnail URL | +| `filesize` | `integer` | Size of the file | +| `license` | `varchar` | License for the model | +| `license_version` | `varchar` | Version of the model's license | +| `creator` | `varchar` | Creator ID of the model from the provider | +| `creator_url` | `varchar` | URL to the creator's page on the provider's site | +| `title` | `varchar` | Title of the model | +| `meta_data` | `jsonb` | Uncategorized metadata for the model | +| `tags` | `jsonb` | Tags that describe the model, for use in searching | +| `watermarked` | `bool` | Whether the model is watermarked - is this needed for 3D models? | +| `last_synced_with_source` | `timestamp` | Time the record was last updated from the provider | +| `removed_from_source` | `bool` | Whether the model has been removed from the upstream source | +| `category` | `varchar` | Category for the model (is this where we'll make the distinction between 3D printers, game assets, etc.?) | +| `filetype` | `varchar` | File type of the primary file for the model | +| `alt_files` | `jsonb` | Additional file types for the model | +| `vertices` | `integer` | Number of vertices within the model | +| `textures` | `integer` | Number of [textures](https://en.wikibooks.org/wiki/Blender_3D:_Noob_to_Pro/Materials_and_Textures) the model uses | +| `materials` | `integer` | Number of [materials](https://en.wikibooks.org/wiki/Blender_3D:_Noob_to_Pro/Materials_and_Textures) the model uses | +| `uv_mapping` | `bool` | Whether the model contains [UV Mapping](https://en.wikipedia.org/wiki/UV_mapping) | +| `vertex_colors` | `bool` | Whether the model contains [vertex colors](https://help.sketchfab.com/hc/en-us/articles/210765623-Vertex-Colors) | +| `pbr` | `bool` | Whether the model supports [physically based rendering](https://en.wikipedia.org/wiki/Physically_based_rendering) | +| `rigged_geometries` | `bool` | Whether the model supports [rigging for animation](https://dreamfarmstudios.com/blog/what-is-3d-rigging/) | +| `animations` | `integer` | Number of animations the model has | +| `morph_geometries` | `integer` | Number of [morph targets for animation](https://en.wikipedia.org/wiki/Morph_target_animation) | # 3D Models in the frontend @@ -129,13 +171,27 @@ Many of the fields towards the end are taken from examples on SketchFab (e.g. ht ### Performance -3D models are often heavy files. We should lazy-load and defer loading of these resources as much as possible. We should probably use *thumbnails* rather than actually render 3D models in the search results. We *could* consider something like [SketchFab's search results](https://sketchfab.com/search?q=snacks&type=models), where they render a 3D model on hover of the search results, when on desktop. - -- We need to consider low power devices and low bandwidth connections. In both of these instances we'll want to try and significantly limit the number of downloaded assets. Some solutions for this might be: - - Write new composables and/or components to detect user bandwidth and power (see https://github.com/GoogleChromeLabs/react-adaptive-hooks as an example in React!) +3D models are often heavy files. We should lazy-load and defer loading of these +resources as much as possible. We should probably use _thumbnails_ rather than +actually render 3D models in the search results. We _could_ consider something +like +[SketchFab's search results](https://sketchfab.com/search?q=snacks&type=models), +where they render a 3D model on hover of the search results, when on desktop. + +- We need to consider low power devices and low bandwidth connections. In both + of these instances we'll want to try and significantly limit the number of + downloaded assets. Some solutions for this might be: + - Write new composables and/or components to detect user bandwidth and power + (see https://github.com/GoogleChromeLabs/react-adaptive-hooks as an example + in React!) - Only display thumbnails on list views - - Have a 'play' button on single result views, that triggers the loading of code-split model viewer code - - Simplify mobile models by default, by using flat colors instead of textures, limiting vertice counts, or other approaches. We need to research what is possible here, and what might give a user a false negative impression about a model. We don't want to inadvertently destroy someone's work to the point that users won't want to download it. + - Have a 'play' button on single result views, that triggers the loading of + code-split model viewer code + - Simplify mobile models by default, by using flat colors instead of textures, + limiting vertice counts, or other approaches. We need to research what is + possible here, and what might give a user a false negative impression about + a model. We don't want to inadvertently destroy someone's work to the point + that users won't want to download it. ### File types @@ -144,30 +200,51 @@ Many of the fields towards the end are taken from examples on SketchFab (e.g. ht - [fbx](https://docs.fileformat.com/3d/fbx/) - [Usdz](https://www.marxentlabs.com/usdz-files/) - [obj](https://all3dp.com/1/obj-file-format-3d-printing-cad/) -- We'll need to find a standardized 3d model viewer that will work with various formats. We *really, really* don't want to write our own code here. We will also need to decide if we want to use this universally, or use SketchFab's viewer for SketchFab models, which is an excellent viewer. - - [Trois](https://github.com/troisjs/trois) is a good candidate. This is a Vue3 renderer for ThreeJS. +- We'll need to find a standardized 3d model viewer that will work with various + formats. We _really, really_ don't want to write our own code here. We will + also need to decide if we want to use this universally, or use SketchFab's + viewer for SketchFab models, which is an excellent viewer. + - [Trois](https://github.com/troisjs/trois) is a good candidate. This is a + Vue3 renderer for ThreeJS. ## Prior Art -- We already show 3D models as [part of images](https://search.openverse.engineering/search/image?q=dogs&source=sketchfab,thingiverse), which is great! Fewer scripts to write on the catalog side. +- We already show 3D models as + [part of images](https://search.openverse.engineering/search/image?q=dogs&source=sketchfab,thingiverse), + which is great! Fewer scripts to write on the catalog side. - To migrate these providers we should: - - Update and modify the provider scripts to support our 3d model type instead of images + - Update and modify the provider scripts to support our 3d model type + instead of images - Purge their existing images from the catalog `image` database table. - Run the updated provider scripts. -- We also already show Sketchfab's 3D model viewer for their single results: https://search-staging.openverse.engineering/image/00a6a7a4-6605-4cba-bacf-a08b0da546b9 +- We also already show Sketchfab's 3D model viewer for their single results: + https://search-staging.openverse.engineering/image/00a6a7a4-6605-4cba-bacf-a08b0da546b9 ## Collaborators -- The [Open Metaverse Interoperability Group](https://github.com/omigroup/omigroup) aims to standardized protocols for interactive elements in 3D scenes. Core contributor @antpb is a part of this group.1 -- Our friend @nebulousflynn at Sketchfab might have some ideas and suggestions as well. +- The + [Open Metaverse Interoperability Group](https://github.com/omigroup/omigroup) + aims to standardized protocols for interactive elements in 3D scenes. Core + contributor @antpb is a part of this group.1 +- Our friend @nebulousflynn at Sketchfab might have some ideas and suggestions + as well. ## Some code considerations -- When a unique identifier is necessary, we should use the string constant `const MODEL_3D = '3d model'` to refer to 3D models. Compare this to `AUDIO` for audio files, for example. -- Similar to the catalog/API, 3D model support should be enabled via a feature flag. This work should be able to be committed directly to `main` without a base feature branch needed. Meta search support for 3D models does not need to be feature flagged. -- e2e tests should be added for any new routes; and updated for existing routes that are modified. PRs will be blocked in the abcense of these tests. -- New content types should always launch in `beta` until we reach a certain number of results and/or are confident in our UI components (this metric(s) need to be determined). -- Many of the todos identified here can be broken down into even smaller pieces in the future, particularly around the user interface. +- When a unique identifier is necessary, we should use the string constant + `const MODEL_3D = '3d model'` to refer to 3D models. Compare this to `AUDIO` + for audio files, for example. +- Similar to the catalog/API, 3D model support should be enabled via a feature + flag. This work should be able to be committed directly to `main` without a + base feature branch needed. Meta search support for 3D models does not need to + be feature flagged. +- e2e tests should be added for any new routes; and updated for existing routes + that are modified. PRs will be blocked in the abcense of these tests. +- New content types should always launch in `beta` until we reach a certain + number of results and/or are confident in our UI components (this metric(s) + need to be determined). +- Many of the todos identified here can be broken down into even smaller pieces + in the future, particularly around the user interface. ## Technical implementation @@ -175,51 +252,74 @@ Many of the fields towards the end are taken from examples on SketchFab (e.g. ht #### Prerequisites -- Need a list of 3D model providers to use in meta search. This could be a mix of sources we intend to use in the Catalog but also some that don't have APIs and will stay in meta search after launching 3D model support. +- Need a list of 3D model providers to use in meta search. This could be a mix + of sources we intend to use in the Catalog but also some that don't have APIs + and will stay in meta search after launching 3D model support. #### Todos - [ ] Add 3D models to the content switcher components: - [ ] https://github.com/WordPress/openverse-frontend/blob/bbccc36ed49186b369cc4a69693fbc6d1411f29a/src/components/VContentSwitcher/VContentSwitcherButton.vue - [ ] https://github.com/WordPress/openverse-frontend/blob/7d74f033ddc4d35889d7c43756c36150beee4192/src/components/VContentSwitcher/VContentTypes.vue -- [ ] Create a `model/index.vue` endpoint for 3D models in the [pages directory](https://github.com/WordPress/openverse-frontend/blob/037f2efb768fdbdcff4b875e7f7c271ff50df59f/src/pages) -- [ ] Add 3D model sources to the [legacy sources lookup](https://github.com/WordPress/openverse-frontend/blob/268532a05004b72fb91b77a875f3a3c1d7fb0599/src/utils/get-legacy-source-url.js#L1) -- [ ] Add 3D models to the [media constants](https://github.com/WordPress/openverse-frontend/blob/4aeb9fd0d9f893f2df1752d6bb1e52351b8c3d35/src/constants/media.js#L13) file, making sure it is set to the `ADDITIONAL` status. +- [ ] Create a `model/index.vue` endpoint for 3D models in the + [pages directory](https://github.com/WordPress/openverse-frontend/blob/037f2efb768fdbdcff4b875e7f7c271ff50df59f/src/pages) +- [ ] Add 3D model sources to the + [legacy sources lookup](https://github.com/WordPress/openverse-frontend/blob/268532a05004b72fb91b77a875f3a3c1d7fb0599/src/utils/get-legacy-source-url.js#L1) +- [ ] Add 3D models to the + [media constants](https://github.com/WordPress/openverse-frontend/blob/4aeb9fd0d9f893f2df1752d6bb1e52351b8c3d35/src/constants/media.js#L13) + file, making sure it is set to the `ADDITIONAL` status. ### 2. Setup the data for 3D models -- [ ] Create a feature flag for 3D model support. All 3D model features will be behind this. *This specific implementation is waiting on a feature flag RFC and should be updated once that is finalized.* +- [ ] Create a feature flag for 3D model support. All 3D model features will be + behind this. _This specific implementation is waiting on a feature flag + RFC and should be updated once that is finalized._ - [ ] Generate mock/sample api response data for 3D models. -- [ ] Create a new media service for Models in the [data directory](https://github.com/WordPress/openverse-frontend/blob/fe24ec9bf6a4d3ad429901cef53744dccd93ed5d/src/data) -- [ ] Update the [media constants](https://github.com/WordPress/openverse-frontend/blob/4aeb9fd0d9f893f2df1752d6bb1e52351b8c3d35/src/constants/media.js#L1) file with new constants for models, and under the `BETA` flag. -- [ ] Update the [media store](https://github.com/WordPress/openverse-frontend/blob/f9f549beace4f76480c8cbf1e91d8940a0aeef2b/src/store/media.js#L347) with 3D model support. +- [ ] Create a new media service for Models in the + [data directory](https://github.com/WordPress/openverse-frontend/blob/fe24ec9bf6a4d3ad429901cef53744dccd93ed5d/src/data) +- [ ] Update the + [media constants](https://github.com/WordPress/openverse-frontend/blob/4aeb9fd0d9f893f2df1752d6bb1e52351b8c3d35/src/constants/media.js#L1) + file with new constants for models, and under the `BETA` flag. +- [ ] Update the + [media store](https://github.com/WordPress/openverse-frontend/blob/f9f549beace4f76480c8cbf1e91d8940a0aeef2b/src/store/media.js#L347) + with 3D model support. ### 3. Implement new 3D model UI #### Prerequisites -Design work for the new components must be completed, or at least in a good enough place that we decide work can begin. +Design work for the new components must be completed, or at least in a good +enough place that we decide work can begin. #### Todos - [ ] Add 3D models to the All Results view - - [ ] Create a `VModelCell.vue` component in the [`VAllResultsGrid` directory](https://github.com/WordPress/openverse-frontend/blob/f5b7518c3942a2ede49d5c7fef2831d30e9867a9/src/components/VAllResultsGrid) - - [ ] Add the logic to display models in the [`VAllResultsGrid.vue`](https://github.com/WordPress/openverse-frontend/blob/8214217f84099a83b9767f6a5c73fdbdf278bc95/src/components/VAllResultsGrid/VAllResultsGrid.vue#L1) component. -- [ ] Create a new single model route `pages/model/_id.vue` for viewing a 3D model in detail. - - [ ] Render the model - - [ ] Render the model info - - [ ] Render the attribution information - - [ ] Render the related models -- [ ] Create a new `VThreeDModel.vue` component that supports displaying 3D models of various file formats + - [ ] Create a `VModelCell.vue` component in the + [`VAllResultsGrid` directory](https://github.com/WordPress/openverse-frontend/blob/f5b7518c3942a2ede49d5c7fef2831d30e9867a9/src/components/VAllResultsGrid) + - [ ] Add the logic to display models in the + [`VAllResultsGrid.vue`](https://github.com/WordPress/openverse-frontend/blob/8214217f84099a83b9767f6a5c73fdbdf278bc95/src/components/VAllResultsGrid/VAllResultsGrid.vue#L1) + component. +- [ ] Create a new single model route `pages/model/_id.vue` for viewing a 3D + model in detail. + - [ ] Render the model + - [ ] Render the model info + - [ ] Render the attribution information + - [ ] Render the related models +- [ ] Create a new `VThreeDModel.vue` component that supports displaying 3D + models of various file formats ## Open Questions - What model formats do we want/need to support? -- What features do people need in a 3D model search engine? What functionality does our model viewer need to have? +- What features do people need in a 3D model search engine? What functionality + does our model viewer need to have? - Should we use SketchFab's viewer for SketchFab models? -- Some models are meant to be 3D printed; some are meant to be used as assets in digital art and games; others are more like standalone pieces of artwork. There are probably different user stories for each of these types. How can we optimize our UI for all of these different experiences? +- Some models are meant to be 3D printed; some are meant to be used as assets in + digital art and games; others are more like standalone pieces of artwork. + There are probably different user stories for each of these types. How can we + optimize our UI for all of these different experiences? ## Related RFCs -- Frontend Feature Flagging *Todo: Add link once created* -- API Feature Flagging *Todo: Add link once created* +- Frontend Feature Flagging _Todo: Add link once created_ +- API Feature Flagging _Todo: Add link once created_ diff --git a/rfcs/20220223-convert_the_store_from_vuex_to_pinia.md b/rfcs/20220223-convert_the_store_from_vuex_to_pinia.md index 6b69c04adad..c8b7aa97814 100644 --- a/rfcs/20220223-convert_the_store_from_vuex_to_pinia.md +++ b/rfcs/20220223-convert_the_store_from_vuex_to_pinia.md @@ -4,130 +4,260 @@ - [x] @sarayourfriend - **Milestone:** https://github.com/WordPress/openverse-frontend/milestone/5 - ## Rationale -Currently we use Vuex for global state management. This works fine for Vue 2 but has some considerable disadvantages for Vue 3, especially when it comes to TypeScript support and composition-api usage. We should switch to Pinia instead. It is the new [official default recommendation]( https://blog.vuejs.org/posts/vue-3-as-the-new-default.html) of the Vue project for state management instead of Vuex. Evan You, the creator of Vue, [has confirmed](https://twitter.com/youyuxi/status/1463429442076745730?lang=en) that Vuex 5 and Pinia are defacto the same. - -Pinia also has better interoperability between Vue 2's composition-api and Vue 3, and would allow us to decouple our state-management API usage from Nuxt. Currently we rely on Nuxt's home-grown `useStore` composable from the `@nuxtjs/composition-api` extension on top of `@vue/composition-api` but there's no guarantee that that composable will stick around in Nuxt 3. Besides, it is not easy to watch for changes in the store from within the `setup` function, at least when rendering on the server, and it caused problems with rendering the correct search input text on SSR. - -Moving to Pinia will allow us to reduce the current verbosity of the store introduced by module, action and mutation constants. In the long run, it will simplify the unit tests setup. +Currently we use Vuex for global state management. This works fine for Vue 2 but +has some considerable disadvantages for Vue 3, especially when it comes to +TypeScript support and composition-api usage. We should switch to Pinia instead. +It is the new +[official default recommendation](https://blog.vuejs.org/posts/vue-3-as-the-new-default.html) +of the Vue project for state management instead of Vuex. Evan You, the creator +of Vue, +[has confirmed](https://twitter.com/youyuxi/status/1463429442076745730?lang=en) +that Vuex 5 and Pinia are defacto the same. + +Pinia also has better interoperability between Vue 2's composition-api and Vue +3, and would allow us to decouple our state-management API usage from Nuxt. +Currently we rely on Nuxt's home-grown `useStore` composable from the +`@nuxtjs/composition-api` extension on top of `@vue/composition-api` but there's +no guarantee that that composable will stick around in Nuxt 3. Besides, it is +not easy to watch for changes in the store from within the `setup` function, at +least when rendering on the server, and it caused problems with rendering the +correct search input text on SSR. + +Moving to Pinia will allow us to reduce the current verbosity of the store +introduced by module, action and mutation constants. In the long run, it will +simplify the unit tests setup. ## Alternatives -We could just stick with the current Vuex approach and during the Nuxt bridge upgrade replace all usages of `useStore` with `useNuxtApp().$store` instead. This would introduce more code changes during the Nuxt bridge upgrade BUT would prevent us from having to re-write our entire state management strategy before migrating to Nuxt 3. +We could just stick with the current Vuex approach and during the Nuxt bridge +upgrade replace all usages of `useStore` with `useNuxtApp().$store` instead. +This would introduce more code changes during the Nuxt bridge upgrade BUT would +prevent us from having to re-write our entire state management strategy before +migrating to Nuxt 3. ## Potential risks -- Pinia is currently maintained by a Vue core member, but it's still not the official Vue Vuex implementation. However, its development is closely linked with the development of the new version of Vuex and they strive for the same API and some feature parity, so we should be able to move to Vuex 5 whenever it is released if we decide to do so. -- Store changes affect the app as a whole, and changes can cause a lot of disruption. To mitigate this, we can ensure that each store rewrite is accompanied with a full rewritten unit test, and try to use types as much as possible. + +- Pinia is currently maintained by a Vue core member, but it's still not the + official Vue Vuex implementation. However, its development is closely linked + with the development of the new version of Vuex and they strive for the same + API and some feature parity, so we should be able to move to Vuex 5 whenever + it is released if we decide to do so. +- Store changes affect the app as a whole, and changes can cause a lot of + disruption. To mitigate this, we can ensure that each store rewrite is + accompanied with a full rewritten unit test, and try to use types as much as + possible. ## Conversion to Pinia -There are some important differences between Vuex (the version we are currently using)and Pinia in store structure. + +There are some important differences between Vuex (the version we are currently +using)and Pinia in store structure. ### Dividing the store: Vuex modules and Pinia stores -Vuex uses a single store which can have multiple modules that can be namespaced. In our setup we have several namespaced modules inside `src/store` that Nuxt uses to create a single store. -In Pinia, the equivalent of a Vuex namespaced module is a single store. Namespacing is achieved by giving a store its `id`. All Pinia stores are separate modules, and are usually kept in a `src/stores` folder to emphasize that. To use a store `X` from store `Y`, you need to call `useXStore` inside `storeY`. -I couldn't find a way to create a central store equivalent to what we have now. So, when using the store in components, we would have to import all the stores that the component uses, instead of using a single `useStore()` call. This means that there will be more than single-line changes in the components. +Vuex uses a single store which can have multiple modules that can be namespaced. +In our setup we have several namespaced modules inside `src/store` that Nuxt +uses to create a single store. In Pinia, the equivalent of a Vuex namespaced +module is a single store. Namespacing is achieved by giving a store its `id`. +All Pinia stores are separate modules, and are usually kept in a `src/stores` +folder to emphasize that. To use a store `X` from store `Y`, you need to call +`useXStore` inside `storeY`. I couldn't find a way to create a central store +equivalent to what we have now. So, when using the store in components, we would +have to import all the stores that the component uses, instead of using a single +`useStore()` call. This means that there will be more than single-line changes +in the components. ### Pinia store definitions -Pinia currently supports two types of store definitions: **options store** and **setup store**. As a team, we lean towards using the _setup store_, not the _options store_. +Pinia currently supports two types of store definitions: **options store** and +**setup store**. As a team, we lean towards using the _setup store_, not the +_options store_. -**Options store** is similar to the Vuex store: it has `state`, `getters` and `actions`. It could be easier to convert the current Vuex modules into the Pinia options stores as the structure is almost the same, with the main difference being the fact that the mutations are converted into actions, and lack of `context` parameter for actions, and `state` parameter for mutation. +**Options store** is similar to the Vuex store: it has `state`, `getters` and +`actions`. It could be easier to convert the current Vuex modules into the Pinia +options stores as the structure is almost the same, with the main difference +being the fact that the mutations are converted into actions, and lack of +`context` parameter for actions, and `state` parameter for mutation. -**Setup store** is a newer kind of store that is more compatible with the composition API. The store definition itself is very similar to `setup` function we use in the components. For more context on the `setup store`, watch a talk by -Eduardo San Martin Morote (the creator of Pinia) where he explains the setup syntax on minute 22: [Everything Beyond State Management in Stores with Pinia by Eduardo San Martin Morote - GitNation](https://portal.gitnation.org/contents/everything-beyond-state-management-in-stores-with-pinia). In short, the **advantages** of using the setup store are: +**Setup store** is a newer kind of store that is more compatible with the +composition API. The store definition itself is very similar to `setup` function +we use in the components. For more context on the `setup store`, watch a talk by +Eduardo San Martin Morote (the creator of Pinia) where he explains the setup +syntax on minute 22: +[Everything Beyond State Management in Stores with Pinia by Eduardo San Martin Morote - GitNation](https://portal.gitnation.org/contents/everything-beyond-state-management-in-stores-with-pinia). +In short, the **advantages** of using the setup store are: -1. It is very similar to the components `setup` function, so it will be easier cognitively to switch between the store code and the component code. +1. It is very similar to the components `setup` function, so it will be easier + cognitively to switch between the store code and the component code. 2. We can use other composables within the store. -3. It is easier to use other stores from inside a store: you would only need to call `useXStore` once to use it in different `actions`. For example, when fetching media for search and fetching a single media item details, we use `UsageData` store to send the usage statistics. With the options store, we would have to use `const usageDataStore = useUsageDataStore()` for each action, and with the setup store we can set it up once and use in any action that needs it. +3. It is easier to use other stores from inside a store: you would only need to + call `useXStore` once to use it in different `actions`. For example, when + fetching media for search and fetching a single media item details, we use + `UsageData` store to send the usage statistics. With the options store, we + would have to use `const usageDataStore = useUsageDataStore()` for each + action, and with the setup store we can set it up once and use in any action + that needs it. -These advantages strongly outweigh some potential **downsides** to using the setup store: +These advantages strongly outweigh some potential **downsides** to using the +setup store: -1. Setup stores are not well documented. Most of the documentation examples use the options store. +1. Setup stores are not well documented. Most of the documentation examples use + the options store. -2. Setup store does not force us to separate the store code into state, getters and actions, so we would have to make effort to somehow make sure that the stores are well-organized, and not just a pile of spaghetti code. - -View a sample simple store in the [Pinia repository](https://github.com/vuejs/pinia/blob/f9ba40a7e86e39f5336b587d248f234ccdbad5ca/packages/pinia/test-dts/storeSetup.test-d.ts). +2. Setup store does not force us to separate the store code into state, getters + and actions, so we would have to make effort to somehow make sure that the + stores are well-organized, and not just a pile of spaghetti code. +View a sample simple store in the +[Pinia repository](https://github.com/vuejs/pinia/blob/f9ba40a7e86e39f5336b587d248f234ccdbad5ca/packages/pinia/test-dts/storeSetup.test-d.ts). ## Prior work -[Original issue](https://github.com/WordPress/openverse-frontend/issues/756) with a discussion of Pinia conversion and initial plan. + +[Original issue](https://github.com/WordPress/openverse-frontend/issues/756) +with a discussion of Pinia conversion and initial plan. [@dhruvkb's PoC PR with Pinia setup and conversion of active-media store to a Pinia options store, which was later rewritten as a setup store](https://github.com/WordPress/openverse-frontend/pull/906) ## Implementation plan -We have considered two options, converting stores one by one, or converting all stores in a feature branch, and decided that the first option is preferable. +We have considered two options, converting stores one by one, or converting all +stores in a feature branch, and decided that the first option is preferable. -1. Convert the store from Vuex to Pinia gradually, one store at a time. -@dhruvkb has created an excellent PoC PR for a gradual conversion to Pinia while keeping other stores in Vuex. +1. Convert the store from Vuex to Pinia gradually, one store at a time. @dhruvkb + has created an excellent PoC PR for a gradual conversion to Pinia while + keeping other stores in Vuex. -To minimise disruption, each store can be changed using a lock-based approach where the developer takes a lock on the store using a notification to the group, makes a PR updating a single store and its usages and this lock is released after the PR is merged. +To minimise disruption, each store can be changed using a lock-based approach +where the developer takes a lock on the store using a notification to the group, +makes a PR updating a single store and its usages and this lock is released +after the PR is merged. -This prevents PRs from diverging too much and allows a seamless transition for all involved. +This prevents PRs from diverging too much and allows a seamless transition for +all involved. -2. Create a feature base branch, do all the conversion in it, and only merge a fully-converted store into main. This will ensure that we are merging a fully-working converted store, and if we find any blocking concern, we can easily revert the changes by closing the feature branch. +2. Create a feature base branch, do all the conversion in it, and only merge a + fully-converted store into main. This will ensure that we are merging a + fully-working converted store, and if we find any blocking concern, we can + easily revert the changes by closing the feature branch. -While Pinia docs suggest that it is possible to migrate large projects from Vuex to Pinia module by module, they also say that using `disableVuex: false` is not recommended. ~Also, I'm not sure how to use Pinia stores from Vuex modules, and we do have several interdependent modules that call one another.~ In this case, we can use Pinia stores inside the Vuex stores by calling `useXStore()` inside getters, mutations and actions. +While Pinia docs suggest that it is possible to migrate large projects from Vuex +to Pinia module by module, they also say that using `disableVuex: false` is not +recommended. ~Also, I'm not sure how to use Pinia stores from Vuex modules, and +we do have several interdependent modules that call one another.~ In this case, +we can use Pinia stores inside the Vuex stores by calling `useXStore()` inside +getters, mutations and actions. ## Converting a Vuex namespaced store module to a Pinia store -There is an excellent guide on conversion in Pinia docs. This section will give steps that need to be followed for each store, and add some details specific to Openverse setup. + +There is an excellent guide on conversion in Pinia docs. This section will give +steps that need to be followed for each store, and add some details specific to +Openverse setup. For each store, there are several things that need to be done: + - convert the store creation to use `defineStore`. - convert state to `reactive` -- convert `getters` without params to `computed` and `getters` with parameters to functions. -- convert `actions` and `mutations` to functions. They will no longer use the `context` or `state` parameter, and will, instead of using an all-capital constant for a name, be renamed to use camelCase (e.g. `[FETCH_MEDIA]` will become `fetchMedia`). +- convert `getters` without params to `computed` and `getters` with parameters + to functions. +- convert `actions` and `mutations` to functions. They will no longer use the + `context` or `state` parameter, and will, instead of using an all-capital + constant for a name, be renamed to use camelCase (e.g. `[FETCH_MEDIA]` will + become `fetchMedia`). - return the necessary state properties, getters and actions. - adjust the types where necessary. -- remove the unused constants from - `~/constants/mutation-types.js`, `~/constants/action-types.js`, `~/constants/store-modules.js`. +- remove the unused constants from - `~/constants/mutation-types.js`, + `~/constants/action-types.js`, `~/constants/store-modules.js`. - refactor the use of the store in components. - add or refactor the store unit test. - refactor the unit tests for components that use this store. - add e2e tests for the changes in components. ### State -When using the setup store, we should try to use a `reactive` to create the initial state object, and not a set of `ref`s. Otherwise, we can create errors by accidentally having a state property name and an action parameter that is updating it with the same name. This doesn't work: + +When using the setup store, we should try to use a `reactive` to create the +initial state object, and not a set of `ref`s. Otherwise, we can create errors +by accidentally having a state property name and an action parameter that is +updating it with the same name. This doesn't work: + ```js -const message = ref('string message') -function setMessage({message}){ - message.value = message +const message = ref("string message") +function setMessage({ message }) { + message.value = message } ``` -Here, the destructured parameter name `message` shadows the state property name `message`, and the `setMessage` doesn't update the state as could be expected. -We can use `const { param1, param2 } = toRefs(state)` to enable store destructuring, and make each property reactive. If we do not return all of the state properties, such as when some properties only used from inside the store itself (e.g. `status` for `activeMediaStore`), we can also add a `readonly` state `state: readonly(state)` property to the return to make it testable. +Here, the destructured parameter name `message` shadows the state property name +`message`, and the `setMessage` doesn't update the state as could be expected. + +We can use `const { param1, param2 } = toRefs(state)` to enable store +destructuring, and make each property reactive. If we do not return all of the +state properties, such as when some properties only used from inside the store +itself (e.g. `status` for `activeMediaStore`), we can also add a `readonly` +state `state: readonly(state)` property to the return to make it testable. ### Actions -Pinia allows changing the state directly in the components (which was frowned upon by Vuex) so we could do away with mutations in theory. But it is better to keep using separate functions (actions) to update the state to keep state changes limited to one file and make it easier to debug. -When using the `setup store`, we don't need the strict divide between mutations that can change the state, and actions that cannot. This way, we can clean up the code from one-line mutations and test them simply by testing the resulting state. However, testing the network-based state changes is not possible this way, so it is better to leave even one line mutations that are based on the network responses. For example, `fetchMedia` in the `mediaStore` sets `isFetching` to true before sending the request, and to `false` after getting a 200 OK response. So, the state test would not detect the changes, and we can use the `hasBeenCalled` mock tests in such cases instead. +Pinia allows changing the state directly in the components (which was frowned +upon by Vuex) so we could do away with mutations in theory. But it is better to +keep using separate functions (actions) to update the state to keep state +changes limited to one file and make it easier to debug. + +When using the `setup store`, we don't need the strict divide between mutations +that can change the state, and actions that cannot. This way, we can clean up +the code from one-line mutations and test them simply by testing the resulting +state. However, testing the network-based state changes is not possible this +way, so it is better to leave even one line mutations that are based on the +network responses. For example, `fetchMedia` in the `mediaStore` sets +`isFetching` to true before sending the request, and to `false` after getting a +200 OK response. So, the state test would not detect the changes, and we can use +the `hasBeenCalled` mock tests in such cases instead. ### Type hinting -In the interest of a quick migration, we can continue to use the existing type definitions for the state and the various mutations/actions from the `types.d.ts` file. Some definitions in the file are out of date (as was the case for `ActiveMediaState`), so it is useful to check them. +In the interest of a quick migration, we can continue to use the existing type +definitions for the state and the various mutations/actions from the +`types.d.ts` file. Some definitions in the file are out of date (as was the case +for `ActiveMediaState`), so it is useful to check them. ### Testing -One of the acceptance criteria for the store conversion PRs should be 100% unit test coverage for the store module, and added e2e tests for the component changes. It is also important to ensure that e2e tests cover both SSR and client-side rendering. -One of the acceptance criteria for the store conversion PRs should be 100% unit test coverage for the store module, and added e2e tests for the component changes. It is also important to ensure that e2e tests cover both SSR and client-side rendering. -Pinia Docs has a cookbook page about testing Pinia stores, and testing components that use Pinia stores. Please note that the setup for component testing with Vue 2 is [slightly different from the Vue 3 setup](https://pinia.vuejs.org/cookbook/testing.html#unit-test-components-vue-2). + +One of the acceptance criteria for the store conversion PRs should be 100% unit +test coverage for the store module, and added e2e tests for the component +changes. It is also important to ensure that e2e tests cover both SSR and +client-side rendering. One of the acceptance criteria for the store conversion +PRs should be 100% unit test coverage for the store module, and added e2e tests +for the component changes. It is also important to ensure that e2e tests cover +both SSR and client-side rendering. Pinia Docs has a cookbook page about testing +Pinia stores, and testing components that use Pinia stores. Please note that the +setup for component testing with Vue 2 is +[slightly different from the Vue 3 setup](https://pinia.vuejs.org/cookbook/testing.html#unit-test-components-vue-2). ## Steps -0. Install Pinia and test compatibility. Create a sample Pinia store in the `stores` directory. +0. Install Pinia and test compatibility. Create a sample Pinia store in the + `stores` directory. + +1. Replace Vuex stores with their Pinia counterparts. The order should be from + smallest modules with fewer connections to the largest modules with more + connections to other modules: -1. Replace Vuex stores with their Pinia counterparts. The order should be from smallest modules with fewer connections to the largest modules with more connections to other modules: -- `active-media` and `nav` stores - already refactored in [#906](https://github.com/WordPress/openverse-frontend/pull/906) -- `search` store - another PoC split into 3 PRs ([#1038](https://github.com/WordPress/openverse-frontend/pull/1038), [#1039](https://github.com/WordPress/openverse-frontend/pull/1039), [#1040](https://github.com/WordPress/openverse-frontend/pull/1040)) to ensure that Pinia stores can be used from Vuex stores, and figure out testing. +- `active-media` and `nav` stores - already refactored in + [#906](https://github.com/WordPress/openverse-frontend/pull/906) +- `search` store - another PoC split into 3 PRs + ([#1038](https://github.com/WordPress/openverse-frontend/pull/1038), + [#1039](https://github.com/WordPress/openverse-frontend/pull/1039), + [#1040](https://github.com/WordPress/openverse-frontend/pull/1040)) to ensure + that Pinia stores can be used from Vuex stores, and figure out testing. - `user` store - `usage-data` store - `provider` store - `media` store -2. In the end, with no Vuex stores remaining, Vuex and all associated dependencies will be removed. The `store/index.js` file can also be removed after the complete migration to Pinia. The `disableVuex: false` flag should also be removed. +2. In the end, with no Vuex stores remaining, Vuex and all associated + dependencies will be removed. The `store/index.js` file can also be removed + after the complete migration to Pinia. The `disableVuex: false` flag should + also be removed. diff --git a/rfcs/20220307-monitoring_across_the_stack.md b/rfcs/20220307-monitoring_across_the_stack.md index 706ab85792c..dbfe9c95300 100644 --- a/rfcs/20220307-monitoring_across_the_stack.md +++ b/rfcs/20220307-monitoring_across_the_stack.md @@ -1,6 +1,9 @@ # Monitoring across the stack -Note: Because I had to spend a good deal of time sketching out how this stuff works, I wrote a good deal of preliminary code. It's probably not perfect so it's really only a starting point, but it at least gives a vision for how we could do this. +Note: Because I had to spend a good deal of time sketching out how this stuff +works, I wrote a good deal of preliminary code. It's probably not perfect so +it's really only a starting point, but it at least gives a vision for how we +could do this. ## Reviewers @@ -14,342 +17,706 @@ TBD ## Rationale -Production Openverse is deployed across several services, each with their own unique parameters for optimal performance and stability. We have little to no visibility into the performance of most of these services aside from Sentry. - -* Airflow - Has some Slack alerting for DAG failures/successes and some built in things we can gather basic performance information from. -* API - Has Sentry error monitoring but zero performance monitoring. -* Nuxt - Has Sentry error monitoring but zero performance monitoring. -* Thumbnail proxy - Has zero monitoring -* Postgres - Has zero monitoring aside from some CloudWatch system metrics being tracked -* Elasticsearch - Same as Postgres as far as I'm aware - -Without visibility into our various services stability and performance we have few or even zero ways to know whether we're introducing performance or stability degradations with new releases (nor whether we're improving things). If our services go down, we'll learn this because a user or contributor notices and lets the team know. This is not ideal. - -As the production instances of the Openverse API start getting used for Jetpack and even WordPress core integrations soon and in the near future we will likely see a tremendous increase in traffic. Without visibility into our services we will be significantly hindered when debugging production performance and stability issues. - -Without measuring _current_ performance, we can't make decisions about how to improve performance that aren't couched in anything better than educated guesses. This is not a situation we want to be in long term. We also have a sense that our services are reasonably speedy at the moment, but we don't know exactly how speedy or what kind of outliers exist. Sometimes we see the API slow down to over 2 seconds per-request. How often does that happen? Is it a 99th percentile or more like 60th (i.e., closer to the average experience). +Production Openverse is deployed across several services, each with their own +unique parameters for optimal performance and stability. We have little to no +visibility into the performance of most of these services aside from Sentry. + +- Airflow - Has some Slack alerting for DAG failures/successes and some built in + things we can gather basic performance information from. +- API - Has Sentry error monitoring but zero performance monitoring. +- Nuxt - Has Sentry error monitoring but zero performance monitoring. +- Thumbnail proxy - Has zero monitoring +- Postgres - Has zero monitoring aside from some CloudWatch system metrics being + tracked +- Elasticsearch - Same as Postgres as far as I'm aware + +Without visibility into our various services stability and performance we have +few or even zero ways to know whether we're introducing performance or stability +degradations with new releases (nor whether we're improving things). If our +services go down, we'll learn this because a user or contributor notices and +lets the team know. This is not ideal. + +As the production instances of the Openverse API start getting used for Jetpack +and even WordPress core integrations soon and in the near future we will likely +see a tremendous increase in traffic. Without visibility into our services we +will be significantly hindered when debugging production performance and +stability issues. + +Without measuring _current_ performance, we can't make decisions about how to +improve performance that aren't couched in anything better than educated +guesses. This is not a situation we want to be in long term. We also have a +sense that our services are reasonably speedy at the moment, but we don't know +exactly how speedy or what kind of outliers exist. Sometimes we see the API slow +down to over 2 seconds per-request. How often does that happen? Is it a 99th +percentile or more like 60th (i.e., closer to the average experience). ## Purpose -This RFC proposes we introduce [Prometheus](https://prometheus.io) and [Grafana](https://grafana.com/) to monitor our infrastructure. It also proposes that we create a new RDS Postgres instance for Grafana. - -Prometheus is a time series database with an advanced query language and the ability to configure aggregate data transformations (for example, based on a given metric you can also tell Prometheus to calculate a running mean of the metric from over the last 5 minutes, or any other span of time). It includes anomaly detection as well as integration with many tools you can use for alerting like Slack, PagerDuty, or email. There are [official and unofficial client libraries](https://prometheus.io/docs/instrumenting/clientlibs/) for all the langauges we currently use or could conceivably use in the future including Python, Node.js, Go, PHP, and JVM languages. - -Grafana is a data visualization tool that supports building complex dashboards. It has the ability to integrate with nearly any source of data you could imagine, including our existing catalog and API databases. In the future we could use it to monitor cataloging progress over time for example. It's a tool good for many things in addition to visualizing metrics. - -The Prometheus and Grafana services encompass the following key features that are important for maintaining the health and stability of our services: - -* Metrics gathering -* Dashboard creation for making it quick and easy to understand the current state of our service health -* Anomaly detection and alarming so that if things get weird, we'll know about it as soon as possible - -Additionally, both are free software. Prometheus is Apache 2 licensed and Grafana is AGPL-3.0. Both are actively developed in the open with heavy community involvement. They are also both widely deployed with lots of blog posts discussing configuration, use cases, and troubleshooting. +This RFC proposes we introduce [Prometheus](https://prometheus.io) and +[Grafana](https://grafana.com/) to monitor our infrastructure. It also proposes +that we create a new RDS Postgres instance for Grafana. + +Prometheus is a time series database with an advanced query language and the +ability to configure aggregate data transformations (for example, based on a +given metric you can also tell Prometheus to calculate a running mean of the +metric from over the last 5 minutes, or any other span of time). It includes +anomaly detection as well as integration with many tools you can use for +alerting like Slack, PagerDuty, or email. There are +[official and unofficial client libraries](https://prometheus.io/docs/instrumenting/clientlibs/) +for all the langauges we currently use or could conceivably use in the future +including Python, Node.js, Go, PHP, and JVM languages. + +Grafana is a data visualization tool that supports building complex dashboards. +It has the ability to integrate with nearly any source of data you could +imagine, including our existing catalog and API databases. In the future we +could use it to monitor cataloging progress over time for example. It's a tool +good for many things in addition to visualizing metrics. + +The Prometheus and Grafana services encompass the following key features that +are important for maintaining the health and stability of our services: + +- Metrics gathering +- Dashboard creation for making it quick and easy to understand the current + state of our service health +- Anomaly detection and alarming so that if things get weird, we'll know about + it as soon as possible + +Additionally, both are free software. Prometheus is Apache 2 licensed and +Grafana is AGPL-3.0. Both are actively developed in the open with heavy +community involvement. They are also both widely deployed with lots of blog +posts discussing configuration, use cases, and troubleshooting. ## Self-hosted vs. hosted We will self-host and use a new Postgres RDS instance for Grafana. -To simplify the hosting situation, let's try to deploy this using ECS from the beginning. +To simplify the hosting situation, let's try to deploy this using ECS from the +beginning. ## What will we monitor -There are three key aspects of the application services I think we should monitor, along with a slew of system level metrics that we should monitor as well. +There are three key aspects of the application services I think we should +monitor, along with a slew of system level metrics that we should monitor as +well. ### System level metrics -* CPU -* RAM -* Storage -* Processes/threads +- CPU +- RAM +- Storage +- Processes/threads -These would be gathered from Airflow, API, Thumbnail proxy, Postgres and Nuxt. These metrics need to be retrieved from AWS. I _think_ that [this page describes how to do that](https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch-Agent-PrometheusEC2.html) but AWS's language around this stuff is so cryptic to me I can't say with confidence it's the right documentation page for it. +These would be gathered from Airflow, API, Thumbnail proxy, Postgres and Nuxt. +These metrics need to be retrieved from AWS. I _think_ that +[this page describes how to do that](https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch-Agent-PrometheusEC2.html) +but AWS's language around this stuff is so cryptic to me I can't say with +confidence it's the right documentation page for it. -It would also be nice if we could integrate Cloudflare data into Grafana so that our visualizations are all in one place. Cache hits and other relevant information from Cloudflare would be excellent to monitor within our greater monitoring stack. +It would also be nice if we could integrate Cloudflare data into Grafana so that +our visualizations are all in one place. Cache hits and other relevant +information from Cloudflare would be excellent to monitor within our greater +monitoring stack. ### Application level metrics -These are _per service_ when available. The Thumbnail proxy and Postgres, for example, may not be able to implement all or any of these. +These are _per service_ when available. The Thumbnail proxy and Postgres, for +example, may not be able to implement all or any of these. -* Overall requests per second -* Per-route requests per second -* Per-route timings per response code -* Per-route response code rate -* Overall response code rate -* DB transaction times -* DB transaction count -* DB query count -* External request count and times +- Overall requests per second +- Per-route requests per second +- Per-route timings per response code +- Per-route response code rate +- Overall response code rate +- DB transaction times +- DB transaction count +- DB query count +- External request count and times ## How we will gather metrics -`django-prometheus` exports Prometheus metrics from a Django app and covers all the information we'd want, at least to start off with. +`django-prometheus` exports Prometheus metrics from a Django app and covers all +the information we'd want, at least to start off with. -There is not a real equivalent for Nuxt so we'll have to write our own either from scratch or based on the ones that already exist. It would be nice to do this as a separate Nuxt module so that we can share it with the wider Nuxt community as an actively maintained and used-in-production Prometheus exporter. +There is not a real equivalent for Nuxt so we'll have to write our own either +from scratch or based on the ones that already exist. It would be nice to do +this as a separate Nuxt module so that we can share it with the wider Nuxt +community as an actively maintained and used-in-production Prometheus exporter. -The rest of our services all have relevant exporters available, either official ones or ones with massive community support. Those are covered in the "Prometheus data integrations" section below. +The rest of our services all have relevant exporters available, either official +ones or ones with massive community support. Those are covered in the +"Prometheus data integrations" section below. ## Monitoring -It's hard to get into specifics about this, but there are a some scenarios up-front that we can anticipate wanting monitors for. - -* Sustained periods of time (60 seconds?) where `count_req_total == count_5xx_total` or `count_req_total >= (count_5xx_total / threshold)` - * Note, queries are rarely that intutive to write and have at least two moving parts that dictate how the alarm works, the query and then the alarm condition. -* Each view has a version of the above relative to request frequency. Views with lower overall requests per second will require longer windows of time before alerting. We could also configure something like `view_request_count > threshold && view_5xx_count == view_request_count` to filter out cases where only a small number of requests have occurred against a view that happened to fail; though this is risky. I think in those cases we'd not want an _alarm_ _per se_ but would still want to be closely monitoring these types of occurrences through some low-priority alert and active dashboard viewing. - -Additionally, we'll eventually be able to incorporate anomaly monitors. Anomaly monitoring boils down to determining whether there are any statistically significant periods of time where any given metric is an unexpected standard deviation away from seasonal-mean[^2] for the metric. For example, if we normally mean 1200 2xx response codes per second, with a standard deviation of 100 and our 2xx response codes drop to 1000 per second for a sustained period of time, then we'd want an alarm to be raised. - -Note that those numbers above are _not_ realistic, they're just simpler for explaining the concept. In reality an anomaly monitor takes a lot of tinkering to get right and often need to be iterated on to prevent false alarms. Additionally, I suspect our services see irregular levels of traffic so our standard deviations for some metrics might be really wild and hard to figure out a good solution for. If a metric is extremely consistent then alarming on deviations as low as 1 standard deviation away from mean could make sense. For irregular metrics we could have to go higher. - -Generally, anomaly monitors have two main variables that need tweaking to get right: - -* How long before alarming; that is, how long should the metric be outside the expected trend before we consider it to be an anomaly? -* How far outside the trend; that is, how many standard deviations away from the center/mean does it need to be for us to consider it anomalous? - -Our ability to fine tune monitors based on these two variables goes up with the more traffic we have, simply because with more data you can create better statistical trend models with which to define anomalous behavior against. If we have views that get relatively few requests per second then it will be difficult to create anomaly monitors for them and we'll have to rely on binary monitors for them. - -There are additional derived statistical information for each metric: percentiles. In particular for view timing this would be a useful thing to keep in mind. If the mean or median response time for a particular view is 300 ms, that looks great. However, if the 95th percentile response time is 1500 ms, then we've got a problem. Most users are experiencing good response times but there are some outliers that are getting significantly worse experiences. Percentile based metrics falling outside of 1 standard deviation from the mean should be actionable and high-value tickets to address. +It's hard to get into specifics about this, but there are a some scenarios +up-front that we can anticipate wanting monitors for. + +- Sustained periods of time (60 seconds?) where + `count_req_total == count_5xx_total` or + `count_req_total >= (count_5xx_total / threshold)` + - Note, queries are rarely that intutive to write and have at least two moving + parts that dictate how the alarm works, the query and then the alarm + condition. +- Each view has a version of the above relative to request frequency. Views with + lower overall requests per second will require longer windows of time before + alerting. We could also configure something like + `view_request_count > threshold && view_5xx_count == view_request_count` to + filter out cases where only a small number of requests have occurred against a + view that happened to fail; though this is risky. I think in those cases we'd + not want an _alarm_ _per se_ but would still want to be closely monitoring + these types of occurrences through some low-priority alert and active + dashboard viewing. + +Additionally, we'll eventually be able to incorporate anomaly monitors. Anomaly +monitoring boils down to determining whether there are any statistically +significant periods of time where any given metric is an unexpected standard +deviation away from seasonal-mean[^2] for the metric. For example, if we +normally mean 1200 2xx response codes per second, with a standard deviation of +100 and our 2xx response codes drop to 1000 per second for a sustained period of +time, then we'd want an alarm to be raised. + +Note that those numbers above are _not_ realistic, they're just simpler for +explaining the concept. In reality an anomaly monitor takes a lot of tinkering +to get right and often need to be iterated on to prevent false alarms. +Additionally, I suspect our services see irregular levels of traffic so our +standard deviations for some metrics might be really wild and hard to figure out +a good solution for. If a metric is extremely consistent then alarming on +deviations as low as 1 standard deviation away from mean could make sense. For +irregular metrics we could have to go higher. + +Generally, anomaly monitors have two main variables that need tweaking to get +right: + +- How long before alarming; that is, how long should the metric be outside the + expected trend before we consider it to be an anomaly? +- How far outside the trend; that is, how many standard deviations away from the + center/mean does it need to be for us to consider it anomalous? + +Our ability to fine tune monitors based on these two variables goes up with the +more traffic we have, simply because with more data you can create better +statistical trend models with which to define anomalous behavior against. If we +have views that get relatively few requests per second then it will be difficult +to create anomaly monitors for them and we'll have to rely on binary monitors +for them. + +There are additional derived statistical information for each metric: +percentiles. In particular for view timing this would be a useful thing to keep +in mind. If the mean or median response time for a particular view is 300 ms, +that looks great. However, if the 95th percentile response time is 1500 ms, then +we've got a problem. Most users are experiencing good response times but there +are some outliers that are getting significantly worse experiences. Percentile +based metrics falling outside of 1 standard deviation from the mean should be +actionable and high-value tickets to address. ## Isolating the monitoring backend -I considered that it might be nice to isolate the monitoring backend integration so that the various Openverse services didn't rely on any specific implementation. This would be nice (especially for alternative deployments of Openverse, which would be cool to be able to support) but is too heavy a lift for us and given we can isolate the integrations via configuration and external modules it wouldn't be worth our time at the moment. +I considered that it might be nice to isolate the monitoring backend integration +so that the various Openverse services didn't rely on any specific +implementation. This would be nice (especially for alternative deployments of +Openverse, which would be cool to be able to support) but is too heavy a lift +for us and given we can isolate the integrations via configuration and external +modules it wouldn't be worth our time at the moment. ## Relevant integrations -Here's the list of integrations we'll want to incorporate into Prometheus and Grafana. +Here's the list of integrations we'll want to incorporate into Prometheus and +Grafana. ### Prometheus data integrations -These are for pulling metrics into Prometheus from various services. This would all be in addition to writing and integration for Nuxt and deploying `django-prometheus` for the API (Airflow should be automatically covered by the exporter listed below). - -* Elasticsearch exporter: https://github.com/prometheus-community/elasticsearch_exporter -* Postgres exporter: https://github.com/prometheus-community/postgres_exporter -* AWS ECS exporter: https://github.com/slok/ecs-exporter -* Cloudflare exporter: https://gitlab.com/gitlab-org/cloudflare_exporter -* AWS CloudWatch exporter: https://github.com/prometheus/cloudwatch_exporter - * This one is interesting. I think we'd define system metrics to gather in CloudWatch and then export them into Prometheus. Hopefully this wouldn't incur additional costs to what we're already doing with CloudWatch. -* Alternative to the CloudWatch exporter is to implement the Node exporter: https://github.com/prometheus/node_exporter - * This has the added benefit of being portable across cloud providers vs CloudWatch being AWS services only. - * Does not cover ECS in an obvious way; could we run the node exporter in the container? Would that produce meaningful information? -* Additional alternative to CloudWatch exporter is EC2 exporter: https://github.com/jmal98/ec2-exporter -* Airflow exporter: https://github.com/epoch8/airflow-exporter +These are for pulling metrics into Prometheus from various services. This would +all be in addition to writing and integration for Nuxt and deploying +`django-prometheus` for the API (Airflow should be automatically covered by the +exporter listed below). + +- Elasticsearch exporter: + https://github.com/prometheus-community/elasticsearch_exporter +- Postgres exporter: https://github.com/prometheus-community/postgres_exporter +- AWS ECS exporter: https://github.com/slok/ecs-exporter +- Cloudflare exporter: https://gitlab.com/gitlab-org/cloudflare_exporter +- AWS CloudWatch exporter: https://github.com/prometheus/cloudwatch_exporter + - This one is interesting. I think we'd define system metrics to gather in + CloudWatch and then export them into Prometheus. Hopefully this wouldn't + incur additional costs to what we're already doing with CloudWatch. +- Alternative to the CloudWatch exporter is to implement the Node exporter: + https://github.com/prometheus/node_exporter + - This has the added benefit of being portable across cloud providers vs + CloudWatch being AWS services only. + - Does not cover ECS in an obvious way; could we run the node exporter in the + container? Would that produce meaningful information? +- Additional alternative to CloudWatch exporter is EC2 exporter: + https://github.com/jmal98/ec2-exporter +- Airflow exporter: https://github.com/epoch8/airflow-exporter ### Grafana alerting integrations -Grafana has built in alerting since v4.0. It works upstream from provider alertmanagers, so if we wanted to consume alerting rules from Prometheus we could build rules in Prometheus and then consume them in Grafana and alert on them. +Grafana has built in alerting since v4.0. It works upstream from provider +alertmanagers, so if we wanted to consume alerting rules from Prometheus we +could build rules in Prometheus and then consume them in Grafana and alert on +them. -[Here is the list of Grafana "contact points" for alerting.](https://grafana.com/docs/grafana/latest/alerting/unified-alerting/contact-points/) A contact point is the notification channels that Grafana can send alerts to. +[Here is the list of Grafana "contact points" for alerting.](https://grafana.com/docs/grafana/latest/alerting/unified-alerting/contact-points/) +A contact point is the notification channels that Grafana can send alerts to. -The most relevant for our usage are probably Slack, Webhook, and Email. Optionally we could incorporate PagerDuty depending on the team's tolerance and how we decide to organize expectations around responding to alerts. +The most relevant for our usage are probably Slack, Webhook, and Email. +Optionally we could incorporate PagerDuty depending on the team's tolerance and +how we decide to organize expectations around responding to alerts. -My preference is for us to primarily rely on email and in particular to use email lists to allow individuals to subscribe/unsubscribe at will from alarm streams and provide an easy historical public record of alarms. We could use something like [GNU Mailman](https://list.org/) to [host this ourselves](https://github.com/maxking/docker-mailman) using FARGATE. +My preference is for us to primarily rely on email and in particular to use +email lists to allow individuals to subscribe/unsubscribe at will from alarm +streams and provide an easy historical public record of alarms. We could use +something like [GNU Mailman](https://list.org/) to +[host this ourselves](https://github.com/maxking/docker-mailman) using FARGATE. -Email also makes it easy to compartmentalize alarms per category, it makes it easy for individuals to subscribe to specific streams of alarms by using different email prefixes per category. For example, each service could have its own mailing list. +Email also makes it easy to compartmentalize alarms per category, it makes it +easy for individuals to subscribe to specific streams of alarms by using +different email prefixes per category. For example, each service could have its +own mailing list. -We can also integrate Slack for good measure and have all alarms sent to the relevant channels via Slack incoming Webhook. It's very easy to configure the Slack integration. +We can also integrate Slack for good measure and have all alarms sent to the +relevant channels via Slack incoming Webhook. It's very easy to configure the +Slack integration. -The general purpose Webhook integration is useful for a future where we migrate away from Slack to Matrix (should that ever happen). +The general purpose Webhook integration is useful for a future where we migrate +away from Slack to Matrix (should that ever happen). #### Runbooks -Runbooks are the expected steps for addressing a particular alarm. As I mentioned elsewhere, alarms should be actionable. If they're not actionable then they're just noise and should be demoted to a warning until they're more accurate (less false positives). The _actions_ you take for any given alarm are directed by an alarm's accompanying runbook. +Runbooks are the expected steps for addressing a particular alarm. As I +mentioned elsewhere, alarms should be actionable. If they're not actionable then +they're just noise and should be demoted to a warning until they're more +accurate (less false positives). The _actions_ you take for any given alarm are +directed by an alarm's accompanying runbook. -All alarms should have a relevant runbook linked in the alarm body. Ideally these would just be flat Markdown files but it'd be best if they could be hosted in the Openverse handbook on WordPress.org as well for redundancy and accessibility. +All alarms should have a relevant runbook linked in the alarm body. Ideally +these would just be flat Markdown files but it'd be best if they could be hosted +in the Openverse handbook on WordPress.org as well for redundancy and +accessibility. ### Grafana data sources -* Prometheus: https://grafana.com/docs/grafana/latest/datasources/prometheus/ -* Elasticsearch: https://grafana.com/docs/grafana/latest/datasources/elasticsearch/ - * Note: not for visualizing system stats, those will come from Prometheus. This would be for querying the index. Totally optional. -* Postgres: https://grafana.com/docs/grafana/latest/datasources/postgres/ - * Just if we wanted to be able to visualize certain query results. Could eventually be relevant for general analytics. +- Prometheus: https://grafana.com/docs/grafana/latest/datasources/prometheus/ +- Elasticsearch: + https://grafana.com/docs/grafana/latest/datasources/elasticsearch/ + - Note: not for visualizing system stats, those will come from Prometheus. + This would be for querying the index. Totally optional. +- Postgres: https://grafana.com/docs/grafana/latest/datasources/postgres/ + - Just if we wanted to be able to visualize certain query results. Could + eventually be relevant for general analytics. ## Configuration and infrastructure (local and cloud) -The discussion about Prometheus configuration and infrastructure go hand in hand, primarily because so much of the configuration needs to directly reference other pieces of infrastructure for the various integrations to work. - -Prometheus works by scraping HTTP endpoints on services that provide a formatted set of metrics data. Essentially services don't need to worry about whether Prometheus is running or concern themselves with how long to wait between sending batched event lists. Each service just writes to a file (or some other storage) that is served to Prometheus over HTTP whenever it requests it. - -That has some really nice properties in that each service doesn't need to worry about what Prometheus is doing. We don't need to worry about it's tolerances for getting new events, rate limits, handling bad requests or timeouts if Prometheus is temporarily unavailable, anything like that. We (or rather the client library) just write and serve the metric data in the appropriate format and Prometheus handles the rest. - -This does have the effect of centralizing a ton of decisions about how to organize things directly into Prometheus's configuration. I don't know where we would want to store this information necessarily. Ideally it'd be somewhere central like our infrastructure repository so that we don't have to duplicate or aggregate various configurations during deployments. - -On the other hand, this massively complicates local development of the monitoring infrastructure and leaves us with two options: - -1. We can choose one of the existing repositories to put a docker-compose stack into that would spin up Prometheus and Grafana as needed for the entire local stack. The catalog, API, and frontend repositories would need to be configured such that that Prometheus stack is able to discover them on the local network. -2. We can place a `docker-compose.monitoring.yml` in the WordPress/openverse repository and optionally pull the file down when certain commands are run to set up monitoring for each individual piece of the stack. - -The second one seems more complicated to me. For one, if you're running all three repositories locally you'd end up with three separate Prometheus and Grafana instances to manage and configure. - -The first one also seems complicated but I believe it's also easier. Remember that Prometheus is the source of truth for the configuration, so we wouldn't have to spread out the knowledge about where Prometheus lives in all our services. As long as our services are configured to serve Prometheus the appropriate data on consistent HTTP endpoints, then any Prometheus instance anywhere can consume them. That does mean that some configuration will be spread in the other direction: that is, rather than storing information about Prometheus in each service, each service's information will be stored in Prometheus. This seems like an appropriate compromise. - -I'm not sure where the best place to put the monitoring docker-compose is. I think I'd like to propose putting it in WordPress/openverse under a `monitoring` directory that would include the `docker-compose.yml` as well as documentation about how to configure monitors, our own best practices, etc. It doesn't make sense to me for us to just pick one of the three application repositories to put it in. The monitoring stack also fits in with the "meta"/"overview" nature of the WordPress/openverse repository. - -The major difference between the production stack and the local stack will be the lack of a dedicated Postgres instance locally. Grafana will happily run on SQLite locally and there's no reason to complicate that for us locally (especially because we already have several other Postgres instances running locally elsewhere in the stack). We can follow the standard `docker-compose.overrides.yml` pattern we have followed to make changes in our stack between local and production. This will mostly involve passing environment variables to Grafana and Prometheus to configure them for production. - -Aside from service configuration, however, there is also the issue of how to store our Prometheus query, alerting, and other configuations locally. Likewise we need to address that for Grafana dashboards. For example, we don't want to have to manually copy the configurations for our anomaly monitors from production. Even if we develop these monitors in a live staging environment (more on that in the section below on the monitor development workflow) we'll still want to store those in code to have them committed to production, rather than manually copying them to prod. As long as the code they're saved to is accessible to the local monitoring stack, we should be able to seed the local stack with the same base monitors and dashboards present in production. +The discussion about Prometheus configuration and infrastructure go hand in +hand, primarily because so much of the configuration needs to directly reference +other pieces of infrastructure for the various integrations to work. + +Prometheus works by scraping HTTP endpoints on services that provide a formatted +set of metrics data. Essentially services don't need to worry about whether +Prometheus is running or concern themselves with how long to wait between +sending batched event lists. Each service just writes to a file (or some other +storage) that is served to Prometheus over HTTP whenever it requests it. + +That has some really nice properties in that each service doesn't need to worry +about what Prometheus is doing. We don't need to worry about it's tolerances for +getting new events, rate limits, handling bad requests or timeouts if Prometheus +is temporarily unavailable, anything like that. We (or rather the client +library) just write and serve the metric data in the appropriate format and +Prometheus handles the rest. + +This does have the effect of centralizing a ton of decisions about how to +organize things directly into Prometheus's configuration. I don't know where we +would want to store this information necessarily. Ideally it'd be somewhere +central like our infrastructure repository so that we don't have to duplicate or +aggregate various configurations during deployments. + +On the other hand, this massively complicates local development of the +monitoring infrastructure and leaves us with two options: + +1. We can choose one of the existing repositories to put a docker-compose stack + into that would spin up Prometheus and Grafana as needed for the entire local + stack. The catalog, API, and frontend repositories would need to be + configured such that that Prometheus stack is able to discover them on the + local network. +2. We can place a `docker-compose.monitoring.yml` in the WordPress/openverse + repository and optionally pull the file down when certain commands are run to + set up monitoring for each individual piece of the stack. + +The second one seems more complicated to me. For one, if you're running all +three repositories locally you'd end up with three separate Prometheus and +Grafana instances to manage and configure. + +The first one also seems complicated but I believe it's also easier. Remember +that Prometheus is the source of truth for the configuration, so we wouldn't +have to spread out the knowledge about where Prometheus lives in all our +services. As long as our services are configured to serve Prometheus the +appropriate data on consistent HTTP endpoints, then any Prometheus instance +anywhere can consume them. That does mean that some configuration will be spread +in the other direction: that is, rather than storing information about +Prometheus in each service, each service's information will be stored in +Prometheus. This seems like an appropriate compromise. + +I'm not sure where the best place to put the monitoring docker-compose is. I +think I'd like to propose putting it in WordPress/openverse under a `monitoring` +directory that would include the `docker-compose.yml` as well as documentation +about how to configure monitors, our own best practices, etc. It doesn't make +sense to me for us to just pick one of the three application repositories to put +it in. The monitoring stack also fits in with the "meta"/"overview" nature of +the WordPress/openverse repository. + +The major difference between the production stack and the local stack will be +the lack of a dedicated Postgres instance locally. Grafana will happily run on +SQLite locally and there's no reason to complicate that for us locally +(especially because we already have several other Postgres instances running +locally elsewhere in the stack). We can follow the standard +`docker-compose.overrides.yml` pattern we have followed to make changes in our +stack between local and production. This will mostly involve passing environment +variables to Grafana and Prometheus to configure them for production. + +Aside from service configuration, however, there is also the issue of how to +store our Prometheus query, alerting, and other configuations locally. Likewise +we need to address that for Grafana dashboards. For example, we don't want to +have to manually copy the configurations for our anomaly monitors from +production. Even if we develop these monitors in a live staging environment +(more on that in the section below on the monitor development workflow) we'll +still want to store those in code to have them committed to production, rather +than manually copying them to prod. As long as the code they're saved to is +accessible to the local monitoring stack, we should be able to seed the local +stack with the same base monitors and dashboards present in production. ### Local infrastructure recommendations -One of the goals I'd like to achieve with the local infrastructure is to make it as easy as running `just monitor` and it spins up everything with tolerances for services that aren't currently running locally. For example, if someone is working primarily on developing frontend monitors, it would do no good for Prometheus to be producing errors about not be able to reach a local API, Airflow, image proxy, or Postgres instance. Same for someone working on the catalog, they should have to deal with answers about the frontend not being discoverable by Prometheus locally. +One of the goals I'd like to achieve with the local infrastructure is to make it +as easy as running `just monitor` and it spins up everything with tolerances for +services that aren't currently running locally. For example, if someone is +working primarily on developing frontend monitors, it would do no good for +Prometheus to be producing errors about not be able to reach a local API, +Airflow, image proxy, or Postgres instance. Same for someone working on the +catalog, they should have to deal with answers about the frontend not being +discoverable by Prometheus locally. ## Monitor development workflow 1. Log into the staging Prometheus and Grafana instances 2. Develop your monitors and dashboards -3. Export them from staging and save them into the appropriate `monitoring/configurations/{prometheus,grafana}` folder - * For Grafana, you can save new dashboards in the `monitoring/grafana/dashboards` folder in the Grafana JSON format. Datasources should be configured in the `monitoring/grafana/provisioning/datasources/datasources.yml`. It is unnecessary to change the `dashboards/dashboards.yml` configuration as the proposed configuration automatically picks up new JSON files. - * For Prometheus, make the necessary changes to the `prometheus.yml.template`, run `just mkpromconf [environment]`. Any rules (if we end up using them) can be places directly into the `rules.d` and they will be automatically picked up by Prometheus the next time it is restarted. -4. Run the local monitoring stack to ensure that the saved configurations in a brand new deployment match the expected configurations from staging. - * This may require exercising the local service a bit to check that metrics appear as expected in the dashboards... can be difficult with small amounts of data though. -5. Deploy them to production via terraform or some other method to update the running Prometheus and Grafana instances with the latest configurations - * Terraform has [providers for uploading dashboard configurations to Grafana](https://registry.terraform.io/providers/grafana/grafana/latest) - * With Prometheus, generate the new configuration file per environment using `just mkpromconf [environment]` and upload the configuration to the environment then restart Prometheus. +3. Export them from staging and save them into the appropriate + `monitoring/configurations/{prometheus,grafana}` folder + - For Grafana, you can save new dashboards in the + `monitoring/grafana/dashboards` folder in the Grafana JSON format. + Datasources should be configured in the + `monitoring/grafana/provisioning/datasources/datasources.yml`. It is + unnecessary to change the `dashboards/dashboards.yml` configuration as the + proposed configuration automatically picks up new JSON files. + - For Prometheus, make the necessary changes to the + `prometheus.yml.template`, run `just mkpromconf [environment]`. Any rules + (if we end up using them) can be places directly into the `rules.d` and + they will be automatically picked up by Prometheus the next time it is + restarted. +4. Run the local monitoring stack to ensure that the saved configurations in a + brand new deployment match the expected configurations from staging. + - This may require exercising the local service a bit to check that metrics + appear as expected in the dashboards... can be difficult with small amounts + of data though. +5. Deploy them to production via terraform or some other method to update the + running Prometheus and Grafana instances with the latest configurations + - Terraform has + [providers for uploading dashboard configurations to Grafana](https://registry.terraform.io/providers/grafana/grafana/latest) + - With Prometheus, generate the new configuration file per environment using + `just mkpromconf [environment]` and upload the configuration to the + environment then restart Prometheus. ## DevEx I've added some new `just` recipes in the example code in this PR. -Grafana configurations are stored as JSON so I think they'd be linted by prettier. They should be copy/paste exports anyway so we don't need to lint them for correctness or even style technically. +Grafana configurations are stored as JSON so I think they'd be linted by +prettier. They should be copy/paste exports anyway so we don't need to lint them +for correctness or even style technically. -Prometheus configurations can be linted by `promtool` using this pre-commit repo: https://github.com/fortman/pre-commit-prometheus. +Prometheus configurations can be linted by `promtool` using this pre-commit +repo: https://github.com/fortman/pre-commit-prometheus. ## Outstanding questions ### Authentication -Do we want to hide the metrics endpoint behind authentication? I don't really see why we would other than to stop spamming (but we could throttle those endpoints to match the prometheus scrape rate for example and it would prevent anyone from doing too much damage). +Do we want to hide the metrics endpoint behind authentication? I don't really +see why we would other than to stop spamming (but we could throttle those +endpoints to match the prometheus scrape rate for example and it would prevent +anyone from doing too much damage). -I like the idea of having fully open Prometheus and Grafana dashboards that don't require authentication to _explore_. Obviously we'd want to hide administration inside of authentication but each Prometheus and Grafana handle that on their own. +I like the idea of having fully open Prometheus and Grafana dashboards that +don't require authentication to _explore_. Obviously we'd want to hide +administration inside of authentication but each Prometheus and Grafana handle +that on their own. ### Alerting Do we want to use Prometheus for alerting or Grafana? Pros for Prometheus: -- It is possible and potentially even trivial to provision alerts based on configuration files + +- It is possible and potentially even trivial to provision alerts based on + configuration files Cons for Prometheus: -- It is much harder to configure and automatically provision changes to Prometheus's configuration -- The UI for building them does not exist so I'm not sure what the best way to write new rules is other than via yaml files which will be harder for folks to contribute to given we're not all full-time devops people with tons of time to learn how to do this without fiddling around with visual helper tools + +- It is much harder to configure and automatically provision changes to + Prometheus's configuration +- The UI for building them does not exist so I'm not sure what the best way to + write new rules is other than via yaml files which will be harder for folks to + contribute to given we're not all full-time devops people with tons of time to + learn how to do this without fiddling around with visual helper tools Pros for Grafana: + - The UI for building them is nice Cons for Grafana: -- There's no way to export or provision them so the only way to add rules is through the UI (there's no JSON version of the form either). Moving rules across environments would be hard... to be fair, developing rules with anything except production data is also very difficult (lots of guessing) so maybe this isn't a problem. -I'd rather use Grafana provided we back up the database of alert configurations. If we do that then not having them in the code is hopefully less of a disaster. +- There's no way to export or provision them so the only way to add rules is + through the UI (there's no JSON version of the form either). Moving rules + across environments would be hard... to be fair, developing rules with + anything except production data is also very difficult (lots of guessing) so + maybe this isn't a problem. + +I'd rather use Grafana provided we back up the database of alert configurations. +If we do that then not having them in the code is hopefully less of a disaster. -It is possible to export _all_ of them via the rules API: `http://grafana/api/ruler/grafana/api/v1/rules`. There's just no way to upload that JSON blob directly. We could scrape the endpoint and back it up into the code base for good measure and to further distribute the configuration. +It is possible to export _all_ of them via the rules API: +`http://grafana/api/ruler/grafana/api/v1/rules`. There's just no way to upload +that JSON blob directly. We could scrape the endpoint and back it up into the +code base for good measure and to further distribute the configuration. ### Updating configurations #### Grafana -Grafana is configured to automatically detect new dashboards uploaded via JSON. It needs a restart to add new datasources and notification channels via the `provisioning` folder[^3]. +Grafana is configured to automatically detect new dashboards uploaded via JSON. +It needs a restart to add new datasources and notification channels via the +`provisioning` folder[^3]. There is currently no way to export or provision alerting rules. #### Prometheus -Prometheus is configured via a single `prometheus.yml` file. I've created a Python script that generatese the appropriate `prometheus.{environment}.yml` file based on a `.conf` file using Python's `configparser` module. It's in the contents of this PR. +Prometheus is configured via a single `prometheus.yml` file. I've created a +Python script that generatese the appropriate `prometheus.{environment}.yml` +file based on a `.conf` file using Python's `configparser` module. It's in the +contents of this PR. ## Overview of proposed changes -* Use the existing Grafana instance in production -* Use ECS to deploy the following services in production: - * Prometheus - * GNU Mailman -* Use a new, small RDS instance for Grafana's Postgres DB. -* Rely on Prometheus primarily as a metrics aggregator and time series database. Do not use it for rule and alarm configuration. -* Rely on Grafana for visbility and alarming upstream from Prometheus. -* Create separate alarm streams per service in Mailman and aggregate all alarms into a Slack channel for extra visibility and redundancy. -* Use `django-prometheus` library to add the Prometheus `/metrics` endpoint to the API -* Expose Grafana dashboards publicly _a la_ [Wikimedia](https://grafana.wikimedia.org/?orgId=1) and other FOSS projects -* Create `@openverse/nuxt-module-prometheus` to create a pluggable Prometheus monitoring integration for Nuxt based on existing (but incomplete) projects -* Integrate Prometheus exporters for the rest of our services and infrastructure -* Deploy Runbooks for all alarms to the Openverse handbook on WordPress.org +- Use the existing Grafana instance in production +- Use ECS to deploy the following services in production: + - Prometheus + - GNU Mailman +- Use a new, small RDS instance for Grafana's Postgres DB. +- Rely on Prometheus primarily as a metrics aggregator and time series database. + Do not use it for rule and alarm configuration. +- Rely on Grafana for visbility and alarming upstream from Prometheus. +- Create separate alarm streams per service in Mailman and aggregate all alarms + into a Slack channel for extra visibility and redundancy. +- Use `django-prometheus` library to add the Prometheus `/metrics` endpoint to + the API +- Expose Grafana dashboards publicly _a la_ + [Wikimedia](https://grafana.wikimedia.org/?orgId=1) and other FOSS projects +- Create `@openverse/nuxt-module-prometheus` to create a pluggable Prometheus + monitoring integration for Nuxt based on existing (but incomplete) projects +- Integrate Prometheus exporters for the rest of our services and infrastructure +- Deploy Runbooks for all alarms to the Openverse handbook on WordPress.org ## Implementation Plan -1. Merge the example local stack implementation from this PR and update any of the things we agree to change during the RFC period. - * Additionally configure pre-commit linting for prometheus configurations -1. Add `django_prometheus` to the API to expose the `/metrics` endpoint consumed by Prometheus. - * As part of this we need to figure out if we want to hide the metrics endpoint behind authentication. See `Outstanding questions/Authentication` above. - * Configure Prometheus to be able to scrape the relevant environment's `/metrics` endpoint (base this off the local configuration) - * Deploy the API and confirm that the metrics are able to be scraped at the new endpoint. -1. Create the same `/metrics` endpoint for Nuxt using the [Node.js client library](https://github.com/siimon/prom-client) - * This will be more work as we'll have to write our own middlware for it. - * There are some existing Nuxt Prometheus modules like [this one](https://github.com/franckaragao/nuxt-prometheus-module) but they're not maintained and far more basic than what we actually want to get. We could fork those and create our own `@openverse/nuxt-prometheus-module` or something with better defaults more akin to the level of visibility that the `django-prometheus` module provides for Django. - * Configure Prometheus to be able to scrape the `/metrics` endpoint and update configurations for each environment. +1. Merge the example local stack implementation from this PR and update any of + the things we agree to change during the RFC period. + - Additionally configure pre-commit linting for prometheus configurations +1. Add `django_prometheus` to the API to expose the `/metrics` endpoint consumed + by Prometheus. + - As part of this we need to figure out if we want to hide the metrics + endpoint behind authentication. See `Outstanding questions/Authentication` + above. + - Configure Prometheus to be able to scrape the relevant environment's + `/metrics` endpoint (base this off the local configuration) + - Deploy the API and confirm that the metrics are able to be scraped at the + new endpoint. +1. Create the same `/metrics` endpoint for Nuxt using the + [Node.js client library](https://github.com/siimon/prom-client) + - This will be more work as we'll have to write our own middlware for it. + - There are some existing Nuxt Prometheus modules like + [this one](https://github.com/franckaragao/nuxt-prometheus-module) but + they're not maintained and far more basic than what we actually want to + get. We could fork those and create our own + `@openverse/nuxt-prometheus-module` or something with better defaults more + akin to the level of visibility that the `django-prometheus` module + provides for Django. + - Configure Prometheus to be able to scrape the `/metrics` endpoint and + update configurations for each environment. 1. Continue the above for each service. -1. Let metrics gather for around 3-6 weeks then begin identifying areas we can create our first alerts around. Once we reach that milestone, create issues for creating alarms and writing runbooks for them. +1. Let metrics gather for around 3-6 weeks then begin identifying areas we can + create our first alerts around. Once we reach that milestone, create issues + for creating alarms and writing runbooks for them.
-[^0]: While not necessary, we may consider switching to asyncio Django for this reason, so we can fire event submission tasks without blocking the response. For example, in the middleware pseudo code above it would be nice to be able to send the response back before incrementing the view status count. Alternatively we could add Celery for this purpose as well. -[^1]: In Django we'll use the view name, the class or function name that handles the view. This should include the absolute module path to ensure disambiguation between views. In Nuxt there isn't the same concept, so we'll just need to rely on the path. Replace intermediate slashes with double underscores, (e.g., `/search/images` turns into `search__images`). -[^2]: The "seasonal-mean" is a mean that incorporates daily, weekly or monthly seasonality. For example, the mean Friday at 1200 UTC is treated as distinct from the mean on Saturday at 1200 UTC. Browsing activity changes during the week. Most services see reduced traffic over the weekends. I'm not sure if that'll be the case for Openverse but implementing this is usually "free" and comes with any standard -[^3]: This could be wrong but I couldn't find any other way to do it that didn't require a script that made HTTP requests. Even the grafana-cli isn't capable of re-evaluating the provisioning scripts. +[^0]: + While not necessary, we may consider switching to asyncio Django for this + reason, so we can fire event submission tasks without blocking the response. + For example, in the middleware pseudo code above it would be nice to be able + to send the response back before incrementing the view status count. + Alternatively we could add Celery for this purpose as well. + +[^1]: + In Django we'll use the view name, the class or function name that handles + the view. This should include the absolute module path to ensure + disambiguation between views. In Nuxt there isn't the same concept, so we'll + just need to rely on the path. Replace intermediate slashes with double + underscores, (e.g., `/search/images` turns into `search__images`). + +[^2]: + The "seasonal-mean" is a mean that incorporates daily, weekly or monthly + seasonality. For example, the mean Friday at 1200 UTC is treated as distinct + from the mean on Saturday at 1200 UTC. Browsing activity changes during the + week. Most services see reduced traffic over the weekends. I'm not sure if + that'll be the case for Openverse but implementing this is usually "free" + and comes with any standard + +[^3]: + This could be wrong but I couldn't find any other way to do it that didn't + require a script that made HTTP requests. Even the grafana-cli isn't capable + of re-evaluating the provisioning scripts. # Appendices ## Appendix A: Relevant documentation -Below are links to relevant documentation in Prometheus and Grafana. Reviewing these is not necessary for reviewing the RFC. I've included them to help implementers have a good starting point for finding things as sometimes even knowing what terms to search for can be a hurdle. - -* Prometheus rules unit tests: https://prometheus.io/docs/prometheus/latest/configuration/unit_testing_rules/ -* Prometheus metric types: https://prometheus.io/docs/concepts/metric_types/ -* Prometheus anomaly detection: https://prometheus.io/blog/2015/06/18/practical-anomaly-detection/ -* Prometheus query functions: https://prometheus.io/docs/prometheus/latest/querying/functions/ -* Prometheus range vectors: https://prometheus.io/docs/prometheus/latest/querying/basics/#range-vector-selectors -* Prometheus naming best practices: https://prometheus.io/docs/practices/naming/ -* Prometheus official and unofficial client libraries: https://prometheus.io/docs/instrumenting/clientlibs/ -* Adding a datasource to Grafana: https://grafana.com/docs/grafana/latest/datasources/add-a-data-source/ -* Grafana dashboard best practices: https://grafana.com/docs/grafana/latest/best-practices/best-practices-for-creating-dashboards/ -* Histograms and heatmaps in Grafana: https://grafana.com/docs/grafana/latest/basics/intro-histograms/ -* Grafana glossary (super helpful!): https://grafana.com/docs/grafana/latest/basics/glossary/ +Below are links to relevant documentation in Prometheus and Grafana. Reviewing +these is not necessary for reviewing the RFC. I've included them to help +implementers have a good starting point for finding things as sometimes even +knowing what terms to search for can be a hurdle. + +- Prometheus rules unit tests: + https://prometheus.io/docs/prometheus/latest/configuration/unit_testing_rules/ +- Prometheus metric types: https://prometheus.io/docs/concepts/metric_types/ +- Prometheus anomaly detection: + https://prometheus.io/blog/2015/06/18/practical-anomaly-detection/ +- Prometheus query functions: + https://prometheus.io/docs/prometheus/latest/querying/functions/ +- Prometheus range vectors: + https://prometheus.io/docs/prometheus/latest/querying/basics/#range-vector-selectors +- Prometheus naming best practices: https://prometheus.io/docs/practices/naming/ +- Prometheus official and unofficial client libraries: + https://prometheus.io/docs/instrumenting/clientlibs/ +- Adding a datasource to Grafana: + https://grafana.com/docs/grafana/latest/datasources/add-a-data-source/ +- Grafana dashboard best practices: + https://grafana.com/docs/grafana/latest/best-practices/best-practices-for-creating-dashboards/ +- Histograms and heatmaps in Grafana: + https://grafana.com/docs/grafana/latest/basics/intro-histograms/ +- Grafana glossary (super helpful!): + https://grafana.com/docs/grafana/latest/basics/glossary/ ## Appendix B: Alternative technologies -The following alternatives were considered before landing on Prometheus and Grafana. +The following alternatives were considered before landing on Prometheus and +Grafana. ### Graphite with Grafana -[Graphite](https://graphiteapp.org/) is very easy to put metrics into and integrates smoothly with Grafana. However, for every bit as easy as Graphite is to put metrics into, it is also that difficult to create queries against. Graphite does not hold your hand at all. It supports anomaly detection through [additional services](https://github.com/etsy/skyline), but they're not actively developed and configuring the anomaly detections requires relatively deep knowledge about statistics that I don't think our team is necessarily prepared to invest time into (nor do we have the existing knowledge here as far as I know). - -Graphite is also older than Prometheus and while it works really well, there's a lot less current information about how to deploy and configure it. There's also almost no support for languages outside of the core langauges supported by the project including poor JavaScript support (which would make monitoring our Nuxt service more difficult). +[Graphite](https://graphiteapp.org/) is very easy to put metrics into and +integrates smoothly with Grafana. However, for every bit as easy as Graphite is +to put metrics into, it is also that difficult to create queries against. +Graphite does not hold your hand at all. It supports anomaly detection through +[additional services](https://github.com/etsy/skyline), but they're not actively +developed and configuring the anomaly detections requires relatively deep +knowledge about statistics that I don't think our team is necessarily prepared +to invest time into (nor do we have the existing knowledge here as far as I +know). + +Graphite is also older than Prometheus and while it works really well, there's a +lot less current information about how to deploy and configure it. There's also +almost no support for languages outside of the core langauges supported by the +project including poor JavaScript support (which would make monitoring our Nuxt +service more difficult). ### CloudWatch -[CloudWatch](https://docs.aws.amazon.com/cloudwatch/index.html) is AWS's proprietary system and service monitoring tool. It supports the three key features I listed above. It is also easy to deploy, literally just click a button and AWS adds it to your account. +[CloudWatch](https://docs.aws.amazon.com/cloudwatch/index.html) is AWS's +proprietary system and service monitoring tool. It supports the three key +features I listed above. It is also easy to deploy, literally just click a +button and AWS adds it to your account. -As long as you're comfortable with AWS IAM credentials management, it is also easy to wire up the AWS SDK clients to the backend. +As long as you're comfortable with AWS IAM credentials management, it is also +easy to wire up the AWS SDK clients to the backend. -However, as it is a proprietary AWS offering, it is targeted at large enterprise clients, most of whom have multiple teams dedicated to managing their infrastructure. These teams are often staffed with engineers who are trained specifically in AWS and will have enterprise level support accounts to lean on to fill in knowledge gaps. +However, as it is a proprietary AWS offering, it is targeted at large enterprise +clients, most of whom have multiple teams dedicated to managing their +infrastructure. These teams are often staffed with engineers who are trained +specifically in AWS and will have enterprise level support accounts to lean on +to fill in knowledge gaps. We don't have anyone on the team who has dedicated AWS knowledge. -AWS's documentation for CloudWatch is downright cryptic. There are decent overviews about the _concepts_, but implementing new monitors in Python or JavaScript might as well be esoteric knowledge. The libraries are extremely low-level for what they're doing and require you to manage batched event reporting by hand, something other client libraries for competing services like Prometheu's clients or statsd for Graphite handle out of the box. +AWS's documentation for CloudWatch is downright cryptic. There are decent +overviews about the _concepts_, but implementing new monitors in Python or +JavaScript might as well be esoteric knowledge. The libraries are extremely +low-level for what they're doing and require you to manage batched event +reporting by hand, something other client libraries for competing services like +Prometheu's clients or statsd for Graphite handle out of the box. -Additionally, it is a proprietary service. We would not be able to get our metrics out of AWS and if we decided to move away from AWS this would be a huge hurdle to overcome if we invest into it. Likewise, anyone else trying to deploy Openverse anywhere other than AWS would have to do work to tear out the CloudWatch stuff as there are no alternative backends for it the way there are for S3. More on this below. +Additionally, it is a proprietary service. We would not be able to get our +metrics out of AWS and if we decided to move away from AWS this would be a huge +hurdle to overcome if we invest into it. Likewise, anyone else trying to deploy +Openverse anywhere other than AWS would have to do work to tear out the +CloudWatch stuff as there are no alternative backends for it the way there are +for S3. More on this below. ### Other extremely low level monitoring tools -There are also some very low level [Linux daemon based time-series databases](https://github.com/oetiker/rrdtool-1.x) that are quite capable and can integrate with Grafana. However, they are all very low level and mostly do not support all of the three features I listed above. +There are also some very low level +[Linux daemon based time-series databases](https://github.com/oetiker/rrdtool-1.x) +that are quite capable and can integrate with Grafana. However, they are all +very low level and mostly do not support all of the three features I listed +above. ### DataDog -[DataDog](https://www.datadoghq.com/) is a SaaS offerring that runs an open source stack mixed with some proprietary magic. Prometheus and Grafana together very much resemble DataDog, even down to the UI. +[DataDog](https://www.datadoghq.com/) is a SaaS offerring that runs an open +source stack mixed with some proprietary magic. Prometheus and Grafana together +very much resemble DataDog, even down to the UI. -DataDog is fantastic. But it also comes at a higher cost than self-hosted Prometheus and Grafana. It is also ultimately a closed system. They provide ways for getting your data out and the client libraries for sending events used are the same (or very similar) to the ones for Graphite and Prometheus but it would still put work on alternative Openverse instances to make significant changes to the monitoring backend. +DataDog is fantastic. But it also comes at a higher cost than self-hosted +Prometheus and Grafana. It is also ultimately a closed system. They provide ways +for getting your data out and the client libraries for sending events used are +the same (or very similar) to the ones for Graphite and Prometheus but it would +still put work on alternative Openverse instances to make significant changes to +the monitoring backend. -DataDog does have very good documentation and holds your hand a _lot_ when it comes to setting up monitors and dashboards. If I had to choose a hosted proprietary solution I'd go with DataDog over AWS. However, I would not choose it over hosted Prometheus and Grafana if given the choice. +DataDog does have very good documentation and holds your hand a _lot_ when it +comes to setting up monitors and dashboards. If I had to choose a hosted +proprietary solution I'd go with DataDog over AWS. However, I would not choose +it over hosted Prometheus and Grafana if given the choice. ### Solar Winds stack -Solar Winds also has their own [server and application monitoring tool](https://www.solarwinds.com/server-application-monitor/use-cases). It's meant to be easy to deploy and automatically configures itself for the services you have running. I don't have any experience with this other than having used it to monitor Tomcat services at a previous job. My impression is that it's targeted mostly at large enterprise and government contracts rather than projects like ours. The services themselves are also not free software, though the SDKs that integrate with them are. Likewise, it doesn't seem to be a very flexible offering; it would probably be difficult to integrate things like monitoring a Nuxt service. +Solar Winds also has their own +[server and application monitoring tool](https://www.solarwinds.com/server-application-monitor/use-cases). +It's meant to be easy to deploy and automatically configures itself for the +services you have running. I don't have any experience with this other than +having used it to monitor Tomcat services at a previous job. My impression is +that it's targeted mostly at large enterprise and government contracts rather +than projects like ours. The services themselves are also not free software, +though the SDKs that integrate with them are. Likewise, it doesn't seem to be a +very flexible offering; it would probably be difficult to integrate things like +monitoring a Nuxt service. ### The ELK stack -The final alternative is the [ELK stack](https://www.elastic.co/what-is/elk-stack). It's not exactly a 1:1 alternative to Prometheus and Grafana, though it could fulfill [some](https://www.elastic.co/guide/en/kibana/current/xpack-ml-anomalies.html) of the [same](https://www.elastic.co/kibana/) use cases. This is a strong alternative that I hope reviewers of this RFC spend time considering. Read through Elastic's documentation about it and see if it could make sense for us. It could also be something that we implement down the road as a supplement to Prometheus and Grafana. Or it can replace it altogether. +The final alternative is the +[ELK stack](https://www.elastic.co/what-is/elk-stack). It's not exactly a 1:1 +alternative to Prometheus and Grafana, though it could fulfill +[some](https://www.elastic.co/guide/en/kibana/current/xpack-ml-anomalies.html) +of the [same](https://www.elastic.co/kibana/) use cases. This is a strong +alternative that I hope reviewers of this RFC spend time considering. Read +through Elastic's documentation about it and see if it could make sense for us. +It could also be something that we implement down the road as a supplement to +Prometheus and Grafana. Or it can replace it altogether. diff --git a/rfcs/20220309-feature_flags.md b/rfcs/20220309-feature_flags.md index 649324e6a91..e6c3cae1eef 100644 --- a/rfcs/20220309-feature_flags.md +++ b/rfcs/20220309-feature_flags.md @@ -9,47 +9,62 @@ ## Rationale -So far we have built features on a branch other than `main` and kept those branches separate until the features were ready to be visible. This has worked out so far but it is +So far we have built features on a branch other than `main` and kept those +branches separate until the features were ready to be visible. This has worked +out so far but it is - **untenable** - Branches tend to drift when commits are pushed to `main` (like constant bug-fixes) and conflicts tend to emerge when the commits touch the same files. + Branches tend to drift when commits are pushed to `main` (like constant + bug-fixes) and conflicts tend to emerge when the commits touch the same files. - **wasteful** - Considerable effort goes into keeping the branches in sync that is not affordable in a small team. Also if the feature is squash-merged into `main`, a lot of history is lost inside one commit. + Considerable effort goes into keeping the branches in sync that is not + affordable in a small team. Also if the feature is squash-merged into `main`, + a lot of history is lost inside one commit. - **un-_fun_** - Everyone wants to build exciting features, not exercise Git-fu when there are conflicts or when it's time to finally merge the feature in. + Everyone wants to build exciting features, not exercise Git-fu when there are + conflicts or when it's time to finally merge the feature in. ## Goals -Feature flags, as an alternative to the current methodology, should allow us to do the following: +Feature flags, as an alternative to the current methodology, should allow us to +do the following: ### Must haves -These features are strict requirements and solutions not providing them will be disqualified. +These features are strict requirements and solutions not providing them will be +disqualified. 1. Develop right on `main`, keeping visibility hidden until we can go live. 1. Allow code for both live and in-dev features to coexist. 1. Deployments to prod such as fixes should not be blocked. 1. Keep the code separate from the feature visibility configuration[^separate]. -1. Toggle the state (enabled, disabled, switchable) of features based on the environment. +1. Toggle the state (enabled, disabled, switchable) of features based on the + environment. -[^separate]: Separate not in the sense of being placed somewhere else, just being untangled or decoupled. +[^separate]: + Separate not in the sense of being placed somewhere else, just being + untangled or decoupled. ### Good to haves -These features are not strictly needed and a solution lacking them will not be discarded. +These features are not strictly needed and a solution lacking them will not be +discarded. 1. Enable features to go-live without technically needing a new release. -1. Associate certain metadata or sub-attributes linked to features such as conditions for toggling etc. -1. Nested feature flags, with the parent flag acting as shortcut to toggle all its children. +1. Associate certain metadata or sub-attributes linked to features such as + conditions for toggling etc. +1. Nested feature flags, with the parent flag acting as shortcut to toggle all + its children. ## Implementation -This RFC considers feature flags for the frontend. The API can just use `beta/` instead of `v1/` to sufficiently mark unstable endpoints. +This RFC considers feature flags for the frontend. The API can just use `beta/` +instead of `v1/` to sufficiently mark unstable endpoints. ### States @@ -57,15 +72,18 @@ Each flag can have 3 statuses. - **enabled** - There is no fencing, everyone can access the feature, even if they don't like or want it. + There is no fencing, everyone can access the feature, even if they don't like + or want it. - **disabled** - The feature is not visible and accessible, and trying to access it should yield an error message. + The feature is not visible and accessible, and trying to access it should + yield an error message. - **switchable** - The feature can be turned on or off by the user. Preferences may be recorded in a cookie. + The feature can be turned on or off by the user. Preferences may be recorded + in a cookie. Based on the one of the three statuses of the flag above, and the preference of the user (if status is switchable), the feature can have two states. @@ -73,12 +91,14 @@ the user (if status is switchable), the feature can have two states. - **on** When either + - status is "enabled" - status is "switchable" and preference is "on" - **off** When either + - status is "disabled" - status is "switchable" and preference is "off" @@ -88,80 +108,106 @@ The feature flag state is determined in two steps. #### Conf files -Conf files are JSON files that define the feature flags. Here is a sample of what the schema of this file looks like: +Conf files are JSON files that define the feature flags. Here is a sample of +what the schema of this file looks like: ```json5 { - "features": { - "feature_name_1": { // feature flag name - "status": "enabled", // enabled | disabled | switchable - "description": "Feature 1 does ...", // what the feature does - "data": { // optional, any data you want to store related to this feature - "release_date": "2023-01-01" - } + features: { + feature_name_1: { + // feature flag name + status: "enabled", // enabled | disabled | switchable + description: "Feature 1 does ...", // what the feature does + data: { + // optional, any data you want to store related to this feature + release_date: "2023-01-01", + }, + }, + feature_name_2: { + status: "switchable", + default: "on", // whether the feature is opt-in or opt-out + description: "Feature 2 does ...", }, - "feature_name_2": { - "status": "switchable", - "default": "on", // whether the feature is opt-in or opt-out - "description": "Feature 2 does ...", - } - } + }, } ``` -Conf files being JSON can be read in any part of the codebase, without any additional libraries. Note that conf files only define the status of the flag and not the final state of the feature. +Conf files being JSON can be read in any part of the codebase, without any +additional libraries. Note that conf files only define the status of the flag +and not the final state of the feature. #### Cookies -For flags that are set to switchable in the conf files, we use the state in the cookie to determine whether the flag is set to on or off. This cookie has the following schema. +For flags that are set to switchable in the conf files, we use the state in the +cookie to determine whether the flag is set to on or off. This cookie has the +following schema. ```json5 { - "features": { - "feature_name_1": "on", - "feature_name_2": "off" - } + features: { + feature_name_1: "on", + feature_name_2: "off", + }, } ``` -The cookie is only referred to for flags that are set to switchable, any other feature definitions in the cookie should be dropped or ignored. If the cookie does not mention the flag, the `default` value from the config is read. +The cookie is only referred to for flags that are set to switchable, any other +feature definitions in the cookie should be dropped or ignored. If the cookie +does not mention the flag, the `default` value from the config is read. Regardless of the storage mechanism we will need a few other key things. ### Store -To prevent feature flags from being all over the place, we can keep them store in a Pinia store and just use the store wherever there is a check. This allows us to toggle features from a centralised place. +To prevent feature flags from being all over the place, we can keep them store +in a Pinia store and just use the store wherever there is a check. This allows +us to toggle features from a centralised place. -Since keys will be added and removed periodically, the store must not expose feature flags directly but rather expose a getter that can handle fallback-to-default. +Since keys will be added and removed periodically, the store must not expose +feature flags directly but rather expose a getter that can handle +fallback-to-default. The flags can be populated in the store from whatever source we choose. ### Utilities -A composable can be provided that interacts with the Pinia store to provide `isFeatureEnabled` functionality. The composable can directly be pulled into a component and used as a boolean `ref`. +A composable can be provided that interacts with the Pinia store to provide +`isFeatureEnabled` functionality. The composable can directly be pulled into a +component and used as a boolean `ref`. ### Directive -To prevent a lot of `v-if="isFeatureEnabled(feature)"` statements in the templates, we can create a new directive. +To prevent a lot of `v-if="isFeatureEnabled(feature)"` statements in the +templates, we can create a new directive. ``` v-flag:{feature_name: str}="{flag_state: str}" ``` -This directive takes the `feature_name: str` argument and the `flag_state: boolean` value. The component is only rendered when the state of the feature flag matches the given value. +This directive takes the `feature_name: str` argument and the +`flag_state: boolean` value. The component is only rendered when the state of +the feature flag matches the given value. The component is shown if the flag is - enabled - switchable (with the user having chosen to enable it) -It internally intefaces with the utility to evaluate whether the component is rendered or not. Here is [some code](https://github.com/mblarsen/vue-browser-acl/blob/4e3bb90d2ba4fcc3edd30b27737f4531dc464329/index.js#L136-L163) that can be used to avoid rendering. +It internally intefaces with the utility to evaluate whether the component is +rendered or not. Here is +[some code](https://github.com/mblarsen/vue-browser-acl/blob/4e3bb90d2ba4fcc3edd30b27737f4531dc464329/index.js#L136-L163) +that can be used to avoid rendering. ### Drawbacks - This implementation has the main drawback of leaving `v-flag` directives around the codebase that are equivalent `v-if="true"` when the feature has been permanently enabled. Similarly a script utility `isFeatureEnabled` that always evaluates to `true` is another a no-op. +This implementation has the main drawback of leaving `v-flag` directives around +the codebase that are equivalent `v-if="true"` when the feature has been +permanently enabled. Similarly a script utility `isFeatureEnabled` that always +evaluates to `true` is another a no-op. -We can periodically clean them up or create tickets to remove them after a feature is completely and irreversibly live. This is important to prevent cruft from accumulating in in the codebase. +We can periodically clean them up or create tickets to remove them after a +feature is completely and irreversibly live. This is important to prevent cruft +from accumulating in in the codebase. ## Proof-of-concept @@ -169,9 +215,15 @@ We can periodically clean them up or create tickets to remove them after a featu ## Disqualified alternatives -One alternative to feature flags is what we have been doing in that past which is working on a branch that gets merged into `main` later. This is bad for reasons listed above in [§Rationale](#rationale). +One alternative to feature flags is what we have been doing in that past which +is working on a branch that gets merged into `main` later. This is bad for +reasons listed above in [§Rationale](#rationale). -Another terrible alternative it to just work on `main` and never deploy till the feature is ready. This might work if the site didn't have any bugs or pressing issues (that need to be solved and deployed fast) or if features were getting ready super-fast (allowing us to deploy them with the fixes). Neither is true for us. +Another terrible alternative it to just work on `main` and never deploy till the +feature is ready. This might work if the site didn't have any bugs or pressing +issues (that need to be solved and deployed fast) or if features were getting +ready super-fast (allowing us to deploy them with the fixes). Neither is true +for us. ### Binned sources @@ -190,7 +242,8 @@ Just skip this section and proceed to [§Sources](#sources). 1. Effortless to set up. 1. Universal way to configure applications. 1. Low maintenance as every feature can be one env var. -1. Fairly easy to test considering env vars can be set per-session, per-command, per-container and per-machine (as needed). +1. Fairly easy to test considering env vars can be set per-session, per-command, + per-container and per-machine (as needed). 1. Managing a couple of `.env` files is not hard. 1. Server-side envs can help tree-shake the off features in prod. @@ -204,11 +257,15 @@ Just skip this section and proceed to [§Sources](#sources). ##### Considerations -Feature flag env vars can be prefixed with a string like `FF_` to separate them from the other env vars. +Feature flag env vars can be prefixed with a string like `FF_` to separate them +from the other env vars. -The limitation for the data to be a string means any extra metadata must be JSON-stringified and then parsed in the code. Only primitive flag states can be handled conveniently. +The limitation for the data to be a string means any extra metadata must be +JSON-stringified and then parsed in the code. Only primitive flag states can be +handled conveniently. -Nesting flags will use hacks like artificial separators e.g. `FF_PARENT__CHILD`. Looks ugly and confusing. +Nesting flags will use hacks like artificial separators e.g. `FF_PARENT__CHILD`. +Looks ugly and confusing.
@@ -280,23 +337,32 @@ Nesting flags will use hacks like artificial separators e.g. `FF_PARENT__CHILD`. ##### Pros 1. Even more effortless to set up. -1. We've done this before for the embedded mode, [_remember_](https://github.com/WordPress/openverse-frontend/pull/65)? +1. We've done this before for the embedded mode, + [_remember_](https://github.com/WordPress/openverse-frontend/pull/65)? 1. Testing is easy because tests can append the right flag param to the URL. ##### Cons -1. Configuration will need to live in the codebase so change of state will need deployment. +1. Configuration will need to live in the codebase so change of state will need + deployment. 1. Exposes half-baked features to users who know the flag. 1. Transient, so switchable flags must be set on every URL or stored in cookies. -1. Flags will be primitive and will not allow complex data. Even stringified JSON would be bad in URLs. +1. Flags will be primitive and will not allow complex data. Even stringified + JSON would be bad in URLs. ##### Considerations -We can clearly demarcate feature flag params with a prefix such as `ff_` (or `feature/` 🚳) to separate them from the actual query params and process them separately. +We can clearly demarcate feature flag params with a prefix such as `ff_` (or +`feature/` 🚳) to separate them from the actual query params and process them +separately. -Due to their nature being run-time rather than build-time, they are best used in pairing with another storage (as an override for the default) rather than on their own. +Due to their nature being run-time rather than build-time, they are best used in +pairing with another storage (as an override for the default) rather than on +their own. -For example, we can have a feature flag which is set to switchable via a config file. This prevents users from accessing this feature. However, a developer who still wants to use this feature can send the query param as true and access it. +For example, we can have a feature flag which is set to switchable via a config +file. This prevents users from accessing this feature. However, a developer who +still wants to use this feature can send the query param as true and access it. diff --git a/rfcs/README.md b/rfcs/README.md index ab40c86c1b1..0a66db0fccf 100644 --- a/rfcs/README.md +++ b/rfcs/README.md @@ -1,34 +1,53 @@ # RFCs -The Openverse contributors have committed to writing RFCs for large features or improvements that span multiple issues. +The Openverse contributors have committed to writing RFCs for large features or +improvements that span multiple issues. -We write RFCs in Markdown and submit PRs for them to this repository to make them easier to comment on. +We write RFCs in Markdown and submit PRs for them to this repository to make +them easier to comment on. ## Process The following is the RFC process for Openverse: 1. Write an RFC document using Markdown. -2. Open a PR into this repository to add the RFC in the `rfcs` folder. Name the file using this format: +2. Open a PR into this repository to add the RFC in the `rfcs` folder. Name the + file using this format: + ``` YYYYMMDD-.md ``` -3. Wait for feedback. Please `@` contributors you think might have specific and applicable knowledge to the problem space. + +3. Wait for feedback. Please `@` contributors you think might have specific and + applicable knowledge to the problem space. 4. Revise based on feedback. -5. Allow for a minimum of **5** days for review by contributors and the community. -6. Continue until approval from at least two core contributors has been given and there are no absolute blockers raised. -7. Create the related milestone and issues for the implementation plan. Link to the milestone in the final RFC and merge it. +5. Allow for a minimum of **5** days for review by contributors and the + community. +6. Continue until approval from at least two core contributors has been given + and there are no absolute blockers raised. +7. Create the related milestone and issues for the implementation plan. Link to + the milestone in the final RFC and merge it. ## Format -There is no concrete format for RFCs but they probably (but not necessarily) should include the following sections: - -* A list of approvers (to be filled in as approvals are given) -* A deadline for feedback (typically two weeks from the date the RFC is originally shared unless there are extenuating circumstances) -* The rationale/reason/goals for the proposed changes. Basically the high-level "why we need this". -* The existing state of things as it relates to the proposed change. Link to previous related RFCs and other prior art. -* Describe any proposed new dependencies/technology and why we need them. When appropriate, include information about alternatives that were considered. This can be as short as "standardizing on the Vue community solution for this problem". -* The list of proposed changes at a high level. -* An implementation plan. This should closely if not exactly mirror the list of issues that would be created for the implementation of the proposed changes and ideally will read like a set of high-level instructions. - -If helpful for the proposed change, open a draft PR in the relevant repositories with any exploratory code that helps support the RFC. +There is no concrete format for RFCs but they probably (but not necessarily) +should include the following sections: + +- A list of approvers (to be filled in as approvals are given) +- A deadline for feedback (typically two weeks from the date the RFC is + originally shared unless there are extenuating circumstances) +- The rationale/reason/goals for the proposed changes. Basically the high-level + "why we need this". +- The existing state of things as it relates to the proposed change. Link to + previous related RFCs and other prior art. +- Describe any proposed new dependencies/technology and why we need them. When + appropriate, include information about alternatives that were considered. This + can be as short as "standardizing on the Vue community solution for this + problem". +- The list of proposed changes at a high level. +- An implementation plan. This should closely if not exactly mirror the list of + issues that would be created for the implementation of the proposed changes + and ideally will read like a set of high-level instructions. + +If helpful for the proposed change, open a draft PR in the relevant repositories +with any exploratory code that helps support the RFC.