feat(nimbus): simplify collision warnings#15468
Open
jaredlockhart wants to merge 17 commits into
Open
Conversation
jaredlockhart
changed the title
Collaborator
Author
|
Before: 
After: 
|
Because * Audience-overlap warnings missed the case where a draft rollout shares a feature with a single-feature LIVE rollout that has different targeting — the failure mode that hit vpn-limited-rollout. * The existing feature_has_live_multifeature_experiments check required the live recipe to be multi-feature, and didn't enforce slot-pool symmetry between experiments and rollouts. This commit * Renames feature_has_live_multifeature_experiments to feature_has_live_overlapping_deliveries and drops the n_feature_configs > 1 filter * Adds is_rollout=self.is_rollout so experiments and rollouts only warn against same-polarity live deliveries (separate slot pools) * Splits the warning text: experiments keep the population-dilution wording; rollouts get a new message explaining that the earliest-published rollout claims the contested slot * Renames the existing tests to match and adds two new tests covering the rollout-vs-rollout collision and the experiment/rollout symmetry Fixes #15467
…ings
Because
* When the warning fires, the user needs to know which feature is the
contention point — naming the colliding delivery alone leaves them
guessing which of the draft's features is the slot in conflict.
This commit
* Changes feature_has_live_overlapping_deliveries to return a list of
{slug, shared_features} dicts instead of bare slugs
* Renders each entry in the warning card as `<slug> (Shares <feat1>,
<feat2>)` when details are available, falling back to bare slug
rendering for warnings that don't carry per-entry detail
* Updates the audience_overlap_warnings dedup helper and the affected
tests to the new shape
Because * The previous warning tests mocked NimbusExperiment properties (excluded_live_deliveries, live_experiments_in_namespace, feature_has_live_overlapping_deliveries) directly, so they only verified the audience_overlap_warnings dict-assembly wiring — never the underlying ORM queries that decide what's actually overlapping. * Mocking internal model properties hides regressions in the very query logic the warnings exist to surface (this is how the rollout-vs-rollout single-feature gap survived). This commit * Replaces mock-based test_excluding_experiments_warning, test_live_feature_overlap_warning, and test_multiple_warnings with factory-built experiments that trigger the real property logic * Drops test_live_rollout_feature_overlap_warning (mock) and test_live_experiments_bucket_warning (mock); the rollout case is covered by the existing factory test test_rollout_feature_overlap_with_live_rollout_different_targeting, and the namespace warning never fires in isolation due to the feature_overlap dedup logic — test_multiple_warnings exercises it in the realistic co-occurring shape * Strengthens test_rollout_feature_overlap_with_live_rollout_different_targeting to assert the rollout-specific text constant
… primitive
Because
* The audience-overlap warning system grew organically: five separate
properties (excluded_live_deliveries, live_experiments_in_namespace,
feature_has_live_overlapping_deliveries, rollout_conflict_warning,
pref_targeting_rollout_collision_warning) each ran their own query
and surfaced their own warning card, with a fragile substring-based
dedup hack.
* All five reduce to one underlying signal — a live recipe of the same
polarity that shares a non-coenrolling feature and overlapping
targeting — with annotations layered on top (same namespace,
matching configuration, sets the same preference).
* The previous implementation also missed two real cases: features
that opt into coenrollment via allowCoenrollment shouldn't trigger
warnings, and there's no signal of how much of the eligible
audience a draft would actually lose to colliding live deliveries.
This commit
* Adds collision_warnings as the unified primitive: one query
returning per-delivery {slug, reasons, publish_date_relation,
estimated_loss}, with reasons annotated for shares-feature,
shares-namespace, matching-configuration, and sets-same-preference
* Filters out features that allow coenrollment in the experiment's
Firefox version range
* Adds publish-date directionality for rollout-vs-rollout collisions
(blocked_by vs would_block) so the user sees who claims the slot
* Computes a coarse estimated_loss_percent: 100% per blocking rollout,
population_percent per colliding experiment, capped at 100%
* Removes the now-redundant individual properties (live_experiments_in_namespace,
rollout_conflict_warning, pref_targeting_rollout_collision_warning,
feature_has_live_overlapping_deliveries) and the substring dedup hack
* Removes the now-orphaned warning text constants
(EXCLUDING_EXPERIMENTS_WARNING, LIVE_EXPERIMENTS_BUCKET_WARNING,
LIVE_FEATURE_OVERLAP_WARNING, LIVE_ROLLOUT_FEATURE_OVERLAP_WARNING,
ERROR_ROLLOUT_BUCKET_EXISTS, PREF_TARGETING_WARNING,
ROLLOUT_BUCKET_WARNING) since the unified card composes its own text
* Updates the warning template to render one card with per-entry
reason badges (each with a tooltip), feature links, an estimated-loss
banner, and a "blocked by" / "would block" arrow indicator
* Replaces the 11-test cluster of mock-based property tests with
factory-based tests that exercise collision_warnings directly and
cover the new annotations, coenrollment skip, publish-date
directionality, and loss estimate
… unified card Because * The rollout-version and multichannel warnings were being rendered as their own standalone cards, but conceptually they belong with the collision warning — all of them are issues with the draft that may affect enrollment. * A single unified card is easier to scan than three separate cards stacked. This commit * Combines collision entries and self-state issues (rollout-version, multichannel) into a single audience_overlap_warnings card * Adds self_issues alongside entries in the warning dict; template renders them as additional list items with their own tooltip * Adapts the card header text based on what's inside (collisions only, self-issues only, or both) * Adds an integration test covering the both-collisions-and-self-issues case
Because * The coarse population-product loss estimate doesn't reflect what's happening in the wild — it ignores audience-overlap weighting and treats every blocked-by-rollout case as a 100% loss. A more trustworthy figure should come from monitoring telemetry, not from static config arithmetic. This commit * Removes estimated_loss from _slot_collisions, estimated_loss_percent from collision_warnings and audience_overlap_warnings, and the "Estimated loss" line from the warning template * Drops the now-stale estimated_loss assertions and the test_collision_warnings_estimated_loss_caps_at_100_percent test * Keeps the publish_date_relation indicator (blocked_by / would_block) since that's structurally correct without depending on a noisy numeric estimate
…iment Because * Per the SDK code (enrollment.rs:809-871), evolve_enrollment_recipes runs the same machinery for both experiments and rollouts: sort_experiments_by_published_date then bucket-sample, with the earlier-published recipe claiming the contested feature slot and the loser getting FeatureConflict. The previous "if self.is_rollout" gate on the publish-date directionality was wrong. * Cross-polarity (experiment vs rollout sharing a feature) is a separate value-override mechanism in map_features_by_feature_id — both deliveries still enroll, the experiment's value just takes precedence at value-resolution time. That's not an enrollment refusal so it stays out of the warning system. This commit * Drops the is_rollout gate on publish_date_relation so blocked_by / would_block surfaces for experiment-vs-experiment too * Restates the "Shares feature" tooltip to describe the SDK behavior uniformly (no more rollout-specific vs experiment-specific text) * Adds a test that publish-date directionality fires for an experiment-vs-experiment collision
Because * The directionality is moot: audience_overlap_warnings only fires on DRAFT/PREVIEW status. A live recipe by definition published before the draft, so the live one always claims the feature slot. The publish-date comparison is just a roundabout way of saying "live beats draft." This commit * Removes the publish_date_relation field from _slot_collisions, collision_warnings, audience_overlap_warnings, and the template * Drops the "blocked by" / "would block" arrow indicator on each entry (the warning header already conveys "these live deliveries will conflict with this draft") * Restates the "Shares feature" tooltip in plain terms — the live delivery already holds the slot * Drops the now-dead would_block test and the experiment-publish-date directionality test (the publish-date branch is gone entirely)
…argeting configs Because * Two deliveries on different channels (e.g., release vs nightly) have disjoint audiences and won't collide in practice; warning on them is noise. * Two deliveries with different non-empty advanced-targeting (named targeting_config_slug) may also have disjoint audiences. We can't decide JEXL contents, but the named-targeting-slug equality is a practical signal: same slug = audiences overlap on that axis, different slug = treat as disjoint. * Firefox version range was considered and dropped — in practice recipes set a minimum and successors trend higher, so version ranges almost always overlap. This commit * Adds _channels_overlap and _targeting_configs_overlap helpers and applies them in _slot_collisions after the existing audience_overlap filter * Empty channels or NO_CHANNEL on either side is treated as "matches all" (no constraint) * Empty targeting_config_slug or NO_TARGETING on either side is treated as "matches all" * Adds tests covering disjoint channels (desktop and non-desktop), empty channels, disjoint targeting configs, and NO_TARGETING intersecting with anything
Because * Long-form user-facing strings (reason labels, tooltip details, card header text) were inlined in audience_overlap_warnings and collision_warnings. The rest of the warning system keeps its strings in nimbus_ui/constants.py — these were the outlier. This commit * Adds COLLISION_LABEL_*, COLLISION_DETAIL_*, COLLISION_SELF_ISSUE_*, and COLLISION_CARD_HEADER_* constants on NimbusUIConstants * Replaces the inline strings in collision_warnings, audience_overlap_warnings, and _collision_card_header with constant references * No behavior change — strings are byte-identical
Because * The collision card currently has only one card-level "Learn more" link pointing at the generic audience-overlap warnings page. Each reason actually has more specific documentation that explains the particular mechanism and consequence. This commit * Adds COLLISION_LEARN_MORE_* URL constants on NimbusUIConstants for excluded, shares-feature, shares-audience, matching-configuration, sets-same-preference, version-below-minimum, and multichannel * Wires learn_more_url onto each reason and self-issue dict in collision_warnings and audience_overlap_warnings * Template renders a small ↗ link next to each reason's ? tooltip when a learn_more_url is set * Card-level "Learn more" remains as the generic landing-page fallback
…doc links The arrow glyph wasn't clear enough as an affordance; the codebase already uses the literal text "Learn more" at the card level. Match that for the per-reason and per-self-issue doc links too.
Three of the per-reason "Learn more" links pointed at tangentially related docs rather than the warning case itself: * Shares feature → enrollment state machine FeatureConflict (SDK runtime mechanism, not draft-time warning) * Shares an audience → bucketing fundamentals (what a namespace is, not why sharing one matters) * Targeting multiple channels → generic workflow page (no anchored section) Drop those links rather than send users to off-topic pages. The card-level "Learn more" still points at the generic audience-overlap landing as the catch-all. Followup tracked in the task folder to add proper docs and re-link.
…nd shares-audience reasons Main had every audience-overlap warning linking to the generic /advanced/warnings#audience-overlap landing. Three of those got tighter anchors in this PR (excluded, sets-same-preference, rollout version) and two ended up with no per-reason link after dropping tangential targets: * Shares feature * Shares an audience The card-level "Learn more" still routes to the generic landing, but per-reason discoverability regressed vs main. Wire the generic AUDIENCE_OVERLAP_WARNING URL back onto those two reasons to preserve the link parity, with a note in the task folder to swap to tighter anchors once experimenter-docs gets per-case sections.
Because * collision_warnings was doing three things at once — excluded deliveries, slot collisions, and pref collisions — interleaved inside one method. The wrapper logic (build slug→entry dict, sort) was scattered across the body. * The "is targeting config X overlap with Y" predicate had grown a redundant has/has-not ladder when a single "matches all" check would do. * The collision-warning helpers were the only methods in the class prefixed with `_`; the convention isn't used anywhere else. This commit * Splits collision_warnings into excluded_collision_reasons, slot_collision_reasons, pref_collision_reasons — each returns (slug, reason) pairs. The wrapper merges them by slug. * Rewrites targeting_configs_overlap as a single boolean expression. * Drops the leading-underscore prefix from the new collision helpers to match the rest of the class.
Because
* The same `{"", NO_CHANNEL}` literal was built four times across the
desktop and non-desktop branches.
This commit
* Hoists it to a single `matches_all` variable at the top of the
method.
Because * The three existing tests all exercise paths that produce a warning card. CI flagged the clean-draft "no collisions, no self-issues" branch as uncovered. This commit * Adds a draft with no contending live recipes and no self-issues, asserts audience_overlap_warnings is [].
jaredlockhart
requested review from
freshstrangemusic,
mikewilli and
yashikakhurana
as code owners
yashikakhurana
approved these changes
May 15, 2026
Contributor
yashikakhurana
left a comment
yashikakhurana
left a comment
There was a problem hiding this comment.
Lets roll this over, all looks good to me, nice test coverage, thank you @jaredlockhart
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
2 participants
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.
Because
This PR
collision_warningsprimitive: same-polarity live recipes sharing a non-coenrolling feature with overlapping locale/country/language/channel/targeting.?tooltips replace the verbose warning-text constants.Validated against last week's SDK
FeatureConflicttelemetry: 100% recall on top-200 pairs by client volume.Fixes #15467