Skip to content

ref(dynamic-sampling): remove all usage of custom ds rules in code#110979

Merged
shellmayr merged 10 commits into
masterfrom
shellmayr/ref/remove-usage-of-custom-ds-rules-in-code
Mar 31, 2026
Merged

ref(dynamic-sampling): remove all usage of custom ds rules in code#110979
shellmayr merged 10 commits into
masterfrom
shellmayr/ref/remove-usage-of-custom-ds-rules-in-code

Conversation

@shellmayr
Copy link
Copy Markdown
Member

  • Remove rule hash function
  • Remove constants
  • Remove all methods on custom dynamic sampling rules before removal of the model

Contributes to TET-1957

@linear-code
Copy link
Copy Markdown

linear-code Bot commented Mar 18, 2026

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Mar 18, 2026
@shellmayr shellmayr marked this pull request as ready for review March 18, 2026 14:01
@shellmayr shellmayr requested review from a team as code owners March 18, 2026 14:01
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Autofix Details

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Removed constant still imported causing runtime ImportError
    • Removed the import of CUSTOM_RULE_START and its reference in RESERVED_IDS dictionary since the constant was removed from sentry.models.dynamicsampling.

Create PR

Or push these changes by commenting:

@cursor push 8bd6f6a9c0
Preview (8bd6f6a9c0)
diff --git a/src/sentry/dynamic_sampling/rules/utils.py b/src/sentry/dynamic_sampling/rules/utils.py
--- a/src/sentry/dynamic_sampling/rules/utils.py
+++ b/src/sentry/dynamic_sampling/rules/utils.py
@@ -6,7 +6,6 @@
 from django.conf import settings
 from redis import StrictRedis
 
-from sentry.models.dynamicsampling import CUSTOM_RULE_START
 from sentry.relay.types import RuleCondition
 from sentry.utils import redis
 
@@ -73,7 +72,6 @@
     RuleType.MINIMUM_SAMPLE_RATE_RULE: 1006,
     RuleType.BOOST_LOW_VOLUME_TRANSACTIONS_RULE: 1400,
     RuleType.BOOST_LATEST_RELEASES_RULE: 1500,
-    RuleType.CUSTOM_RULE: CUSTOM_RULE_START,
 }
 REVERSE_RESERVED_IDS = {value: key for key, value in RESERVED_IDS.items()}

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

Comment thread src/sentry/dynamic_sampling/rules/utils.py
@github-actions
Copy link
Copy Markdown
Contributor

Backend Test Failures

Failures on 199ca1d in this run:

tests/sentry/backup/test_exhaustive.py::ExhaustiveTests::test_exhaustive_dirty_pkslog
tests/sentry/backup/test_exhaustive.py:39: in test_exhaustive_dirty_pks
    verify_models_in_output(expected_models, actual)
tests/sentry/backup/__init__.py:39: in verify_models_in_output
    raise AssertionError(
E   AssertionError: Some `expected_models` entries were not found: ['sentry.customdynamicsamplingrule', 'sentry.customdynamicsamplingruleproject']
E   
E               If you are seeing this in CI, it means that this test produced an `export.json` backup
E               file that was missing the above models. This check is in place to ensure that ALL models
E               of a certain category are covered by this particular test - by omitting a certain kind
E               of model from the backup output entirely, we end up in a situation where backing up the
E               model in question to JSON is untested.
E   
E               To fix this, you'll need to modify the body of the test to add at least one of these
E               models to the database before exporting. The process for doing so is test-specific, but
E               if the test body contains a fixture factory like `self.create_exhaustive_...`, that
E               function will be a good place to start. If it does not, you can just write the model to
E               the database at the appropriate point in the test: `MyNewModel.objects.create(...)`.
tests/sentry/backup/test_exhaustive.py::ExhaustiveTests::test_uniquenesslog
tests/sentry/backup/test_exhaustive.py:63: in test_uniqueness
    verify_models_in_output(expected_models, actual)
tests/sentry/backup/__init__.py:39: in verify_models_in_output
    raise AssertionError(
E   AssertionError: Some `expected_models` entries were not found: ['sentry.customdynamicsamplingrule', 'sentry.customdynamicsamplingruleproject']
E   
E               If you are seeing this in CI, it means that this test produced an `export.json` backup
E               file that was missing the above models. This check is in place to ensure that ALL models
E               of a certain category are covered by this particular test - by omitting a certain kind
E               of model from the backup output entirely, we end up in a situation where backing up the
E               model in question to JSON is untested.
E   
E               To fix this, you'll need to modify the body of the test to add at least one of these
E               models to the database before exporting. The process for doing so is test-specific, but
E               if the test body contains a fixture factory like `self.create_exhaustive_...`, that
E               function will be a good place to start. If it does not, you can just write the model to
E               the database at the appropriate point in the test: `MyNewModel.objects.create(...)`.
tests/sentry/backup/test_exports.py::ScopingTests::test_global_export_scopinglog
tests/sentry/backup/test_exports.py:399: in test_global_export_scoping
    self.verify_model_inclusion(unencrypted, ExportScope.Global)
tests/sentry/backup/test_exports.py:182: in verify_model_inclusion
    raise AssertionError(
E   AssertionError: The following models were not included in the export: ${<class 'sentry.models.dynamicsampling.CustomDynamicSamplingRuleProject'>, <class 'sentry.models.dynamicsampling.CustomDynamicSamplingRule'>}; this is despite it being included in at least one of the following relocation scopes: {<RelocationScope.Organization: 3>, <RelocationScope.User: 2>, <RelocationScope.Config: 4>, <RelocationScope.Global: 5>}
tests/sentry/backup/test_exports.py::ScopingTests::test_organization_export_scopinglog
tests/sentry/backup/test_exports.py:263: in test_organization_export_scoping
    self.verify_model_inclusion(unencrypted, ExportScope.Organization)
tests/sentry/backup/test_exports.py:182: in verify_model_inclusion
    raise AssertionError(
E   AssertionError: The following models were not included in the export: ${<class 'sentry.models.dynamicsampling.CustomDynamicSamplingRuleProject'>, <class 'sentry.models.dynamicsampling.CustomDynamicSamplingRule'>}; this is despite it being included in at least one of the following relocation scopes: {<RelocationScope.Organization: 3>, <RelocationScope.User: 2>}
tests/sentry/backup/test_imports.py::ScopingTests::test_global_import_scopinglog
tests/sentry/backup/test_imports.py:885: in test_global_import_scoping
    self.verify_model_inclusion(ImportScope.Global)
tests/sentry/backup/test_imports.py:813: in verify_model_inclusion
    assert model.objects.count() > 0
E   AssertionError: assert 0 > 0
E    +  where 0 = <bound method QuerySet.count of <sentry.db.models.manager.base.BaseManager object at 0x7f2e8d5c38c0>>()
E    +    where <bound method QuerySet.count of <sentry.db.models.manager.base.BaseManager object at 0x7f2e8d5c38c0>> = <sentry.db.models.manager.base.BaseManager object at 0x7f2e8d5c38c0>.count
E    +      where <sentry.db.models.manager.base.BaseManager object at 0x7f2e8d5c38c0> = <class 'sentry.models.dynamicsampling.CustomDynamicSamplingRuleProject'>.objects
tests/sentry/backup/test_imports.py::ScopingTests::test_organization_import_scopinglog
tests/sentry/backup/test_imports.py:845: in test_organization_import_scoping
    self.verify_model_inclusion(ImportScope.Organization)
tests/sentry/backup/test_imports.py:813: in verify_model_inclusion
    assert model.objects.count() > 0
E   AssertionError: assert 0 > 0
E    +  where 0 = <bound method QuerySet.count of <sentry.db.models.manager.base.BaseManager object at 0x7f2e8d5c38c0>>()
E    +    where <bound method QuerySet.count of <sentry.db.models.manager.base.BaseManager object at 0x7f2e8d5c38c0>> = <sentry.db.models.manager.base.BaseManager object at 0x7f2e8d5c38c0>.count
E    +      where <sentry.db.models.manager.base.BaseManager object at 0x7f2e8d5c38c0> = <class 'sentry.models.dynamicsampling.CustomDynamicSamplingRuleProject'>.objects

Comment thread src/sentry/organizations/services/organization/impl.py
@shellmayr shellmayr force-pushed the shellmayr/ref/remove-usage-of-custom-ds-rules-in-code branch from 1ba8b2e to b180ec6 Compare March 19, 2026 13:44
@shellmayr shellmayr requested a review from a team as a code owner March 19, 2026 13:44
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix prepared a fix for the issue found in the latest run.

  • ✅ Fixed: Model removed from merge coverage but still has User FK
    • Added CustomDynamicSamplingRule back to @expect_models decorators, model_list, and set created_by_id=owner_id in the fixture to ensure the model is properly tested for user merge coverage.

Create PR

Or push these changes by commenting:

@cursor push aff5a71e10
Preview (aff5a71e10)
diff --git a/src/sentry/organizations/services/organization/impl.py b/src/sentry/organizations/services/organization/impl.py
--- a/src/sentry/organizations/services/organization/impl.py
+++ b/src/sentry/organizations/services/organization/impl.py
@@ -20,6 +20,7 @@
 from sentry.incidents.models.incident import IncidentActivity
 from sentry.models.activity import Activity
 from sentry.models.dashboard import Dashboard, DashboardFavoriteUser
+from sentry.models.dynamicsampling import CustomDynamicSamplingRule
 from sentry.models.groupassignee import GroupAssignee
 from sentry.models.groupbookmark import GroupBookmark
 from sentry.models.groupsearchview import GroupSearchView
@@ -580,6 +581,7 @@
                 Activity,
                 AlertRule,
                 AlertRuleActivity,
+                CustomDynamicSamplingRule,
                 Dashboard,
                 DashboardFavoriteUser,
                 GroupAssignee,

diff --git a/src/sentry/testutils/helpers/backups.py b/src/sentry/testutils/helpers/backups.py
--- a/src/sentry/testutils/helpers/backups.py
+++ b/src/sentry/testutils/helpers/backups.py
@@ -826,6 +826,7 @@
             num_samples=100,
             condition_hash="abc123def456abc123def456abc123def4560000",
             sample_rate=0.5,
+            created_by_id=owner_id,
         )
         CustomDynamicSamplingRuleProject.objects.create(
             custom_dynamic_sampling_rule=custom_rule,

diff --git a/tests/sentry/users/models/test_user.py b/tests/sentry/users/models/test_user.py
--- a/tests/sentry/users/models/test_user.py
+++ b/tests/sentry/users/models/test_user.py
@@ -13,6 +13,7 @@
 from sentry.models.activity import Activity
 from sentry.models.authidentity import AuthIdentity
 from sentry.models.dashboard import Dashboard, DashboardFavoriteUser
+from sentry.models.dynamicsampling import CustomDynamicSamplingRule
 from sentry.models.groupassignee import GroupAssignee
 from sentry.models.groupbookmark import GroupBookmark
 from sentry.models.groupsearchview import GroupSearchView
@@ -382,6 +383,7 @@
         Activity,
         AlertRule,
         AlertRuleActivity,
+        CustomDynamicSamplingRule,
         Dashboard,
         DashboardFavoriteUser,
         GroupAssignee,
@@ -425,6 +427,7 @@
         Activity,
         AlertRule,
         AlertRuleActivity,
+        CustomDynamicSamplingRule,
         Dashboard,
         DashboardFavoriteUser,
         GroupAssignee,

This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.

Comment thread tests/sentry/users/models/test_user.py
@shellmayr shellmayr merged commit 44531be into master Mar 31, 2026
107 checks passed
@shellmayr shellmayr deleted the shellmayr/ref/remove-usage-of-custom-ds-rules-in-code branch March 31, 2026 10:44
@shellmayr shellmayr added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Mar 31, 2026
@getsentry-bot
Copy link
Copy Markdown
Contributor

PR reverted: 005905e

getsentry-bot added a commit that referenced this pull request Mar 31, 2026
… code (#110979)"

This reverts commit 44531be.

Co-authored-by: shellmayr <6788060+shellmayr@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

Backend Test Failures

Failures on 44531be in this run:

tests/sentry/backup/test_coverage.py::test_all_eligible_organization_scoped_models_tested_for_user_mergelog
[gw1] linux -- Python 3.13.1 /home/runner/work/sentry/sentry/.venv/bin/python3
tests/sentry/backup/test_coverage.py:135: in test_all_eligible_organization_scoped_models_tested_for_user_merge
    assert not {str(u) for u in untested}, (
E   AssertionError: The aforementioned models are not covered in the `ORG_MEMBER_MERGE` backup tests; please go to `tests/sentry/models/test_user.py::UserMergeToTest` and make sure at least one test in the suite contains covers each of the missing models.
E   assert not {'sentry.customdynamicsamplingrule'}

dashed pushed a commit that referenced this pull request Apr 1, 2026
…110979)

- Remove rule hash function
- Remove constants
- Remove all methods on custom dynamic sampling rules before removal of
the model

Contributes to TET-1957
dashed pushed a commit that referenced this pull request Apr 1, 2026
… code (#110979)"

This reverts commit 44531be.

Co-authored-by: shellmayr <6788060+shellmayr@users.noreply.github.com>
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 15, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components Trigger: Revert Add to a merged PR to revert it (skips CI)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants