Repository navigation
FEAT: Indexed attack analytics - #2792
Roman Lutz (romanlutz) merged 9 commits into
Conversation
|
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 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:
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. |
|
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 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>
dfa8cf6 to
0971f29
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
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. |
|
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. |
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>
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
mainand the async memory API from #2832. SDK calculations, REST, History's counting switch, benchmark, and GUI remain separate.901e6c7bf9d4, follows publishedmainhead6ea3eb4b61c3. 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.report_async,results_async, andfacets_asynconget_session_asyncwith request-owned cancellation and transactions. Both the deprecated sync and asyncget_attack_resultssignatures accept opt-inALL_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
mainrevision. 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 frommainand 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 normaluv run --frozen --no-sync pre-commit run: passed. Ruff lint/format and the migration immutability hook also passed.doc/contributing/11_memory_models.mdand the related analytics contract description. JupyText was not run because the PR diff contains no notebooks or JupyText sources.