Skip to content

test: pin query-count baselines for tag inheritance hot paths#14811

Merged
valentijnscholten merged 4 commits intoDefectDojo:devfrom
valentijnscholten:perf/tag-inheritance-baselines
May 7, 2026
Merged

test: pin query-count baselines for tag inheritance hot paths#14811
valentijnscholten merged 4 commits intoDefectDojo:devfrom
valentijnscholten:perf/tag-inheritance-baselines

Conversation

@valentijnscholten
Copy link
Copy Markdown
Member

@valentijnscholten valentijnscholten commented May 5, 2026

Summary

  • Adds unittests/test_tag_inheritance_perf.py with assertNumQueries baselines on the six hottest tag inheritance paths.
  • Numbers are pinned against current dev behavior so subsequent optimization PRs show up as concrete query-count reductions instead of relying on manual benchmarking.
  • The class is intentionally temporary — pins move down as the redesign work lands, and the file can be deleted (or the assertions relaxed to upper bounds) once the targets are met.

Hot paths covered

Scenario Pinned baseline
Product tag add → 100 findings 4758
Product tag remove → 100 findings 4540
Create 100 findings under inheritance 4025
Create 1 finding under inheritance 64
Finding remove inherited tag (sticky re-add) 44
Finding add user tag (sticky path) 17

Each test sets up its fixture outside the query-counting block, exercises one operation under assertNumQueries, and asserts a positive correctness check so query-count tightening cannot smuggle in a behavior bug.

Notes

  • Class uses @override_settings(CELERY_TASK_ALWAYS_EAGER=True, CELERY_TASK_EAGER_PROPAGATES=True) so dispatched Celery tasks execute synchronously in the test connection and their queries are captured. Product tag tests still call propagate_tags_on_product_sync(product) explicitly because the m2m_changed handler dispatches with countdown=5 and dojo_dispatch_task skips foreground execution when no current user is set in tests.
  • Existing tag inheritance suite (unittests.test_tag_inheritance, 36 tests) and bulk tag suite (unittests.test_tag_utils_bulk, 29 tests) continue to pass unchanged.
  • No production code is modified in this PR.

@valentijnscholten valentijnscholten added this to the 2.59.0 milestone May 5, 2026
@valentijnscholten valentijnscholten mentioned this pull request May 5, 2026
Merged
valentijnscholten added a commit to valentijnscholten/django-DefectDojo that referenced this pull request May 5, 2026
@valentijnscholten
perf(tags): bulk-propagate inherited tags + gate child post_save on c…
6b20f7a 
…reate

Replaces the per-row `.save()` loop in `propagate_tags_on_product_sync`
with bulk SQL through the existing tag-utils helpers. For every child
model (Engagement/Test/Finding/Endpoint/Location), reads current
inherited_tags in one query, computes the per-child diff against the
Product's tags, and applies adds/removes via `bulk_add_tag_mapping` and
the new `bulk_remove_tags_from_instances` helper. Both `tags` and
`inherited_tags` fields are kept in sync.

Also gates the per-child `inherit_tags_on_instance` post_save handler on
`created=True`. The previous behavior fired on every save (create OR
update), repeatedly re-applying inherited tags to children whose tag
state had not changed. Sticky enforcement on user-driven tag edits is
unchanged (still handled by `make_inherited_tags_sticky` on m2m_changed).

Pinned query-count baselines from PR DefectDojo#14811 drop accordingly:

  product_tag_add  -> 100 findings : 4758 -> 91   (~52x fewer queries)
  product_tag_remove -> 100 findings : 4540 -> 53 (~85x fewer queries)

Sticky and child-creation paths are unchanged in this PR. Phase B
targets those (centralized inheritance module + drop the duplicate
`inherited_tags` TagField).
valentijnscholten added a commit to valentijnscholten/django-DefectDojo that referenced this pull request May 5, 2026
@valentijnscholten
perf(tags): bulk-propagate inherited tags + gate child post_save on c…
9015ac8 
…reate

Replaces the per-row `.save()` loop in `propagate_tags_on_product_sync`
with bulk SQL through the existing tag-utils helpers. For every child
model (Engagement/Test/Finding/Endpoint/Location), reads current
inherited_tags in one query, computes the per-child diff against the
Product's tags, and applies adds/removes via `bulk_add_tag_mapping` and
the new `bulk_remove_tags_from_instances` helper. Both `tags` and
`inherited_tags` fields are kept in sync.

