ref(utils): Various clarifications in `SafeRolloutComparator` code by lobsterkatie · Pull Request #115946 · getsentry/sentry

lobsterkatie · 2026-05-20T20:32:25Z

This makes some clarifying changes to the SafeRolloutComparator utility, based on my experience getting oriented to using it. No behavioral changes.

Rename the internal option-name-generation methods to make it clearer a) that "eval" means "run the experimental code" and b) what is being block- or allowlisted per callsite (either running the experiment or using the results of the experiment).
Expand the example code in the docstring.
Tweak docstrings and comments for various methods.
Rename a few other variables.
Remove TODOs about creating a dashboard, since that's been done.
Consolidate imports.
Create constants for option names in tests, just to cut down on verbosity.

thetruecpaul · 2026-05-21T18:17:41Z

-      3. Start rolling out the "evaluate experimental branch" option.
-      4. Monitor correctness through standard dashboard. (TODO @cpaul: build dashboard)
-      5. Start adding known-good callsites to the "use experimental branch" allowlist.
+      1. Set up your `SafeRolloutComparator` subclass (in Sentry) & options (in options automator).


How do we handle referencing internal repos (like options automator) in open-source repos?

Their existence isn't a secret - we talk about the automator here, here, and here, for example, and getsentry here, here, and here.

thetruecpaul · 2026-05-21T18:18:05Z

-      4. Monitor correctness through standard dashboard. (TODO @cpaul: build dashboard)
-      5. Start adding known-good callsites to the "use experimental branch" allowlist.
+      1. Set up your `SafeRolloutComparator` subclass (in Sentry) & options (in options automator).
+      2. Add code like that below your first callsite. (Further callsites can be added at any time.)


"Add code like that" — what does this mean?

Meant to be "code like that [that you can find] below," but I get how it can be read ambiguously. Changed to Use the comparator in your first callsite (see example below)..

thetruecpaul · 2026-05-21T18:19:31Z

+      1. Set up your `SafeRolloutComparator` subclass (in Sentry) & options (in options automator).
+      2. Add code like that below your first callsite. (Further callsites can be added at any time.)
+      3. Start rolling out the experiment by switching the "should run experiment" option to True
+         and increasing the sample rate option.


"increasing" is wrong here — default sample rate is 1.0 ==> 100%, so folks can OPTIONALLY set the sample rate option but don't have to.

Ah, right. Good catch. Changed to Start rolling out the experiment by switching the "should run experiment" option to True and, if you've set a sample rate option, increasing the sample rate. (If not set, the sample rate defaults to 100%.)

thetruecpaul · 2026-05-21T18:22:50Z

+         and increasing the sample rate option.
+      4. Monitor correctness using the metrics and optional mismatch logs emitted when the
+         experimental branch is run.
+      5. Start adding known-good callsites to the "use new data" allowlist.


The method is called should_use_experimental_data but the allowlist uses new — IMO we should unify on experimental vs new.

Used new for length reasons, but can switch it back to experimental.

thetruecpaul · 2026-05-21T18:25:54Z

+        use_experimental_data: bool,
+        is_exact_match: bool,
+        is_reasonable_match: bool | None,
+        is_experimental_data_nullish: bool | None,


Nit: I'd keep as null_result to keep consistency with what we're sending to DataDog.

I left all of the DD and logging labels alone so as not to break existing dashboards, etc, but I like "nullish" because is sort of the equivalent of "falsy" - an empty list isn't the same as an actual None, for example, the same way an empty string isn't the same as False.

thetruecpaul · 2026-05-21T18:27:04Z

        experimental_data: TData,
        callsite: str,
-        is_experimental_data_a_null_result: bool | None = None,
+        is_experimental_data_nullish: bool | None = None,


See above re: nullish

thetruecpaul · 2026-05-21T18:27:38Z

+        control_data_func: Callable[[], TData],
+        experimental_data_func: Callable[[], TData],


Nit: I like thunk. Will let you choose though.

It's a neat word, just not sure how widely understood it's going to be.

…ption`

…allowlist_option`

…log_allowlist_option`

github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 20, 2026

lobsterkatie added the Trigger: Override Selective Testing Run the full test suite; necessary in cases where selected tests are flaky due to reshuffling label May 20, 2026

lobsterkatie force-pushed the kmclb-clarify-language-in-SafeRolloutComparator-code branch from 96d8e03 to eb960be Compare May 20, 2026 20:39

thetruecpaul reviewed May 21, 2026

View reviewed changes

thetruecpaul approved these changes May 21, 2026

View reviewed changes

lobsterkatie added 19 commits May 21, 2026 11:45

s/reasonable_match/is_reasonable_match

8a3cb76

s/exact_match/is_exact_match

95a3303

s/is_experimental_data_a_null_result/is_experimental_data_nullish

ead3e07

s/should_use_experiment/should_use_experimental_data

dbc1c1f

s/use_experimental/use_experimental_data

05abac4

s/control_thunk/control_data_func

6998bb6

s/experimental_thunk/experimental_data_func

b4671a5

s/should_log_mismatch/_should_log_mismatch

ac35fbb

s/_callsite_blocklist_option_name/`_callsite_experiment_blocklist_o…

22b9c46

…ption`

s/_should_eval_option_name/_should_run_experiment_option

eaba6f2

s/_callsite_allowlist_option_name/`_callsite_use_experimental_data_…

912af18

…allowlist_option`

s/_sample_rate_option_name/_experiment_sample_rate_option

25709b6

s/_mismatch_log_callsite_allowlist_option_name/`_callsite_mismatch_…

9d581ba

…log_allowlist_option`

remove TODOs re: building dashboards

33c8e60

update docstrings and comments

c79e098

consolidate imports

e26d0e7

move type definition after logger definition

526830a

use constants for option names in tests

f0bb5c1

explain why we instantiate the test comparator

2f4f867

lobsterkatie force-pushed the kmclb-clarify-language-in-SafeRolloutComparator-code branch from eb960be to 2f4f867 Compare May 21, 2026 21:29

lobsterkatie marked this pull request as ready for review May 26, 2026 17:32

lobsterkatie requested review from a team as code owners May 26, 2026 17:32

lobsterkatie merged commit 9d49bb6 into master May 26, 2026
115 of 117 checks passed

lobsterkatie deleted the kmclb-clarify-language-in-SafeRolloutComparator-code branch May 26, 2026 17:32

sentry-release-bot Bot mentioned this pull request May 27, 2026

publish: getsentry/sentry@26.5.1 getsentry/publish#8288

Closed

3 tasks

		control_data_func: Callable[[], TData],
		experimental_data_func: Callable[[], TData],

Uh oh!

Conversation

lobsterkatie commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lobsterkatie commented May 20, 2026 •

edited

Loading