feat(core): read-only Matomo client + offline mock (groundwork for #11956) - #13252
Draft
mekarpeles wants to merge 2 commits into
Draft
feat(core): read-only Matomo client + offline mock (groundwork for #11956)#13252mekarpeles wants to merge 2 commits into
mekarpeles wants to merge 2 commits into
Conversation
…fline Groundwork for the Core Vitals Retention Score (#11956). The scorer follows in a second PR; this is only the access layer, split out so that the part with the security surface and the external dependency can be reviewed on its own terms rather than alongside a scoring formula whose weights and semantics are still being settled. `openlibrary/core/matomo.py` fetches visit detail from Matomo and nothing else: - POSTs rather than GETs, so the auth token stays in the request body instead of the query string where proxies and access logs would capture it. - `_ALLOWED_METHODS` is an exact-match allowlist, not a prefix one. A prefix list is bypassable -- `API.getBulkRequest` sits under `API.` and tunnels arbitrary methods via its `urls[]` parameter. Caller params are also applied *before* the fixed keys, so nothing can redirect the request at another module, site, or token. Treat both as guardrails against mistakes rather than a security boundary: the credential is the only real control, so `matomo_api` should hold a view-only Matomo user. - Pagination stops on an empty page, not a short one. Matomo enforces server-side row caps, so a request for 500 rows can legitimately return fewer, and reading that as the end of the feed silently truncates results. Offsets follow what was actually received, and visits are deduped on `idVisit` because the feed is live and newest-first, so pages overlap as new visits arrive. - `budget_seconds` is a real wall clock. `timeout` is per socket read and bounds no overall duration, so "200 pages x 30s" was never the limit it looked like. Truncation for any reason sets `truncated`, which callers are expected to surface -- a truncated window must not be mistaken for a quiet one. - The date range is derived from `since` rather than a fixed `last7`, which clamped longer windows while still reporting the window asked for. It is padded a day either side because Matomo interprets `date` in the *site's* timezone while `since` is UTC: an exact UTC range returned zero visits for the 06:00 hour against the real instance. `minTimestamp` still does the precise filtering. - One `requests.Session` with a short retry, rather than a fresh TLS handshake per page against shared IA infrastructure. Imports nothing from Open Library, since this is intended to move into a standalone Core Vitals service later; that should be a file copy. `matomo.archive.org` is IP-restricted to IA's network, which made every layer above this untestable without a VPN. `docker/mockservices` now serves `Live.getLastVisitsDetails` in the real wire format -- `dimension1` flat on the visit, and never the nested `customDimensions` that reading it wrongly implies -- from a deterministic 12-visit feed that honours paging. `test_matomo_inprocess.py` runs that same mock app in-process on a loopback port, so the real client makes real HTTP requests on every CI run. The container-gated tests next door skip when `mockservices` is not up, and CI runs `make test-py` with no containers at all, so they would otherwise have covered nothing there.
The comment named openlibrary/admin/vitals.py, which does not consume it -- the reader is openlibrary/core/matomo.py. A stale pointer like that is what sent someone looking in the wrong place last time. Also states the view-only expectation, since the allowlist guards against mistakes rather than against a token that can write.
48 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Groundwork for the Core Vitals Retention Score (#11956). The scorer follows in a stacked PR; this is only the Matomo access layer.
Split out deliberately so the part with the security surface and the external dependency can be reviewed on its own terms, rather than alongside a scoring formula whose weights and counting semantics are still being settled with @mekarpeles. Every open question on that discussion lives in the scorer; none live here.
What's here
openlibrary/core/matomo.py— a read-only client that fetches visit detail and nothing else.docker/mockservices, becausematomo.archive.orgis IP-restricted to IA's network and everything above this was otherwise untestable without a VPN.matomo_apiinconf/openlibrary.yml(blank here; the real value belongs in olsystem).Read-only, and honest about how much that's worth
_ALLOWED_METHODSis an exact-match allowlist, not a prefix one. A prefix list is bypassable —API.getBulkRequestsits underAPI.and tunnels arbitrary methods through itsurls[]parameter. Caller params are also applied before the fixed keys, so nothing can redirect a request at anothermodule,idSite, ortoken_auth.Both are guardrails against mistakes, not a security boundary. The credential is the only real control, so
matomo_apishould hold a view-only Matomo user — flagging that as the one deploy-side ask.The token goes in the POST body, never a query string, so it stays out of access and proxy logs. A test asserts it never appears in a URL or in an exception message.
Things that would otherwise have produced quietly-wrong numbers
idVisit. The feed is live and newest-first, so pages overlap as new visits arrive. Measured: two duplicated visits inflated a downstream total by 40%.budget_secondsis a real wall clock.timeoutis per socket read and bounds no overall duration, so "200 pages × 30s" was never the limit it appeared to be. Truncation for any reason setstruncated, which callers surface — a truncated window must not read as a quiet one.since, not a fixedlast7which clamped longer windows while still reporting the window asked for. It's padded a day either side because Matomo interpretsdatein the site's timezone whilesinceis UTC — an exact UTC range returned zero visits for the 06:00 hour against the real instance.minTimestampstill does the precise filtering.requests.Sessionwith a short retry, rather than a fresh TLS handshake per page against shared IA infrastructure.Testing
docker/mockservicesservesLive.getLastVisitsDetailsin the real wire format —dimension1flat on the visit, and never the nestedcustomDimensionsthat reading it wrongly implies — from a deterministic 12-visit feed that honours paging.test_matomo_inprocess.pyruns that same mock app in-process on a loopback port, so the real client makes real HTTP requests on every CI run. This matters: the container-gated tests next door skip whenmockservicesisn't up, and CI runsmake test-pywith no containers at all, so they cover nothing there. Verified with all services down — 14 pass, container-gated ones skip.Also verified live against production Matomo through an allowlisted proxy.
Notes for review
MAX_WINDOW/budget_seconds/truncatedexist to serve the scorer.