T037 — Instrument the five phases of get_activity with stats_logger
timing metrics under the `superset.activity_view.<kind>.<phase>_ms`
key convention (plan §D-17). Wrap each phase with a stats_timing
context manager:
* superset.activity_view.<kind>.relationship_resolution_ms (_resolve_scope)
* superset.activity_view.<kind>.fetch_ms (_fetch_change_records)
* superset.activity_view.<kind>.visibility_filter_ms
* superset.activity_view.<kind>.denormalize_ms
* superset.activity_view.<kind>.decorate_ms
`<kind>` is the lowercased path-entity model class name (dashboard /
slice / sqlatable), enabling per-endpoint-family Grafana panels.
T038 — Add request- and response-shape attributes via the existing
counter/gauge interface:
* superset.activity_view.<kind>.requests.include_<value> (incr)
* superset.activity_view.<kind>.requests.has_since_filter_<true|false> (incr)
* superset.activity_view.<kind>.page_size (gauge)
* superset.activity_view.<kind>.record_count (gauge — post-visibility-filter)
* superset.activity_view.<kind>.related_entity_count.charts (gauge)
* superset.activity_view.<kind>.related_entity_count.datasets (gauge)
Confirmed no PII: entity names, diff content, user identifiers — none
flow into the metric layer. Only counts and shape tags.
T050 — Cross-coupling sanity test (unit-scope): asserts
_METRIC_PREFIX == "superset.activity_view" so a code review catches
accidental drift from sc-103156's eventual "superset.versioning.*"
sibling namespace. Both endpoint families belong to one Grafana panel
under "superset.versioning.* OR superset.activity_view.*".
Full suite: 66 unit + 35 integration + 1 xfailed (T044 sc-103156
restore-kind dependency, unchanged).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Surfaced by the SQLAlchemy review (Warning #1) and required before
T045's p95 perf budget can be met on dashboards with many historical
dataset edits.
Before: _decorate_records fired one COUNT(DISTINCT slice_id) query
per related dataset record via _compute_impact → _count_dashboard_
charts_pointing_at_dataset_at_tx. For a dashboard activity stream
with N dataset-edit records, that's N round-trips to compute impacts.
After: _decorate_records collects the distinct (dataset_id, target_tx)
pairs once via _collect_impact_pairs, fires a single batched query
via _batch_chart_counts that pulls the (slice, dataset, validity-
window) state for every relevant slice, and filters by validity in
Python per pair. The result is a {(dataset_id, target_tx): count}
mapping consumed by the new pure helper _impact_for_record per row.
Round-trip count drops from O(N records) to O(1) for the impact
calculation. The SQL stays small and dialect-portable — same per-kind
IN-clause + Python validity filter pattern as _fetch_change_records.
Trade-off documented in _batch_chart_counts' docstring: the SELECT
pulls (slice_id, datasource_id, two validity-window pairs) for every
slice ever on the dashboard whose dataset matches one of the
requested dataset_ids. For a busy dashboard with 100 slices and 50
versions each, that's ~5000 rows into Python vs N small COUNT scans.
For N > ~5 (which is typical) the batch wins.
Removed:
* _compute_impact (replaced by _impact_for_record + _collect_impact_pairs)
* _count_dashboard_charts_pointing_at_dataset_at_tx (replaced by
_batch_chart_counts)
Test changes: the three _compute_impact unit tests (no-impact paths)
become six _impact_for_record tests (positive count + four no-impact
paths + zero-count → None). Five new _collect_impact_pairs tests
cover dashboard/chart/dataset path branching plus dedupe and empty.
Full suite: 27/27 integration + 65/65 unit (was 57; +8 from the
restructure). No semantic regression on either side of the cut.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five structural changes — no behaviour change — applied as one commit
per the tidy-first discipline. All from the clean-code review of
e4070a4716.
* Tidy 1 — Fix _fetch_change_records sort key. Was calling .timestamp()
on issued_at with an `else 0` fallback; the fallback was dead defense
(column is non-null per sc-103156 schema) and .timestamp() introduces
non-determinism on tz-naive datetimes. Sort on the datetimes directly.
* Tidy 2 — parse_activity_query_params now raises ActivityParamsError
(subclass of ValueError) instead of returning (Optional[dict],
Optional[str]). The tuple was forcing every caller into a defensive
`if error or params is None: return self.response_400(message=error
or "Invalid query parameters")`. The new shape — `try: params =
parse(...); except ActivityParamsError as exc: return
response_400(str(exc))` — is shorter, type-safe, and the contract
is enforced at the boundary.
* Tidy 3 — Test helper now uses Flask client's query_string= parameter
instead of f-string concatenation. Handles URL-encoding correctly
for the day a test passes a value containing & / = / + / etc.
* Tidy 4 — get_activity pipeline collapses to a single rolling
`records` variable instead of the mid-stream `raw / visible_raw /
enriched / visible` naming. Each function call's name documents
what the step does; no intermediate variable names needed.
* Tidy 5 — Extracted four per-parameter parsers: _parse_optional_iso,
_parse_include, _parse_page, _parse_page_size. The "parse one
parameter" concept now has a name. Cost: four small helpers (each
10-15 lines, one job). Benefit: parse_activity_query_params is a
10-line table-driven dispatcher.
Test changes: parser unit tests now use pytest.raises(ActivityParamsError)
instead of unpacking the (params, error) tuple. Added one test
confirming ActivityParamsError subclasses ValueError so the standard
library exception hierarchy still catches it. Total unit tests: 57.
Integration tests still 10/10 green.
Deferred per the review: the UUID-parse + entity-find + ownership-
check dance is duplicated between activity / list_versions / get_version
and will grow to T028 / T033. Refactor when T028 lands — three real
callers > one prospective one.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements the dashboard cross-entity activity stream (US1 MVP).
Closes T015-T018, T023-T024, T026 from sc-107283 tasks.md. T019-T022
(complex fixture choreography) and T025 (RBAC fixture for restricted
chart access) deferred to a follow-up; T025's logical coverage is
provided by the new unit tests for _can_read kind dispatch.
* T015 Rename _dashboard_related_scope → _resolve_dashboard_scope and
_chart_related_scope → _resolve_chart_scope, parallel to _resolve_scope
/ _resolve_path_entity. Aligns the codebase with the task spec names.
* T016 New activity(uuid_str) method on DashboardRestApi:
@expose("/<uuid_str>/activity/"), @protect + @safe + @statsd_metrics
+ @event_logger. Same row-level ownership check pattern as
/versions/. Registered in include_route_methods and mapped to "write"
in MODEL_API_RW_METHOD_PERMISSION_MAP so the can_write Dashboard
permission gates access (Alpha-non-owner gets 403 from
raise_for_ownership, not 404 from missing permission).
* New parse_activity_query_params helper in activity.py — shared parser
for since/until/include/page/page_size used by all three endpoint
families. Tolerates the Z suffix Python <3.11 fromisoformat rejects.
Silently clamps page_size to the contract max instead of rejecting.
Two correctness/scale bugs found and fixed under integration-test load:
* SQLite SQLITE_MAX_EXPR_DEPTH (1000) was tripping on dashboards with
many slices × many historical attachment windows. _fetch_change_records
used to emit one OR-clause per (entity_kind, entity_id, window) tuple;
now it issues one SELECT per kind with entity_id IN (...) and filters
by exact windows in Python via _row_within_any_window. SQL shape is
proportional to the number of kinds (≤3); the per-entity window
precision is preserved.
* _merge_entity_windows now unions overlapping/touching windows within
each entity via _union_windows. Sequential fixture loads create many
redundant Continuum shadow rows; without merging, the unfiltered
windows still produce many OR branches downstream.
* Pipeline-ordering fix in get_activity: visibility filter runs BEFORE
decoration. Decoration strips entity_id (not in API contract) and the
filter needs it — and dropping invisible records early also avoids
paying for name lookup + tombstone probes on records the requester
can't see (AV-008's silent-filter contract).
Tests:
* tests/integration_tests/versioning/activity_view_tests.py — 10 tests
for TestDashboardActivityView covering: 404 for unknown UUID, 400 for
malformed UUID / invalid include / invalid since, 403 for non-owner,
200 envelope smoke, chart-edit-appears-as-related, include=self
filter, include=related filter, page_size clamping.
* tests/unit_tests/versioning/test_activity.py — grew from 30 to 56
tests. New coverage: parse_activity_query_params (7 cases), _can_read
per-kind dispatch (4 cases — covers T025 at unit scope), _union_windows
(9 parametrized cases), _merge_entity_windows window-union case,
_row_within_any_window (6 cases).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five structural changes — no behaviour change — applied as one commit
per the tidy-first discipline (separate from the prior feat commits
that landed the primitives and orchestrator):
* Add tests/unit_tests/versioning/test_activity.py — 30 tests for the
pure-function helpers: _intersect_windows (11 boundary cases),
_resolve_scope branching (per kind + per include mode), entity-
window merging, AV-012 summary headlines, changed_by projection,
kind-translation round-trip, _can_read fall-through, _compute_impact
no-impact paths. Runs in <500ms, no DB, no Flask. The DB-touching
helpers wait for the integration suite (Phase 3+).
* Hoist _datasets_used_by_chart(slice_id) out of the inner attachment-
window loop in _dashboard_related_scope. Previously a chart with N
attachment windows fired N queries for identical data; now once per
slice. Removes a perf cliff at the historical-rich case the spec
explicitly calls out (US1 AS-3).
* Delete the unused requesting_user parameter from
_filter_records_by_visibility and from get_activity. It was never
threaded through to the security manager calls (which read the user
from Flask-Login implicitly) — speculative future-shape that the
reader had to mentally trace through. If CLI/Celery bypass becomes
necessary, add it then with a real call site.
* Delete the unused module-level logger. No observability lands here
until T037/T038; reintroduce when those tasks add real logger calls.
* Fix the file docstring — it claimed _resolve_version_tables was
reused; it isn't. Trimmed to (find_active_by_uuid,
derive_version_uuid).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
