Repository navigation
Record API requests in Matomo - #29
matthewberry wants to merge 2 commits into
Conversation
Each request is sent to Matomo as a page view after the response, so API usage shows up next to the sites' visit numbers. Disabled unless MATOMO_SITE_ID is set; MATOMO_TOKEN lets Matomo record the caller's IP.
matthewberry
left a comment
There was a problem hiding this comment.
I found no crash on the normal path. Details are inline. Two findings were checked with the repo's installed packages (pydantic-settings 2.9.1, httpx 0.28.1): the empty MATOMO_SITE_ID startup failure is reproduced, and the InvalidURL one is half-reproduced (the exception class is confirmed, the end-to-end behavior isn't). The rest come from reading the code.
The most important ones are the startup failure (config.py:21) and the test suite possibly sending hits to production Matomo (test_matomo.py:14). The others are about load and privacy.
Note that cid/cip depend on #28: before it, every API caller would have shown up as Traefik's IP. Now that it's merged, they reflect real clients, which is why the cid comment matters.
|
|
||
| # Matomo usage tracking; disabled unless MATOMO_SITE_ID is set | ||
| MATOMO_URL: str = "https://matomo.mmli2.ncsa.illinois.edu" | ||
| MATOMO_SITE_ID: Optional[int] = None |
There was a problem hiding this comment.
An empty value stops the app from starting. Reproduced with pydantic-settings 2.9.1: MATOMO_SITE_ID='' raises ValidationError: Input should be a valid integer ... input_value=''. .env.example ships the line as # MATOMO_SITE_ID=, so uncommenting it as written (or setting an empty value in a deployment) prevents the pod from booting. That's the opposite of off by default. env_ignore_empty=True in model_config would treat it as unset.
There was a problem hiding this comment.
Fixed in 216400a, but not with env_ignore_empty=True. I tried it with pydantic-settings 2.9.1, and it turns every empty setting into a missing one. CLEAN_DB_PASSWORD is required, and the chart sets it to '' in local-dev mode, so that setup would then fail with "Field required". Instead, a mode="before" validator turns "" into None for MATOMO_SITE_ID and MATOMO_TOKEN only. test_empty_settings_mean_unset covers it, and it fails if the validator is removed.
There was a problem hiding this comment.
You're right, and my suggestion was wrong. I checked it with pydantic-settings 2.9.1: env_ignore_empty=True turns an empty CLEAN_DB_PASSWORD into "Field required", and the chart's plaintext mode does render it empty. Scoping the validator to the two Matomo settings is the better fix. I also confirmed that removing it makes the suite error out before any test runs.
| async with httpx.AsyncClient(timeout=5) as client: | ||
| response = await client.post(f"{settings.MATOMO_URL}/matomo.php", data=params) | ||
| response.raise_for_status() | ||
| except httpx.HTTPError as e: |
There was a problem hiding this comment.
This doesn't catch everything httpx can raise. In httpx 0.28.1, issubclass(httpx.InvalidURL, httpx.HTTPError) is False. With a malformed MATOMO_URL (for example a non-numeric port), the exception should escape into the background task. Since the done callback only discards the task, asyncio would log "Task exception was never retrieved" with a traceback on every API request instead of this warning. I didn't run that end to end. Catching Exception here, since tracking must never matter to the request, would cover it.
There was a problem hiding this comment.
Fixed in 216400a. I confirmed it with httpx 0.28.1: issubclass(httpx.InvalidURL, httpx.HTTPError) is False, and posting to http://matomo:not-a-port raises InvalidURL. send_hit now catches Exception and logs a warning. The parametrized test_send_failure_is_logged_not_raised covers both a refused connection and InvalidURL, and the InvalidURL case fails with the old except httpx.HTTPError.
There was a problem hiding this comment.
Confirmed: the full suite passes at 216400a (41 tests), and with except httpx.HTTPError put back, one test fails.
| try: | ||
| await self.app(scope, receive, send) | ||
| finally: | ||
| task = asyncio.create_task(send_hit(build_hit(Request(scope)))) |
There was a problem hiding this comment.
Nothing limits how many of these run at once, and each one opens its own HTTPS connection (AsyncClient per hit, line 39). Typeahead sends a request per keystroke, so if Matomo is slow or unreachable, tasks accumulate for up to 5 s each, each holding a TCP+TLS connection, and the timeout is the only limit. A single AsyncClient created in lifespan (closed next to _db.disconnect()) with connection limits would reuse connections and cap concurrency. It would also give shutdown a place to wait for _pending, so in-flight hits aren't lost.
There was a problem hiding this comment.
Fixed in 216400a. matomo.start() in lifespan creates one shared AsyncClient with httpx.Limits(max_connections=10) and a 5 s timeout. Hits waiting for a connection give up after the pool timeout, and those failures are logged. matomo.stop() waits up to 5 s for _pending, then closes the client. test_stop_waits_for_in_flight_hits covers the wait and fails if it's removed. I haven't measured how the limit behaves under real typeahead traffic.
There was a problem hiding this comment.
Looks good. With the validator and the wait in stop() removed, the suite catches both. The task count itself is still uncapped (each request starts a task that can wait up to 5 s for a connection), but with sockets capped at 10 that seems acceptable. I agree it's worth watching once real typeahead traffic hits it.
| "urlref": request.headers.get("referer", ""), | ||
| "lang": request.headers.get("accept-language", ""), | ||
| # API clients send no Matomo cookie, so derive a stable visitor id from IP + user agent. | ||
| "cid": hashlib.sha256(f"{ip}|{ua}".encode()).hexdigest()[:16], |
There was a problem hiding this comment.
This is an unsalted hash of IP|user-agent, so it can be reversed. Anyone who can see Matomo's visitor ids can hash every IPv4 address with a few common user agents (curl, python-requests, current browsers) and recover callers' IPs, which undoes Matomo's IP anonymization. Mixing in a secret salt (for example, one that rotates daily) keeps the id stable within a day without making it reversible.
There was a problem hiding this comment.
Fixed in 216400a, keyed with the token instead of a rotating salt. cid is now HMAC-SHA256(MATOMO_TOKEN, "ip|ua")[:16]. I didn't rotate it daily because that would count a regular caller as a new unique visitor every day, which would inflate the cumulative unique-visitor charts. The tradeoffs: without a token, no cid is sent at all, and rotating the token resets visitor ids. The test checks that cid doesn't match the unkeyed SHA-256 and changes when the token changes.
There was a problem hiding this comment.
Agreed on not rotating it; inflating the unique-visitor charts every day would be worse. One question for deployment: without MATOMO_TOKEN, no cip or cid is sent, so Matomo would see every API caller as the pod's own address and visitor counts would mean nothing. That was already true before this commit. Will production set the token?
| "rec": 1, | ||
| "apiv": 1, | ||
| "send_image": 0, | ||
| "url": str(request.url), |
There was a problem hiding this comment.
This sends the full URL, query string included. Every distinct search, and every typeahead keystroke, becomes its own page URL in Matomo. That floods the Pages report and stores what users searched for next to their visitor id. I haven't checked whether the Matomo site is configured to strip query parameters. If it isn't, sending only the path, or a list of allowed parameters, would avoid it.
There was a problem hiding this comment.
Fixed in 216400a. url is now request.url with the query string removed, so every search maps to its endpoint's page. Search terms are no longer recorded. If we want them later, Matomo's site-search parameters could carry an allow-listed subset on purpose. test_records_request_path_without_query fails if the query string is sent again.
There was a problem hiding this comment.
Confirmed: URL.replace(query="") gives https://h/api/v1/search, with no trailing ?. Agreed on leaving site search for later.
| } | ||
| # Matomo ignores cip (the caller's IP) unless the request carries a token with write access. | ||
| if settings.MATOMO_TOKEN: | ||
| params.update(token_auth=settings.MATOMO_TOKEN, cip=ip) |
There was a problem hiding this comment.
Minor: when request.client is None, cip is sent as '' along with the token, and every such hit gets the same cid (hash of |ua). In production request.client is set, so this only affects servers or test transports that don't provide one. Leaving out cip when there's no address would avoid it.
There was a problem hiding this comment.
Fixed in 216400a. cip, cid and token_auth are only added when there's both a token and request.client. test_no_caller_address_sends_no_ip_or_visitor_id builds a hit from a scope with client: None.
There was a problem hiding this comment.
Looks good, thanks.
|
|
||
| @pytest.fixture | ||
| def tracking(monkeypatch): | ||
| monkeypatch.setattr(settings, "MATOMO_SITE_ID", 7) |
There was a problem hiding this comment.
Only tests using this fixture control MATOMO_SITE_ID. Settings read .env (env_file=".env"), and conftest.py doesn't clear it. So a developer with MATOMO_SITE_ID set locally who runs the full suite would have the TestClient tests in other modules record hits against the live Matomo site. I didn't run this, and I'm not certain the background tasks finish before the client closes. Forcing MATOMO_SITE_ID to None for every test in conftest.py would remove the risk.
There was a problem hiding this comment.
Fixed in 216400a, in two ways. conftest.py now sets MATOMO_SITE_ID="" before settings load; environment variables take precedence over .env, and the new validator reads "" as unset. Separately, tracking only turns on in lifespan, and the existing TestClient(app) fixture doesn't use a with block, so it never runs lifespan. I didn't test whether the background tasks would have finished before the client closed. Both changes make that question moot.
There was a problem hiding this comment.
Agreed. Either change alone would have been enough, and with both the question is moot.
- Treat empty MATOMO_SITE_ID/MATOMO_TOKEN as unset instead of failing to start - Catch every exception when sending a hit (httpx.InvalidURL is not an HTTPError) - Share one client with a connection limit, created and closed in lifespan; shutdown waits up to 5 s for in-flight hits - Send the path without the query string - Key the visitor id with the token so it can't be reversed into an IP; send cid/cip only with a token and a known caller address - Force tracking off in tests regardless of a local .env
matthewberry
left a comment
There was a problem hiding this comment.
Second pass on 216400a. Only the first item below was reproduced; the other three come from reading the code. I left out points that earlier threads already settled: the uncapped task count, the missing-token question, and query strings. None of these block merging. The first one is a cheap fix for something the PR description promises ("Can't break the API").
| try: | ||
| await self.app(scope, receive, send) | ||
| finally: | ||
| task = asyncio.create_task(send_hit(build_hit(Request(scope)))) |
There was a problem hiding this comment.
build_hit(Request(scope)) runs right here in the finally, outside send_hit's try/except, so an error while building the hit escapes the middleware. Reproduced with the Starlette installed in .venv: a request with Host: [abc makes request.url.replace(query="") raise ValueError: Invalid IPv6 URL. That gets logged as "Exception in ASGI application". If the endpoint had already raised, this error replaces the original one in ServerErrorMiddleware and in the logs.
Reachability is limited: the mmli2 ingress routes by host, so a request like this can't arrive through Traefik. It can only arrive by calling the pod or service directly. Still, building the hit inside the task (for example asyncio.create_task(send_hit(...)), with send_hit calling build_hit inside its try) would make "tracking can't break the API" true for the whole path, not just the POST. Note that Request(scope) would then have to be built before the task starts, because the scope belongs to this call.
| response.raise_for_status() | ||
| except Exception as e: | ||
| # Tracking must never affect the API, whatever goes wrong. | ||
| logger.warning(f"Matomo tracking failed: {e!r}") |
There was a problem hiding this comment.
Minor, not run: when Matomo is down, every API request logs one warning, and with typeahead that's one per keystroke. A long outage would flood the pod logs and bury real errors. Logging the first failure and then suppressing (or rate-limiting) until a hit succeeds again would keep the signal without the noise.
| return | ||
| if _pending: | ||
| await asyncio.wait(_pending, timeout=5) | ||
| await _client.aclose() |
There was a problem hiding this comment.
Minor, not run: when asyncio.wait times out, the leftover tasks are still running when aclose() closes the client underneath them. Each one then logs a tracking-failure warning, and any task still pending when the loop closes can produce "Task was destroyed but it is pending". Cancelling what's left in _pending before aclose() would make shutdown quiet.
| yield | ||
| # Disconnect from database on shutdown | ||
| await _db.disconnect() | ||
| await matomo.stop() |
There was a problem hiding this comment.
Minor, not run: if _db.disconnect() raises, matomo.stop() never runs, so in-flight hits are dropped and the client isn't closed. Hits don't depend on the DB, so stopping Matomo first, or putting the shutdown in a try/finally, would avoid tying the two together.
Problem
We don't track how many API calls we get. Nothing in the app sends requests to Matomo or any other analytics tool, and the only middleware is CORS and GZip. In the mmli2 cluster, Traefik has Prometheus metrics turned on but nothing scrapes them, and its access logs are off.
Change
app/core/matomo.pyaddsMatomoTrackingMiddleware, whichapp/main.pyinstalls as the outermost middleware. For each HTTP request it sends Matomo one page view (Tracking API,POST {MATOMO_URL}/matomo.php) with:url(scheme, host and path, with the query string removed) andaction_name(the path). With query strings, every search and every typeahead keystroke would be its own page, and search terms would be stored next to visitor ids.ua,urlrefandlang, from theUser-Agent,RefererandAccept-Languageheaders.MATOMO_TOKENis set and the caller's address is known:token_auth, pluscip(the caller's IP). Matomo ignorescipwithout a token that has write access to the site.cid: the first 16 hex characters ofHMAC-SHA256(key=MATOMO_TOKEN, "IP|user agent"). API clients send no Matomo cookie, so this is what Matomo uses to count unique visitors. Keying it with the token means the id can't be reversed into an IP by hashing candidate addresses. It also means rotating the token starts everyone over as new visitors.How it behaves:
MATOMO_SITE_IDis set when the app starts (matomo.start()inlifespan). An empty value (MATOMO_SITE_ID=) counts as unset; it no longer stops the app from starting.httpx.AsyncClientwith at most 10 connections and a 5 s timeout, so a slow Matomo can't pile up open connections.httpx.InvalidURL, which isn't anhttpx.HTTPError.matomo.stop()waits up to 5 s for hits still being sent, then closes the client.finally), plus/docsand/openapi.json.OPTIONSrequests aren't recorded, so each browser call counts once.New settings:
MATOMO_URL(defaults tohttps://matomo.mmli2.ncsa.illinois.edu),MATOMO_SITE_ID,MATOMO_TOKEN. They're listed in.env.example.Caller IP: the middleware uses
request.client.host. That's the real caller only because of #28 (FORWARDED_ALLOW_IPS).To turn it on (not part of this PR)
MATOMO_SITE_IDtocontroller.extraEnvinchart/values.mmli2.yaml. AddMATOMO_TOKENto themoldb-cleandb-apiSealedSecret, which the deployment loads withenvFrom.Testing
tests/test_matomo.pysends requests through the real app withhttpx.ASGITransport, usingclient=("203.0.113.5", …). It checks:cip, acidthat is the same for the same IP and user agent and differs when the user agent or token changes, and thatciddoesn't match the unkeyed hash;ciporcidis sent when there's no caller address;MATOMO_SITE_IDis unset;MATOMO_SITE_IDandMATOMO_TOKENload as unset;send_hitlogs instead of raising, both when a connection is refused and forInvalidURL;stop()waits for a hit that's still being sent.conftest.pyforcesMATOMO_SITE_IDto empty, so a developer's.envcan't send test requests to the real Matomo.To check the tests, I put the original code back one change at a time for the empty-value,
InvalidURL, query-string and shutdown-wait fixes, and confirmed that a test fails each time. Full suite: 41 passed.Not tested:
/matomo.phpanswers with a 200);