-
Notifications
You must be signed in to change notification settings - Fork 1
feat: "newest courses first" Algolia sort — config-gated replica registry + fail-safe #118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
macdiesel
wants to merge
22
commits into
master
Choose a base branch
from
bbeggs/newest-courses-sort-replica
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
07e2bdb
feat: add recently-published Algolia replica for "newest courses firs…
macdiesel 2ddbfd6
docs: convert newest-sort reference doc to ADR 0014
macdiesel 91cd95b
test: cover recency replica build, secured-key restrictIndices, and c…
macdiesel 4e0f2a5
style: silence pylint protected-access in new algolia replica tests
macdiesel f02d2d6
refactor: unify set_index_settings to target any index by name
macdiesel a017025
fix: update incremental_reindex_algolia for unified set_index_settings
macdiesel 34af3cb
refactor: drive optional Algolia sort replicas from a config registry
macdiesel 7b9964f
refactor: fold the base replica into the unified replica registry
macdiesel d7ded39
docs: clarify what the base REPLICA_INDEX_NAME replica does
macdiesel c449a44
feat: fail safe when configuring sort replicas
macdiesel 771bb1b
docs: how-to guide for adding a new Algolia sort replica
macdiesel da3bfa4
refactor: rename recency sort to "recently released" (earliest run st…
macdiesel de56e12
refactor: address review feedback on recency-sort replica
macdiesel dbd3072
Merge branch 'master' into bbeggs/newest-courses-sort-replica
macdiesel c5644d3
refactor: address Troy's review comments on recency-sort replica
macdiesel 0a14b05
fix: report the effective Algolia index name on set_index_settings fa…
macdiesel 90790d3
fix: harden Algolia index configuration against registry drift and un…
macdiesel 672f1e4
docs: refine ADR 0014 per review (virtual-replica tradeoff, cost, con…
macdiesel 43c6165
refactor: settings-driven Algolia replica registry (ADDITIONAL_REPLIC…
macdiesel c3fc9ae
Merge branch 'master' into bbeggs/newest-courses-sort-replica
macdiesel 004ee06
style: fix E302 (two blank lines before _configured_replicas)
macdiesel 9303a22
refactor: address PR #118 review feedback (VIRTUAL rename, ADR + doc …
macdiesel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,157 @@ | ||
| Newest-Courses-First Search Sort via a Recency-Sorted Algolia Replica | ||
| ===================================================================== | ||
|
|
||
| Status | ||
| ------ | ||
|
|
||
| Accepted | ||
|
|
||
| Context | ||
| ------- | ||
|
|
||
| The enterprise Learner Portal search page sorts a single Algolia index by | ||
| relevance. Algolia does not re-sort an index at query time, so each alternate | ||
| sort order is a separate *replica* index with its own ``customRanking``; the | ||
| consumer switches sort by pointing its search at a different index name. | ||
|
|
||
| The fundamental design of the enterprise-catalog logic assumes we will only ever | ||
| provision *one* replica: there is a single setting | ||
| (``ALGOLIA['REPLICA_INDEX_NAME']`` — the base "duration" replica the Learner | ||
| Portal points its video search at) and a single Python function that provisions | ||
| that one replica. | ||
|
|
||
| Historically the whole ``ALGOLIA`` dict has been *replaced* (not merged) from the | ||
| deployment YAML, so anything code wants to keep in it — like a replica's ranking | ||
| definition — would be lost unless ops restated it in ``edx-internal``. | ||
|
|
||
| We want to offer learners a "newest courses first" sort. The problem is how to | ||
| add a *second* replica — and roll it out across the repositories that together | ||
| own enterprise search — without a partially-configured replica degrading or | ||
| breaking the existing relevance search. | ||
|
|
||
| Decision | ||
| -------- | ||
|
|
||
| Add a recency-sorted replica (the ``enterprise_catalog_recently_released_desc`` | ||
| entry in ``ALGOLIA['ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS']``) that leads its ranking | ||
| with ``desc(recently_released_timestamp)`` — a per-course | ||
| Unix timestamp of the *earliest course-run start of any status* (the Discovery | ||
| course release date, the same signal as the ``is_new_content`` flag, via the | ||
| shared ``_earliest_course_run_start`` helper — ENT-11386). Courses with no run | ||
| start get ``0`` so they sort last under a descending ranking — deliberately not | ||
| the far-future ``ALGOLIA_DEFAULT_TIMESTAMP``, which would float undated courses to | ||
| the top. The primary index (``ALGOLIA['INDEX_NAME']``) keeps the relevance | ||
| ranking. | ||
|
|
||
| The replica is declared as an Algolia *virtual* replica (``virtual(name)``), so it | ||
| mirrors the primary's records instead of duplicating them. This is a deliberate | ||
| cost/precision tradeoff versus a standard replica — see *Alternatives considered* | ||
| and *Consequences*. | ||
|
|
||
| The sort is rolled out across three repositories: | ||
|
|
||
| #. **enterprise-catalog** (this service) builds and configures the replica. | ||
| #. **edx-enterprise** exposes the ``enterprise.search_default_sort_newest`` | ||
| waffle flag via ``enterprise_features`` — the eligibility gate / kill-switch. | ||
| #. **frontend-app-learner-portal-enterprise** points the course ``<Index>`` at | ||
| the replica when the flag is on *and* the Optimizely "newest" experiment | ||
| variant is active for the user. | ||
|
|
||
| The design generalizes the old one-replica assumption to a settings-driven map, and | ||
| gates *user exposure* with the waffle flag rather than gating *declaration* on ops: | ||
|
|
||
| * Backend: additional sort replicas are declared in | ||
| ``ALGOLIA['ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS']`` — an ``index_name -> index settings`` | ||
| map defined in ``settings/base.py`` as config-as-code (the ``customRanking`` is code, | ||
| since it sorts on a field the indexer computes). ``ALGOLIA`` is added to | ||
| ``DICT_UPDATE_KEYS`` so the deployment YAML is now *merged*, not replaced: ops can | ||
| override per-environment index names and credentials while these code-defined replica | ||
| settings are preserved. One map is the single source of truth — its entries are | ||
| declared on the primary index, configured during a reindex, and added to the secured-key | ||
| ``restrictIndices`` — so adding a future sort is one new entry plus the field its | ||
| ``customRanking`` sorts on. (The base ``REPLICA_INDEX_NAME`` keeps its own | ||
| per-environment key and its required-core-pair lifecycle in ``init_index`` / | ||
| ``index_exists``.) | ||
| * Backend (fail-safe): configuring each replica in ``configure_algolia_index`` is | ||
| wrapped so that any ``AlgoliaException`` is logged and skipped. One replica | ||
| failing to configure never aborts the reindex — the primary (relevance) index | ||
| and the other replicas are still configured. The failed replica keeps its prior | ||
| settings (or, when brand new, mirrors the primary's relevance ranking) until the | ||
| next successful run, so the degraded state is still the safe base sort. | ||
| * MFE: the course search uses the replica only when its index-name config var is | ||
| non-empty (and the flag + experiment gates pass); otherwise it falls back to | ||
| the primary (relevance) index. | ||
|
|
||
| **The open question this ADR records:** how should we handle the case where the | ||
| replica *name is configured* but the Algolia index does **not yet exist** (e.g. | ||
| the MFE env var is set before this service has been deployed and reindexed)? | ||
|
|
||
| The proposed answer is to **rely on operational guarantees rather than runtime | ||
| index-existence detection**: | ||
|
|
||
| #. the documented rollout order — deploy + ``reindex_algolia`` here *before* | ||
| pointing the MFE at the replica; and | ||
| #. the ``enterprise.search_default_sort_newest`` waffle flag, which doubles as a | ||
| readiness gate and an instant kill-switch — it should not be enabled until the | ||
| replica is live, and flipping it off immediately reverts every learner to the | ||
| relevance index. | ||
|
|
||
| We deliberately do **not** add code that detects a missing Algolia index at | ||
| search time and silently falls back to the primary index. | ||
|
|
||
| Consequences | ||
| ------------ | ||
|
Comment on lines
+102
to
+103
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is one missing consequence: cost. Another full replica (non-virtual) doubles our record count. |
||
|
|
||
| * **Exposure is MFE/flag-gated, not declaration-gated:** the backend now declares the | ||
| replica in every environment (it ships in ``ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS``), so | ||
| "is it declared" is no longer the gate. The course search falls back to the primary | ||
| (relevance) index whenever the MFE's replica env var is unset or the flag/experiment is | ||
| off (the ``&& recentlyReleasedIndexName`` guard), so user exposure is controlled by the | ||
| MFE env var + waffle flag. Because the replica is *virtual* (no extra records), always | ||
| declaring it costs nothing. | ||
| * **No added record cost:** because the replica is *virtual*, it mirrors the | ||
| primary's records rather than duplicating them, so adding it does not grow our | ||
| Algolia record count. A standard (non-virtual) replica would roughly double the | ||
| indexed record count — and its cost — for each sort we add (see *Alternatives*). | ||
| * **Non-course records sort last, by design:** the replica is *virtual* over the | ||
| primary index, so it mirrors every record — programs, executive education, videos, | ||
| etc. — not just courses. ``recently_released_timestamp`` is only computed in the | ||
| ``content_type == COURSE`` branch, so non-course records have no such attribute and | ||
| Algolia ranks them last under ``desc(recently_released_timestamp)``. This is fine | ||
| because the consumer (the Learner Portal) points only its course ``<Index>`` at the | ||
| replica and filters by content type; the "newest courses first" sort is, by | ||
| contract, a course sort. Were a future caller to query this replica for non-course | ||
| content, those records would all tie at the bottom — that caller would need its own | ||
| recency field. | ||
| * **The waffle flag is the readiness contract:** enabling it asserts "the replica | ||
| is live." This keeps the safe path a single, instantly reversible toggle | ||
| rather than per-request defensive logic in the search hot path. | ||
| * **Escape hatch is recorded:** if the operational mitigation proves too fragile | ||
| in practice, the documented next step is an ``onError``/try-primary fallback in | ||
| the MFE search path (see *Alternatives*). Capturing the question here lets us | ||
| revisit it without re-discovering the trade-off. | ||
|
|
||
| Alternatives considered | ||
| ------------------------ | ||
|
|
||
| * **Runtime index-existence detection / fall back to base on Algolia error.** | ||
| Rejected for now: react-instantsearch would need an error path that re-renders | ||
| against the primary index, adding per-search complexity and an extra failure | ||
| mode, to protect a transient window the kill-switch flag already guards. It | ||
| remains the documented escape hatch if the operational approach proves | ||
| insufficient. | ||
| * **Always declare the replica (no config gate).** Rejected: would create a | ||
| ``virtual(None)`` replica on the primary index in environments where the name | ||
| is unset, and would couple every environment to the rollout. | ||
| * **A standard (non-virtual) replica instead of a virtual one.** A standard | ||
| replica is a full, independent copy of the index that sorts strictly by its own | ||
| ``ranking``/``customRanking`` — a fully deterministic newest-first order | ||
| regardless of the search query. We chose a *virtual* replica instead: a virtual | ||
| replica reuses the primary's records, so it adds no record count or cost (see | ||
| *Consequences*), whereas a standard replica roughly doubles our indexed record | ||
| count and its associated cost. The accepted tradeoff is that a virtual replica | ||
| always keeps textual relevance as the top-priority sort factor, so under a text | ||
| query "newest first" is relevance-biased rather than strictly chronological (it | ||
| *is* strictly chronological when browsing with no query). If a strictly | ||
| deterministic order under query later proves necessary, switching this one | ||
| replica to a standard replica is the documented escape hatch. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,144 @@ | ||
| Add a new Algolia sort replica | ||
| ============================== | ||
|
|
||
| The Learner Portal search page sorts a single Algolia index by relevance. Algolia does | ||
| not re-sort an index at query time, so every alternate sort order is a separate *replica* | ||
| index with its own ``customRanking``; the consumer (the MFE) switches sort by pointing its | ||
| search at a different index name. | ||
|
|
||
| Beyond the base replica (``ALGOLIA['REPLICA_INDEX_NAME']``, used by the MFE video search), | ||
| every additional sort replica is declared in **one place** -- the | ||
| ``ALGOLIA['ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS']`` map -- so adding a sort is mostly additive. | ||
| This guide walks through it end to end. See | ||
| ``docs/decisions/0014-newest-courses-sort-replica.rst`` for the design rationale, and treat the | ||
| **recently-released ("newest first") replica** as the canonical example to copy. | ||
|
|
||
| The mental model: one settings-driven map | ||
| ------------------------------------------ | ||
|
|
||
| ``ALGOLIA['ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS']`` maps an **index name** to that replica's | ||
| **Algolia index settings** (its ``customRanking``). It is defined in ``settings/base.py`` as | ||
| config-as-code: a replica's ranking sorts on a field the indexing code must compute, so the | ||
| definition is intrinsically code, not deployment config. | ||
|
|
||
| ``ALGOLIA`` is *merged* (not replaced) from the deployment config -- it is listed in | ||
| ``DICT_UPDATE_KEYS`` (``settings/production.py``) -- so an environment can override the per-env | ||
| index names / credentials while the code-defined ``ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS`` defaults are | ||
| preserved. | ||
|
|
||
| The backend declares and configures every replica in this map on each reindex, and the secured API | ||
| key grants access to them. A sort is only *user-visible* once the MFE points a search at it, gated | ||
| by a waffle flag / Optimizely experiment -- so the flag, not the map, controls exposure (see | ||
| *Frontend* below and ADR 0014). | ||
|
|
||
| Worked example | ||
| -------------- | ||
|
|
||
| Suppose we want a **"price: low to high"** sort, using an index named | ||
| ``enterprise_catalog_price_asc``. | ||
|
|
||
| Backend steps (this service) | ||
| ---------------------------- | ||
|
|
||
| **1. Make sure the field you sort on is indexed.** A ``customRanking`` can only sort on a numeric | ||
| attribute that exists on the records. If your sort uses a field that is *already* indexed (the | ||
| price example reuses ``first_enrollable_paid_seat_price``), skip this step. If it needs a new | ||
| signal, in ``enterprise_catalog/apps/catalog/algolia_utils.py``: | ||
|
|
||
| * write a ``get_course_<signal>(course)`` helper that returns the numeric value (see | ||
| ``get_course_recently_released_timestamp`` -- note it returns ``0`` for "no value" so those | ||
| records sort last under a ``desc`` ranking; pick a sentinel that sorts your missing values to the | ||
| *end* of *your* order); | ||
| * add the field name to ``ALGOLIA_FIELDS``; | ||
| * set it on the course object in ``_algolia_object_from_product`` (the ``content_type == COURSE`` | ||
| branch). | ||
|
|
||
| **2. Define the replica's ranking and register it.** In ``enterprise_catalog/settings/base.py``, | ||
| add a settings constant and an entry in ``ALGOLIA['ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS']`` keyed by | ||
| the index name. Lead the ``customRanking`` with your sort criterion, then append the primary | ||
| index's shared tie-breakers so records that tie on your criterion (and any "missing value" bucket) | ||
| fall back to the relevance ordering and pagination stays deterministic: | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| ALGOLIA_PRICE_ASC_REPLICA_INDEX_SETTINGS = { | ||
| 'customRanking': [ | ||
| 'asc(first_enrollable_paid_seat_price)', | ||
| # shared tie-breakers (same as the primary index) -- keep ties stable | ||
| 'asc(metadata_language)', | ||
| 'asc(visible_via_association)', | ||
| 'asc(created)', | ||
| 'desc(course_bayesian_average)', | ||
| 'desc(recent_enrollment_count)', | ||
| ], | ||
| } | ||
|
|
||
| ALGOLIA = { | ||
| 'INDEX_NAME': '', | ||
| 'REPLICA_INDEX_NAME': '', | ||
| 'ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS': { | ||
| 'enterprise_catalog_recently_released_desc': ALGOLIA_RECENTLY_RELEASED_REPLICA_INDEX_SETTINGS, | ||
| 'enterprise_catalog_price_asc': ALGOLIA_PRICE_ASC_REPLICA_INDEX_SETTINGS, | ||
| }, | ||
| 'APPLICATION_ID': '', | ||
| 'API_KEY': '', | ||
| } | ||
|
|
||
| **That is all the wiring.** You do **not** touch ``_get_algolia_replica_names``, | ||
| ``_configured_replicas``, ``configure_algolia_index``, or the secured-key ``replica_index_names`` | ||
| -- they all read ``ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS``, so the new replica is automatically declared | ||
| on the primary index, has its settings applied during a reindex, and is added to the secured API | ||
| key's ``restrictIndices``. | ||
|
|
||
| Tests | ||
| ----- | ||
|
|
||
| * Extend the configure / registry tests in | ||
| ``enterprise_catalog/apps/catalog/tests/test_algolia_utils.py`` (e.g. | ||
| ``test_get_algolia_replica_names_combines_base_and_additional_replicas`` and | ||
| ``test_configure_algolia_index_configures_additional_replica``) to cover the new replica, using | ||
| ``override_settings(ALGOLIA={...})``. | ||
| * If you added a field computation, unit-test the ``get_course_<signal>`` helper. | ||
| * The secured-key tests in ``api_client/tests/test_algolia.py`` exercise ``restrictIndices``; add | ||
| the new index name to the "all indices" expectation if you want explicit coverage. | ||
|
|
||
| Deploy / ops | ||
| ------------ | ||
|
|
||
| Once the code is merged and deployed: | ||
|
|
||
| #. The replica's name and settings ship in code (``ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS``), so no | ||
| edx-internal change is required to *declare* it -- and normally ops should not override it at all. | ||
| The ``ALGOLIA`` merge is *shallow*: the deployment YAML's top-level ``ALGOLIA`` keys override the | ||
| code defaults, but nested dicts are **not** deep-merged. So setting | ||
| ``ALGOLIA['ADDITIONAL_VIRTUAL_REPLICA_INDEX_SETTINGS']`` in edx-internal **replaces the entire map** | ||
| for that environment (dropping every code-defined replica it does not restate) rather than | ||
| overriding individual entries. Prefer setting the index name in code; override the map only when | ||
| you intend to fully restate it. | ||
| #. Run ``./manage.py reindex_algolia``. A *virtual* replica exists as soon as it is declared on the | ||
| primary index's settings (it mirrors the primary's records), so the replica is live after one | ||
| reindex -- no separate population step. | ||
|
|
||
| Frontend (only if the MFE will use the sort) | ||
| -------------------------------------------- | ||
|
|
||
| The backend builds the replica regardless; a sort is only *user-visible* once the MFE points a | ||
| search at it. In ``frontend-app-learner-portal-enterprise``: | ||
|
|
||
| * add an ``ALGOLIA_<NAME>_REPLICA_INDEX_NAME`` env var in ``src/index.tsx`` and | ||
| ``src/types/types.d.ts`` whose value matches the backend index name; | ||
| * point the relevant ``<Index indexName=...>`` at it (see ``SearchVideo.jsx``, which uses the base | ||
| replica, or ``SearchCourse.jsx`` for the recency replica); | ||
| * gate it behind a waffle flag and/or an Optimizely experiment, exactly as the recency sort does | ||
| (the flag doubles as a kill-switch -- see ADR 0014). | ||
|
|
||
| Safety properties | ||
| ----------------- | ||
|
|
||
| * **Virtual replicas, no extra records.** Each replica is declared ``virtual(name)``, so it mirrors | ||
| the primary's records rather than duplicating them -- no added Algolia record count or cost. | ||
| * **Fail-safe configuration.** ``configure_algolia_index`` wraps each replica's settings call so an | ||
| ``AlgoliaException`` is logged and skipped -- one replica failing to configure never aborts the | ||
| reindex, and the primary index plus the other replicas stay configured. | ||
| * **Flag-gated exposure.** Declaring a replica does not make it user-visible; the MFE only queries | ||
| it when its waffle flag / experiment is on, so the flag is the kill-switch. |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.