-
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
base: master
Are you sure you want to change the base?
Changes from 16 commits
07e2bdb
2ddbfd6
91cd95b
4e0f2a5
f02d2d6
a017025
34af3cb
7b9964f
d7ded39
c449a44
771bb1b
da3bfa4
de56e12
dbd3072
c5644d3
0a14b05
90790d3
672f1e4
43c6165
c3fc9ae
004ee06
9303a22
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,144 @@ | ||||||||||
| Newest-Courses-First Search Sort via a Recency-Sorted Algolia Replica | ||||||||||
| ===================================================================== | ||||||||||
|
|
||||||||||
| Status | ||||||||||
| ------ | ||||||||||
|
|
||||||||||
| Proposed | ||||||||||
|
|
||||||||||
| Context | ||||||||||
| ------- | ||||||||||
|
macdiesel marked this conversation as resolved.
|
||||||||||
|
|
||||||||||
| 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. We | ||||||||||
| already maintain one such replica — the base ``ALGOLIA['REPLICA_INDEX_NAME']``, | ||||||||||
| which the Learner Portal points its video search at. | ||||||||||
|
|
||||||||||
| Two facts about this machinery constrain how any new replica can be rolled out: | ||||||||||
|
|
||||||||||
| * ``ALGOLIA`` is *replaced* (not merged) from the deployment YAML, so a replica | ||||||||||
| is only live once ops sets its index-name key in ``edx-internal`` and a | ||||||||||
| ``reindex_algolia`` run declares it on the primary. | ||||||||||
|
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. AI docs do this a lot where they explain low-level details, rather than what actually matters and what the end goals are. Very few people will read this in the first place, let alone understand it and have the correct takeaway, even if it is factually true. My version is simpler:
Suggested change
|
||||||||||
| * An Algolia *virtual* replica exists as soon as it is declared on the primary | ||||||||||
| index's settings (it mirrors the primary's records); it does not wait for a | ||||||||||
| populated record set. So once this service is deployed and ``reindex_algolia`` | ||||||||||
| has run, the replica exists. | ||||||||||
|
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. Selecting virtual replicas would be an Alternative Considered, not part of the Context section. Please move this below. Also I don't think I agree with this as a foundation for rejecting virtual replicas. Virtual replicas simply don't support fully customizable sort orders, since there's always a built-in "relevance" factor as the top priority sort order. We'd use virtual replicas to save money and record usage, but the tradeoff is less precise and non-deterministic sorting. |
||||||||||
|
|
||||||||||
| We want to offer learners a "newest courses first" sort. The problem is how to | ||||||||||
| add that sort — 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 (``ALGOLIA['RECENTLY_RELEASED_REPLICA_INDEX_NAME']``) | ||||||||||
| 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 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 replica is **config-gated on both sides** and is never queried unless its | ||||||||||
| name is configured: | ||||||||||
|
|
||||||||||
| * Backend: **all** sort replicas are driven by one registry | ||||||||||
| (``ALGOLIA_REPLICA_CONFIG_KEYS`` — the base duration replica first, then additive | ||||||||||
| sorts like recency). Each is declared on the primary index, configured, and | ||||||||||
| added to the secured-key ``restrictIndices`` **only** when its ``settings.ALGOLIA`` | ||||||||||
| index-name key is set; an unconfigured replica is skipped entirely (no | ||||||||||
| ``virtual(None)`` replica is created), so the code is inert until ops provides a | ||||||||||
| name. This is the single source of truth for which replicas exist — adding a | ||||||||||
| future sort is a registry entry plus the field its ``customRanking`` sorts on, | ||||||||||
| not a change to the gating logic. (The client's ``init_index`` / | ||||||||||
| ``index_exists`` still eagerly manage the primary + base-replica *handles* — that | ||||||||||
| is the required-core-pair lifecycle, a separate concern from which replicas get | ||||||||||
| declared/configured.) | ||||||||||
| * 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. |
||||||||||
|
|
||||||||||
| * **Covered:** "replica name not configured" → the base (relevance) index is | ||||||||||
| used. This is handled explicitly in both the backend (conditional replica | ||||||||||
| declaration) and the MFE (the ``&& recentlyReleasedIndexName`` guard), so the | ||||||||||
| default-to-base behavior is guaranteed for the unconfigured case. | ||||||||||
| * **Not covered in code:** "replica name configured but the Algolia index does | ||||||||||
| not exist yet" → the MFE would query a missing index and surface an error / | ||||||||||
| empty results rather than falling back. This is a transient, | ||||||||||
| operator-controlled window mitigated by the rollout order and the kill-switch | ||||||||||
| flag, not by code. | ||||||||||
| * **Reindex degrades gracefully:** if configuring a replica fails, the error is | ||||||||||
| logged and the reindex continues with the primary index and the other replicas | ||||||||||
| intact, so a problem with one sort can never take down core search indexing. | ||||||||||
| The worst case is that one replica lags its ranking until the next run — never a | ||||||||||
| broken or empty primary index. | ||||||||||
|
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. Nit: all the wording on this one is just weird. If I'm reading it correctly, it's just saying that we make a best effort to configure a replica, and any errors on configuration won't crash initialization. That's an implementation detail, not a consequence. Please drop this bullet too.
Member
Author
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. Agreed — dropped in 9303a22. |
||||||||||
| * **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. | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,167 @@ | ||
| 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. | ||
|
|
||
| All replicas are driven by **one registry**, 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: names vs. definitions | ||
| --------------------------------------- | ||
|
|
||
| There are two layers, and the split matters: | ||
|
|
||
| * **Index *names* live in Django settings** (``settings.ALGOLIA``), populated per-environment | ||
| from the deployment config (edx-internal). These vary by environment and are owned by ops. | ||
| * **Replica *definitions* live in code** — which replicas exist (the registry) and how each is | ||
| ranked (``customRanking``). A replica's ranking sorts on a field that the indexing code must | ||
| compute, so the definition is intrinsically code, not config. | ||
|
|
||
| A replica is declared, configured, and made queryable **only when its index-name key holds a | ||
| non-empty value**. With the name unset (the default everywhere until ops sets it), the new | ||
| replica is completely inert — nothing is declared, no ``virtual(None)`` is created, and the | ||
| secured API key does not reference it. This is what makes it safe to merge and deploy the code | ||
| *before* ops turns it on. | ||
|
|
||
| Worked example | ||
| -------------- | ||
|
|
||
| Suppose we want a **"price: low to high"** sort. We'll use the settings key | ||
| ``PRICE_ASC_REPLICA_INDEX_NAME`` and (in a given environment) an index named | ||
| ``enterprise_catalog_price_asc``. | ||
|
|
||
| Backend steps (this service) | ||
| ---------------------------- | ||
|
|
||
| **1. Declare the settings key.** In ``enterprise_catalog/settings/base.py``, add the key to the | ||
| ``ALGOLIA`` dict with an empty default and a one-line comment describing the sort: | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| ALGOLIA = { | ||
| 'INDEX_NAME': '', | ||
| 'REPLICA_INDEX_NAME': '', # base replica, desc(duration); MFE video search | ||
| 'RECENTLY_RELEASED_REPLICA_INDEX_NAME': '', # "newest first", desc(recently_released_timestamp) | ||
| # "price: low to high", asc(first_enrollable_paid_seat_price) | ||
| 'PRICE_ASC_REPLICA_INDEX_NAME': '', | ||
| 'APPLICATION_ID': '', | ||
| 'API_KEY': '', | ||
| } | ||
|
|
||
| The empty default keeps it inert until ops provides a real index name. | ||
|
|
||
| **2. Register the key.** Add it to ``ALGOLIA_REPLICA_CONFIG_KEYS`` in | ||
| ``enterprise_catalog/apps/api_client/constants.py`` — the single source of truth for *which* | ||
| replicas exist, shared by the indexer and the secured-key restriction. (It lives here, not in | ||
| ``settings``/``algolia_utils``, so both ``api_client.algolia`` and ``catalog.algolia_utils`` can | ||
| import it without a circular dependency.) | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| ALGOLIA_REPLICA_CONFIG_KEYS = ( | ||
| 'REPLICA_INDEX_NAME', | ||
| 'RECENTLY_RELEASED_REPLICA_INDEX_NAME', | ||
| 'PRICE_ASC_REPLICA_INDEX_NAME', | ||
| ) | ||
|
|
||
| **3. 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). | ||
|
|
||
| **4. Define the replica's ranking.** Still in ``algolia_utils.py``, add a settings dict. Lead 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)', | ||
| ], | ||
| } | ||
|
|
||
| **5. Map the key to its settings.** Add an entry to | ||
| ``ALGOLIA_REPLICA_INDEX_SETTINGS_BY_CONFIG_KEY`` in ``algolia_utils.py``: | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| ALGOLIA_REPLICA_INDEX_SETTINGS_BY_CONFIG_KEY = { | ||
| 'REPLICA_INDEX_NAME': ALGOLIA_REPLICA_INDEX_SETTINGS, | ||
| 'RECENTLY_RELEASED_REPLICA_INDEX_NAME': ALGOLIA_RECENTLY_RELEASED_REPLICA_INDEX_SETTINGS, | ||
| 'PRICE_ASC_REPLICA_INDEX_NAME': ALGOLIA_PRICE_ASC_REPLICA_INDEX_SETTINGS, | ||
| } | ||
|
|
||
| **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 loop the registry, 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`` once its name is configured. | ||
|
|
||
| Tests | ||
| ----- | ||
|
|
||
| * Extend the registry / configure tests in | ||
| ``enterprise_catalog/apps/catalog/tests/test_algolia_utils.py`` (e.g. | ||
| ``test_get_algolia_replica_names_only_includes_configured_replicas`` and | ||
| ``test_configure_algolia_index_configures_*``) to cover the new key, 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`` already loop the registry; add | ||
| the new index name to the "all indices" expectation if you want explicit coverage. | ||
|
|
||
| Deploy / ops | ||
| ------------ | ||
|
|
||
| Once the code is merged and deployed: | ||
|
|
||
| #. Ops sets ``PRICE_ASC_REPLICA_INDEX_NAME`` (to e.g. ``enterprise_catalog_price_asc``) in the | ||
| ``ALGOLIA`` config in edx-internal. **The whole** ``ALGOLIA`` **dict is replaced, not merged**, | ||
| so the entire dict must be restated with the new key included. | ||
| #. 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`` (mirror of the backend settings key); | ||
| * point the relevant ``<Index indexName=...>`` at it (see ``SearchVideo.jsx``, which uses the | ||
| base replica, or ``SearchCourse.jsx`` for the recency replica); | ||
| * if the sort changes default behavior, 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 you get for free | ||
| ---------------------------------- | ||
|
|
||
| * **Inert by default.** No configured name → the replica is never declared, configured, or | ||
| queryable. Merge and deploy ahead of ops with no effect. | ||
| * **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. | ||
| * **No** ``virtual(None)``. Unconfigured keys are skipped entirely, never declared as a broken | ||
| ``virtual(None)`` replica on the primary index. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to "Accepted" in 9303a22 to match the convention in 0012/0013 (this PR implements the decision).