Also gates the per-child `inherit_tags_on_instance` post_save handler on
`created=True`. The previous behavior fired on every save (create OR
update), repeatedly re-applying inherited tags to children whose tag
state had not changed. Sticky enforcement on user-driven tag edits is
unchanged (still handled by `make_inherited_tags_sticky` on m2m_changed).

Pinned query-count baselines from PR DefectDojo#14811 drop accordingly:

  product_tag_add  -> 100 findings : 4758 -> 91   (~52x fewer queries)
  product_tag_remove -> 100 findings : 4540 -> 53 (~85x fewer queries)

Sticky and child-creation paths are unchanged in this PR. Phase B
targets those (centralized inheritance module + drop the duplicate
`inherited_tags` TagField).
mtesauro
mtesauro approved these changes May 6, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

valentijnscholten added a commit to valentijnscholten/django-DefectDojo that referenced this pull request May 6, 2026
@valentijnscholten
perf(tags): bulk-propagate inherited tags + gate child post_save on c…
64b0287 
…reate

Replaces the per-row `.save()` loop in `propagate_tags_on_product_sync`
with bulk SQL through the existing tag-utils helpers. For every child
model (Engagement/Test/Finding/Endpoint/Location), reads current
inherited_tags in one query, computes the per-child diff against the
Product's tags, and applies adds/removes via `bulk_add_tag_mapping` and
the new `bulk_remove_tags_from_instances` helper. Both `tags` and
`inherited_tags` fields are kept in sync.

Also gates the per-child `inherit_tags_on_instance` post_save handler on
`created=True`. The previous behavior fired on every save (create OR
update), repeatedly re-applying inherited tags to children whose tag
state had not changed. Sticky enforcement on user-driven tag edits is
unchanged (still handled by `make_inherited_tags_sticky` on m2m_changed).

Pinned query-count baselines from PR DefectDojo#14811 drop accordingly:

  product_tag_add  -> 100 findings : 4758 -> 91   (~52x fewer queries)
  product_tag_remove -> 100 findings : 4540 -> 53 (~85x fewer queries)

Sticky and child-creation paths are unchanged in this PR. Phase B
targets those (centralized inheritance module + drop the duplicate
`inherited_tags` TagField).
This was referenced May 6, 2026
Merged
Draft
Closed
@valentijnscholten
test: pin query-count baselines for tag inheritance hot paths
70b6adb 
Adds unittests/test_tag_inheritance_perf.py with assertNumQueries baselines
on the six hottest tag inheritance paths (Product tag add/remove propagating
to N findings, child create under inheritance, sticky enforcement on child
tag edits). Numbers are pinned against current `dev` behavior so subsequent
optimization work shows up as concrete query-count reductions instead of
relying on manual benchmarking.

The class is intentionally temporary: pins move down as the redesign work
lands and the file can be deleted once the targets are met.
@valentijnscholten
test: add Endpoint (V2) and Location (V3) propagation baselines
961b73e 
Extends the perf test class with two more pinned hot paths so all child
models exercised by `propagate_tags_on_product_sync` are covered:

  product_tag_add  -> 100 endpoints (V2) : 3958
  product_tag_remove -> 100 endpoints (V2): 3740
  product_tag_add  -> 100 locations (V3) : 4532
  product_tag_remove -> 100 locations (V3): 4307

Both V2 and V3 paths run regardless of the ambient `V3_FEATURE_LOCATIONS`
setting via per-test `@override_settings(...)`. CI matrix runs the suite
in both modes, so dynamic pin selection (`_pin(v2=..., v3=...)`) handles
the small per-mode count differences on the existing finding tests.
@valentijnscholten
test: add ZAP import/reimport baselines + V2/V3 variants for every sc…
acd87b3 
…enario

Two additions:

