Improvements to analysis and detector engine - #167
Open
hellais wants to merge 19 commits into
Open
Conversation
The daily batch_measurement_processing DAG passed op_kwargs={"day": ...} to
run_make_analysis(), which takes `timestamp`/`ts` and has no `day` parameter.
Airflow forwards op_kwargs verbatim as keyword arguments, so every run of the
daily make_analysis task raised TypeError at task runtime rather than at DAG
parse time.
Add an AST-based test asserting op_kwargs keys are accepted by the referenced
python_callable, for all DAG files. It's static rather than importing the DAG
because importing needs an Airflow metadata DB for Variable.get; verified it
fails on the pre-fix source.
fingerprints_dns was declared twice with incompatible schemas: create_tables.py used ENGINE = URL over the upstream CSV with all-String columns, while the fingerprints updater used EmbeddedRocksDB with typed enums. Both used CREATE TABLE IF NOT EXISTS, so whichever ran first on a deployment won, making the analysis query's join behaviour deployment-dependent — and on the URL variant every join performs a live HTTPS fetch. Define the DDL once in create_tables.py and build both the live and _tmp tables from it in the updater, which also collapses four near-identical copies of the schema into one. The DNS and HTTP scope enums differ (HTTP adds 'injb' and 'prov') so that stays parameterised. Also declare fingerprints_http in make_create_queries(), which previously omitted it entirely. Deployments carrying the URL variant need a manual rebuild; documented in docs/migration-notes.md.
The DNS/TCP/TLS scoring cascades were literal multiIf blocks inside the
analysis query's f-string. That had two costs: the rules had no unit tests
(exercising one needed ClickHouse, a fixture measurement and the whole query),
and the output recorded what score a row got but not which rule produced it, so
the distribution of rule firings was unknowable and weights could not be
attributed to outcomes.
Move the rules to analysis/rules.py as a list of Rule records and generate both
the outcome cascade and a matching rule-id cascade from the same list, so a
row's rule id always names the rule that scored it. The three nested TLS
branches are flattened by conjoining the outer condition onto each, which
preserves first-match semantics exactly.
Behaviour-preserving: verified the generated outcome cascades emit the same
tuple sequences in the same order as the previous hand-written SQL (13 DNS,
7 TCP, 9 TLS).
Persist the driving rule as top_{dns,tcp,tls}_rule_id via argMax over the
blocked possibility, so the id explains the *_blocked_max it accompanies. This
is what makes weight changes a join rather than a reprocessing run, and it is a
prerequisite for calibrating the rules against labelled data.
Adds tests/test_rules.py: ids unique, weights in range, triples never sum above
1, both cascades agreeing on conditions and order, and the outer projection
matching the table's column count (the positional INSERT .. SELECT makes that
drift silent otherwise). Requires an ALTER TABLE before deploy, in
docs/migration-notes.md.
Three fixes to the blockpage rule's join, none of which change scoring for the
current fingerprint set but all of which are load-bearing for the HTTP layer:
- expected_countries is a comma-separated string upstream ("IT, IR"), but
groupArray() produced ['IT, IR'] and has(..., 'IR') against that is false.
Split and trim first. Every current DNS fingerprint names a single country,
which is why this had not bitten.
- A 'fp' scope marks a known false-positive fingerprint, so matching one is
evidence against blocking — but scope was computed and never read, so an fp
row would have scored 1.0 blocked. Gate the rule on it. The DNS set has no
fp rows today; the HTTP set does.
- The join is an equality match, which only implements pattern_type = 'full'.
Make that explicit with a WHERE rather than silently never matching a
prefix/contains/regexp row.
Checked against the live data before changing anything: all 227 DNS
fingerprints are 'full' with no 'fp' scope and no multi-country rows, so these
were latent rather than active defects. The HTTP set is 1414 contains, 275
prefix, 14 regexp and does use fp scope, so the same join cannot be reused for
HTTP without this handling.
Adds a canary test that fails if upstream introduces a DNS pattern_type the
query cannot evaluate, rather than letting those fingerprints go quietly
unmatched, plus a check that expected_countries still parses as documented.
Deliberately not changing weights: confidence_no_fp is a 0-10 self-assessment
(151 of 227 rows are 5, not 10), so folding it in would halve the score for
most blockpage matches. That is a real calibration decision and needs labelled
data to adjudicate, not a guess.
The telegram, whatsapp, signal and facebook_messenger transformers wrote every observation with target_id unset. Those tests probe a fixed pool of named platform endpoints and carry no per-measurement input, so there is nothing for a target-keyed analysis to group them by — the web analysis keys on domain(input), which for these is empty and would collapse a whole measurement into one group. WebObservation.target_id already exists for this and is used by the tor transformer; these four just never set it. Adds oonipipeline/targets.py: the target registry plus a resolver, keyed by service role rather than hostname so a series survives an endpoint rename. Signal is the motivating case — chat is textsecure-service.whispersystems.org in the archive and chat.signal.org in the current spec, and an identity change there would read as a blocking event to the detector. Also records combination_rule per target (WhatsApp's 16 endpoints are a redundant pool, so any_of; Signal's services are each independently required, so all_of), which the per-target scorer will need. Tagging is applied after consume_web_observations rather than by calling it once per target: that call maps DNS/TCP/TLS/HTTP observations to each other by transaction_id and ip:port across the whole measurement, so splitting the input would change which rows get merged. The existing exact-count assertions (33 telegram, 137 whatsapp, 14 facebook, 19 signal) all still hold, which is what confirms it. Telegram addresses its datacentres by IP with no hostname at all, so the resolver falls back to the address and treats any bare IP in a telegram measurement as pool membership rather than pinning a DC list that would go stale. Adds a drift canary over all 12 IM fixtures asserting every endpoint resolves to a declared target, so a platform rename fails loudly instead of silently dropping rows out of analysis. It found that PII scrubbing leaves the literal string "scrubbed" in place of an address; those rows are excluded explicitly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #167 +/- ##
==========================================
+ Coverage 83.51% 83.71% +0.19%
==========================================
Files 85 89 +4
Lines 5314 5593 +279
==========================================
+ Hits 4438 4682 +244
- Misses 876 911 +35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The clickhouse_server fixture only knew how to get a database from
docker-compose, and create_db_for_fixture calls pytest.skip when it cannot
connect. On a machine without docker that means test_analysis, test_detector,
test_e2e, test_db and the updater tests all skip — the suite reports green
while covering none of the SQL.
Add an OONIPIPELINE_TEST_CLICKHOUSE_URL override that bypasses the docker
fixtures entirely, and fail rather than silently fall through when it is set
but nothing is listening. The docker fixtures are now requested lazily so the
override path does not need docker installed at all.
OONIPIPELINE_TEST_CLICKHOUSE_URL=clickhouse://test:test@127.0.0.1:19000/default \
pytest tests/
Note that on ClickHouse older than 25.x the analysis query needs
?allow_experimental_analyzer=1 appended: it aliases an expression to the name
of a column it reads (dns_answers_contain_bogon), which the old analyzer
rejects with "Different expressions with the same alias". The compose fixture
pins 25.2 where the new analyzer is the default, so this only shows up when
pointing at an older local instance.
The analysis query builds its control by aggregating a full day of obs_web_ctrl, plus a second full-day scan of obs_web for the addresses probes found TLS-consistent — on every hourly run. Two problems follow. The work is done ~24 times over, and both joins need grace_hash to fit. Worse, the control depends on when the job ran: the 01:00 run scores against roughly an hour of control data and the 23:00 run against twenty-three, so ctrl_dns_success_rate > 0.5 is a threshold over very different sample sizes depending on time of day. Re-running a backfill produced different scores than the original run, and analysis_web_measurement being a ReplacingMergeTree keyed on measurement_uid meant the newer value silently won. Adds obs_web_ctrl_rollup, keyed (hostname, ts_hour, ip), written by a new make_ctrl_rollup task that runs between observations and analysis. Consumers read a closed trailing window derived only from the analysis window, never from wall-clock time, so re-running an hour reads exactly the same buckets. Long-form rather than a Map per hostname so the two sources can be written in one pass and the grain stays explicit. ReplacingMergeTree rather than Summing so a re-run replaces its rows instead of accumulating onto them — which is what makes backfills safe — at the cost of needing FINAL on read. Strictly additive: the analysis query still computes its control inline and is untouched. Switching it over is a separate change, gated on test_rollup_control_matches_inline_control, which runs both paths over the same four web_connectivity fixtures and asserts every count and map agrees.
The module docstring still described a SummingMergeTree with the two sources written independently and summed at merge time, which was the earlier design. It became a single unioned write into a ReplacingMergeTree so that re-running a window replaces rather than accumulates; describe that instead. Stop quoting a fixed 24h in the backfill instructions and point at DEFAULT_CTRL_LOOKBACK, so the note cannot drift from the value in the code.
1. Architecture -> show the high level architecture of the system with the division into the various layers. Include in here also the specific components of infra that are used. 2. Ontology -> Explain the different entities and what they mean. Include as part of this the semantics of the data structure 3. User guide -> How an end user might end up consuming the various datasets which are produced 4. Developer guide -> How somebody developing this system should approach it 5. Implementation plan -> Document what the current implementation state is and what the next steps in development looklike
hellais
changed the title
hellais
marked this pull request as ready for review
July 30, 2026 10:11
Removes obs_web_ctrl_rollup and everything that fed it: the analysis and task modules, the DDL, the hourly DAG step, the CLI run step, and the parity test. Reverts b02923b and 41709d3. It was scoped on two claims about the inline control derivation, and reading the generated SQL showed both were wrong. Both control subqueries bind the same start_time/end_time as the experiment, so: - It is not non-deterministic. Re-running an hour reads the same rows and produces the same control. The toStartOfDay grouping makes it look like a day window; it is a join key that is constant within an hourly window. - It is not ~24x redundant. Each run scans one hour of obs_web_ctrl, the minimum it could scan. The genuine defect is the opposite of what was written down: the control window is too narrow, so a hostname the test helper did not resolve in that hour has no control at all, the LEFT JOIN misses, and the DNS cascade cannot tell "the control disagrees" from "there is no control". Both land on answer_unmatched at 0.75 blocked. That is a scoring bug, and it belongs in the rule set rather than in a new table: the failure branch already handles the same case correctly via failure_no_ctrl. Reverted rather than dropped from history because both commits are already on origin/pipelinev5.1; rewriting a shared branch is not worth it to unship an additive table nothing reads. Docs updated throughout: architecture tier table, data flow, schedule and table inventory; ontology §5; the developer guide's repo layout and test matrix; and the implementation plan, where the rollup is no longer listed as shipped or partially built. The migration note becomes a drop-if-present step rather than being deleted, so a cluster that ran the original create does not keep an orphaned table. Verified: obs_web_ctrl_rollup is absent from a freshly provisioned schema, no code references remain, 65 offline tests and 12 analysis tests pass.
Measurements now carry a top-level probe_id: a pseudonymous probe identifier issued via anonymous credentials, so repeated measurements from one probe can be linked without identifying it. Adds it to BaseMeasurement, so every nettest inherits it, and to ProbeMeta, so it lands on every observation without touching individual transformers. Carried through to analysis_web_measurement as well, since that is the tier the detector and the aggregation API read, and without it there the field could never be used for what it is for. Normalised to "" when absent. Every measurement collected before the scheme shipped omits the key, and "" has to mean unknown rather than "one probe" — counting it would collapse an entire archive's worth of distinct probes into a single observer. Queries therefore need uniqIf(probe_id, probe_id != ''), which is how the docs now write it throughout. obs_web_ctrl deliberately does not get the column: it records the test helper's view, which has no probe, and WebControlObservation carries no ProbeMeta. This retires the "OONI has no stable probe identifier" caveat that appeared in four docs. The cell-state design in ontology §9 now counts distinct probes rather than using uniq(report_id) as a proxy, with report_id kept as the fallback for historical windows where coverage will be empty. Verified end to end against ClickHouse: a measurement carrying probe_id reaches analysis_web_measurement with the value intact, one without it yields "", and the column exists on obs_web, obs_http_middlebox and analysis_web_measurement but not obs_web_ctrl. 81 tests pass. Needs an ALTER before deploy, in docs/migration-notes.md §4.
Promotes the hierarchical mechanism taxonomy out of the deferred appendix and into ontology §12. The trigger recorded against A.3 has fired: the labelled corpus needs a label vocabulary that is independent of the rules being evaluated. Labelling with rule ids would be circular, since a rule can then only be shown to reproduce itself, and the corpus would rot on every rule split, starting with the answer_no_ctrl split already scheduled in §3.3. So labels use the taxonomy, predictions use rule ids, and evaluation is a mapping between them. Specifies the <layer>.<action>.<qualifier> structure, the v1 vocabulary, the label record, and hierarchical scoring. Three ambiguities in the earlier sketch are resolved: - Reset is placed at the layer where it manifested, per the existing definition of layer. The earlier sketch had both tcp.reset.sni and tls.reset.sni, which a labeller could not choose between; it is now tcp.reset during connection establishment, tls.reset.sni after the ClientHello, http.reset.host after the Host header. - throttle is an action rather than a family, so reset-versus-throttle under one trigger are alternatives within a frame rather than split across two. - access_denied.geo is dropped. Geoblocking is an ordinary technique attributed to the server via locus; no value may appear on two axes. Two properties matter most for the corpus. Labellers record the deepest node the evidence supports, so internal nodes like tls.mitm are the common case and not a fallback; forcing a leaf manufactures precision the measurement does not have. And evaluation is therefore hierarchical, so predicting an ancestor of the truth is correct-but-imprecise rather than a miss, with under-specific and over-specific reported separately because they call for different fixes. The rule-id to node mapping doubles as a to-do list: every internal-node entry marks a place where the evidence to reach a leaf already sits in obs_web but no rule reads it. locus is kept on the label record even though the pipeline cannot infer it. Ground truth should record what is true rather than what the engine can compute, and those labels are the evidence for whether building that inference is worth it.
Use an multi-agent review which was fed some pointers as to what aspects of the documentation to focus on, but was blinded from the codebase. Integrate the feedback from the review based on actually inspecting the codebase and synthesizing it into the design docs
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
1 participant
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.
This PR introduces that make the analysis engine easier to understand, adds support for tagging IM observations with the platform endpoint that was measured and fixes a few bugs.
It also includes some documentation on the current design of the system, the desired target result and a plan to get there.
You should read these docs to better understand what's going on here and what we are aiming for:
These changes are building blocks in allowing us to better measure how the analysis engine is working, without changing how it works from a functional perspective.
A few manual migrations will need to be run on the database in order for this to run properly, which are documented inside of
docs/migration-notes.md.Below is the concise summary of changes:
Code and documentation in this PR was developed with AI assistance