Skip to content

FEAT: Indexed attack analytics - #2792

Merged
Roman Lutz (romanlutz) merged 9 commits into
microsoft:mainfrom
romanlutz:romanlutz-analytics-storage-and-queries
Oct 6, 2026
Merged

Roman Lutz (romanlutz) merged 9 commits into
microsoft:mainfrom
romanlutz:romanlutz-analytics-storage-and-queries

Conversation

@romanlutz

@romanlutz Roman Lutz (romanlutz) commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Description

#2742 established shared analytics contracts, but saved attack results still need exact indexed queries without loading conversations, scores, media, or full result objects. This is the storage/query slice in the five-part analytics series, based directly on main and the async memory API from #2832. SDK calculations, REST, History's counting switch, benchmark, and GUI remain separate.

  • One additive migration, 901e6c7bf9d4, follows published main head 6ea3eb4b61c3. It adds indexed canonical/legacy identifier lookup and a persisted, indexed frozen v1 objective-target evaluation hash. The latter groups behaviorally equivalent targets even when endpoints/deployments differ; a bounded backfill handles supported saved and normalized historical identifiers. Unsupported target metadata remains in the typed missing group with a warning. Content hashes remain exact storage/inspection identities; each saved result ID counts separately. Evaluation rule changes require a new version/migration, not reinterpretation of v1 groups. Scenario remains a run-ID dimension, while attack type and model remain name dimensions.
  • Build raw outcome totals, metadata filters, groups/matrices, bounded facets, compact-profile probes, and lightweight cursor pages. A report and its first page share one consistent read, with fresh continuation pages. Recorded converter lists preserve hashless/legacy members; compact-profile overflow uses complete SQL aggregation rather than sampling.
  • Expose report_async, results_async, and facets_async on get_session_async with request-owned cancellation and transactions. Both the deprecated sync and async get_attack_results signatures accept opt-in ALL_RESULTS; their latest-per-conversation default and History's existing behavior stay unchanged. SQLite lock waits and aioodbc statements use request deadlines. SQL Server scalar facets group by projected keys, and set-based filter values keep accepted 500-value requests below 2,100 positional parameters.

Migration/operational notes: The PR adds only one Alembic file and does not edit any published main revision. Before merge, the PR branch was rewritten to replace its earlier PR-only migration and no-op merges. Any persistent database previously stamped with one of those removed PR-only IDs would need rebuilding or an explicit migration repair; this work did not delete or migrate an existing database. The migration does not create result IDs, delete historical duplicates, or rescore outcomes. SQL Server reports require SNAPSHOT support and do not change server settings. Query/profile limits bound returned metadata, not all database work or peak memory. Live Azure SQL validation and policies for malformed legacy converter names and SQL Server trailing-space comparisons remain follow-ups.

Tests and Documentation

  • uv run --frozen --no-sync pytest -q -n 4 tests\unit\memory tests\unit\models\test_analytics.py tests\unit\common\test_pagination.py tests\unit\backend\test_attack_service.py: 1,504 passed, one existing Azure-only skip after consolidation. It exercises isolated SQLite, async selection, default History compatibility, filters, UTC/DST bounds, cancellations, and offline SQL Server compilation. No live Azure or shared database was accessed.
  • uv run --frozen --no-sync pytest -q tests\unit\memory\test_analytics_migration.py: 13 passed on the rewritten commit. Includes fresh upgrade from main and an older published revision, downgrade/re-upgrade, 501-row historical backfill, outcome boundaries, and offline SQL Server index/backfill SQL checks.
  • uv run --frozen --no-sync ty check pyrit, uv run --frozen --no-sync ty check tests\unit\memory\test_analytics_migration.py, uv run --frozen --no-sync python -m build_scripts.memory_migrations check, uv run --frozen --no-sync python -m build_scripts.validate_docs, and normal uv run --frozen --no-sync pre-commit run: passed. Ruff lint/format and the migration immutability hook also passed.
  • Updated doc/contributing/11_memory_models.md and the related analytics contract description. JupyText was not run because the PR diff contains no notebooks or JupyText sources.

@romanlutz Roman Lutz (romanlutz) changed the title FEAT analytics storage and queries FEAT: Indexed attack analytics Sep 23, 2026
@richlundeen

Copy link
Copy Markdown
Contributor

I recommend merging #2832 first, once its own conflicts and checks are resolved, then adapting this PR to the async memory API. The indexes and SQL compiler can largely stay. The main overlap is session ownership and cancellation: this PR adds get_session(timeout=...) and direct sqlite3/ODBC hooks, while #2832 introduces async sessions with aiosqlite/aioodbc and deprecates the synchronous session API. Starting the new reader with report_async, results_async, and facets_async avoids another sync-to-async migration. Please also carry ALL_RESULTS through the shared implementation and both result-query API signatures.