1. New TagInheritanceImportPerfBaselines class pins query counts for the
   importer hot path (production's heaviest tag-inheritance scenario).
   Both first-import and no-change-reimport are covered, each with V2
   and V3 method variants:

     zap_import_v2                : 1461
     zap_import_v3                : 1319
     zap_reimport_no_change_v2    : 77
     zap_reimport_no_change_v3    : 95

2. Restructures the existing baseline class so every scenario has both
   a _v2 and _v3 method variant via per-test @override_settings. The
   whole suite now runs both modes in a single invocation; no need to
   run twice with different DD_V3_FEATURE_LOCATIONS env.

Phase A leaves the importer numbers ~unchanged (importer hot loop is
creation-driven, not the bulk-propagation path Phase A targets). Phase
B's tag_inheritance.batch() context manager is the lever that lowers
these numbers.
@valentijnscholten
test: warm ContentType cache in tag inheritance perf baselines
c3e0412 
First V3 Location op in the class paid a one-time ContentType lookup,
producing a matrix-dependent off-by-one (V3-default-on CI: 4531;
V3-default-off CI + local: 4532). Match the warm-up pattern used in
test_importers_performance and pin EXPECTED_PRODUCT_TAG_ADD_100_LOCATIONS
to the post-warm value (4531).
@valentijnscholten valentijnscholten force-pushed the perf/tag-inheritance-baselines branch from 6391cb9 to c3e0412 Compare May 6, 2026 22:06
valentijnscholten added a commit to valentijnscholten/django-DefectDojo that referenced this pull request May 6, 2026
@valentijnscholten
perf(tags): bulk-propagate inherited tags + gate child post_save on c…
d2c4fc5 
…reate

Replaces the per-row `.save()` loop in `propagate_tags_on_product_sync`
with bulk SQL through the existing tag-utils helpers. For every child
model (Engagement/Test/Finding/Endpoint/Location), reads current
inherited_tags in one query, computes the per-child diff against the
Product's tags, and applies adds/removes via `bulk_add_tag_mapping` and
the new `bulk_remove_tags_from_instances` helper. Both `tags` and
`inherited_tags` fields are kept in sync.

Also gates the per-child `inherit_tags_on_instance` post_save handler on
`created=True`. The previous behavior fired on every save (create OR
update), repeatedly re-applying inherited tags to children whose tag
state had not changed. Sticky enforcement on user-driven tag edits is
unchanged (still handled by `make_inherited_tags_sticky` on m2m_changed).

Pinned query-count baselines from PR DefectDojo#14811 drop accordingly:

  product_tag_add  -> 100 findings : 4758 -> 91   (~52x fewer queries)
  product_tag_remove -> 100 findings : 4540 -> 53 (~85x fewer queries)

Sticky and child-creation paths are unchanged in this PR. Phase B
targets those (centralized inheritance module + drop the duplicate
`inherited_tags` TagField).
@valentijnscholten valentijnscholten merged commit 76a2323 into DefectDojo:dev May 7, 2026
158 checks passed
valentijnscholten added a commit to valentijnscholten/django-DefectDojo that referenced this pull request May 7, 2026
@valentijnscholten
perf(tags): bulk-propagate inherited tags + gate child post_save on c…
bd62203 
…reate

Replaces the per-row `.save()` loop in `propagate_tags_on_product_sync`
with bulk SQL through the existing tag-utils helpers. For every child
model (Engagement/Test/Finding/Endpoint/Location), reads current
inherited_tags in one query, computes the per-child diff against the
Product's tags, and applies adds/removes via `bulk_add_tag_mapping` and
the new `bulk_remove_tags_from_instances` helper. Both `tags` and
`inherited_tags` fields are kept in sync.

Also gates the per-child `inherit_tags_on_instance` post_save handler on
`created=True`. The previous behavior fired on every save (create OR
update), repeatedly re-applying inherited tags to children whose tag
state had not changed. Sticky enforcement on user-driven tag edits is
unchanged (still handled by `make_inherited_tags_sticky` on m2m_changed).

Pinned query-count baselines from PR DefectDojo#14811 drop accordingly:

  product_tag_add  -> 100 findings : 4758 -> 91   (~52x fewer queries)
  product_tag_remove -> 100 findings : 4540 -> 53 (~85x fewer queries)

Sticky and child-creation paths are unchanged in this PR. Phase B
targets those (centralized inheritance module + drop the duplicate
`inherited_tags` TagField).
mtesauro pushed a commit that referenced this pull request May 8, 2026
@valentijnscholten
perf(tags): bulk-propagate inherited tags + gate child post_save on c…
6788368 
…reate (#14812)

Replaces the per-row `.save()` loop in `propagate_tags_on_product_sync`
with bulk SQL through the existing tag-utils helpers. For every child
model (Engagement/Test/Finding/Endpoint/Location), reads current
inherited_tags in one query, computes the per-child diff against the
Product's tags, and applies adds/removes via `bulk_add_tag_mapping` and
the new `bulk_remove_tags_from_instances` helper. Both `tags` and
`inherited_tags` fields are kept in sync.

Also gates the per-child `inherit_tags_on_instance` post_save handler on
`created=True`. The previous behavior fired on every save (create OR
update), repeatedly re-applying inherited tags to children whose tag
state had not changed. Sticky enforcement on user-driven tag edits is
unchanged (still handled by `make_inherited_tags_sticky` on m2m_changed).

Pinned query-count baselines from PR #14811 drop accordingly:

  product_tag_add  -> 100 findings : 4758 -> 91   (~52x fewer queries)
  product_tag_remove -> 100 findings : 4540 -> 53 (~85x fewer queries)

Sticky and child-creation paths are unchanged in this PR. Phase B
targets those (centralized inheritance module + drop the duplicate
`inherited_tags` TagField).
mtesauro pushed a commit that referenced this pull request May 8, 2026
@valentijnscholten
perf(tags): centralize tag inheritance + replace signal disconnect wi…
7ac0d9d 
…th batch context manager (Phase B) (#14827)

* perf(tags): bulk-propagate inherited tags + gate child post_save on create

Replaces the per-row `.save()` loop in `propagate_tags_on_product_sync`
with bulk SQL through the existing tag-utils helpers. For every child
model (Engagement/Test/Finding/Endpoint/Location), reads current
inherited_tags in one query, computes the per-child diff against the
Product's tags, and applies adds/removes via `bulk_add_tag_mapping` and
the new `bulk_remove_tags_from_instances` helper. Both `tags` and
`inherited_tags` fields are kept in sync.

Also gates the per-child `inherit_tags_on_instance` post_save handler on
`created=True`. The previous behavior fired on every save (create OR
update), repeatedly re-applying inherited tags to children whose tag
state had not changed. Sticky enforcement on user-driven tag edits is
unchanged (still handled by `make_inherited_tags_sticky` on m2m_changed).

Pinned query-count baselines from PR #14811 drop accordingly:

  product_tag_add  -> 100 findings : 4758 -> 91   (~52x fewer queries)
  product_tag_remove -> 100 findings : 4540 -> 53 (~85x fewer queries)

Sticky and child-creation paths are unchanged in this PR. Phase B
targets those (centralized inheritance module + drop the duplicate
`inherited_tags` TagField).

* perf(tags): replace process-global signal disconnect with thread-local batch context

Adds dojo/tag_inheritance.py with a thread-local batch() context manager
and is_in_batch() predicate. While the calling thread is inside a batch,
make_inherited_tags_sticky (m2m_changed) early-returns; the calling code
takes responsibility for applying inheritance in bulk.

Replaces the previous pattern in dojo/importers/location_manager.py:556:

    disconnected = signals.m2m_changed.disconnect(
        make_inherited_tags_sticky, sender=Location.tags.through,
    )
    try:
        ...
    finally:
        if disconnected:
            signals.m2m_changed.connect(...)

Signal.disconnect mutates Django's process-global receiver list and is
not thread-safe. While disconnected, every thread/greenlet in the same
process loses sticky enforcement. Safe only under Celery --pool=prefork
and single-threaded gunicorn; broken under --pool=threads|gevent|eventlet,
gunicorn --threads >1, or ASGI threadpools. Also fragile on hard process
exit mid-import (handler stays disconnected for the rest of the process
lifetime).

The new batch context lives in threading.local(): each thread has its
own depth counter, the signal handler stays globally connected, and the
suppression decision is per-thread. No global mutation, no reconnect
hazard.

This is Phase B Stage 1. Subsequent stages will wrap the broader importer
orchestration in batch(), replace the duplicate inherited_tags TagField
with a JSON column, and drop _manage_inherited_tags / per-model
inherit_tags().

Pinned perf-test note: V3 zap_scan_import baseline rises 1243 -> 1263
(~1.6%). The previous process-global disconnect was narrower in scope
(Location.tags.through only); the batch context covers all child-tag
through-tables. Net trade is positive given the threading bug fix; full
Phase B reductions arrive in later stages.

* variable naming
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mtesauro mtesauro mtesauro approved these changes

@Maffooch Maffooch Maffooch approved these changes

@Jino-T Jino-T Jino-T approved these changes

@blakeaowens blakeaowens blakeaowens approved these changes

@paulOsinski paulOsinski Awaiting requested review from paulOsinski

Assignees

No one assigned

Labels

unittests

Projects

None yet

Milestone

2.59.0

Development

Successfully merging this pull request may close these issues.

5 participants

@valentijnscholten @mtesauro @Maffooch @Jino-T @blakeaowens