Repository navigation
Add search query and click logging, with retention - #284
Open
acoustid-bot wants to merge 1 commit into
Open
acoustid-bot wants to merge 1 commit into
acoustid-bot wants to merge 1 commit into
Conversation
The tables behind relevance work for #281: which queries return nothing, and which result people pick when they do return something. Click position is the part nothing else in the system can see -- a query whose right answer sits reliably at rank 7 is a ranking problem, and it is invisible without this. No account ids and no IP addresses, deliberately. A query string alongside either is personal data; alongside an opaque rotating token it is not. What is stored is an HMAC of a token the client generates for itself, keyed on a secret and on the current UTC day, so the same visitor hashes differently tomorrow and the rows cannot be stitched into a history of one person. The day is part of the message rather than the key, so rotation needs no stored state and nothing has to be rotated on a schedule -- the input changes on its own at midnight. The session is nullable and a missing token is an ordinary case rather than an error: the query is the useful part, and refusing to log without a token would quietly bias the data toward whoever runs JavaScript. Hashing a fallback instead would be worse than storing nothing, since it would file every tokenless visitor under one shared identifier. Retention ships with the table rather than following it. A policy that arrives after the data is a policy applied to whatever accumulated in the meantime, and "cleanup once it matters" is how a log of what people searched for becomes a permanent record nobody decided to keep. 90 days, deleted daily by a worker task in bounded batches so it is a series of short transactions rather than one long one on a cluster that is also serving the website. Clicks go with their query through ON DELETE CASCADE, so there is no second statement to keep in step and no way to leave orphans -- which would be a residue of searches that had supposedly been forgotten. Verified beyond the unit tests: the migration was run against a scratch database through the whole chain, and the resulting schema checked column by column, including that the foreign key really carries ON DELETE CASCADE and that deleting a query does take its clicks with it. One test asserts the cascade on the model rather than on the database, and the comment explains why: the test schema is built with metadata.create_all, which is idempotent, so an existing table keeps whatever it was first created with and only a fresh database would notice the declaration changing. The behavioural test alone passed with the cascade removed, which is how that was found. Nothing calls this yet. Wiring it into the search endpoint comes with that endpoint, so that logging is in place from the first query rather than added afterwards.
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 1 minute. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
The tables behind relevance work for #281, and the retention that goes with them. Third of four pieces (indexer #282, endpoint #283, this, then the UI).
Independent of #282 and #283 — different files, no conflicts, mergeable in any order. Nothing calls it yet: the wiring into the search endpoint comes with the UI change, so that logging is in place from the first real query rather than added afterwards.
What it records, and what it deliberately does not
Two tables on the app cluster.
search_queryholds the query text, how many results came back, and a session hash;search_clickholds which recording was chosen and at what position. Position is the part nothing else in the system can see — a query whose right answer sits reliably at rank 7 is a ranking problem, and it is invisible without this. It is also what would have caught the cross-field defect in #282 months earlier than a person noticing.No account ids and no IP addresses. A query string alongside either of those is personal data; alongside an opaque rotating token it is not. What gets stored is an HMAC of a token the client generates for itself, keyed on a secret and the current UTC day.
The day is part of the message rather than the key, which is the detail that makes this practical: rotation needs no stored state and nothing has to be rotated on a schedule, because the input changes on its own at midnight. The same visitor therefore hashes to something different tomorrow, and yesterday's rows cannot be stitched into a history of one person. Even holding the secret, re-identifying someone means knowing the token they happened to have on that specific day.
sessionis nullable, and a missing token is an ordinary case rather than an error. The query is the useful part, and refusing to log without a token would quietly bias the data toward whichever visitors run JavaScript. Hashing some fallback instead would be worse than storing nothing, because it would file every tokenless visitor under a single shared identifier.Retention ships with the table, not after it
90 days, deleted daily by a worker task. This is in the same commit as the tables on purpose: a retention policy that arrives after the data is a policy applied to whatever happened to accumulate in the meantime, and "we will add cleanup once it matters" is how a log of what people searched for becomes a permanent record nobody decided to keep.
Deleted in bounded batches, so cleanup is a series of short transactions rather than one long one — the same cluster serves the website. Clicks go with their query through
ON DELETE CASCADE, so there is no second statement to keep in step and no way to leave orphans behind; orphaned clicks would be a residue of searches that had supposedly been forgotten.Verification
The unit tests cover the privacy properties and the retention behaviour, each mutation-tested by breaking the guard and confirming a test fails: dropping the day from the hash (no rotation), dropping the secret (reducing it to a digest anyone could recompute), inventing a hash when there is no token, and ignoring the batch limit.
Beyond unit tests, the migration was run through the whole chain against a scratch database and the result checked column by column — including that the foreign key really carries
ON DELETE CASCADEand that deleting a query does take its clicks with it. That matters because production gets its schema from the migration, not fromtables.py.One test asserts the cascade on the model rather than on the database, and that distinction is worth reading. The behavioural test passed with the cascade removed from
tables.py, which is how this was found: the test schema is built withmetadata.create_all, which is idempotent, so an existing table keeps whatever constraints it was first created with and only a fresh database would ever notice the declaration changing. The model-level assertion is environment-independent and keepstables.pyin step with the migration.Full suite 423 passed, 1 skipped. Lint clean.
Open question for review
The client-supplied token is the one design choice I would flag. The alternative was the Flask session cookie, which I rejected because
/search.jsondeliberately avoids touching the session, and deriving anything from an IP was ruled out from the start. A client token means the UI has to generate and keep one (a random value insessionStorage), which is work that lands with the UI change. If that trade is wrong, the schema does not have to change — only what gets passed in — sincesessionis just an opaque string.