I support the database-side aggregation and indexing here. Frequent analytics reads justify this direction; I would not require a large benchmark suite before merging. My correctness concerns are separate:

  1. Confirmed migration blocker: two Alembic heads. The new revision and 9b2d4f6a8c0e both descend from 7a9c1e3f5b2d, with no merge revision. Normal initialization upgrades to head, so this blocks initialization when migrations are enabled. Please add a merge revision and update the single-head expectation, rather than changing an existing migration.

  2. SQL Server grouping concern: repeated positional parameters. In _scalar_facet, operation/operator keys contain parameterized CASE/COALESCE expressions repeated in SELECT and GROUP BY. _grouping_projection only projects expressions containing JsonScalar, so these simple keys remain inline. With pyodbc positional binding, repeated constants become separate server parameters; SQL Server can reject the SELECT expression as not matching the GROUP BY expression. Please verify this with a parameterized driver execution, not only SQL compilation or literal-bound SQL. Projecting the derived keys once and grouping by their columns should avoid this issue.

  3. SQL Server parameter-budget concern. _predicate emits a separate EXISTS and repeats the full metadata/key expression for every selected converter value. The contract allows 100 values per predicate and 500 overall. The repeated bound constants/paths, plus repeated filters across fact branches, can exceed SQL Server's 2,100-parameter limit even for an accepted request. Please check the rendered positional parameter count at the supported limits. A single membership query over a bound value set for ANY, with a set-based ALL equivalent, would reduce both query size and repeated work.

The two SQL Server concerns are from static review and have not been reproduced against a live server. They should be verified before treating SQL Server support as complete.

For scale, the strongest parts are avoiding full AttackResult/score/conversation loading, aggregating repeated metadata before array expansion, and using keyset pagination for results. The important limit is that bounded output is not bounded database work: exact totals still process the matching cohort, and compact-profile overflow adds a probe before the full SQL path. A small representative performance check would help establish where these extra paths pay off, without making a broad benchmark project a prerequisite.

Review assisted by GitHub Copilot.

@richlundeen

Copy link
Copy Markdown
Contributor

One design change I would like here: use evaluation hashes rather than content hashes for almost all identifier-based analytics grouping. Those queries better match what we want to compare: outcomes for behaviorally equivalent targets and attack configurations, rather than separate groups for every exact stored identifier.

For example, the current objective_target dimension groups by the target content hash. Two target configurations that differ only in operational details, such as endpoint or deployment configuration, can therefore split into separate groups even when the target evaluation rules consider them equivalent. Grouping by ObjectiveTargetEvaluationIdentifier.eval_hash would make that comparison more useful. I would apply the same principle wherever we group by an identifier, using the evaluation rules for that component. Class-name and model-name breakdowns can remain useful separate dimensions.

This is a change to analytics grouping identity, not storage identity: keep content hashes for primary/foreign keys, deduplication, and exact configuration inspection. Keep result IDs as the counting unit; sharing an evaluation hash must not collapse distinct saved results into one result. Group filters, facets, and drill-downs should use the same evaluation identity as the chart so their counts agree.

For scale, please persist/index the required evaluation hashes and backfill supported historical identifiers rather than reconstructing and hashing every result at query time. We should also make the historical evaluation-rule policy explicit, so a rule change does not silently change the meaning of a group.

Comment drafted with GitHub Copilot.

Add SQLite and SQL Server analytics queries over distinct saved result IDs with native async sessions, bounded result pages and facets, exact grouped outcomes, legacy metadata support, and opt-in result selection.

Use one additive migration after current main for the computed identifier lookup, workload indexes, and frozen v1 target-evaluation key with bounded historical backfill. Keep content hashes for exact inspection and every result ID as a counting unit.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@romanlutz
Roman Lutz (romanlutz) force-pushed the romanlutz-analytics-storage-and-queries branch from dfa8cf6 to 0971f29 Compare October 3, 2026 11:59
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Comment thread pyrit/memory/memory_models.py Outdated
Comment thread pyrit/memory/attack_analytics_query.py
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@romanlutz

Copy link
Copy Markdown
Contributor Author

Partially accepted your earlier feedback: #2832 is merged, the reader uses native async sessions and ALL_RESULTS in both result-query APIs, and this branch has one analytics migration after the published main head rather than a merge revision over unshipped PR-only migrations. Scalar and array grouping now project keys, and set-based filters pass offline qmark parameter-budget checks; live Azure SQL validation and representative performance measurements remain follow-ups.

@romanlutz

Copy link
Copy Markdown
Contributor Author

Partially accepted your earlier design feedback: objective-target groups, filters, and facets use a persisted, indexed, historically backfilled frozen v1 evaluation hash, while result IDs remain the counting unit and exact content hashes remain for inspection. This batch also keeps that key current on identifier replacement; the other exposed axes are class/model names or saved run IDs, so a separate attack-configuration evaluation axis is outside this storage slice.

@romanlutz
Roman Lutz (romanlutz) added this pull request to the merge queue Oct 6, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 6, 2026
@romanlutz
Roman Lutz (romanlutz) added this pull request to the merge queue Oct 6, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 6, 2026
@romanlutz
Roman Lutz (romanlutz) added this pull request to the merge queue Oct 6, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 6, 2026
@romanlutz
Roman Lutz (romanlutz) added this pull request to the merge queue Oct 6, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 6, 2026
@romanlutz
Roman Lutz (romanlutz) added this pull request to the merge queue Oct 6, 2026
@romanlutz
Roman Lutz (romanlutz) removed this pull request from the merge queue due to a manual request Oct 6, 2026
Roman Lutz (romanlutz) and others added 5 commits October 6, 2026 12:24
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@romanlutz
Roman Lutz (romanlutz) added this pull request to the merge queue Oct 6, 2026
Merged via the queue into microsoft:main with commit 345a7ab Oct 6, 2026
50 checks passed
@romanlutz
Roman Lutz (romanlutz) deleted the romanlutz-analytics-storage-and-queries branch October 6, 2026 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants