From 1adb4fa1e223ea5b8398b62505d8644ebddf707b Mon Sep 17 00:00:00 2001 From: Ryan Albrecht Date: Tue, 26 May 2026 08:56:30 -0700 Subject: [PATCH 01/61] ref(plugins): Inline PluginComponentBase into its two subclasses (#116112) ## Summary - Inlined the abstract `PluginComponentBase` class directly into its only two consumers: `PluginSettings` and `IssueActions` - Both classes now extend `Component` directly with all base class logic (state management, save/load lifecycle, `renderField`, API client) copied in - Deleted `static/app/plugins/pluginComponentBase.tsx` ## Test plan - [x] TypeScript typecheck passes (`pnpm run typecheck`) - [x] Existing plugin tests pass (`pluginConfig.spec.tsx`, `pluginDetailedView.spec.tsx`) - [x] ESLint passes Co-authored-by: Claude Opus 4.6 --- .../app/plugins/components/issueActions.tsx | 165 +++++++++++++++--- static/app/plugins/components/settings.tsx | 163 ++++++++++++++--- static/app/plugins/pluginComponentBase.tsx | 158 ----------------- 3 files changed, 282 insertions(+), 204 deletions(-) delete mode 100644 static/app/plugins/pluginComponentBase.tsx diff --git a/static/app/plugins/components/issueActions.tsx b/static/app/plugins/components/issueActions.tsx index 5a74d6f2ee1bff..0b6b95ca229111 100644 --- a/static/app/plugins/components/issueActions.tsx +++ b/static/app/plugins/components/issueActions.tsx @@ -1,14 +1,22 @@ -import {Fragment} from 'react'; +import {Component, Fragment} from 'react'; +import isFunction from 'lodash/isFunction'; import {Alert} from '@sentry/scraps/alert'; import {Button, LinkButton} from '@sentry/scraps/button'; +import { + addErrorMessage, + addLoadingMessage, + addSuccessMessage, + clearIndicators, +} from 'sentry/actionCreators/indicator'; +import {Client} from 'sentry/api'; import {Form} from 'sentry/components/deprecatedforms/form'; +import {GenericField} from 'sentry/components/deprecatedforms/genericField'; import {FormState} from 'sentry/components/forms/state'; import {LoadingError} from 'sentry/components/loadingError'; import {LoadingIndicator} from 'sentry/components/loadingIndicator'; import {t} from 'sentry/locale'; -import {PluginComponentBase} from 'sentry/plugins/pluginComponentBase'; import {GroupStore} from 'sentry/stores/groupStore'; import type {Group} from 'sentry/types/group'; import type {Plugin} from 'sentry/types/integrations'; @@ -17,10 +25,16 @@ import type {Project} from 'sentry/types/project'; import {trackAnalytics} from 'sentry/utils/analytics'; import {getAnalyticsDataForGroup} from 'sentry/utils/events'; +const callbackWithArgs = function (context: any, callback: any, ...args: any) { + return isFunction(callback) ? callback.bind(context, ...args) : undefined; +}; + +type GenericFieldProps = Parameters[0]; + type Field = { depends?: string[]; has_autocomplete?: boolean; -} & Parameters[0]['config']; +} & Omit['config']; type ActionType = 'link' | 'create' | 'unlink'; type FieldStateValue = (typeof FormState)[keyof typeof FormState]; @@ -45,6 +59,7 @@ type State = { createFormData: Record; dependentFieldState: Record; linkFormData: Record; + state: FormState; unlinkFormData: Record; createFieldList?: Field[]; error?: { @@ -58,12 +73,27 @@ type State = { linkFieldList?: Field[]; loading?: boolean; unlinkFieldList?: Field[]; -} & PluginComponentBase['state']; +}; -export class IssueActions extends PluginComponentBase { +export class IssueActions extends Component { constructor(props: Props) { super(props); + [ + 'onLoadSuccess', + 'onLoadError', + 'onSave', + 'onSaveSuccess', + 'onSaveError', + 'onSaveComplete', + 'renderField', + // @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message + ].map(method => (this[method] = this[method].bind(this))); + + if (this.fetchData) { + this.fetchData = this.onLoad.bind(this, this.fetchData.bind(this)); + } + this.createIssue = this.onSave.bind(this, this.createIssue.bind(this)); this.linkIssue = this.onSave.bind(this, this.linkIssue.bind(this)); this.unlinkIssue = this.onSave.bind(this, this.unlinkIssue.bind(this)); @@ -71,17 +101,35 @@ export class IssueActions extends PluginComponentBase { this.errorHandler = this.onLoadError.bind(this, this.errorHandler.bind(this)); this.state = { - ...this.state, - loading: ['link', 'create'].includes(this.props.actionType), state: ['link', 'create'].includes(this.props.actionType) ? FormState.LOADING : FormState.READY, + loading: ['link', 'create'].includes(this.props.actionType), createFormData: {}, linkFormData: {}, + unlinkFormData: {}, dependentFieldState: {}, - }; + } as Readonly; } + componentDidMount() { + const plugin = this.props.plugin; + if (!plugin.issue && this.props.actionType !== 'unlink') { + this.fetchData(); + } + } + + componentWillUnmount() { + this.api.clear(); + window.clearTimeout(this.successMessageTimeout); + window.clearTimeout(this.errorMessageTimeout); + } + + successMessageTimeout: number | undefined = undefined; + errorMessageTimeout: number | undefined = undefined; + + api = new Client(); + getGroup() { return this.props.group; } @@ -130,13 +178,6 @@ export class IssueActions extends PluginComponentBase { return this.state[key] || []; } - componentDidMount() { - const plugin = this.props.plugin; - if (!plugin.issue && this.props.actionType !== 'unlink') { - this.fetchData(); - } - } - getPluginCreateEndpoint() { return `/organizations/${this.getOrganization().slug}/issues/${this.getGroup().id}/plugins/${this.props.plugin.slug}/create/`; } @@ -161,7 +202,6 @@ export class IssueActions extends PluginComponentBase { const pluginSlug = this.props.plugin.slug; const url = `/organizations/${this.getOrganization().slug}/issues/${groupId}/plugins/${pluginSlug}/options/`; - // find the fields that this field is dependent on const dependentFormValues = Object.fromEntries( field.depends.map((fieldKey: any) => [fieldKey, formData[fieldKey]]) ); @@ -187,11 +227,9 @@ export class IssueActions extends PluginComponentBase { return; } - // find the location of the field in our list and replace it const indexOfField = fieldList.findIndex(({name}) => name === field.name); field = {...field, choices}; - // make a copy of the array to avoid mutation fieldList = fieldList.slice(); fieldList[indexOfField] = field; @@ -210,7 +248,6 @@ export class IssueActions extends PluginComponentBase { getInputProps(field: Field) { const props: {isLoading?: boolean; readonly?: boolean} = {}; - // special logic for fields that have dependencies if (field.depends && field.depends.length > 0) { switch (this.state.dependentFieldState[field.name]) { case FormState.LOADING: @@ -251,10 +288,20 @@ export class IssueActions extends PluginComponentBase { this.setState(state); } + onLoad(callback: any, ...args: any[]) { + this.setState( + { + state: FormState.LOADING, + }, + callbackWithArgs(this, callback, ...args) + ); + } + onLoadSuccess() { - super.onLoadSuccess(); + this.setState({ + state: FormState.READY, + }); - // dependent fields need to be set to disabled upon loading const fieldList = this.getFieldList(); fieldList.forEach(field => { if (field.depends && field.depends.length > 0) { @@ -263,6 +310,68 @@ export class IssueActions extends PluginComponentBase { }); } + onLoadError(callback: any, ...args: any[]) { + this.setState( + { + state: FormState.ERROR, + }, + callbackWithArgs(this, callback, ...args) + ); + addErrorMessage(t('An error occurred.')); + } + + onSave(callback: any, ...args: any[]) { + if (this.state.state === FormState.SAVING) { + return; + } + callback = callbackWithArgs(this, callback, ...args); + this.setState( + { + state: FormState.SAVING, + }, + () => { + addLoadingMessage(t('Saving changes…')); + callback?.(); + } + ); + } + + onSaveSuccess(callback: any, ...args: any[]) { + callback = callbackWithArgs(this, callback, ...args); + this.setState( + { + state: FormState.READY, + }, + () => callback?.() + ); + + window.clearTimeout(this.successMessageTimeout); + this.successMessageTimeout = window.setTimeout(() => { + addSuccessMessage(t('Success!')); + }, 0); + } + + onSaveError(callback: any, ...args: any[]) { + callback = callbackWithArgs(this, callback, ...args); + this.setState( + { + state: FormState.ERROR, + }, + () => callback?.() + ); + + window.clearTimeout(this.errorMessageTimeout); + this.errorMessageTimeout = window.setTimeout(() => { + addErrorMessage(t('Unable to save changes. Please try again.')); + }, 0); + } + + onSaveComplete(callback: any, ...args: any[]) { + clearIndicators(); + callback = callbackWithArgs(this, callback, ...args); + callback?.(); + } + fetchData() { if (this.props.actionType === 'create') { this.api.request(this.getPluginCreateEndpoint(), { @@ -358,7 +467,6 @@ export class IssueActions extends PluginComponentBase { changeField(action: ActionType, name: string, value: any) { const formDataKey = this.getFormDataKey(action); - // copy so we don't mutate const formData = {...this.state[formDataKey]}; const fieldList = this.getFieldList(); @@ -366,19 +474,15 @@ export class IssueActions extends PluginComponentBase { let callback = () => {}; - // only works with one impacted field const impactedField = fieldList.find(({depends}) => { if (!depends?.length) { return false; } - // must be dependent on the field we just set return depends.includes(name); }); if (impactedField) { - // if every dependent field is set, then search if (impactedField.depends?.some(dependentField => !formData[dependentField])) { - // otherwise reset the options callback = () => this.resetOptionsOfDependentField(impactedField); } else { callback = () => this.loadOptionsForDependentField(impactedField); @@ -387,6 +491,15 @@ export class IssueActions extends PluginComponentBase { this.setState(prevState => ({...prevState, [formDataKey]: formData}), callback); } + renderField(props: Omit): React.ReactNode { + props = {...props}; + const newProps = { + ...props, + formState: this.state.state, + }; + return ; + } + renderForm(): React.ReactNode { switch (this.props.actionType) { case 'create': diff --git a/static/app/plugins/components/settings.tsx b/static/app/plugins/components/settings.tsx index fd854e0bb94ed0..84626d1289548a 100644 --- a/static/app/plugins/components/settings.tsx +++ b/static/app/plugins/components/settings.tsx @@ -1,28 +1,43 @@ +import {Component} from 'react'; import {css} from '@emotion/react'; import isEqual from 'lodash/isEqual'; +import isFunction from 'lodash/isFunction'; import {Alert} from '@sentry/scraps/alert'; import {LinkButton} from '@sentry/scraps/button'; import {Stack} from '@sentry/scraps/layout'; +import { + addErrorMessage, + addLoadingMessage, + addSuccessMessage, + clearIndicators, +} from 'sentry/actionCreators/indicator'; +import {Client} from 'sentry/api'; import {Form} from 'sentry/components/deprecatedforms/form'; +import {GenericField} from 'sentry/components/deprecatedforms/genericField'; import {FormState} from 'sentry/components/forms/state'; import {LoadingIndicator} from 'sentry/components/loadingIndicator'; import {t, tct} from 'sentry/locale'; -import {PluginComponentBase} from 'sentry/plugins/pluginComponentBase'; import type {Plugin} from 'sentry/types/integrations'; import type {Organization} from 'sentry/types/organization'; import type {Project} from 'sentry/types/project'; import {parseRepo} from 'sentry/utils/git/parseRepo'; import {isScmPlugin, trackIntegrationAnalytics} from 'sentry/utils/integrationUtil'; +const callbackWithArgs = function (context: any, callback: any, ...args: any) { + return isFunction(callback) ? callback.bind(context, ...args) : undefined; +}; + +type GenericFieldProps = Parameters[0]; + type Props = { organization: Organization; plugin: Plugin; project: Project; -} & PluginComponentBase['props']; +}; -type Field = Parameters[0]['config']; +type Field = GenericFieldProps['config']; type BackendField = Field & {defaultValue?: any; value?: any}; @@ -32,29 +47,61 @@ type State = { formData: Record; initialData: Record | null; rawData: Record; + state: FormState; wasConfiguredOnPageLoad: boolean; -} & PluginComponentBase['state']; +}; export class PluginSettings< P extends Props = Props, S extends State = State, -> extends PluginComponentBase { +> extends Component { constructor(props: P) { super(props); - Object.assign(this.state, { + [ + 'onLoadSuccess', + 'onLoadError', + 'onSave', + 'onSaveSuccess', + 'onSaveError', + 'onSaveComplete', + 'renderField', + // @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message + ].map(method => (this[method] = this[method].bind(this))); + + if (this.fetchData) { + this.fetchData = this.onLoad.bind(this, this.fetchData.bind(this)); + } + if (this.onSubmit) { + this.onSubmit = this.onSave.bind(this, this.onSubmit.bind(this)); + } + + this.state = { + state: FormState.LOADING, fieldList: null, initialData: null, formData: null, errors: {}, rawData: {}, - // override default FormState.READY if api requests are - // necessary to even load the form - state: FormState.LOADING, wasConfiguredOnPageLoad: false, - }); + } as unknown as Readonly; } + componentDidMount() { + this.fetchData(); + } + + componentWillUnmount() { + this.api.clear(); + window.clearTimeout(this.successMessageTimeout); + window.clearTimeout(this.errorMessageTimeout); + } + + successMessageTimeout: number | undefined = undefined; + errorMessageTimeout: number | undefined = undefined; + + api = new Client(); + trackPluginEvent = ( eventKey: | 'integrations.installation_start' @@ -78,10 +125,6 @@ export class PluginSettings< }); }; - componentDidMount() { - this.fetchData(); - } - getPluginEndpoint() { const org = this.props.organization; const project = this.props.project; @@ -92,16 +135,90 @@ export class PluginSettings< // eslint-disable-next-line @sentry/no-unnecessary-type-annotation const formData: State['formData'] = this.state.formData; formData[name] = value; - // upon changing a field, remove errors const errors = this.state.errors; delete errors[name]; this.setState({formData, errors}); } + onLoad(callback: any, ...args: any[]) { + this.setState( + { + state: FormState.LOADING, + }, + callbackWithArgs(this, callback, ...args) + ); + } + + onLoadSuccess() { + this.setState({ + state: FormState.READY, + }); + } + + onLoadError(callback: any, ...args: any[]) { + this.setState( + { + state: FormState.ERROR, + }, + callbackWithArgs(this, callback, ...args) + ); + addErrorMessage(t('An error occurred.')); + } + + onSave(callback: any, ...args: any[]) { + if (this.state.state === FormState.SAVING) { + return; + } + callback = callbackWithArgs(this, callback, ...args); + this.setState( + { + state: FormState.SAVING, + }, + () => { + addLoadingMessage(t('Saving changes…')); + callback?.(); + } + ); + } + + onSaveSuccess(callback: any, ...args: any[]) { + callback = callbackWithArgs(this, callback, ...args); + this.setState( + { + state: FormState.READY, + }, + () => callback?.() + ); + + window.clearTimeout(this.successMessageTimeout); + this.successMessageTimeout = window.setTimeout(() => { + addSuccessMessage(t('Success!')); + }, 0); + } + + onSaveError(callback: any, ...args: any[]) { + callback = callbackWithArgs(this, callback, ...args); + this.setState( + { + state: FormState.ERROR, + }, + () => callback?.() + ); + + window.clearTimeout(this.errorMessageTimeout); + this.errorMessageTimeout = window.setTimeout(() => { + addErrorMessage(t('Unable to save changes. Please try again.')); + }, 0); + } + + onSaveComplete(callback: any, ...args: any[]) { + clearIndicators(); + callback = callbackWithArgs(this, callback, ...args); + callback?.(); + } + onSubmit() { if (!this.state.wasConfiguredOnPageLoad) { - // Users cannot install plugins like other integrations but we need the events for the funnel - // we will treat a user saving a plugin that wasn't already configured as an installation event this.trackPluginEvent('integrations.installation_start'); } @@ -161,7 +278,6 @@ export class PluginSettings< formData[field.name] = field.value || field.defaultValue; // @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message initialData[field.name] = field.value; - // for simplicity sake, we will consider a plugin was configured if we have any value that is stored in the DB wasConfiguredOnPageLoad = wasConfiguredOnPageLoad || !!field.value; }); this.setState( @@ -170,8 +286,6 @@ export class PluginSettings< formData, initialData, wasConfiguredOnPageLoad, - // call this here to prevent FormState.READY from being - // set before fieldList is }, this.onLoadSuccess ); @@ -180,6 +294,15 @@ export class PluginSettings< }); } + renderField(props: Omit): React.ReactNode { + props = {...props}; + const newProps = { + ...props, + formState: this.state.state, + }; + return ; + } + render() { if (this.state.state === FormState.LOADING) { return ; diff --git a/static/app/plugins/pluginComponentBase.tsx b/static/app/plugins/pluginComponentBase.tsx deleted file mode 100644 index abd254eee5711e..00000000000000 --- a/static/app/plugins/pluginComponentBase.tsx +++ /dev/null @@ -1,158 +0,0 @@ -import {Component} from 'react'; -import isFunction from 'lodash/isFunction'; - -import { - addErrorMessage, - addLoadingMessage, - addSuccessMessage, - clearIndicators, -} from 'sentry/actionCreators/indicator'; -import {Client} from 'sentry/api'; -import {GenericField} from 'sentry/components/deprecatedforms/genericField'; -import {FormState} from 'sentry/components/forms/state'; -import {t} from 'sentry/locale'; - -const callbackWithArgs = function (context: any, callback: any, ...args: any) { - return isFunction(callback) ? callback.bind(context, ...args) : undefined; -}; - -type GenericFieldProps = Parameters[0]; - -type Props = Record; - -type State = {state: FormState}; - -export abstract class PluginComponentBase< - P extends Props = Props, - S extends State = State, -> extends Component { - constructor(props: P) { - super(props); - - [ - 'onLoadSuccess', - 'onLoadError', - 'onSave', - 'onSaveSuccess', - 'onSaveError', - 'onSaveComplete', - 'renderField', - // @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message - ].map(method => (this[method] = this[method].bind(this))); - - if (this.fetchData) { - this.fetchData = this.onLoad.bind(this, this.fetchData.bind(this)); - } - if (this.onSubmit) { - this.onSubmit = this.onSave.bind(this, this.onSubmit.bind(this)); - } - this.state = { - state: FormState.READY, - } as Readonly; - } - - componentWillUnmount() { - this.api.clear(); - window.clearTimeout(this.successMessageTimeout); - window.clearTimeout(this.errorMessageTimeout); - } - - successMessageTimeout: number | undefined = undefined; - errorMessageTimeout: number | undefined = undefined; - - api = new Client(); - - fetchData() { - // Allow children to implement this - } - - onSubmit() { - // Allow children to implement this - } - - onLoad(callback: any, ...args: any[]) { - this.setState( - { - state: FormState.LOADING, - }, - callbackWithArgs(this, callback, ...args) - ); - } - - onLoadSuccess() { - this.setState({ - state: FormState.READY, - }); - } - - onLoadError(callback: any, ...args: any[]) { - this.setState( - { - state: FormState.ERROR, - }, - callbackWithArgs(this, callback, ...args) - ); - addErrorMessage(t('An error occurred.')); - } - - onSave(callback: any, ...args: any[]) { - if (this.state.state === FormState.SAVING) { - return; - } - callback = callbackWithArgs(this, callback, ...args); - this.setState( - { - state: FormState.SAVING, - }, - () => { - addLoadingMessage(t('Saving changes\u2026')); - callback?.(); - } - ); - } - - onSaveSuccess(callback: any, ...args: any[]) { - callback = callbackWithArgs(this, callback, ...args); - this.setState( - { - state: FormState.READY, - }, - () => callback?.() - ); - - window.clearTimeout(this.successMessageTimeout); - this.successMessageTimeout = window.setTimeout(() => { - addSuccessMessage(t('Success!')); - }, 0); - } - - onSaveError(callback: any, ...args: any[]) { - callback = callbackWithArgs(this, callback, ...args); - this.setState( - { - state: FormState.ERROR, - }, - () => callback?.() - ); - - window.clearTimeout(this.errorMessageTimeout); - this.errorMessageTimeout = window.setTimeout(() => { - addErrorMessage(t('Unable to save changes. Please try again.')); - }, 0); - } - - onSaveComplete(callback: any, ...args: any[]) { - clearIndicators(); - callback = callbackWithArgs(this, callback, ...args); - callback?.(); - } - - renderField(props: Omit): React.ReactNode { - props = {...props}; - const newProps = { - ...props, - formState: this.state.state, - }; - return ; - } -} From beebb2ba48c1f6be274f743e3d9e3e76bed26926 Mon Sep 17 00:00:00 2001 From: Richard Roggenkemper <46740234+roggenkemper@users.noreply.github.com> Date: Tue, 26 May 2026 11:59:11 -0400 Subject: [PATCH 02/61] feat(search): Register feature flag for recommended issue sort (#116191) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Register `organizations:issue-stream-recommended-sort` as a temporary feature flag with `api_expose=True` so the frontend can check it. This enables per-org enablement via FlagPole for the experimental recommended sort added in #111043. The flag will be used to gate showing "Recommended" in the issue stream sort dropdown for orgs that opt in. No behavioral changes — just the flag registration. fixes https://linear.app/getsentry/issue/ID-1566/add-feature-flag-for-sort-option --- src/sentry/features/temporary.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/sentry/features/temporary.py b/src/sentry/features/temporary.py index b184dd0aaea14f..7c50bb15b625c5 100644 --- a/src/sentry/features/temporary.py +++ b/src/sentry/features/temporary.py @@ -331,6 +331,8 @@ def register_temporary_features(manager: FeatureManager) -> None: manager.add("organizations:issue-feed.eap-search", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) # Remove trace and breadcrumbs from issue summary input manager.add("organizations:issue-summary-experimental", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=False) + # Enable the experimental "recommended" sort option in the issue stream + manager.add("organizations:issue-stream-recommended-sort", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) # Lets organizations manage grouping configs manager.add("organizations:set-grouping-config", OrganizationFeature, FeatureHandlerStrategy.INTERNAL, api_expose=True) From 1070716e8c33ccba227e62f5dfdc755b7c7a4738 Mon Sep 17 00:00:00 2001 From: Richard Roggenkemper <46740234+roggenkemper@users.noreply.github.com> Date: Tue, 26 May 2026 11:59:45 -0400 Subject: [PATCH 03/61] fix(issues): Make GroupSearchViewPermission fail closed for unknown object types (#116183) `GroupSearchViewPermission.has_object_permission` returned `True` for unrecognized object types (the fallback at the end of the method), granting access by default. Changed the fallback to return `False` so unknown object types are denied Fixes ID-1562 --- .../endpoints/bases/group_search_view.py | 2 +- ..._organization_group_search_view_details.py | 24 ++++++++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/sentry/issues/endpoints/bases/group_search_view.py b/src/sentry/issues/endpoints/bases/group_search_view.py index 26c91ceb772412..70841dbbe885bf 100644 --- a/src/sentry/issues/endpoints/bases/group_search_view.py +++ b/src/sentry/issues/endpoints/bases/group_search_view.py @@ -35,4 +35,4 @@ def has_object_permission(self, request: Request, view: APIView, obj: object) -> return False - return True + return False diff --git a/tests/sentry/issues/endpoints/test_organization_group_search_view_details.py b/tests/sentry/issues/endpoints/test_organization_group_search_view_details.py index b249c46818ddc0..e86f9084d810a1 100644 --- a/tests/sentry/issues/endpoints/test_organization_group_search_view_details.py +++ b/tests/sentry/issues/endpoints/test_organization_group_search_view_details.py @@ -1,10 +1,13 @@ +from unittest.mock import MagicMock + from django.urls import reverse from django.utils import timezone +from sentry.issues.endpoints.bases.group_search_view import GroupSearchViewPermission from sentry.models.groupsearchview import GroupSearchView, GroupSearchViewVisibility from sentry.models.groupsearchviewlastvisited import GroupSearchViewLastVisited from sentry.models.groupsearchviewstarred import GroupSearchViewStarred -from sentry.testutils.cases import APITestCase +from sentry.testutils.cases import APITestCase, TestCase from sentry.testutils.helpers import with_feature @@ -593,3 +596,22 @@ def test_put_without_feature_flag(self) -> None: response = self.client.put(self.url, data=data) assert response.status_code == 404 + + +class GroupSearchViewPermissionTest(TestCase): + def test_denies_unknown_object_types(self) -> None: + permission = GroupSearchViewPermission() + request = MagicMock() + request.method = "PUT" + view = MagicMock() + + assert permission.has_object_permission(request, view, object()) is False + + def test_denies_related_model_types(self) -> None: + permission = GroupSearchViewPermission() + request = MagicMock() + request.method = "GET" + view = MagicMock() + + starred = MagicMock(spec=GroupSearchViewStarred) + assert permission.has_object_permission(request, view, starred) is False From 396de956d856b4861d3c421417b9013b20110a3f Mon Sep 17 00:00:00 2001 From: Richard Roggenkemper <46740234+roggenkemper@users.noreply.github.com> Date: Tue, 26 May 2026 12:07:17 -0400 Subject: [PATCH 04/61] fix(security): Add project-level access check to GroupEventJsonView (#116184) `GroupEventJsonView` checked org-level `event:read` scope via `OrganizationView` but did not verify the user has access to the resolved group's project. In orgs with closed membership (team-based access), a user on team A could read raw event JSON (PII, stacktraces, request bodies) from any project by guessing group IDs. Added `request.access.has_project_access(group.project)` check after resolving the group, returning 404 on denial (consistent with the existing cross-org pattern) Fixes ID-1563 Co-authored-by: Claude Opus 4.6 --- src/sentry/web/frontend/group_event_json.py | 3 ++ .../web/frontend/test_group_event_json.py | 52 +++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/src/sentry/web/frontend/group_event_json.py b/src/sentry/web/frontend/group_event_json.py index 867619aa0085a7..57f1270546eb7e 100644 --- a/src/sentry/web/frontend/group_event_json.py +++ b/src/sentry/web/frontend/group_event_json.py @@ -20,6 +20,9 @@ def get(self, request: HttpRequest, organization, group_id, event_id_or_latest) except Group.DoesNotExist: raise Http404 + if not request.access.has_project_access(group.project): + raise Http404 + event: Event | GroupEvent | None if event_id_or_latest == "latest": event = group.get_latest_event() diff --git a/tests/sentry/web/frontend/test_group_event_json.py b/tests/sentry/web/frontend/test_group_event_json.py index 35df546c5c17ea..4c00f971f3d15a 100644 --- a/tests/sentry/web/frontend/test_group_event_json.py +++ b/tests/sentry/web/frontend/test_group_event_json.py @@ -22,6 +22,58 @@ def test_does_render(self) -> None: data = json.loads(resp.content.decode("utf-8")) assert data["event_id"] == self.event.event_id + def test_cross_project_access_denied(self) -> None: + """User on team A cannot read event JSON from a project they lack access to.""" + org = self.create_organization(flags=0) + team_b = self.create_team(organization=org, name="team-b") + project_b = self.create_project(organization=org, teams=[team_b]) + min_ago = before_now(minutes=1).isoformat() + event_b = self.store_event( + data={"fingerprint": ["group-b"], "timestamp": min_ago}, + project_id=project_b.id, + ) + + team_a = self.create_team(organization=org, name="team-a") + restricted_user = self.create_user() + self.create_member( + user=restricted_user, + organization=org, + role="member", + teams=[team_a], + ) + + self.login_as(restricted_user) + path = ( + f"/organizations/{org.slug}/issues/{event_b.group_id}/events/{event_b.event_id}/json/" + ) + resp = self.client.get(path) + assert resp.status_code == 404 + + def test_cross_project_access_denied_latest(self) -> None: + """User on team A cannot read latest event JSON from a project they lack access to.""" + org = self.create_organization(flags=0) + team_b = self.create_team(organization=org, name="team-b") + project_b = self.create_project(organization=org, teams=[team_b]) + min_ago = before_now(minutes=1).isoformat() + event_b = self.store_event( + data={"fingerprint": ["group-b"], "timestamp": min_ago}, + project_id=project_b.id, + ) + + team_a = self.create_team(organization=org, name="team-a") + restricted_user = self.create_user() + self.create_member( + user=restricted_user, + organization=org, + role="member", + teams=[team_a], + ) + + self.login_as(restricted_user) + path = f"/organizations/{org.slug}/issues/{event_b.group_id}/events/latest/json/" + resp = self.client.get(path) + assert resp.status_code == 404 + def test_cross_organization_access_denied(self) -> None: """Ensures users cannot access event data from other organizations.""" victim_org = self.create_organization(name="victim-org") From 34d677bf1ddde53f1b3dd780f4f05deac747973c Mon Sep 17 00:00:00 2001 From: Tony Le Date: Tue, 26 May 2026 12:24:13 -0400 Subject: [PATCH 05/61] ref(spans): Extract flush_segment pipeline helpers (#116149) Refactors the `flush_segments` to be an orchestration function with unit testable helpers: - `_load_flush_candidates`: loading ready flush candidates from queue shards - `_acquire_locks_for_flush_candidates`: acquiring per-segment flush locks - `_load_segment_data`: loading payload keys and payload bytes - `_build_flushed_segments`: building producer-ready `FlushedSegment` objects - `_record_segment_loss_metrics`: recording segment loss / expiration metrics Also adds/moves datamodels (`FlushCandidate`, `FlushedSegment`, `OutputSpan`) into `buffer_types.py` alongside the existing span buffer value types. --- src/sentry/spans/buffer.py | 236 +++++++++--------- src/sentry/spans/buffer_types.py | 101 +++++++- .../spans/consumers/process/test_flusher.py | 4 +- tests/sentry/spans/test_buffer.py | 123 ++++++++- 4 files changed, 334 insertions(+), 130 deletions(-) diff --git a/src/sentry/spans/buffer.py b/src/sentry/spans/buffer.py index ad1aade2ed2133..de0b9f0269692a 100644 --- a/src/sentry/spans/buffer.py +++ b/src/sentry/spans/buffer.py @@ -101,10 +101,9 @@ import logging import math import time -import uuid from collections.abc import Generator, Mapping, MutableMapping, Sequence from hashlib import blake2b -from typing import Any, NamedTuple, cast +from typing import Any, cast import orjson import zstandard @@ -127,8 +126,12 @@ ) from sentry.spans.buffer_types import ( EvalshaResult, + FlushCandidate, + FlushedSegment, InsertedSubsegment, LoadedSegmentData, + OutputSpan, + QueueKey, Span, Subsegment, ) @@ -143,8 +146,6 @@ from sentry.utils import metrics, redis from sentry.utils.outcomes import Outcome, track_outcome -QueueKey = bytes - logger = logging.getLogger(__name__) @@ -155,9 +156,6 @@ def get_redis_client() -> RedisCluster[bytes] | StrictRedis[bytes]: add_buffer_script = redis.load_redis_script("spans/add-buffer.lua") -type SpanPayload = dict[str, Any] - - def _compute_salt(spans: Sequence[Span]) -> str: return blake2b( b"".join(s.span_id.encode("ascii") for s in spans), @@ -165,63 +163,6 @@ def _compute_salt(spans: Sequence[Span]) -> str: ).hexdigest() -class OutputSpan(NamedTuple): - payload: SpanPayload - - -class FlushedSegment(NamedTuple): - queue_key: QueueKey - spans: list[OutputSpan] - project_id: int # Used to track outcomes - payload_keys: list[PayloadKey] = [] # For cleanup - - def to_messages(self) -> list[dict[str, Any]]: - """ - Build producer messages for this segment. - - If the segment size exceeds `spans.buffer.max_segment_bytes`, the segment is split - into multiple messages with skip_enrichment=True. Otherwise, returns a single message. - - Each message gets a unique flush_id generated at call time, ensuring duplicate - flushes from Redis produce distinct IDs. - """ - max_segment_bytes = options.get("spans.buffer.max-segment-bytes") - - spans: list[SpanPayload] = [span.payload for span in self.spans] - - sizes = [len(orjson.dumps(s)) for s in spans] - if sum(sizes) <= max_segment_bytes: - return [{"flush_id": uuid.uuid4().hex, "spans": spans}] - - messages: list[dict[str, Any]] = [] - current: list[SpanPayload] = [] - current_size = 0 - - for span, size in zip(spans, sizes): - if current and current_size + size > max_segment_bytes: - messages.append( - {"flush_id": uuid.uuid4().hex, "spans": current, "skip_enrichment": True} - ) - current = [] - current_size = 0 - current.append(span) - current_size += size - - if current: - messages.append( - {"flush_id": uuid.uuid4().hex, "spans": current, "skip_enrichment": True} - ) - - if len(messages) > 1: - metrics.timing( - "spans.buffer.oversized_segments_chunked", - len(messages), - ) - metrics.timing("spans.buffer.oversized_segments_size", sum(sizes)) - - return messages - - class SpansBuffer: def __init__(self, assigned_shards: list[int], slice_id: int | None = None): self.assigned_shards = list(assigned_shards) @@ -656,13 +597,71 @@ def _acquire_flush_locks(self, segment_keys: Sequence[SegmentKey]) -> set[Segmen return locks_acquired def flush_segments(self, now: int) -> dict[SegmentKey, FlushedSegment]: - cutoff = now + """ + Select queued segments and prepare them for Kafka production. - queue_keys = [] + This orchestrates the flush path: load ready queue entries, acquire + per-segment locks, load payload data, emit loss/flush observability, and + return producer-ready FlushedSegment objects. SpanFlusher produces those + objects to Kafka and calls done_flush_segments after successful delivery. + """ shard_factor = max(1, len(self.assigned_shards)) max_flush_segments = options.get("spans.buffer.max-flush-segments") max_segments_per_shard = math.ceil(max_flush_segments / shard_factor) + flush_candidates, load_ids_latency_ms = self._load_flush_candidates( + cutoff=now, + max_segments_per_shard=max_segments_per_shard, + ) + + flush_candidates = self._acquire_locks_for_flush_candidates(flush_candidates) + segment_keys = [candidate.segment_key for candidate in flush_candidates] + + loaded_segment_data, load_data_latency_ms, decompress_latency_ms = self._load_segment_data( + segment_keys + ) + + segment_to_queue = { + candidate.segment_key: candidate.queue_key for candidate in flush_candidates + } + self._record_segment_loss_metrics( + segment_keys, + segment_to_queue, + now, + loaded_segment_data.payloads, + ) + + flushed_segments, num_has_root_spans, any_shard_at_limit = self._build_flushed_segments( + flush_candidates, + loaded_segment_data, + max_segments_per_shard, + now, + ) + + self._flusher_logger.log_loaded_segments( + segment_keys, + loaded_segment_data.payloads, + load_ids_latency_ms=load_ids_latency_ms, + load_data_latency_ms=load_data_latency_ms, + decompress_latency_ms=decompress_latency_ms, + ) + + metrics.timing("spans.buffer.flush_segments.num_segments", len(flushed_segments)) + metrics.timing("spans.buffer.flush_segments.has_root_span", num_has_root_spans) + + self.any_shard_at_limit = any_shard_at_limit + return flushed_segments + + def _load_flush_candidates( + self, + cutoff: int, + max_segments_per_shard: int, + ) -> tuple[list[FlushCandidate], int]: + """ + Read ready segment keys from the assigned queue shards. + """ + queue_keys = [] + ids_start = time.monotonic() with metrics.timer("spans.buffer.flush_segments.load_segment_ids"): with self.client.pipeline(transaction=False) as p: @@ -676,43 +675,52 @@ def flush_segments(self, now: int) -> dict[SegmentKey, FlushedSegment]: result = p.execute() load_ids_latency_ms = int((time.monotonic() - ids_start) * 1000) - segment_keys: list[tuple[int, QueueKey, SegmentKey, float]] = [] + flush_candidates: list[FlushCandidate] = [] for shard, queue_key, keys_with_scores in zip(self.assigned_shards, queue_keys, result): for segment_key, score in keys_with_scores: - segment_keys.append((shard, queue_key, segment_key, score)) - - acquired_locks = self._acquire_flush_locks([k for _, _, k, _ in segment_keys]) - segment_keys = [entry for entry in segment_keys if entry[2] in acquired_locks] - segment_key_values = [k for _, _, k, _ in segment_keys] + flush_candidates.append(FlushCandidate(shard, queue_key, segment_key, score)) - data_start = time.monotonic() - with metrics.timer("spans.buffer.flush_segments.load_segment_data"): - # Pass queue mapping to enable TTL expiration detection - segment_to_queue = { - segment_key: queue_key for _, queue_key, segment_key, _ in segment_keys - } - loaded_segment_data, decompress_latency_ms = self._load_segment_data(segment_key_values) - load_data_latency_ms = int((time.monotonic() - data_start) * 1000) + return flush_candidates, load_ids_latency_ms - self._record_segment_loss_metrics( - segment_key_values, - segment_to_queue, - now, - loaded_segment_data.payloads, + def _acquire_locks_for_flush_candidates( + self, flush_candidates: Sequence[FlushCandidate] + ) -> list[FlushCandidate]: + """ + Acquire per-segment flush locks and keep the candidates that won. + """ + acquired_locks = self._acquire_flush_locks( + [candidate.segment_key for candidate in flush_candidates] ) + return [ + flush_candidate + for flush_candidate in flush_candidates + if flush_candidate.segment_key in acquired_locks + ] - return_segments = {} + def _build_flushed_segments( + self, + flush_candidates: Sequence[FlushCandidate], + loaded_segment_data: LoadedSegmentData, + max_segments_per_shard: int, + now: int, + ) -> tuple[dict[SegmentKey, FlushedSegment], int, bool]: + """ + Convert loaded payload bytes into FlushedSegment objects that contain + the segment metadata and span payloads. + """ + return_segments: dict[SegmentKey, FlushedSegment] = {} num_has_root_spans = 0 any_shard_at_limit = False - for shard, queue_key, segment_key, score in segment_keys: + for flush_candidate in flush_candidates: + segment_key = flush_candidate.segment_key segment_span_id = segment_key_to_span_id(segment_key).decode("ascii") segment = loaded_segment_data.payloads.get(segment_key, []) project_id, _, _ = parse_segment_key(segment_key) if len(segment) >= max_segments_per_shard: any_shard_at_limit = True - output_spans = [] + output_spans: list[OutputSpan] = [] has_root_span = False metrics.timing("spans.buffer.flush_segments.num_spans_per_segment", len(segment)) # This incr metric is needed to get a rate overall. @@ -737,10 +745,11 @@ def flush_segments(self, now: int) -> dict[SegmentKey, FlushedSegment]: output_spans.append(OutputSpan(payload=cast(dict[str, Any], span))) metrics.incr( - "spans.buffer.flush_segments.num_segments_per_shard", tags={"shard_i": shard} + "spans.buffer.flush_segments.num_segments_per_shard", + tags={"shard_i": flush_candidate.shard}, ) return_segments[segment_key] = FlushedSegment( - queue_key=queue_key, + queue_key=flush_candidate.queue_key, spans=output_spans, project_id=int(project_id.decode("ascii")), payload_keys=loaded_segment_data.payload_keys.get(segment_key, []), @@ -752,46 +761,38 @@ def flush_segments(self, now: int) -> dict[SegmentKey, FlushedSegment]: segment_span_id=segment_span_id, has_root_span=has_root_span, num_spans=len(segment), - shard=shard, - queue_key=queue_key, + shard=flush_candidate.shard, + queue_key=flush_candidate.queue_key, timestamp=now, ).emit(self._get_debug_trace_logger) - self._flusher_logger.log_loaded_segments( - segment_key_values, - loaded_segment_data.payloads, - load_ids_latency_ms=load_ids_latency_ms, - load_data_latency_ms=load_data_latency_ms, - decompress_latency_ms=decompress_latency_ms, - ) - - metrics.timing("spans.buffer.flush_segments.num_segments", len(return_segments)) - metrics.timing("spans.buffer.flush_segments.has_root_span", num_has_root_spans) - - self.any_shard_at_limit = any_shard_at_limit - return return_segments + return return_segments, num_has_root_spans, any_shard_at_limit def _load_segment_data( self, segment_keys: list[SegmentKey], - ) -> tuple[LoadedSegmentData, int]: + ) -> tuple[LoadedSegmentData, int, int]: """ - Loads the segments from Redis, given a list of segment keys. + Load payload keys and span payload bytes for segment keys. - :param segment_keys: List of segment keys to load. - :return: Loaded payloads and payload keys, plus decompression latency. + This is the load-data orchestration step for flushing. It returns the + loaded payload data plus phase latencies for cumulative flusher logging. """ - page_size = options.get("spans.buffer.segment-page-size") - payload_keys = self._load_payload_keys(segment_keys) - payloads, decompress_latency_ms = self._load_payloads_from_keys( - segment_keys, - payload_keys, - page_size, - ) + data_start = time.monotonic() + with metrics.timer("spans.buffer.flush_segments.load_segment_data"): + page_size = options.get("spans.buffer.segment-page-size") + payload_keys = self._load_payload_keys(segment_keys) + payloads, decompress_latency_ms = self._load_payloads_from_keys( + segment_keys, + payload_keys, + page_size, + ) + load_data_latency_ms = int((time.monotonic() - data_start) * 1000) return ( LoadedSegmentData(payloads, payload_keys), + load_data_latency_ms, decompress_latency_ms, ) @@ -880,6 +881,9 @@ def _record_segment_loss_metrics( now: int, payloads: dict[SegmentKey, list[bytes]], ) -> None: + """ + Emit loss and expiration metrics for loaded segments. + """ # Fetch ingested counts for all segments to calculate dropped spans with self.client.pipeline(transaction=False) as p: for key in segment_keys: diff --git a/src/sentry/spans/buffer_types.py b/src/sentry/spans/buffer_types.py index 4b18624fd356f6..ce6d14f0b0b3eb 100644 --- a/src/sentry/spans/buffer_types.py +++ b/src/sentry/spans/buffer_types.py @@ -1,19 +1,27 @@ """ -Shared value types for the span buffer. +Shared span buffer data structures. -These types describe data passed between buffer pipeline steps. They do not -own Redis operations, logging behavior, or Django model state. +These types describe data passed between buffer pipeline steps. Some include +small representation helpers, but Redis operations and Django model state stay +outside this module. """ from __future__ import annotations +import uuid from collections.abc import Sequence from typing import Any, NamedTuple +import orjson + +from sentry import options from sentry.spans.segment_key import PayloadKey, SegmentKey +from sentry.utils import metrics type DataPoint = tuple[bytes, float] type EvalshaData = list[DataPoint] +type QueueKey = bytes +type SpanPayload = dict[str, Any] # NamedTuples are faster to construct than dataclasses @@ -105,5 +113,92 @@ def is_detached_segment(self) -> bool: class LoadedSegmentData(NamedTuple): + """ + Raw payload data loaded for flush candidates. + """ + payloads: dict[SegmentKey, list[bytes]] payload_keys: dict[SegmentKey, list[PayloadKey]] + + +class OutputSpan(NamedTuple): + """ + A span payload after flush-time segment metadata has been attached. + """ + + payload: SpanPayload + + +class FlushCandidate(NamedTuple): + """ + A queued segment that is ready to be considered for flushing. + + At this stage only queue metadata is known; payload keys and span payloads + are loaded after the flush lock is acquired. + """ + + shard: int + queue_key: QueueKey + segment_key: SegmentKey + score: float + + +class FlushedSegment(NamedTuple): + """ + A buffered segment selected, loaded, and prepared for Kafka production. + + The segment has not been produced to Kafka yet. SpanFlusher calls + `to_messages()` and produces those messages, then cleanup happens through + done_flush_segments. + """ + + queue_key: QueueKey + spans: list[OutputSpan] + project_id: int # Used to track outcomes + payload_keys: list[PayloadKey] = [] # For cleanup + + def to_messages(self) -> list[dict[str, Any]]: + """ + Build producer messages for this segment. + + If the segment size exceeds `spans.buffer.max_segment_bytes`, the segment is split + into multiple messages with skip_enrichment=True. Otherwise, returns a single message. + + Each message gets a unique flush_id generated at call time, ensuring duplicate + flushes from Redis produce distinct IDs. + """ + max_segment_bytes = options.get("spans.buffer.max-segment-bytes") + + spans: list[SpanPayload] = [span.payload for span in self.spans] + + sizes = [len(orjson.dumps(s)) for s in spans] + if sum(sizes) <= max_segment_bytes: + return [{"flush_id": uuid.uuid4().hex, "spans": spans}] + + messages: list[dict[str, Any]] = [] + current: list[SpanPayload] = [] + current_size = 0 + + for span, size in zip(spans, sizes): + if current and current_size + size > max_segment_bytes: + messages.append( + {"flush_id": uuid.uuid4().hex, "spans": current, "skip_enrichment": True} + ) + current = [] + current_size = 0 + current.append(span) + current_size += size + + if current: + messages.append( + {"flush_id": uuid.uuid4().hex, "spans": current, "skip_enrichment": True} + ) + + if len(messages) > 1: + metrics.timing( + "spans.buffer.oversized_segments_chunked", + len(messages), + ) + metrics.timing("spans.buffer.oversized_segments_size", sum(sizes)) + + return messages diff --git a/tests/sentry/spans/consumers/process/test_flusher.py b/tests/sentry/spans/consumers/process/test_flusher.py index c2f33f437104e1..2e8687c733179d 100644 --- a/tests/sentry/spans/consumers/process/test_flusher.py +++ b/tests/sentry/spans/consumers/process/test_flusher.py @@ -10,8 +10,8 @@ from django.test import override_settings from sentry.conf.types.kafka_definition import Topic -from sentry.spans.buffer import FlushedSegment, OutputSpan, SpansBuffer -from sentry.spans.buffer_types import Span +from sentry.spans.buffer import SpansBuffer +from sentry.spans.buffer_types import FlushedSegment, OutputSpan, Span from sentry.spans.consumers.process.flusher import MultiProducer, SpanFlusher from sentry.testutils.helpers.options import override_options from tests.sentry.spans.test_buffer import DEFAULT_OPTIONS diff --git a/tests/sentry/spans/test_buffer.py b/tests/sentry/spans/test_buffer.py index 8f8bc6825daf72..ae1fe4d1c388fa 100644 --- a/tests/sentry/spans/test_buffer.py +++ b/tests/sentry/spans/test_buffer.py @@ -15,14 +15,14 @@ from sentry.conf.types.kafka_definition import Topic, get_topic_codec from sentry.constants import DataCategory -from sentry.spans.buffer import ( - FlushedSegment, - OutputSpan, - SpansBuffer, -) +from sentry.spans.buffer import SpansBuffer from sentry.spans.buffer_types import ( EvalshaResult, + FlushCandidate, + FlushedSegment, InsertedSubsegment, + LoadedSegmentData, + OutputSpan, Span, Subsegment, ) @@ -1967,6 +1967,30 @@ def get(self, key: bytes) -> bytes | None: def zscore(self, key: bytes, value: bytes) -> float | None: return self.zsets.get(key, {}).get(value) + def zrangebyscore( + self, + key: bytes, + min_score: int, + max_score: int, + start: int = 0, + num: int | None = None, + withscores: bool = False, + ): + values = [ + (value, score) + for value, score in self.zsets.get(key, {}).items() + if min_score <= score <= max_score + ] + values.sort(key=lambda item: item[1]) + if num is not None: + values = values[start : start + num] + else: + values = values[start:] + + if withscores: + return values + return [value for value, _ in values] + class _LoadSegmentDataPipeline: def __init__(self, client: _LoadSegmentDataRedis) -> None: @@ -1988,6 +2012,17 @@ def sscan(self, key: bytes, cursor: int = 0, count: int | None = None) -> None: def get(self, key: bytes) -> None: self.commands.append(("get", (key,))) + def zrangebyscore( + self, + key: bytes, + min_score: int, + max_score: int, + start: int = 0, + num: int | None = None, + withscores: bool = False, + ) -> None: + self.commands.append(("zrangebyscore", (key, min_score, max_score, start, num, withscores))) + def execute(self): results = [] for command, args in self.commands: @@ -2015,6 +2050,73 @@ def load_segment_buffer() -> Iterator[SpansBuffer]: yield buffer +def test_load_flush_candidates_reads_ready_segments(load_segment_buffer: SpansBuffer) -> None: + buffer = load_segment_buffer + queue_key = buffer._get_queue_key(0) + ready_segment_key = _segment_id(1, "a" * 32, "b" * 16) + later_segment_key = _segment_id(1, "a" * 32, "c" * 16) + buffer.client.zadd(queue_key, {ready_segment_key: 5, later_segment_key: 15}) + + flush_candidates, load_ids_latency_ms = buffer._load_flush_candidates( + cutoff=10, + max_segments_per_shard=10, + ) + + assert flush_candidates == [FlushCandidate(0, queue_key, ready_segment_key, 5)] + assert load_ids_latency_ms >= 0 + + +def test_acquire_locks_for_flush_candidates_keeps_acquired_locks() -> None: + buffer = SpansBuffer(assigned_shards=[0]) + first_segment_key = _segment_id(1, "a" * 32, "b" * 16) + second_segment_key = _segment_id(1, "a" * 32, "c" * 16) + first_candidate = FlushCandidate(0, b"queue", first_segment_key, 5) + second_candidate = FlushCandidate(0, b"queue", second_segment_key, 10) + + with mock.patch.object( + buffer, "_acquire_flush_locks", return_value={second_segment_key} + ) as acquire_flush_locks: + flush_candidates = buffer._acquire_locks_for_flush_candidates( + [first_candidate, second_candidate] + ) + + assert flush_candidates == [second_candidate] + acquire_flush_locks.assert_called_once_with([first_segment_key, second_segment_key]) + + +def test_build_flushed_segments_adds_segment_metadata() -> None: + buffer = SpansBuffer(assigned_shards=[0]) + trace_id = "a" * 32 + segment_span_id = "b" * 16 + child_span_id = "c" * 16 + segment_key = _segment_id(1, trace_id, segment_span_id) + queue_key = buffer._get_queue_key(0) + payload_key = _payload_key(1, trace_id, "1" * 32) + + flushed_segments, num_has_root_spans, _ = buffer._build_flushed_segments( + [FlushCandidate(0, queue_key, segment_key, 5)], + LoadedSegmentData( + payloads={segment_key: [_payload(segment_span_id), _payload(child_span_id)]}, + payload_keys={segment_key: [payload_key]}, + ), + max_segments_per_shard=2, + now=10, + ) + + flushed_segment = flushed_segments[segment_key] + output_payloads = {span.payload["span_id"]: span.payload for span in flushed_segment.spans} + assert flushed_segment.queue_key == queue_key + assert flushed_segment.project_id == 1 + assert flushed_segment.payload_keys == [payload_key] + assert output_payloads[segment_span_id]["is_segment"] is True + assert output_payloads[child_span_id]["is_segment"] is False + assert ( + output_payloads[child_span_id]["attributes"]["sentry.segment.id"]["value"] + == segment_span_id + ) + assert num_has_root_spans == 1 + + def test_load_payload_keys_from_distributed_keys( load_segment_buffer: SpansBuffer, ) -> None: @@ -2077,7 +2179,7 @@ def test_load_segment_data_reads_payloads_from_distributed_keys( buffer.client.set(b"span-buf:ic:" + segment_key, 3) buffer.client.set(b"span-buf:ibc:" + segment_key, len(span_a) + len(span_b) + len(span_c)) - loaded_segment_data, _ = buffer._load_segment_data([segment_key]) + loaded_segment_data, _, _ = buffer._load_segment_data([segment_key]) assert set(loaded_segment_data.payloads[segment_key]) == {span_a, span_b, span_c} assert set(loaded_segment_data.payload_keys[segment_key]) == { @@ -2102,10 +2204,13 @@ def test_load_segment_data_decompresses_payload_batches( buffer.client.sadd(payload_key, compressed) buffer.client.set(b"span-buf:ic:" + segment_key, 2) - loaded_segment_data, decompress_latency_ms = buffer._load_segment_data([segment_key]) + loaded_segment_data, load_data_latency_ms, decompress_latency_ms = buffer._load_segment_data( + [segment_key] + ) assert set(loaded_segment_data.payloads[segment_key]) == {span_a, span_b} assert loaded_segment_data.payload_keys[segment_key] == [payload_key] + assert load_data_latency_ms >= 0 assert decompress_latency_ms >= 0 @@ -2130,7 +2235,7 @@ def test_record_segment_loss_metrics_records_dropped_spans( mock.patch("sentry.spans.buffer.track_outcome") as track_outcome, ): project_model.objects.get_from_cache.return_value = mock_project - loaded_segment_data, _ = buffer._load_segment_data([segment_key]) + loaded_segment_data, _, _ = buffer._load_segment_data([segment_key]) buffer._record_segment_loss_metrics( [segment_key], {}, @@ -2178,7 +2283,7 @@ def test_record_segment_loss_metrics_records_empty_expired_segments( buffer.client.zadd(queue_key, {segment_key: deadline}) with mock.patch("sentry.spans.buffer.metrics.incr") as metrics_incr: - loaded_segment_data, _ = buffer._load_segment_data([segment_key]) + loaded_segment_data, _, _ = buffer._load_segment_data([segment_key]) buffer._record_segment_loss_metrics( [segment_key], {segment_key: queue_key}, From 50400eb1f4d52d3c7c3a310667824d079f49cea0 Mon Sep 17 00:00:00 2001 From: Nick Date: Tue, 26 May 2026 13:30:33 -0300 Subject: [PATCH 06/61] fix(discover): Add org id to project filter (#116174) Add in org id to projects filter, and add a regression test. Closes BROWSE-519 --------- Co-authored-by: Claude Opus 4 --- src/sentry/data_export/processors/discover.py | 6 +++++- tests/sentry/data_export/processors/test_discover.py | 8 ++++++++ tests/sentry/data_export/test_tasks.py | 2 +- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/sentry/data_export/processors/discover.py b/src/sentry/data_export/processors/discover.py index 3aa75c1d166f61..272ade9dbffdaf 100644 --- a/src/sentry/data_export/processors/discover.py +++ b/src/sentry/data_export/processors/discover.py @@ -57,7 +57,11 @@ def __init__(self, organization: Organization, discover_query: dict[str, Any]): @staticmethod def get_projects(organization_id: int, query: dict[str, Any]) -> list[Project]: - projects = list(Project.objects.filter(id__in=query.get("project"))) + projects = list( + Project.objects.filter( + id__in=query.get("project") or [], organization_id=organization_id + ) + ) if len(projects) == 0: raise ExportError("Requested project does not exist") return projects diff --git a/tests/sentry/data_export/processors/test_discover.py b/tests/sentry/data_export/processors/test_discover.py index 9800ca6f7e10e8..ecae3eb052b793 100644 --- a/tests/sentry/data_export/processors/test_discover.py +++ b/tests/sentry/data_export/processors/test_discover.py @@ -35,6 +35,14 @@ def test_get_projects(self) -> None: with pytest.raises(ExportError): DiscoverProcessor.get_projects(organization_id=self.org.id, query={"project": [-1]}) + def test_get_projects_rejects_cross_org_project(self) -> None: + other_org = self.create_organization() + other_project = self.create_project(organization=other_org) + with pytest.raises(ExportError): + DiscoverProcessor.get_projects( + organization_id=self.org.id, query={"project": [other_project.id]} + ) + def test_handle_issue_id_fields(self) -> None: processor = DiscoverProcessor(organization=self.org, discover_query=self.discover_query) assert processor.header_fields == ["count_id", "fake_field", "issue"] diff --git a/tests/sentry/data_export/test_tasks.py b/tests/sentry/data_export/test_tasks.py index 0960f6c38de47f..129741bed5e33c 100644 --- a/tests/sentry/data_export/test_tasks.py +++ b/tests/sentry/data_export/test_tasks.py @@ -633,7 +633,7 @@ def setUp(self) -> None: super().setUp() self.user = self.create_user() self.org = self.create_organization() - self.project = self.create_project() + self.project = self.create_project(organization=self.org) self.data = load_data("transaction") @patch("sentry.data_export.tasks.MAX_BATCH_SIZE", 200) From d8c535549fba66725f409c46a7bb809f0d1eb382 Mon Sep 17 00:00:00 2001 From: Nick Date: Tue, 26 May 2026 13:30:50 -0300 Subject: [PATCH 07/61] fix(discover): Add missing check for DiscoverSavedQueryVisitEndpoint (#116187) This PR adds in a missing check in the `DiscoverSavedQueryVisitEndpoint` endpoint. Closes BROWSE-518 Co-authored-by: Claude Opus 4.6 --- .../endpoints/discover_saved_query_detail.py | 2 ++ .../test_discover_saved_query_detail.py | 17 +++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/src/sentry/discover/endpoints/discover_saved_query_detail.py b/src/sentry/discover/endpoints/discover_saved_query_detail.py index db245784a67c87..77703a1f165ef1 100644 --- a/src/sentry/discover/endpoints/discover_saved_query_detail.py +++ b/src/sentry/discover/endpoints/discover_saved_query_detail.py @@ -181,6 +181,8 @@ def post(self, request: Request, organization, query) -> Response: if not self.has_feature(organization, request): return self.respond(status=404) + self.check_object_permissions(request, query) + query.visits = F("visits") + 1 query.last_visited = timezone.now() query.save(update_fields=["visits", "last_visited"]) diff --git a/tests/snuba/api/endpoints/test_discover_saved_query_detail.py b/tests/snuba/api/endpoints/test_discover_saved_query_detail.py index 72ac78e68dfd73..31188fb976caea 100644 --- a/tests/snuba/api/endpoints/test_discover_saved_query_detail.py +++ b/tests/snuba/api/endpoints/test_discover_saved_query_detail.py @@ -624,3 +624,20 @@ def test_visit_query_no_access(self) -> None: query = DiscoverSavedQuery.objects.get(id=self.query.id) assert query.visits == 1 assert query.last_visited == last_visited + + def test_visit_disallow_when_no_project_access(self) -> None: + self.org.flags.allow_joinleave = False + self.org.save() + + user_no_team = self.create_user(is_superuser=False) + self.create_member(user=user_no_team, organization=self.org, role="member", teams=[]) + self.login_as(user_no_team) + + with self.feature("organizations:discover-query"): + response = self.client.post(self.url(self.query.id)) + + assert response.status_code == 403 + assert response.data == {"detail": "You do not have permission to perform this action."} + + query = DiscoverSavedQuery.objects.get(id=self.query.id) + assert query.visits == 1 From 54dfe15ebca123abe9650008a4c86697360d7d16 Mon Sep 17 00:00:00 2001 From: Isaac Wang Date: Tue, 26 May 2026 09:34:52 -0700 Subject: [PATCH 08/61] fix(search): Prevent Ask AI from doubling pasted query text (#116050) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Fixes a bug where pasting text into the search bar and clicking "Ask AI" would double the query text (e.g. "show me errors" → "show me errors show me errors") - Root cause: when the entire query is free text, the `initialSeerQuery` builder filters it out, falls back to the unfiltered query, then appends `inputValue` again - Adds `!inputValue` guard to the fallback branch in 4 strategy wrappers (Logs, Spans, Metrics, Discover Issues), matching the fix already present in `issueList/issueListSeerComboBox.tsx` Fixes AIML-2875 ## Test plan - [x] Existing `askSeerComboBox.spec.tsx` tests pass - [x] Lint passes on all changed files - [x] Manual: paste a natural language query into Logs/Spans/Metrics search bar → click Ask AI → query text should appear once, not doubled - [x] Manual: existing filter + pasted free text (e.g. `browser.name:Firefox` + paste "show me errors") → Ask AI shows `browser.name:Firefox show me errors` Co-authored-by: Claude Opus 4.6 --- static/app/views/discover/results/issueListSeerComboBox.tsx | 6 ++++-- static/app/views/explore/logs/logsTabSeerComboBox.tsx | 6 ++++-- static/app/views/explore/metrics/metricsTabSeerComboBox.tsx | 6 ++++-- static/app/views/explore/spans/spansTabSeerComboBox.tsx | 6 ++++-- 4 files changed, 16 insertions(+), 8 deletions(-) diff --git a/static/app/views/discover/results/issueListSeerComboBox.tsx b/static/app/views/discover/results/issueListSeerComboBox.tsx index 60812a4621b653..e7ab8a7009d64d 100644 --- a/static/app/views/discover/results/issueListSeerComboBox.tsx +++ b/static/app/views/discover/results/issueListSeerComboBox.tsx @@ -74,10 +74,12 @@ export function IssueListSeerComboBox({onSearch}: IssueListSeerComboBoxProps) { ?.join(' ') ?.trim(); - // Use filteredCommittedQuery if it exists and has content, otherwise fall back to queryToUse + // Use filteredCommittedQuery if it has content. + // Only fall back to queryToUse when there's no inputValue to filter by. + // This prevents duplication when the entire query is free text matching inputValue. if (filteredCommittedQuery && filteredCommittedQuery.length > 0) { initialSeerQuery = filteredCommittedQuery; - } else if (queryDetails?.queryToUse) { + } else if (!inputValue && queryDetails?.queryToUse) { initialSeerQuery = queryDetails.queryToUse; } diff --git a/static/app/views/explore/logs/logsTabSeerComboBox.tsx b/static/app/views/explore/logs/logsTabSeerComboBox.tsx index a011615f242ca1..012b4e2f4f3039 100644 --- a/static/app/views/explore/logs/logsTabSeerComboBox.tsx +++ b/static/app/views/explore/logs/logsTabSeerComboBox.tsx @@ -85,10 +85,12 @@ export function LogsTabSeerComboBox() { ?.join(' ') ?.trim(); - // Use filteredCommittedQuery if it exists and has content, otherwise fall back to queryToUse + // Use filteredCommittedQuery if it has content. + // Only fall back to queryToUse when there's no inputValue to filter by. + // This prevents duplication when the entire query is free text matching inputValue. if (filteredCommittedQuery && filteredCommittedQuery.length > 0) { initialSeerQuery = filteredCommittedQuery; - } else if (queryDetails?.queryToUse) { + } else if (!inputValue && queryDetails?.queryToUse) { initialSeerQuery = queryDetails.queryToUse; } diff --git a/static/app/views/explore/metrics/metricsTabSeerComboBox.tsx b/static/app/views/explore/metrics/metricsTabSeerComboBox.tsx index 442acdded38c6d..1c9132810284c6 100644 --- a/static/app/views/explore/metrics/metricsTabSeerComboBox.tsx +++ b/static/app/views/explore/metrics/metricsTabSeerComboBox.tsx @@ -108,10 +108,12 @@ export function MetricsTabSeerComboBox({traceMetric}: MetricsTabSeerComboBoxProp .join(' ') .trim(); - // Use filteredCommittedQuery if it exists and has content, otherwise fall back to queryToUse + // Use filteredCommittedQuery if it has content. + // Only fall back to queryToUse when there's no inputValue to filter by. + // This prevents duplication when the entire query is free text matching inputValue. if (filteredCommittedQuery && filteredCommittedQuery.length > 0) { initialSeerQuery = filteredCommittedQuery; - } else if (queryDetails.queryToUse) { + } else if (!inputValue && queryDetails.queryToUse) { initialSeerQuery = queryDetails.queryToUse; } diff --git a/static/app/views/explore/spans/spansTabSeerComboBox.tsx b/static/app/views/explore/spans/spansTabSeerComboBox.tsx index 5b0ba14570dbea..4689a37557c9bb 100644 --- a/static/app/views/explore/spans/spansTabSeerComboBox.tsx +++ b/static/app/views/explore/spans/spansTabSeerComboBox.tsx @@ -108,10 +108,12 @@ export function SpansTabSeerComboBox() { ?.join(' ') ?.trim(); - // Use filteredCommittedQuery if it exists and has content, otherwise fall back to queryToUse + // Use filteredCommittedQuery if it has content. + // Only fall back to queryToUse when there's no inputValue to filter by. + // This prevents duplication when the entire query is free text matching inputValue. if (filteredCommittedQuery && filteredCommittedQuery.length > 0) { initialSeerQuery = filteredCommittedQuery; - } else if (queryDetails?.queryToUse) { + } else if (!inputValue && queryDetails?.queryToUse) { initialSeerQuery = queryDetails.queryToUse; } From 49abc57f208c0e0e11615bb57309b3b713aaccbd Mon Sep 17 00:00:00 2001 From: Charlie Luo Date: Tue, 26 May 2026 12:36:25 -0400 Subject: [PATCH 09/61] fix(issues): match short id when combined with filters (#116153) If a user searches an issue short-id directly, we move them to the issue details page. However, if the user inputted the short id + any filters, we wouldn't match on that. Match short ids in those cases by scanning every query parameter. Co-authored-by: Claude --- src/sentry/api/helpers/group_index/index.py | 15 ++++++++---- .../test_organization_group_index.py | 23 +++++++++++++++++++ 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/sentry/api/helpers/group_index/index.py b/src/sentry/api/helpers/group_index/index.py index ef930c08ff022f..74c29660361895 100644 --- a/src/sentry/api/helpers/group_index/index.py +++ b/src/sentry/api/helpers/group_index/index.py @@ -175,11 +175,16 @@ def get_by_short_id( is_short_id_lookup: str, query: str, ) -> Group | None: - if is_short_id_lookup == "1" and looks_like_short_id(query): - try: - return Group.objects.by_qualified_short_id(organization_id, query) - except Group.DoesNotExist: - pass + if is_short_id_lookup != "1": + return None + # A short id token anywhere in the query is treated as a direct hit, + # so it composes with filters. + for token in query.split(): + if looks_like_short_id(token): + try: + return Group.objects.by_qualified_short_id(organization_id, token) + except Group.DoesNotExist: + continue return None diff --git a/tests/sentry/issues/endpoints/test_organization_group_index.py b/tests/sentry/issues/endpoints/test_organization_group_index.py index e0e955ccab90eb..7039f5bd06573f 100644 --- a/tests/sentry/issues/endpoints/test_organization_group_index.py +++ b/tests/sentry/issues/endpoints/test_organization_group_index.py @@ -732,6 +732,29 @@ def test_lookup_by_short_id_no_perms(self) -> None: assert len(response.data) == 0 assert response.get("X-Sentry-Direct-Hit") != "1" + def test_lookup_by_short_id_with_filter(self) -> None: + group = self.group + short_id = group.qualified_short_id + + self.login_as(user=self.user) + response = self.get_success_response(query=f"is:unresolved {short_id}", shortIdLookup=1) + assert len(response.data) == 1 + assert response.data[0]["id"] == str(group.id) + assert response["X-Sentry-Direct-Hit"] == "1" + + def test_lookup_by_short_id_with_filter_resolved(self) -> None: + group = self.group + group.status = GroupStatus.RESOLVED + group.substatus = None + group.save() + short_id = group.qualified_short_id + + self.login_as(user=self.user) + response = self.get_success_response(query=f"is:unresolved {short_id}", shortIdLookup=1) + assert len(response.data) == 1 + assert response.data[0]["id"] == str(group.id) + assert response["X-Sentry-Direct-Hit"] == "1" + def test_lookup_by_group_id(self) -> None: self.login_as(user=self.user) response = self.get_success_response(group=self.group.id) From f49b1af40696751a2351b94028aa7e2e9ed99769 Mon Sep 17 00:00:00 2001 From: Richard Roggenkemper <46740234+roggenkemper@users.noreply.github.com> Date: Tue, 26 May 2026 12:49:03 -0400 Subject: [PATCH 10/61] feat(search): Surface recommended sort in UI when active via query param (#116186) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `RECOMMENDED` to the `IssueSortOptions` enum with a "Recommended" label and tooltip. The option **only appears** in the sort dropdown when `?sort=recommended` is already in the URL query param. before: Screenshot 2026-05-26 at 10 51 20 AM after: Screenshot 2026-05-26 at 10 51 01 AM fixes https://linear.app/getsentry/issue/ID-1567/fix-none-dropdown-for-sort --- static/app/views/issueList/actions/sortOptions.tsx | 3 +++ static/app/views/issueList/utils.tsx | 3 +++ 2 files changed, 6 insertions(+) diff --git a/static/app/views/issueList/actions/sortOptions.tsx b/static/app/views/issueList/actions/sortOptions.tsx index 42367ca003b241..719f9551040e59 100644 --- a/static/app/views/issueList/actions/sortOptions.tsx +++ b/static/app/views/issueList/actions/sortOptions.tsx @@ -31,6 +31,8 @@ function getSortTooltip(key: IssueSortOptions) { return t('Number of events.'); case IssueSortOptions.USER: return t('Number of users affected.'); + case IssueSortOptions.RECOMMENDED: + return t('Issues ranked by combined recency, severity, and impact signals.'); case IssueSortOptions.DATE: default: return t('Last time the issue occurred.'); @@ -53,6 +55,7 @@ export function IssueListSortOptions({ IssueSortOptions.TRENDS, IssueSortOptions.FREQ, IssueSortOptions.USER, + ...(sort === IssueSortOptions.RECOMMENDED ? [IssueSortOptions.RECOMMENDED] : []), ]; return ( diff --git a/static/app/views/issueList/utils.tsx b/static/app/views/issueList/utils.tsx index 8eef70fd17aa48..2655ad97f341e7 100644 --- a/static/app/views/issueList/utils.tsx +++ b/static/app/views/issueList/utils.tsx @@ -30,6 +30,7 @@ export enum IssueSortOptions { FREQ = 'freq', USER = 'user', INBOX = 'inbox', + RECOMMENDED = 'recommended', } export const DEFAULT_ISSUE_STREAM_SORT = IssueSortOptions.DATE; @@ -46,6 +47,8 @@ export function getSortLabel(key: string) { return t('Users'); case IssueSortOptions.INBOX: return t('Date Added'); + case IssueSortOptions.RECOMMENDED: + return t('Recommended'); case IssueSortOptions.DATE: default: return t('Last Seen'); From 0db91ad472953e571d9628bc2a2143e0bb97478e Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Tue, 26 May 2026 18:55:07 +0200 Subject: [PATCH 11/61] chore(taskworker): Move devenv for profiles consumer to taskbroker (#116194) fix STREAM-1041 Remove the profiles consumer from sentry devserver (that cannot be supported, up to devinfra to make that happen with their own devservices stuff), and introduce a new taskbroker + worker instance to replace it. Taskbroker eventually will get a way to read from many topics, which would eliminate these extra containers entirely. Next steps are to roll this out to s4s2, then to prod, and finally remove the ingest-profiles consumer from code. --- devservices/config.yml | 44 ++++++++++++++++++++++--- src/sentry/runner/commands/devserver.py | 4 +-- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/devservices/config.yml b/devservices/config.yml index 81397fc1ee163d..176478efee9dd4 100644 --- a/devservices/config.yml +++ b/devservices/config.yml @@ -84,13 +84,18 @@ x-sentry-service-config: repo_name: chartcuterie branch: master repo_link: https://github.com/getsentry/chartcuterie.git - taskbroker: + taskbroker: &taskbroker description: Service used to process asynchronous tasks remote: repo_name: taskbroker branch: main repo_link: https://github.com/getsentry/taskbroker.git mode: containerized + # ingest-profiles runs taskbroker in passthrough mode (STREAM-1041). Ideally + # this would be a remote dependency like taskbroker, but devservices doesn't + # support passing custom environment variables to remote services (DI-1956). + ingest-profiles: + description: Kafka consumer for processing profiling data via taskbroker passthrough mode memcached: description: Memcached used for caching spotlight: @@ -125,6 +130,8 @@ x-sentry-service-config: description: Dedicated taskworker for eval Relay project config tasks taskworker-scheduler: description: Task scheduler that can spawn tasks based on their schedules + taskworker-ingest-profiles: + description: Taskworker for ingest-profiles passthrough broker # Kafka consumer services for event ingestion ingest-events: description: Kafka consumer for processing ingested events @@ -134,8 +141,6 @@ x-sentry-service-config: description: Kafka consumer for processing ingested transactions ingest-monitors: description: Kafka consumer for processing monitor check-ins - ingest-profiles: - description: Kafka consumer for processing profiling data ingest-occurrences: description: Kafka consumer for processing issue occurrences ingest-feedback-events: @@ -241,6 +246,7 @@ x-sentry-service-config: ingest-occurrences, taskbroker, taskworker, + taskworker-ingest-profiles, post-process-forwarder-errors, post-process-forwarder-transactions, ] @@ -333,6 +339,7 @@ x-sentry-service-config: spotlight, taskbroker, taskworker, + taskworker-ingest-profiles, vroom, ] full: @@ -367,6 +374,7 @@ x-sentry-service-config: post-process-forwarder-issue-platform, taskbroker, taskworker, + taskworker-ingest-profiles, taskworker-scheduler, ] @@ -385,6 +393,8 @@ x-programs: command: sentry run taskworker --namespace relay --concurrency 1 --processing-pool-name eval-relay taskworker-scheduler: command: sentry run taskworker-scheduler + taskworker-ingest-profiles: + command: sentry run taskworker --rpc-host localhost:50052 --namespace ingest.profiling.passthrough ingest-events: command: sentry run consumer ingest-events --consumer-group=sentry-consumer --auto-offset-reset=latest --no-strict-offset-reset ingest-attachments: @@ -399,8 +409,6 @@ x-programs: command: sentry run consumer monitors-clock-tasks --consumer-group=sentry-consumer --auto-offset-reset=latest --no-strict-offset-reset monitors-incident-occurrences: command: sentry run consumer monitors-incident-occurrences --consumer-group=sentry-consumer --auto-offset-reset=latest --no-strict-offset-reset - ingest-profiles: - command: sentry run consumer ingest-profiles --consumer-group=sentry-consumer --auto-offset-reset=latest --no-strict-offset-reset ingest-occurrences: command: sentry run consumer ingest-occurrences --consumer-group=sentry-consumer --auto-offset-reset=latest --no-strict-offset-reset process-spans: @@ -483,6 +491,32 @@ services: - devservices labels: - orchestrator=devservices + # Taskbroker in passthrough mode for ingest-profiles topic (STREAM-1041). + # This is a local service rather than a remote dependency because devservices + # doesn't support passing custom environment variables to remote services (DI-1956). + ingest-profiles: + image: ghcr.io/getsentry/taskbroker:nightly + environment: + TASKBROKER_KAFKA_CLUSTER: 'kafka-kafka-1:9093' + TASKBROKER_KAFKA_TOPIC: 'profiles' + TASKBROKER_KAFKA_CONSUMER_GROUP: 'ingest-profiles' + TASKBROKER_CREATE_MISSING_TOPICS: 'true' + TASKBROKER_RAW_MODE: 'true' + TASKBROKER_RAW_NAMESPACE: 'ingest.profiling.passthrough' + TASKBROKER_RAW_APPLICATION: 'sentry' + TASKBROKER_RAW_TASKNAME: 'sentry.profiles.task.process_profile_from_kafka' + # TODO(STREAM-1041): Remove debug logging once passthrough mode is stable + TASKBROKER_LOG_FILTER: 'debug' + ports: + - '127.0.0.1:50052:50051' + networks: + - devservices + extra_hosts: + - host.docker.internal:host-gateway + labels: + - orchestrator=devservices + restart: unless-stopped + platform: linux/amd64 networks: devservices: diff --git a/src/sentry/runner/commands/devserver.py b/src/sentry/runner/commands/devserver.py index 2ee49029679afc..c26098bf89ca42 100644 --- a/src/sentry/runner/commands/devserver.py +++ b/src/sentry/runner/commands/devserver.py @@ -332,8 +332,8 @@ def devserver( kafka_consumers.add("monitors-clock-tasks") kafka_consumers.add("monitors-incident-occurrences") - if settings.SENTRY_USE_PROFILING: - kafka_consumers.add("ingest-profiles") + # ingest-profiles is now handled by taskbroker passthrough mode + # via devservices (STREAM-1041) if settings.SENTRY_USE_SPANS_BUFFER: kafka_consumers.add("process-spans") From 131c63db2bb0eea786affc2344f967f5c57ddf80 Mon Sep 17 00:00:00 2001 From: Shashank Jarmale Date: Tue, 26 May 2026 10:01:31 -0700 Subject: [PATCH 12/61] chore(typing): Remove `tests.sentry.issues.test_utils` from mypy ignore list (#116070) Resolves [ENG-6461](https://linear.app/getsentry/issue/ENG-6461/remove-testssentryissuestest-utils-from-ignore-list). Fixes the `mypy` errors in `tests/sentry/issues/test_utils.py`. --- pyproject.toml | 1 - src/sentry/issues/ingest.py | 4 ++-- tests/sentry/issues/test_utils.py | 9 ++++++--- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 4330e14cf42188..83af6f485ede78 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -398,7 +398,6 @@ module = [ "sentry.snuba.metrics.query_builder", "sentry.testutils.cases", "tests.sentry.api.helpers.test_group_index", - "tests.sentry.issues.test_utils", ] disable_error_code = [ "arg-type", diff --git a/src/sentry/issues/ingest.py b/src/sentry/issues/ingest.py index 22d6096d9e4b59..45d3541bbdd7cf 100644 --- a/src/sentry/issues/ingest.py +++ b/src/sentry/issues/ingest.py @@ -1,7 +1,7 @@ from __future__ import annotations import logging -from collections.abc import Mapping +from collections.abc import Mapping, Sequence from datetime import datetime from hashlib import md5 from typing import Any, TypedDict @@ -104,7 +104,7 @@ def process_occurrence_data(data: dict[str, Any]) -> None: data["fingerprint"] = hash_fingerprint(data["fingerprint"]) -def hash_fingerprint(fingerprint: list[str]) -> list[str]: +def hash_fingerprint(fingerprint: Sequence[str]) -> list[str]: return [md5(part.encode("utf-8")).hexdigest() for part in fingerprint] diff --git a/tests/sentry/issues/test_utils.py b/tests/sentry/issues/test_utils.py index 6a4bbdac99b259..c568432f1581b0 100644 --- a/tests/sentry/issues/test_utils.py +++ b/tests/sentry/issues/test_utils.py @@ -10,7 +10,7 @@ from sentry.event_manager import GroupInfo from sentry.issues.escalating.escalating import GroupsCountResponse from sentry.issues.grouptype import ProfileFileIOGroupType -from sentry.issues.ingest import process_occurrence_data, save_issue_occurrence +from sentry.issues.ingest import hash_fingerprint, save_issue_occurrence from sentry.issues.issue_occurrence import IssueEvidence, IssueOccurrence, IssueOccurrenceData from sentry.issues.occurrence_consumer import process_event_and_issue_occurrence from sentry.issues.status_change_message import StatusChangeMessage, StatusChangeMessageData @@ -58,7 +58,7 @@ def build_occurrence_data(self, **overrides: Any) -> IssueOccurrenceData: } kwargs.update(overrides) # type: ignore[typeddict-item] - process_occurrence_data(kwargs) + kwargs["fingerprint"] = hash_fingerprint(kwargs["fingerprint"]) return kwargs def build_occurrence(self, **overrides: Any) -> IssueOccurrence: @@ -99,7 +99,7 @@ def build_statuschange_data(self, **overrides: Any) -> StatusChangeMessageData: } kwargs.update(overrides) # type: ignore[typeddict-item] - process_occurrence_data(kwargs) + kwargs["fingerprint"] = hash_fingerprint(kwargs["fingerprint"]) return kwargs def build_statuschange(self, **overrides: Any) -> StatusChangeMessage: @@ -107,6 +107,9 @@ def build_statuschange(self, **overrides: Any) -> StatusChangeMessage: class SearchIssueTestMixin(OccurrenceTestMixin): + def store_event(self, data: dict[str, Any], project_id: int, **kwargs: Any) -> Event: + raise NotImplementedError + def store_search_issue( self, project_id: int, From bd1a8f715ca92bbb8010a9d4aba566db528a96e6 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 26 May 2026 10:22:41 -0700 Subject: [PATCH 13/61] feat(alerts): Add cleanup task to NotificationMessage (#116027) Adds a cleanup task to evict `NotificationMessage` rows older than 90 days. --- src/sentry/runner/commands/cleanup.py | 38 +++++++++- tests/sentry/runner/commands/test_cleanup.py | 78 ++++++++++++++++++++ 2 files changed, 115 insertions(+), 1 deletion(-) diff --git a/src/sentry/runner/commands/cleanup.py b/src/sentry/runner/commands/cleanup.py index 9c08b979df23d8..88aa6140b96077 100644 --- a/src/sentry/runner/commands/cleanup.py +++ b/src/sentry/runner/commands/cleanup.py @@ -15,7 +15,7 @@ import sentry_sdk from django.conf import settings from django.db import router as db_router -from django.db.models import QuerySet +from django.db.models import Exists, OuterRef, QuerySet from django.utils import timezone from sentry_sdk import capture_exception @@ -526,6 +526,7 @@ def _run_specialized_cleanups( remove_expired_values_for_org_members(is_filtered, days, models_attempted) delete_api_models(is_filtered, models_attempted) exported_data(is_filtered, models_attempted) + remove_old_notification_messages(is_filtered, models_attempted) def _handle_project_organization_cleanup( @@ -1163,3 +1164,38 @@ def cleanup_unused_files() -> None: if File.objects.filter(blob=blob).exists(): continue blob.delete() + + +@continue_on_error("specialized_cleanup_notification_messages") +def remove_old_notification_messages( + is_filtered: Callable[[type[BaseModel]], bool], models_attempted: set[str] +) -> None: + from sentry.notifications.models.notificationmessage import NotificationMessage + + NOTIFICATION_MESSAGE_TTL_IN_DAYS = 90 + NOTIFICATION_MESSAGE_BATCH_SIZE = 1000 + + if is_filtered(NotificationMessage): + debug_output(">> Skipping NotificationMessage") + return + + debug_output("Removing expired values for NotificationMessage") + models_attempted.add(NotificationMessage.__name__.lower()) + + cutoff = timezone.now() - timedelta(days=NOTIFICATION_MESSAGE_TTL_IN_DAYS) + + # only delete rows with no child instance + has_child = NotificationMessage.objects.filter(parent_notification_message_id=OuterRef("id")) + + while True: + ids = list( + NotificationMessage.objects.filter(date_added__lt=cutoff) + .annotate(has_child=Exists(has_child)) + .filter(has_child=False) + .values_list("id", flat=True) + .order_by("id")[:NOTIFICATION_MESSAGE_BATCH_SIZE] + ) + if not ids: + break + + NotificationMessage.objects.filter(id__in=ids).delete() diff --git a/tests/sentry/runner/commands/test_cleanup.py b/tests/sentry/runner/commands/test_cleanup.py index bd85c4b303733f..0a3c5debe066a3 100644 --- a/tests/sentry/runner/commands/test_cleanup.py +++ b/tests/sentry/runner/commands/test_cleanup.py @@ -2,6 +2,7 @@ from datetime import timedelta from io import BytesIO +from typing import Any from unittest.mock import patch import click @@ -11,12 +12,14 @@ from sentry.models.files.file import File from sentry.models.group import Group from sentry.models.groupopenperiodactivity import GroupOpenPeriodActivity +from sentry.notifications.models.notificationmessage import NotificationMessage from sentry.runner.commands.cleanup import ( _cleanup, generate_bulk_query_deletes, models_which_use_expiry_deletions, prepare_deletes_by_project, remove_cross_project_bulk_query_models, + remove_old_notification_messages, run_bulk_deletes_by_project, task_execution, ) @@ -391,3 +394,78 @@ def test_expiry_cleanup_ignores_days_flag(self) -> None: assert not EventAttachment.objects.filter(id=expired.id).exists() assert EventAttachment.objects.filter(id=fresh.id).exists() + + +class RemoveOldNotificationMessagesTest(TestCase): + def setUp(self) -> None: + super().setUp() + self.NOTIFICATION_MESSAGE_TTL_IN_DAYS = 90 + self.NotificationMessage = NotificationMessage + self.action = self.create_action() + self.group = self.create_group() + + def _create_message(self, days_old: int, **kwargs: Any) -> NotificationMessage: + from django.utils import timezone + + kwargs.setdefault("action", self.action) + kwargs.setdefault("group", self.group) + message = self.NotificationMessage.objects.create(**kwargs) + self.NotificationMessage.objects.filter(id=message.id).update( + date_added=timezone.now() - timedelta(days=days_old) + ) + message.refresh_from_db() + return message + + def test_deletes_messages_older_than_retention(self) -> None: + self._create_message(days_old=self.NOTIFICATION_MESSAGE_TTL_IN_DAYS + 1) + self._create_message(days_old=self.NOTIFICATION_MESSAGE_TTL_IN_DAYS + 365) + recent = self._create_message(days_old=self.NOTIFICATION_MESSAGE_TTL_IN_DAYS - 1) + brand_new = self._create_message(days_old=0) + + remove_old_notification_messages(lambda model: False, set()) + + remaining_ids = set(self.NotificationMessage.objects.values_list("id", flat=True)) + assert remaining_ids == {recent.id, brand_new.id} + + def test_no_op_when_nothing_to_delete(self) -> None: + recent = self._create_message(days_old=1) + + remove_old_notification_messages(lambda model: False, set()) + + assert self.NotificationMessage.objects.filter(id=recent.id).exists() + + def test_cascades_self_reference(self) -> None: + parent = self._create_message(days_old=self.NOTIFICATION_MESSAGE_TTL_IN_DAYS + 5) + child = self._create_message( + days_old=self.NOTIFICATION_MESSAGE_TTL_IN_DAYS + 5, + parent_notification_message=parent, + ) + + remove_old_notification_messages(lambda model: False, set()) + + assert not self.NotificationMessage.objects.filter(id__in=[parent.id, child.id]).exists() + + def test_keeps_old_parent_with_in_retention_child(self) -> None: + parent = self._create_message(days_old=self.NOTIFICATION_MESSAGE_TTL_IN_DAYS + 5) + child = self._create_message( + days_old=self.NOTIFICATION_MESSAGE_TTL_IN_DAYS - 1, + parent_notification_message=parent, + ) + + remove_old_notification_messages(lambda model: False, set()) + + remaining_ids = set(self.NotificationMessage.objects.values_list("id", flat=True)) + assert remaining_ids == {parent.id, child.id} + + def test_respects_is_filtered(self) -> None: + old = self._create_message(days_old=self.NOTIFICATION_MESSAGE_TTL_IN_DAYS + 1) + + remove_old_notification_messages(lambda model: True, set()) + + assert self.NotificationMessage.objects.filter(id=old.id).exists() + + def test_records_models_attempted(self) -> None: + models_attempted: set[str] = set() + remove_old_notification_messages(lambda model: False, models_attempted) + + assert "notificationmessage" in models_attempted From 5b9be1fbdeac0135778267aa8bd263e3b184d65d Mon Sep 17 00:00:00 2001 From: Charlie Luo Date: Tue, 26 May 2026 13:23:37 -0400 Subject: [PATCH 14/61] fix(supergroups): move to post process task (#116195) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We were previously using the ingest-errors queue by mistake — switch to post process --- src/sentry/tasks/seer/lightweight_rca_cluster.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/sentry/tasks/seer/lightweight_rca_cluster.py b/src/sentry/tasks/seer/lightweight_rca_cluster.py index 9d8abcbb4dc75a..557fe964e90fe7 100644 --- a/src/sentry/tasks/seer/lightweight_rca_cluster.py +++ b/src/sentry/tasks/seer/lightweight_rca_cluster.py @@ -3,14 +3,15 @@ from sentry.models.group import Group from sentry.seer.supergroups.lightweight_rca_cluster import trigger_lightweight_rca_cluster from sentry.tasks.base import instrumented_task -from sentry.taskworker.namespaces import ingest_errors_tasks +from sentry.taskworker.namespaces import ingest_errors_postprocess_tasks, ingest_errors_tasks logger = logging.getLogger(__name__) @instrumented_task( name="sentry.tasks.seer.lightweight_rca_cluster.trigger_lightweight_rca_cluster_task", - namespace=ingest_errors_tasks, + namespace=ingest_errors_postprocess_tasks, + alias_namespace=ingest_errors_tasks, ) def trigger_lightweight_rca_cluster_task(group_id: int, **kwargs) -> None: try: From 9d49bb6befeed8f40e58cd57908c7775d457b6a3 Mon Sep 17 00:00:00 2001 From: Katie Byers Date: Tue, 26 May 2026 10:32:17 -0700 Subject: [PATCH 15/61] ref(utils): Various clarifications in `SafeRolloutComparator` code (#115946) 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. --- src/sentry/api/bases/organization_events.py | 2 +- .../endpoints/organization_events_trace.py | 4 +- .../api/endpoints/organization_trace_meta.py | 2 +- .../api/endpoints/organization_traces.py | 4 +- .../utils/get_errors_counts_timeseries.py | 2 +- src/sentry/api/helpers/events.py | 2 +- .../source_code_management/commit_context.py | 2 +- src/sentry/issues/escalating/escalating.py | 2 +- src/sentry/issues/related/trace_connected.py | 2 +- src/sentry/monitors/utils.py | 2 +- src/sentry/reprocessing2.py | 2 +- .../search/eap/occurrences/rollout_utils.py | 8 + src/sentry/search/snuba/executors.py | 4 +- .../services/eventstore/snuba/backend.py | 4 +- src/sentry/snuba/trace.py | 2 +- src/sentry/tagstore/snuba/backend.py | 16 +- src/sentry/tasks/summaries/utils.py | 4 +- src/sentry/utils/rollout.py | 278 ++++++++++-------- .../issues/endpoints/test_group_events.py | 4 +- .../services/eventstore/snuba/test_backend.py | 9 +- tests/sentry/utils/test_rollout.py | 88 +++--- .../test_organization_events_occurrences.py | 10 +- ...anization_events_timeseries_occurrences.py | 6 +- .../test_organization_events_trace.py | 17 +- .../endpoints/test_organization_trace_meta.py | 19 +- tests/snuba/tagstore/test_tagstore_backend.py | 4 +- 26 files changed, 272 insertions(+), 227 deletions(-) diff --git a/src/sentry/api/bases/organization_events.py b/src/sentry/api/bases/organization_events.py index ee2821e4f35866..225bb94a439595 100644 --- a/src/sentry/api/bases/organization_events.py +++ b/src/sentry/api/bases/organization_events.py @@ -152,7 +152,7 @@ def get_dataset(self, request: Request, organization: Organization) -> Any: # Feature flag the occurrence endpoint if ( dataset_label == SupportedTraceItemType.OCCURRENCES.value - and not EAPOccurrencesComparator.should_use_experiment("api.events.endpoints") + and not EAPOccurrencesComparator.should_use_experimental_data("api.events.endpoints") ): raise ParseError(detail=f"{dataset_label} is not supported currently") elif dataset_label == SupportedTraceItemType.REPLAYS.value and not features.has( diff --git a/src/sentry/api/endpoints/organization_events_trace.py b/src/sentry/api/endpoints/organization_events_trace.py index a46ec41d5c3237..85a9183cd73b06 100644 --- a/src/sentry/api/endpoints/organization_events_trace.py +++ b/src/sentry/api/endpoints/organization_events_trace.py @@ -360,7 +360,7 @@ def load_performance_issues(self, light: bool, snuba_params: SnubaParams) -> Non control_data=occurrence_ids, experimental_data=eap_occurrence_ids, callsite=callsite, - is_experimental_data_a_null_result=len(eap_occurrence_ids) == 0, + is_experimental_data_nullish=len(eap_occurrence_ids) == 0, reasonable_match_comparator=lambda snuba, eap: { row["occurrence_id"] for row in eap }.issubset({row["occurrence_id"] for row in snuba}), @@ -827,7 +827,7 @@ def query_trace_data( control_data=transformed_results[1], experimental_data=eap_errors, callsite=errors_callsite, - is_experimental_data_a_null_result=len(eap_errors) == 0, + is_experimental_data_nullish=len(eap_errors) == 0, reasonable_match_comparator=lambda snuba, eap: {e["id"] for e in eap}.issubset( {e["id"] for e in snuba} ), diff --git a/src/sentry/api/endpoints/organization_trace_meta.py b/src/sentry/api/endpoints/organization_trace_meta.py index 704b40bad04820..e20c0b3e00c81f 100644 --- a/src/sentry/api/endpoints/organization_trace_meta.py +++ b/src/sentry/api/endpoints/organization_trace_meta.py @@ -94,7 +94,7 @@ def run_errors_query(trace_id: str, snuba_params: SnubaParams) -> int: snuba_count, eap_count, callsite, - is_experimental_data_a_null_result=(eap_count == 0), + is_experimental_data_nullish=(eap_count == 0), reasonable_match_comparator=lambda snuba, eap: eap <= snuba, debug_context={ "trace_id": trace_id, diff --git a/src/sentry/api/endpoints/organization_traces.py b/src/sentry/api/endpoints/organization_traces.py index 6f534e1474dccc..a18d13551db1f1 100644 --- a/src/sentry/api/endpoints/organization_traces.py +++ b/src/sentry/api/endpoints/organization_traces.py @@ -369,7 +369,7 @@ def enrich_eap_traces_with_extra_data( traces_errors, eap_traces_errors, errors_callsite, - is_experimental_data_a_null_result=len(eap_traces_errors) == 0, + is_experimental_data_nullish=len(eap_traces_errors) == 0, reasonable_match_comparator=_reasonable_trace_count_map_match, debug_context=debug_context, ) @@ -386,7 +386,7 @@ def enrich_eap_traces_with_extra_data( traces_occurrences, eap_traces_occurrences, occurrences_callsite, - is_experimental_data_a_null_result=len(eap_traces_occurrences) == 0, + is_experimental_data_nullish=len(eap_traces_occurrences) == 0, reasonable_match_comparator=_reasonable_trace_count_map_match, debug_context=debug_context, ) diff --git a/src/sentry/api/endpoints/release_thresholds/utils/get_errors_counts_timeseries.py b/src/sentry/api/endpoints/release_thresholds/utils/get_errors_counts_timeseries.py index 5571be13a193d0..01c14a8b9ec85f 100644 --- a/src/sentry/api/endpoints/release_thresholds/utils/get_errors_counts_timeseries.py +++ b/src/sentry/api/endpoints/release_thresholds/utils/get_errors_counts_timeseries.py @@ -61,7 +61,7 @@ def get_errors_counts_timeseries_by_project_and_release( snuba_results, eap_results, "release_thresholds.get_errors_counts_timeseries", - is_experimental_data_a_null_result=len(eap_results) == 0, + is_experimental_data_nullish=len(eap_results) == 0, reasonable_match_comparator=lambda snuba_rows, eap_rows: keyed_counts_subset_match( snuba_rows, eap_rows, diff --git a/src/sentry/api/helpers/events.py b/src/sentry/api/helpers/events.py index 2161c40f2fec3e..4e35a3d088a08f 100644 --- a/src/sentry/api/helpers/events.py +++ b/src/sentry/api/helpers/events.py @@ -191,7 +191,7 @@ def run_group_events_query( snuba_data, eap_data, callsite, - is_experimental_data_a_null_result=len(eap_data) == 0, + is_experimental_data_nullish=len(eap_data) == 0, reasonable_match_comparator=_reasonable_group_events_match, debug_context={ "group_id": group.id, diff --git a/src/sentry/integrations/source_code_management/commit_context.py b/src/sentry/integrations/source_code_management/commit_context.py index 777ef2fbb2c235..2f0064f6aaf493 100644 --- a/src/sentry/integrations/source_code_management/commit_context.py +++ b/src/sentry/integrations/source_code_management/commit_context.py @@ -594,7 +594,7 @@ def get_top_5_issues_by_count( snuba_results, eap_results, "integrations.pr_comment.get_top_5_issues_by_count", - is_experimental_data_a_null_result=len(eap_results) == 0, + is_experimental_data_nullish=len(eap_results) == 0, reasonable_match_comparator=lambda snuba_rows, eap_rows: keyed_counts_subset_match( snuba_rows, eap_rows, diff --git a/src/sentry/issues/escalating/escalating.py b/src/sentry/issues/escalating/escalating.py index 30ec7894d028c6..c0f7d3aa31b010 100644 --- a/src/sentry/issues/escalating/escalating.py +++ b/src/sentry/issues/escalating/escalating.py @@ -103,7 +103,7 @@ def query_groups_past_counts(groups: Iterable[Group]) -> list[GroupsCountRespons snuba_results, eap_results, "issues.escalating.query_groups_past_counts", - is_experimental_data_a_null_result=len(eap_results) == 0, + is_experimental_data_nullish=len(eap_results) == 0, reasonable_match_comparator=lambda snuba_rows, eap_rows: keyed_counts_subset_match( snuba_rows, eap_rows, diff --git a/src/sentry/issues/related/trace_connected.py b/src/sentry/issues/related/trace_connected.py index bdb39a00d0786e..aa6d106b20c4cf 100644 --- a/src/sentry/issues/related/trace_connected.py +++ b/src/sentry/issues/related/trace_connected.py @@ -134,7 +134,7 @@ def trace_connected_issues(event: Event | GroupEvent) -> tuple[list[int], dict[s snuba_results, eap_results, "issues.related.trace_connected_issues", - is_experimental_data_a_null_result=len(eap_results) == 0, + is_experimental_data_nullish=len(eap_results) == 0, reasonable_match_comparator=lambda snuba, eap: eap.issubset(snuba), debug_context={ "trace_id": event.trace_id, diff --git a/src/sentry/monitors/utils.py b/src/sentry/monitors/utils.py index 61307c1bb25d17..dc0b72b1551811 100644 --- a/src/sentry/monitors/utils.py +++ b/src/sentry/monitors/utils.py @@ -277,7 +277,7 @@ def fetch_associated_groups( snuba_result, eap_result, callsite, - is_experimental_data_a_null_result=len(eap_result) == 0, + is_experimental_data_nullish=len(eap_result) == 0, reasonable_match_comparator=lambda snuba, eap: ( eap.keys() <= snuba.keys() and all(eap[gid] <= snuba[gid] for gid in eap) ), diff --git a/src/sentry/reprocessing2.py b/src/sentry/reprocessing2.py index 81f5f8299c2844..381c83aff41872 100644 --- a/src/sentry/reprocessing2.py +++ b/src/sentry/reprocessing2.py @@ -664,7 +664,7 @@ def start_group_reprocessing( control_data=snuba_count, experimental_data=eap_count, callsite=callsite, - is_experimental_data_a_null_result=eap_count == 0, + is_experimental_data_nullish=eap_count == 0, reasonable_match_comparator=lambda snuba, eap: eap <= snuba, debug_context={ "organization_id": organization.id, diff --git a/src/sentry/search/eap/occurrences/rollout_utils.py b/src/sentry/search/eap/occurrences/rollout_utils.py index 55242c9ad05ebf..4810e1fd885a4c 100644 --- a/src/sentry/search/eap/occurrences/rollout_utils.py +++ b/src/sentry/search/eap/occurrences/rollout_utils.py @@ -3,3 +3,11 @@ class EAPOccurrencesComparator(SafeRolloutComparator): ROLLOUT_NAME = "occurrences_on_eap" + + +EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION = ( + EAPOccurrencesComparator._should_run_experiment_option() +) +EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION = ( + EAPOccurrencesComparator._callsite_use_experimental_data_allowlist_option() +) diff --git a/src/sentry/search/snuba/executors.py b/src/sentry/search/snuba/executors.py index 79fd2c2931526f..f5a1d293720672 100644 --- a/src/sentry/search/snuba/executors.py +++ b/src/sentry/search/snuba/executors.py @@ -562,8 +562,8 @@ def _run_eap_query() -> tuple[list[tuple[int, Any]], int]: ): try: return EAPOccurrencesComparator.check_and_choose_with_timings( - control_thunk=_run_snuba_query, - experimental_thunk=_run_eap_query, + control_data_func=_run_snuba_query, + experimental_data_func=_run_eap_query, callsite=callsite, null_result_determiner=lambda r: len(r[0]) == 0, reasonable_match_comparator=_reasonable_search_result_match, diff --git a/src/sentry/services/eventstore/snuba/backend.py b/src/sentry/services/eventstore/snuba/backend.py index 1dac358f108133..2d677de72d2ba6 100644 --- a/src/sentry/services/eventstore/snuba/backend.py +++ b/src/sentry/services/eventstore/snuba/backend.py @@ -410,7 +410,7 @@ def __get_events( control_data=control_data, experimental_data=experimental_data, callsite=callsite, - is_experimental_data_a_null_result=eap_results is None, + is_experimental_data_nullish=eap_results is None, reasonable_match_comparator=lambda ctl, exp: exp.issubset(ctl), debug_context={ "project_ids": list(filter.project_ids) if filter.project_ids else [], @@ -584,7 +584,7 @@ def get_event_by_id( control_data=control_group_id, experimental_data=eap_group_id, callsite=callsite, - is_experimental_data_a_null_result=eap_result is None, + is_experimental_data_nullish=eap_result is None, reasonable_match_comparator=lambda snuba, eap: snuba == eap, debug_context={ "project_id": project_id, diff --git a/src/sentry/snuba/trace.py b/src/sentry/snuba/trace.py index 408c689c703958..16b16b6227b79a 100644 --- a/src/sentry/snuba/trace.py +++ b/src/sentry/snuba/trace.py @@ -697,7 +697,7 @@ def query_trace_data( control_data=errors_data, experimental_data=eap_errors_data, callsite=callsite, - is_experimental_data_a_null_result=len(eap_errors_data) == 0, + is_experimental_data_nullish=len(eap_errors_data) == 0, reasonable_match_comparator=lambda snuba, eap: {e["id"] for e in eap}.issubset( {e["id"] for e in snuba} ), diff --git a/src/sentry/tagstore/snuba/backend.py b/src/sentry/tagstore/snuba/backend.py index 2048d20eb963e2..ddb8e1576c5667 100644 --- a/src/sentry/tagstore/snuba/backend.py +++ b/src/sentry/tagstore/snuba/backend.py @@ -508,7 +508,7 @@ def serialize_group_tag_key(item: GroupTagKey) -> dict[str, Any]: snuba_output, eap_output, eap_callsite, - is_experimental_data_a_null_result=eap_output.count == 0, + is_experimental_data_nullish=eap_output.count == 0, reasonable_match_comparator=reasonable_group_tag_key_match, debug_context={ "group_id": group.id, @@ -917,7 +917,7 @@ def get_group_list_tag_value( control_data=snuba_result, experimental_data=eap_result, callsite=callsite, - is_experimental_data_a_null_result=len(eap_result) == 0, + is_experimental_data_nullish=len(eap_result) == 0, reasonable_match_comparator=_reasonable_group_list_tag_value_match, debug_context={ "project_ids": list(project_ids), @@ -1005,7 +1005,7 @@ def get_generic_group_list_tag_value( control_data=snuba_result, experimental_data=eap_result, callsite=callsite, - is_experimental_data_a_null_result=len(eap_result) == 0, + is_experimental_data_nullish=len(eap_result) == 0, reasonable_match_comparator=_reasonable_group_list_tag_value_match, debug_context={ "project_ids": list(project_ids), @@ -1171,7 +1171,7 @@ def get_group_tag_value_count( control_data=snuba_result, experimental_data=eap_result, callsite=callsite, - is_experimental_data_a_null_result=eap_result == 0, + is_experimental_data_nullish=eap_result == 0, reasonable_match_comparator=lambda control, experimental: experimental <= control, debug_context={ "group_id": group.id, @@ -1401,7 +1401,7 @@ def get_release_tags(self, organization_id, project_ids, environment_id, version control_data=snuba_result, experimental_data=eap_result, callsite=callsite, - is_experimental_data_a_null_result=len(eap_result) == 0, + is_experimental_data_nullish=len(eap_result) == 0, reasonable_match_comparator=_reasonable_release_tags_match, debug_context={ "organization_id": organization_id, @@ -1596,7 +1596,7 @@ def get_groups_user_counts( control_data=snuba_result, experimental_data=eap_result, callsite=callsite, - is_experimental_data_a_null_result=len(eap_result) == 0, + is_experimental_data_nullish=len(eap_result) == 0, reasonable_match_comparator=_reasonable_user_counts_match, debug_context={ "project_ids": list(project_ids), @@ -1747,7 +1747,7 @@ def get_generic_groups_user_counts( control_data=snuba_result, experimental_data=eap_result, callsite=callsite, - is_experimental_data_a_null_result=len(eap_result) == 0, + is_experimental_data_nullish=len(eap_result) == 0, reasonable_match_comparator=_reasonable_user_counts_match, debug_context={ "project_ids": list(project_ids), @@ -2286,7 +2286,7 @@ def get_group_tag_value_iter( control_data=snuba_result, experimental_data=eap_result, callsite=callsite, - is_experimental_data_a_null_result=len(eap_result) == 0, + is_experimental_data_nullish=len(eap_result) == 0, reasonable_match_comparator=_reasonable_group_tag_value_iter_match, debug_context={ "group_id": group.id, diff --git a/src/sentry/tasks/summaries/utils.py b/src/sentry/tasks/summaries/utils.py index f6883de820d69c..12e323d022d59f 100644 --- a/src/sentry/tasks/summaries/utils.py +++ b/src/sentry/tasks/summaries/utils.py @@ -157,7 +157,7 @@ def project_key_errors( snuba_rows, eap_rows, callsite, - is_experimental_data_a_null_result=len(eap_rows) == 0, + is_experimental_data_nullish=len(eap_rows) == 0, reasonable_match_comparator=lambda snuba, eap: keyed_counts_subset_match( snuba, eap, @@ -352,7 +352,7 @@ def project_key_performance_issues(ctx: OrganizationReportContext, project: Proj snuba_rows, eap_rows, callsite, - is_experimental_data_a_null_result=len(eap_rows) == 0, + is_experimental_data_nullish=len(eap_rows) == 0, reasonable_match_comparator=lambda snuba, eap: keyed_counts_subset_match( snuba, eap, diff --git a/src/sentry/utils/rollout.py b/src/sentry/utils/rollout.py index 03284686e1c6ce..b3c2f24f79b5a9 100644 --- a/src/sentry/utils/rollout.py +++ b/src/sentry/utils/rollout.py @@ -4,145 +4,160 @@ from typing import Any, TypeVar from sentry import options -from sentry.utils import metrics -from sentry.utils.safe import trim - -TData = TypeVar("TData") -logger = logging.getLogger(__name__) - +from sentry.options import register from sentry.options.manager import ( FLAG_ALLOW_EMPTY, FLAG_AUTOMATOR_MODIFIABLE, FLAG_MODIFIABLE_BOOL, FLAG_MODIFIABLE_RATE, ) +from sentry.utils import metrics +from sentry.utils.safe import trim from sentry.utils.types import Bool, Float, Sequence +logger = logging.getLogger(__name__) + +TData = TypeVar("TData") + class SafeRolloutComparator: """ SafeRolloutComparator is a tool designed to help you roll out a change to existing logic safely. - In particular, it can (at a callsite-by-callsite granularity) help to track both the - _exact_ and _reasonable_ rate at which the experimental branch matches the control branch. - Once a callsite looks correct enough, you can switch the code behavior to actually use the - data from the experimental branch. + In particular, it can (with callsite-by-callsite granularity) help to track rate at which the + experimental branch both exactly matches and "reasonably" matches the control branch. (What + counts as a "reasonable" (close enough) match is definable by providing a comparison function.) + Once a callsite looks correct enough, you can switch the code behavior to actually use the data + from the experimental branch by adding the callsite indentifier to the "use experimental data" + allowlist option provided by the class. The flow is generally: - 1. Set up your SafeRolloutComparator class & options. - 2. Add your first callsite. (Further callsites can be added at any time.) - 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). + 2. Use the comparator in your first callsite (see example below). (More callsites can be added + at any time.) + 3. 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%.) + 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 experimental data" allowlist. 6. Complete your migration, secure in your knowledge that it's safe to do so. - 7. Clean up your control branch & SafeRolloutComparator when you're done. Success! + 7. Clean up your control branch & `SafeRolloutComparator` when you're done. Success! Used like: - ```python - callsite = "BarClass::baz" - control_data = old_slow_trustworthy_method() - if FooComparator.should_check_experiment(callsite): - experimental_data = new_fast_risky_method() - data = FooComparator.check_and_choose( - control_data, - experimental_data, - callsite, - len(experimental_data) == 0, - lambda ctl, exp: exp.issubset(ctl) - ) - else: - data = control_data + ``` + # Setup + class FooComparator(SafeRolloutComparator): + ROLLOUT_NAME="some_new_feature" + + # Example callsite: + def some_function(): + # ... + + # A unique identifier for the callsite, for option names and metrics/logs tagging + callsite = "some.module.path.some_function" + + control_data = old_slow_trustworthy_method() + if FooComparator.should_check_experiment(callsite): + experimental_data = new_fast_risky_method() + data = FooComparator.check_and_choose( + control_data, + experimental_data, + callsite, + is_experimental_data_nullish=len(experimental_data) == 0, + reasonable_match_comparator=lambda ctl, exp: exp.issubset(ctl) + ) + else: + data = control_data ``` """ - # This is your rollout, which determines your option names and which you can filter - # the DataDog dashboards to show. - # TODO @cpaul: construct DataDog dashboards once you have a rollout using this. + # This identifies your overall rollout, and is used in option names and metrics/log tagging ROLLOUT_NAME: str @classmethod - def _should_eval_option_name(cls) -> str: + def _should_run_experiment_option(cls) -> str: """ - This is the high-level eval rollout option. If this option is disabled, the - should_check_experiment function will return False. + This is the high-level experiment rollout option. If this option is disabled (the default), + the `should_check_experiment` function will return False. """ return f"dynamic.saferollouts.{cls.ROLLOUT_NAME}.should_eval_experimental" @classmethod - def _callsite_blocklist_option_name(cls) -> str: + def _callsite_experiment_blocklist_option(cls) -> str: """ - This is the callsite-level eval rollout option. If the option contains a callsite, - the should_check_experiment function will return False. (This is useful if you see - one callsite in particular start throwing.) + This is the callsite-level experimemt rollout option. If the option value contains a + callsite, the `should_check_experiment` function will return False. (This is useful if you + see one callsite in particular start throwing.) Defaults to an empty list. """ return f"dynamic.saferollouts.{cls.ROLLOUT_NAME}.eval_callsite_blocklist" @classmethod - def _callsite_allowlist_option_name(cls) -> str: + def _callsite_use_experimental_data_allowlist_option(cls) -> str: """ - This is the callsite-level use-experimental-path rollout option. If the option - contains a callsite, then that callsite will use the experimental-path data. - This should generally only be used once you've determined that there is a high - rate of partial- or exact- match at the callsite. + This is the callsite-level use-experimental-path rollout option. If the option value + contains a callsite, then that callsite will use the experimental-path data. This should + generally only be used once you've determined that there is a high rate of partial- or + exact- match at the callsite. Defaults to an empty list. """ return f"dynamic.saferollouts.{cls.ROLLOUT_NAME}.use_experimental_data_callsite_allowlist" @classmethod - def _sample_rate_option_name(cls) -> str: + def _experiment_sample_rate_option(cls) -> str: """ - This is the sample rate for evaluating the experimental branch. When set to a value - less than 1.0, only that percentage of requests will actually perform the double-read. - This is useful for limiting latency impact on high-traffic callsites while still - collecting representative metrics. Default is 1.0 (100% of requests are evaluated). + This is the sample rate for evaluating the experimental branch. When set to a value less + than 1.0, only that percentage of requests will actually evaluate both branches. This is + useful for limiting latency impact on high-traffic callsites while still collecting + representative metrics. Default is 1.0 (100% of requests are evaluated). """ return f"dynamic.saferollouts.{cls.ROLLOUT_NAME}.eval_experimental_sample_rate" @classmethod - def _mismatch_log_callsite_allowlist_option_name(cls) -> str: + def _callsite_mismatch_log_allowlist_option(cls) -> str: """ - Controls which callsites emit structured mismatch logs. Add a callsite - string to enable logging for it, or ``"*"`` to enable for all callsites. + Controls which callsites emit structured mismatch logs. Add a callsite identifier to enable + logging for it, or set the option to `["*"]` to enable logging for all callsites. Defaults + to an empty list (no mismatch logging). """ return f"dynamic.saferollouts.{cls.ROLLOUT_NAME}.mismatch_log_callsite_allowlist" def __init_subclass__(cls, **kwargs: Any) -> None: super().__init_subclass__(**kwargs) - from sentry.options import register register( - cls._should_eval_option_name(), + cls._should_run_experiment_option(), type=Bool, default=False, flags=FLAG_MODIFIABLE_BOOL | FLAG_AUTOMATOR_MODIFIABLE, ) register( - cls._callsite_blocklist_option_name(), + cls._callsite_experiment_blocklist_option(), type=Sequence, default=[], flags=FLAG_ALLOW_EMPTY | FLAG_AUTOMATOR_MODIFIABLE, ) register( - cls._callsite_allowlist_option_name(), + cls._callsite_use_experimental_data_allowlist_option(), type=Sequence, default=[], flags=FLAG_ALLOW_EMPTY | FLAG_AUTOMATOR_MODIFIABLE, ) register( - cls._sample_rate_option_name(), + cls._experiment_sample_rate_option(), type=Float, default=1.0, flags=FLAG_MODIFIABLE_RATE | FLAG_AUTOMATOR_MODIFIABLE, ) register( - cls._mismatch_log_callsite_allowlist_option_name(), + cls._callsite_mismatch_log_allowlist_option(), type=Sequence, default=[], flags=FLAG_ALLOW_EMPTY | FLAG_AUTOMATOR_MODIFIABLE, ) @classmethod - def should_log_mismatch(cls, callsite: str) -> bool: - allowlist = set(options.get(cls._mismatch_log_callsite_allowlist_option_name())) + def _should_log_mismatch(cls, callsite: str) -> bool: + allowlist = set(options.get(cls._callsite_mismatch_log_allowlist_option())) return "*" in allowlist or callsite in allowlist @classmethod @@ -162,16 +177,16 @@ def _maybe_log_mismatch( cls, *, callsite: str, - use_experimental: bool, - exact_match: bool, - reasonable_match: bool | None, - is_experimental_data_a_null_result: bool | None, + use_experimental_data: bool, + is_exact_match: bool, + is_reasonable_match: bool | None, + is_experimental_data_nullish: bool | None, control_data: TData, experimental_data: TData, debug_context: dict[str, Any] | None, data_serializer: Callable[[TData], Any] | None, ) -> None: - if not cls.should_log_mismatch(callsite): + if not cls._should_log_mismatch(callsite): return serialize = data_serializer or cls._default_serialize_for_log @@ -181,10 +196,10 @@ def _maybe_log_mismatch( extra={ "rollout_name": cls.ROLLOUT_NAME, "callsite": callsite, - "source_of_truth": ("experimental" if use_experimental else "control"), - "exact_match": exact_match, - "reasonable_match": reasonable_match, - "is_null_result": is_experimental_data_a_null_result, + "source_of_truth": ("experimental" if use_experimental_data else "control"), + "exact_match": is_exact_match, + "reasonable_match": is_reasonable_match, + "is_null_result": is_experimental_data_nullish, "debug_context": trim(cls._default_serialize_for_log(debug_context)), "control_data_raw": trim(serialize(control_data)), "experimental_data_raw": trim(serialize(experimental_data)), @@ -194,43 +209,46 @@ def _maybe_log_mismatch( @classmethod def should_check_experiment(cls, callsite: str) -> bool: """ - This function should control whether you evaluate your experimental branch at - all. Useful for rolling out by region or blocklisting callsites that throw. + This function controls whether you evaluate your experimental branch at all. Useful for + rolling out by region or blocklisting callsites that throw. The check includes: 1. Global eval option must be enabled 2. Callsite must not be in the blocklist 3. Random sampling based on the sample_rate option (default 1.0 = 100%) """ - if not options.get(cls._should_eval_option_name()): + if not options.get(cls._should_run_experiment_option()): return False - if callsite in options.get(cls._callsite_blocklist_option_name()): + if callsite in options.get(cls._callsite_experiment_blocklist_option()): return False - sample_rate = options.get(cls._sample_rate_option_name()) + sample_rate = options.get(cls._experiment_sample_rate_option()) return random.random() < sample_rate @classmethod - def should_use_experiment(cls, callsite: str) -> bool: + def should_use_experimental_data(cls, callsite: str) -> bool: """ - This function should control whether you use the result of your experimental - data. Useful for allowlisting known-safe callsites. - If you are transitioning from an existing, intended-to-be equivalent dataset, - you should instead use check_and_choose (which standardizes the choice logic - and has better logging). + This function controls whether you use the result of your experimental data. Useful for + allowlisting known-safe callsites. + + Note: If you are transitioning from an existing, intended-to-be-equivalent dataset, you + should instead use `check_and_choose` (which has this check built in and has better + logging). """ - use_experimental = callsite in options.get(cls._callsite_allowlist_option_name()) + use_experimental_data = callsite in options.get( + cls._callsite_use_experimental_data_allowlist_option() + ) tags: dict[str, str] = { "rollout_name": cls.ROLLOUT_NAME, "callsite": callsite, - "use_experimental": ("true" if use_experimental else "false"), + "use_experimental": ("true" if use_experimental_data else "false"), } metrics.incr( "SafeRolloutComparator.should_use_experiment", tags=tags, ) - return use_experimental + return use_experimental_data @classmethod def check_and_choose( @@ -238,72 +256,71 @@ def check_and_choose( control_data: TData, experimental_data: TData, callsite: str, - is_experimental_data_a_null_result: bool | None = None, + is_experimental_data_nullish: bool | None = None, reasonable_match_comparator: Callable[[TData, TData], bool] | None = None, debug_context: dict[str, Any] | None = None, data_serializer: Callable[[TData], Any] | None = None, ) -> TData: """ - This function does two things. - First, it compares control & experimental data and logs info to DataDog. - Second, it determines which of the inputs should be returned & used downstream. + This function does two things: + - First, it compares control & experimental data and logs info to DataDog. + - Second, it determines which of the inputs should be returned & used downstream. Inputs: * control_data: Some data from the control branch (e.g. dict[str, str]) * experimental_data: Some data from the experimental branch (of same type as control) - * callsite: A unique string for each place that uses this class. Should be the - same as passed to should_check_experiment. - * is_null_result: Whether the result is a "null result" (e.g. empty array). This - helps to determine whether a "match" is significant. - * reasonable_match_comparator: Optional predicate for semantic correctness - (e.g. subset semantics with retention gaps), returning True if the read is - "reasonable" and False otherwise. An example might be checking whether the - experimental data is a subset of the control data (useful in case of migrating - something where you don't yet have full retention in the experimental branch). + * callsite: A unique string identifying place that uses this class. Should be the same as + what's passed to `should_check_experiment`. + * is_experimental_data_nullish: Whether the result is a "null result" (e.g. empty array). + This helps to determine whether a "match" is significant. + * reasonable_match_comparator: Optional predicate for semantic correctness, returning True + if the data is "reasonably the same" and False otherwise. An example might be checking + whether the experimental data is a subset of the control data (useful in case of + migrating something where you don't yet have full retention in the experimental branch). * debug_context: Optional structured metadata included on mismatch logs. - * data_serializer: Optional serializer for control/experimental payloads in - logs. Defaults to `_default_serialize_for_log`. + * data_serializer: Optional serializer for control/experimental payloads in logs. Defaults + to `_default_serialize_for_log`. """ - use_experimental = cls.should_use_experiment(callsite) - exact_match = control_data == experimental_data - reasonable_match: bool | None = None + use_experimental_data = cls.should_use_experimental_data(callsite) + is_exact_match = control_data == experimental_data + is_reasonable_match: bool | None = None # Part 1: Compare results, log debug info, and emit metrics tags: dict[str, str] = { "rollout_name": cls.ROLLOUT_NAME, "callsite": callsite, - "exact_match": str(exact_match), - "source_of_truth": ("experimental" if use_experimental else "control"), + "exact_match": str(is_exact_match), + "source_of_truth": ("experimental" if use_experimental_data else "control"), } - if is_experimental_data_a_null_result is not None: - tags["is_null_result"] = str(is_experimental_data_a_null_result) + if is_experimental_data_nullish is not None: + tags["is_null_result"] = str(is_experimental_data_nullish) if reasonable_match_comparator is not None: try: - reasonable_match = reasonable_match_comparator(control_data, experimental_data) + is_reasonable_match = reasonable_match_comparator(control_data, experimental_data) except Exception: logger.exception( "saferollout.comparator_error", extra={"rollout_name": cls.ROLLOUT_NAME, "callsite": callsite}, ) - reasonable_match = None + is_reasonable_match = None else: - tags["reasonable_match"] = str(reasonable_match) + tags["reasonable_match"] = str(is_reasonable_match) # Log mismatch only for true mismatches: when a reasonable comparator # exists, only log if it returned False; otherwise log on exact mismatch. - has_mismatch = reasonable_match is False or ( - reasonable_match is None and exact_match is False + has_mismatch = is_reasonable_match is False or ( + is_reasonable_match is None and is_exact_match is False ) if has_mismatch: try: cls._maybe_log_mismatch( callsite=callsite, - use_experimental=use_experimental, - exact_match=exact_match, - reasonable_match=reasonable_match, - is_experimental_data_a_null_result=is_experimental_data_a_null_result, + use_experimental_data=use_experimental_data, + is_exact_match=is_exact_match, + is_reasonable_match=is_reasonable_match, + is_experimental_data_nullish=is_experimental_data_nullish, control_data=control_data, experimental_data=experimental_data, debug_context=debug_context, @@ -321,13 +338,13 @@ def check_and_choose( ) # Part 2: determine which data to return - return experimental_data if use_experimental else control_data + return experimental_data if use_experimental_data else control_data @classmethod def check_and_choose_with_timings( cls, - control_thunk: Callable[[], TData], - experimental_thunk: Callable[[], TData], + control_data_func: Callable[[], TData], + experimental_data_func: Callable[[], TData], callsite: str, null_result_determiner: Callable[[TData], bool] | None = None, reasonable_match_comparator: Callable[[TData, TData], bool] | None = None, @@ -335,15 +352,16 @@ def check_and_choose_with_timings( data_serializer: Callable[[TData], Any] | None = None, ) -> TData: """ - This method is essentially the same as check_and_choose, but captures timing - information around the control/experimental branches. - - This information is captured with Sentry spans, not in Datadog. + This method is a wrapper for `check_and_choose` which also captures timing information for + the control/experimental branches. To enable that, instead of taking the control and + experimental values, it instead takes callbacks which calculate them. It also takes a + callback for determining if the experimental result is nullish, rather than a boolean. All + other parameters match those of `check_and_choose`. """ + # Insurance - this should already have been checked by the caller if not cls.should_check_experiment(callsite): - # Don't bother collecting data in the case where we're only evaluating the - # control branch. - return control_thunk() + # Don't bother collecting data if we're only evaluating the control branch + return control_data_func() with metrics.timer( "SafeRolloutComparator.check_and_choose_with_timings", @@ -353,7 +371,7 @@ def check_and_choose_with_timings( "branch": "control", }, ): - control_data = control_thunk() + control_data = control_data_func() with metrics.timer( "SafeRolloutComparator.check_and_choose_with_timings", @@ -363,17 +381,17 @@ def check_and_choose_with_timings( "branch": "experimental", }, ): - experimental_data = experimental_thunk() + experimental_data = experimental_data_func() - is_experimental_data_a_null_result = None + is_experimental_data_nullish = None if null_result_determiner is not None: - is_experimental_data_a_null_result = null_result_determiner(experimental_data) + is_experimental_data_nullish = null_result_determiner(experimental_data) return cls.check_and_choose( control_data=control_data, experimental_data=experimental_data, callsite=callsite, - is_experimental_data_a_null_result=is_experimental_data_a_null_result, + is_experimental_data_nullish=is_experimental_data_nullish, reasonable_match_comparator=reasonable_match_comparator, debug_context=debug_context, data_serializer=data_serializer, diff --git a/tests/sentry/issues/endpoints/test_group_events.py b/tests/sentry/issues/endpoints/test_group_events.py index 7d6e6c39879118..ce6e3fbc7a7970 100644 --- a/tests/sentry/issues/endpoints/test_group_events.py +++ b/tests/sentry/issues/endpoints/test_group_events.py @@ -8,7 +8,7 @@ from sentry.issues.grouptype import ProfileFileIOGroupType from sentry.models.group import Group -from sentry.search.eap.occurrences.rollout_utils import EAPOccurrencesComparator +from sentry.search.eap.occurrences.rollout_utils import EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION from sentry.services.eventstore.models import Event from sentry.testutils.cases import APITestCase, PerformanceIssueTestCase, SnubaTestCase from sentry.testutils.helpers import parse_link_header @@ -590,7 +590,7 @@ def test_double_read_with_eap_enabled(self) -> None: url = f"/api/0/issues/{event_1.group.id}/events/" - with self.options({EAPOccurrencesComparator._should_eval_option_name(): True}): + with self.options({EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION: True}): response = self.do_request(url) assert response.status_code == 200, response.content diff --git a/tests/sentry/services/eventstore/snuba/test_backend.py b/tests/sentry/services/eventstore/snuba/test_backend.py index 26ef60d9ab8856..16357bf73e79d8 100644 --- a/tests/sentry/services/eventstore/snuba/test_backend.py +++ b/tests/sentry/services/eventstore/snuba/test_backend.py @@ -10,7 +10,10 @@ build_group_id_in_filter, build_keyset_pagination_filter, ) -from sentry.search.eap.occurrences.rollout_utils import EAPOccurrencesComparator +from sentry.search.eap.occurrences.rollout_utils import ( + EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION, + EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION, +) from sentry.search.eap.rpc_utils import and_trace_item_filters from sentry.services.eventstore.base import Filter from sentry.services.eventstore.models import Event @@ -1050,8 +1053,8 @@ def test_get_events_double_read_end_to_end(self) -> None: with self.options( { - EAPOccurrencesComparator._should_eval_option_name(): True, - EAPOccurrencesComparator._callsite_allowlist_option_name(): [callsite], + EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION: True, + EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION: [callsite], } ): events = self.eventstore.get_events( diff --git a/tests/sentry/utils/test_rollout.py b/tests/sentry/utils/test_rollout.py index e1431992124ae9..2cb7bc7a9e8b57 100644 --- a/tests/sentry/utils/test_rollout.py +++ b/tests/sentry/utils/test_rollout.py @@ -10,29 +10,43 @@ class TestRolloutComparator(SafeRolloutComparator): ROLLOUT_NAME = "test_rollout" +TEST_SHOULD_RUN_EXPERIMENT_OPTION = TestRolloutComparator._should_run_experiment_option() +TEST_EXPERIMENT_SAMPLE_RATE_OPTION = TestRolloutComparator._experiment_sample_rate_option() +TEST_CALLSITE_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION = ( + TestRolloutComparator._callsite_use_experimental_data_allowlist_option() +) +TEST_CALLSITE_EXPERIMENT_BLOCKLIST_OPTION = ( + TestRolloutComparator._callsite_experiment_blocklist_option() +) +TEST_CALLSITE_MISMATCH_LOG_ALLOWLIST_OPTION = ( + TestRolloutComparator._callsite_mismatch_log_allowlist_option() +) + + class SafeRolloutComparatorTestCase(TestCase): def setUp(self) -> None: super().setUp() - self.comp = TestRolloutComparator() + # We need to instantiate the comparator class in order for the options to be registered + TestRolloutComparator() def test_options_registered(self) -> None: option_names = [o.name for o in all_options()] - assert TestRolloutComparator._should_eval_option_name() in option_names - assert TestRolloutComparator._callsite_allowlist_option_name() in option_names - assert TestRolloutComparator._callsite_blocklist_option_name() in option_names - assert TestRolloutComparator._sample_rate_option_name() in option_names - assert TestRolloutComparator._mismatch_log_callsite_allowlist_option_name() in option_names + assert TEST_SHOULD_RUN_EXPERIMENT_OPTION in option_names + assert TEST_CALLSITE_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION in option_names + assert TEST_CALLSITE_EXPERIMENT_BLOCKLIST_OPTION in option_names + assert TEST_EXPERIMENT_SAMPLE_RATE_OPTION in option_names + assert TEST_CALLSITE_MISMATCH_LOG_ALLOWLIST_OPTION in option_names def test_return_as_expected(self) -> None: - with override_options({TestRolloutComparator._should_eval_option_name(): False}): + with override_options({TEST_SHOULD_RUN_EXPERIMENT_OPTION: False}): assert TestRolloutComparator.should_check_experiment("test_1") is False with override_options( { - TestRolloutComparator._should_eval_option_name(): True, - TestRolloutComparator._callsite_blocklist_option_name(): ["test_blocked"], - TestRolloutComparator._sample_rate_option_name(): 1.0, + TEST_SHOULD_RUN_EXPERIMENT_OPTION: True, + TEST_CALLSITE_EXPERIMENT_BLOCKLIST_OPTION: ["test_blocked"], + TEST_EXPERIMENT_SAMPLE_RATE_OPTION: 1.0, } ): assert TestRolloutComparator.should_check_experiment("test_2") is True @@ -40,8 +54,8 @@ def test_return_as_expected(self) -> None: with override_options( { - TestRolloutComparator._callsite_allowlist_option_name(): ["test_allowed"], - TestRolloutComparator._mismatch_log_callsite_allowlist_option_name(): [], + TEST_CALLSITE_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION: ["test_allowed"], + TEST_CALLSITE_MISMATCH_LOG_ALLOWLIST_OPTION: [], } ): assert TestRolloutComparator.check_and_choose("ctl", "exp", "test_3") == "ctl" @@ -50,9 +64,9 @@ def test_return_as_expected(self) -> None: def test_eval_experimental_sample_rate(self) -> None: with override_options( { - TestRolloutComparator._should_eval_option_name(): True, - TestRolloutComparator._callsite_blocklist_option_name(): [], - TestRolloutComparator._sample_rate_option_name(): 0.5, + TEST_SHOULD_RUN_EXPERIMENT_OPTION: True, + TEST_CALLSITE_EXPERIMENT_BLOCKLIST_OPTION: [], + TEST_EXPERIMENT_SAMPLE_RATE_OPTION: 0.5, } ): with patch("sentry.utils.rollout.random.random", return_value=0.3): @@ -70,9 +84,9 @@ def test_eval_experimental_sample_rate(self) -> None: def test_eval_experimental_respects_blocklist(self) -> None: with override_options( { - TestRolloutComparator._should_eval_option_name(): True, - TestRolloutComparator._callsite_blocklist_option_name(): ["test_blocked"], - TestRolloutComparator._sample_rate_option_name(): 1.0, + TEST_SHOULD_RUN_EXPERIMENT_OPTION: True, + TEST_CALLSITE_EXPERIMENT_BLOCKLIST_OPTION: ["test_blocked"], + TEST_EXPERIMENT_SAMPLE_RATE_OPTION: 1.0, } ): # Even with 100% sample rate, blocklisted callsites should be blocked @@ -83,30 +97,24 @@ def test_eval_experimental_respects_blocklist(self) -> None: def test_eval_experimental_sample_rate_respects_eval_disabled(self) -> None: with override_options( { - TestRolloutComparator._should_eval_option_name(): False, - TestRolloutComparator._callsite_blocklist_option_name(): [], - TestRolloutComparator._sample_rate_option_name(): 1.0, + TEST_SHOULD_RUN_EXPERIMENT_OPTION: False, + TEST_CALLSITE_EXPERIMENT_BLOCKLIST_OPTION: [], + TEST_EXPERIMENT_SAMPLE_RATE_OPTION: 1.0, } ): assert TestRolloutComparator.should_check_experiment("test_disabled") is False def test_should_log_mismatch_allowlist(self) -> None: - with override_options( - {TestRolloutComparator._mismatch_log_callsite_allowlist_option_name(): []} - ): - assert TestRolloutComparator.should_log_mismatch("callsite") is False + with override_options({TEST_CALLSITE_MISMATCH_LOG_ALLOWLIST_OPTION: []}): + assert TestRolloutComparator._should_log_mismatch("callsite") is False - with override_options( - {TestRolloutComparator._mismatch_log_callsite_allowlist_option_name(): ["callsite"]} - ): - assert TestRolloutComparator.should_log_mismatch("callsite") is True - assert TestRolloutComparator.should_log_mismatch("other") is False + with override_options({TEST_CALLSITE_MISMATCH_LOG_ALLOWLIST_OPTION: ["callsite"]}): + assert TestRolloutComparator._should_log_mismatch("callsite") is True + assert TestRolloutComparator._should_log_mismatch("other") is False - with override_options( - {TestRolloutComparator._mismatch_log_callsite_allowlist_option_name(): ["*"]} - ): - assert TestRolloutComparator.should_log_mismatch("callsite") is True - assert TestRolloutComparator.should_log_mismatch("other") is True + with override_options({TEST_CALLSITE_MISMATCH_LOG_ALLOWLIST_OPTION: ["*"]}): + assert TestRolloutComparator._should_log_mismatch("callsite") is True + assert TestRolloutComparator._should_log_mismatch("other") is True @patch("sentry.utils.rollout.SafeRolloutComparator.check_and_choose") def test_check_and_choose_with_timings_forwards_debug_args( @@ -117,14 +125,14 @@ def test_check_and_choose_with_timings_forwards_debug_args( with override_options( { - TestRolloutComparator._should_eval_option_name(): True, - TestRolloutComparator._callsite_blocklist_option_name(): [], - TestRolloutComparator._sample_rate_option_name(): 1.0, + TEST_SHOULD_RUN_EXPERIMENT_OPTION: True, + TEST_CALLSITE_EXPERIMENT_BLOCKLIST_OPTION: [], + TEST_EXPERIMENT_SAMPLE_RATE_OPTION: 1.0, } ): TestRolloutComparator.check_and_choose_with_timings( - control_thunk=lambda: "control", - experimental_thunk=lambda: "experimental", + control_data_func=lambda: "control", + experimental_data_func=lambda: "experimental", callsite="test_callsite", debug_context={"x": "y"}, data_serializer=serializer, diff --git a/tests/snuba/api/endpoints/test_organization_events_occurrences.py b/tests/snuba/api/endpoints/test_organization_events_occurrences.py index 985c4d943bac49..30d03b2eb854f3 100644 --- a/tests/snuba/api/endpoints/test_organization_events_occurrences.py +++ b/tests/snuba/api/endpoints/test_organization_events_occurrences.py @@ -7,7 +7,9 @@ from rest_framework.response import Response from sentry.models.group import Group -from sentry.search.eap.occurrences.rollout_utils import EAPOccurrencesComparator +from sentry.search.eap.occurrences.rollout_utils import ( + EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION, +) from sentry.testutils.cases import OccurrenceTestCase from sentry.testutils.helpers.datetime import before_now from tests.snuba.api.endpoints.test_organization_events import ( @@ -25,7 +27,7 @@ def setUp(self) -> None: def request_with_feature_flag(self, payload: dict) -> Response: with self.options( - {EAPOccurrencesComparator._callsite_allowlist_option_name(): self.callsite_name} + {EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION: self.callsite_name} ): response = self.do_request({**payload, "dataset": "occurrences"}) assert response.status_code == 200, response.content @@ -524,7 +526,7 @@ def request_with_feature_flag(self, payload: dict) -> Response: "organizations:trace-item-details-array-fields": True, } with self.options( - {EAPOccurrencesComparator._callsite_allowlist_option_name(): self.callsite_name} + {EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION: self.callsite_name} ): response = self.do_request({**payload, "dataset": "occurrences"}, features=features) assert response.status_code == 200, response.content @@ -782,7 +784,7 @@ def test_array_fields_dropped_when_feature_flag_off(self) -> None: expected_http_url = expected[0]["http_url"] with self.options( - {EAPOccurrencesComparator._callsite_allowlist_option_name(): self.callsite_name} + {EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION: self.callsite_name} ): response = self.do_request( { diff --git a/tests/snuba/api/endpoints/test_organization_events_timeseries_occurrences.py b/tests/snuba/api/endpoints/test_organization_events_timeseries_occurrences.py index 98be3d08c43325..ea4ee225e6f715 100644 --- a/tests/snuba/api/endpoints/test_organization_events_timeseries_occurrences.py +++ b/tests/snuba/api/endpoints/test_organization_events_timeseries_occurrences.py @@ -5,7 +5,9 @@ import pytest from django.urls import reverse -from sentry.search.eap.occurrences.rollout_utils import EAPOccurrencesComparator +from sentry.search.eap.occurrences.rollout_utils import ( + EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION, +) from sentry.testutils.cases import OccurrenceTestCase from sentry.testutils.helpers.datetime import before_now from tests.snuba.api.endpoints.test_organization_events import ( @@ -71,7 +73,7 @@ def _store_occurrences_and_request_timeseries( ] self.store_eap_items(occurrences) with self.options( - {EAPOccurrencesComparator._callsite_allowlist_option_name(): self.callsite_name} + {EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION: self.callsite_name} ): return self._do_request( data={ diff --git a/tests/snuba/api/endpoints/test_organization_events_trace.py b/tests/snuba/api/endpoints/test_organization_events_trace.py index 92ecd4dd0b4c96..c8ddf4dad6460b 100644 --- a/tests/snuba/api/endpoints/test_organization_events_trace.py +++ b/tests/snuba/api/endpoints/test_organization_events_trace.py @@ -13,7 +13,10 @@ ) from sentry.issues.issue_occurrence import IssueOccurrence from sentry.models.group import Group -from sentry.search.eap.occurrences.rollout_utils import EAPOccurrencesComparator +from sentry.search.eap.occurrences.rollout_utils import ( + EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION, + EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION, +) from sentry.search.events.types import SnubaParams from sentry.testutils.cases import OccurrenceTestCase, TraceTestCase from sentry.utils.samples import load_data @@ -1503,8 +1506,8 @@ def test_load_performance_issues_with_eap_as_source_of_truth(self) -> None: with self.options( { - EAPOccurrencesComparator._should_eval_option_name(): True, - EAPOccurrencesComparator._callsite_allowlist_option_name(): [ + EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION: True, + EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION: [ "api.trace.load_performance_issues" ], } @@ -1563,8 +1566,8 @@ def test_query_trace_data_errors_with_eap_as_source_of_truth(self) -> None: with self.options( { - EAPOccurrencesComparator._should_eval_option_name(): True, - EAPOccurrencesComparator._callsite_allowlist_option_name(): [ + EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION: True, + EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION: [ "api.trace.query_trace_data.errors" ], } @@ -1698,8 +1701,8 @@ def test_simple_with_eap_as_source_of_truth(self) -> None: ) with self.options( { - EAPOccurrencesComparator._should_eval_option_name(): True, - EAPOccurrencesComparator._callsite_allowlist_option_name(): [ + EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION: True, + EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION: [ "api.trace.count_performance_issues" ], } diff --git a/tests/snuba/api/endpoints/test_organization_trace_meta.py b/tests/snuba/api/endpoints/test_organization_trace_meta.py index 91ca9a762a5c47..ba0aa6922762c4 100644 --- a/tests/snuba/api/endpoints/test_organization_trace_meta.py +++ b/tests/snuba/api/endpoints/test_organization_trace_meta.py @@ -3,7 +3,10 @@ import pytest from django.urls import NoReverseMatch, reverse -from sentry.search.eap.occurrences.rollout_utils import EAPOccurrencesComparator +from sentry.search.eap.occurrences.rollout_utils import ( + EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION, + EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION, +) from sentry.testutils.cases import OccurrenceTestCase, TraceMetricsTestCase, UptimeResultEAPTestCase from sentry.testutils.helpers.datetime import before_now from tests.snuba.api.endpoints.test_organization_events_trace import ( @@ -115,8 +118,8 @@ def test_simple_with_eap_as_source_of_truth(self) -> None: ) with self.options( { - EAPOccurrencesComparator._should_eval_option_name(): True, - EAPOccurrencesComparator._callsite_allowlist_option_name(): [ + EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION: True, + EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION: [ "api.trace.count_performance_issues" ], } @@ -180,8 +183,8 @@ def test_with_errors_eap_eval_not_allowlisted_uses_snuba(self) -> None: ) with self.options( { - EAPOccurrencesComparator._should_eval_option_name(): True, - EAPOccurrencesComparator._callsite_allowlist_option_name(): [], + EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION: True, + EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION: [], } ): with self.feature(self.FEATURES): @@ -212,10 +215,8 @@ def test_with_errors_eap_allowlisted_uses_eap(self) -> None: ) with self.options( { - EAPOccurrencesComparator._should_eval_option_name(): True, - EAPOccurrencesComparator._callsite_allowlist_option_name(): [ - "api.trace.count_errors" - ], + EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION: True, + EAP_OCCURRENCES_USE_EXPERIMENTAL_DATA_ALLOWLIST_OPTION: ["api.trace.count_errors"], } ): with self.feature(self.FEATURES): diff --git a/tests/snuba/tagstore/test_tagstore_backend.py b/tests/snuba/tagstore/test_tagstore_backend.py index 31f156793ca4c2..d98f2fff8b60a4 100644 --- a/tests/snuba/tagstore/test_tagstore_backend.py +++ b/tests/snuba/tagstore/test_tagstore_backend.py @@ -10,7 +10,7 @@ from sentry.models.environment import Environment from sentry.models.release import Release from sentry.models.releaseprojectenvironment import ReleaseProjectEnvironment, ReleaseStages -from sentry.search.eap.occurrences.rollout_utils import EAPOccurrencesComparator +from sentry.search.eap.occurrences.rollout_utils import EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION from sentry.search.events.constants import ( RELEASE_STAGE_ALIAS, SEMVER_ALIAS, @@ -1203,7 +1203,7 @@ def test_error_upsampling_tag_value_counts(self) -> None: assert total_count == 15 def test_eap_read_path(self) -> None: - with self.options({EAPOccurrencesComparator._should_eval_option_name(): True}): + with self.options({EAP_OCCURRENCES_SHOULD_RUN_EXPERIMENT_OPTION: True}): gk = self.ts.get_group_tag_key( self.proj1group1, None, From 08181567d53b40758d09d64297c7833d0125a7b5 Mon Sep 17 00:00:00 2001 From: Charlie Luo Date: Tue, 26 May 2026 13:37:22 -0400 Subject: [PATCH 16/61] fix(api-docs): correct event/replay/processing-error ID schemas (#116201) While working on https://github.com/getsentry/sentry/pull/116059, I noticed that some of our API parameters are marked as UUID, when they actually accept hex id values, so are technically non-comforming. Adjust the OpenAPI spec to indicate this, and add a hint that these parameters are hex ids. I considered using a more holistic solution by making a new `OpenAPIParameter`, but decided against it for now, since these cases are rather limited. If we find that we keep needing to use this form of hex parameter, we can reconsider Co-authored-by: Claude --- src/sentry/apidocs/parameters.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/sentry/apidocs/parameters.py b/src/sentry/apidocs/parameters.py index 35a79c3fc797ed..d43b25b6ae8232 100644 --- a/src/sentry/apidocs/parameters.py +++ b/src/sentry/apidocs/parameters.py @@ -10,6 +10,10 @@ # NOTE: Please add new params by path vs query, then in alphabetical order +# Some Sentry IDs are 32-char hex, rather than the UUID dashed form. +# Those cases match this pattern with OpenApiTypes.STR +SENTRY_HEX_ID_PATTERN = r"^[0-9a-f]{32}$" + def build_typed_list(type: Any): """ @@ -761,8 +765,9 @@ class MonitorParams: name="processing_error_id", location="path", required=False, - type=OpenApiTypes.UUID, - description="The ID of the processing error.", + type=OpenApiTypes.STR, + pattern=SENTRY_HEX_ID_PATTERN, + description="The ID of the processing error. It is a 32-character hexadecimal string.", ) @@ -788,8 +793,9 @@ class EventParams: name="event_id", location="path", required=True, - type=OpenApiTypes.UUID, - description="The ID of the event.", + type=OpenApiTypes.STR, + pattern=SENTRY_HEX_ID_PATTERN, + description="The ID of the event. It is a 32-character hexadecimal string as reported by the client.", ) FRAME_IDX = OpenApiParameter( @@ -935,8 +941,9 @@ class ReplayParams: name="replay_id", location="path", required=True, - type=OpenApiTypes.UUID, - description="""The ID of the replay you'd like to retrieve.""", + type=OpenApiTypes.STR, + pattern=SENTRY_HEX_ID_PATTERN, + description="""The ID of the replay you'd like to retrieve. It is a 32-character hexadecimal string.""", ) SEGMENT_ID = OpenApiParameter( From 8f9b260c3c0901f4f81112be0753e5bfdf53e469 Mon Sep 17 00:00:00 2001 From: Colleen O'Rourke Date: Tue, 26 May 2026 10:46:48 -0700 Subject: [PATCH 17/61] chore(alerts): Remove AlertRuleSerializer (#116052) Remove the `AlertRuleSerializer` and `IncidentSerializer` (the actual serializer serializers, as opposed to the validator serializers) as they are no longer in use - only places they were "used" is for API responses (they're unpublished) and a debug email view. A follow up will clean up the responses e.g. `AlertRuleSerializerResponse` as there are many imports and this PR is already large. --- pyproject.toml | 1 - .../apidocs/examples/metric_alert_examples.py | 226 ------------- src/sentry/apidocs/parameters.py | 10 - .../organization_alert_rule_details.py | 33 -- .../organization_alert_rule_index.py | 28 -- .../endpoints/serializers/alert_rule.py | 313 +----------------- .../endpoints/serializers/incident.py | 121 +------ .../templates/sentry/debug/mail/preview.html | 1 - src/sentry/web/debug_urls.py | 2 - .../debug/debug_incident_trigger_email.py | 74 ----- .../endpoints/serializers/test_alert_rule.py | 156 +-------- .../endpoints/serializers/test_incident.py | 98 ------ .../test_project_alert_rule_index.py | 129 -------- tests/sentry/incidents/test_charts.py | 44 +-- .../actions/test_notify_event_service.py | 8 +- 15 files changed, 25 insertions(+), 1219 deletions(-) delete mode 100644 src/sentry/apidocs/examples/metric_alert_examples.py delete mode 100644 src/sentry/web/frontend/debug/debug_incident_trigger_email.py delete mode 100644 tests/sentry/incidents/endpoints/serializers/test_incident.py diff --git a/pyproject.toml b/pyproject.toml index 83af6f485ede78..d0ef17c0135ae5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1639,7 +1639,6 @@ module = [ "sentry.web.frontend.debug.debug_error_embed", "sentry.web.frontend.debug.debug_feedback_issue", "sentry.web.frontend.debug.debug_generic_issue", - "sentry.web.frontend.debug.debug_incident_trigger_email", "sentry.web.frontend.debug.debug_invalid_identity_email", "sentry.web.frontend.debug.debug_mfa_added_email", "sentry.web.frontend.debug.debug_mfa_removed_email", diff --git a/src/sentry/apidocs/examples/metric_alert_examples.py b/src/sentry/apidocs/examples/metric_alert_examples.py deleted file mode 100644 index d1e61a0470a2e4..00000000000000 --- a/src/sentry/apidocs/examples/metric_alert_examples.py +++ /dev/null @@ -1,226 +0,0 @@ -from drf_spectacular.utils import OpenApiExample - - -class MetricAlertExamples: - LIST_METRIC_ALERT_RULES = [ - OpenApiExample( - "List metric alert rules for an organization", - value=[ - { - "id": "7", - "name": "Counting Bad Request and Unauthorized Errors in Prod", - "organizationId": "237655244234", - "queryType": 0, - "dataset": "events", - "query": "tags[http.status_code]:[400, 401]", - "aggregate": "count()", - "thresholdType": 0, - "resolveThreshold": None, - "timeWindow": 1440, - "environment": "prod", - "triggers": [ - { - "id": "394289", - "alertRuleId": "17723", - "label": "critical", - "thresholdType": 0, - "alertThreshold": 100, - "resolveThreshold": None, - "dateCreated": "2023-09-25T22:15:26.375126Z", - "actions": [ - { - "id": "394280", - "alertRuleTriggerId": "92481", - "type": "slack", - "targetType": "specific", - "targetIdentifier": "30489048931789", - "inputChannelId": "#my-channel", - "integrationId": "8753467", - "sentryAppId": None, - "dateCreated": "2023-09-25T22:15:26.375126Z", - } - ], - }, - ], - "projects": ["super-cool-project"], - "owner": "user:53256", - "originalAlertRuleId": None, - "comparisonDelta": None, - "dateModified": "2023-09-25T22:15:26.375126Z", - "dateCreated": "2023-09-25T22:15:26.375126Z", - "createdBy": {"id": 983948, "name": "John Doe", "email": "john.doe@sentry.io"}, - } - ], - status_codes=["200"], - response_only=True, - ) - ] - - CREATE_METRIC_ALERT_RULE = [ - OpenApiExample( - "Create a metric alert rule for an organization", - value={ - "id": "177104", - "name": "Apdex % Check", - "organizationId": "4505676595200000", - "queryType": 2, - "dataset": "metrics", - "query": "", - "aggregate": "percentage(sessions_crashed, sessions) AS _crash_rate_alert_aggregate", - "thresholdType": 0, - "resolveThreshold": 80.0, - "timeWindow": 120, - "environment": None, - "triggers": [ - { - "id": "293990", - "alertRuleId": "177104", - "label": "critical", - "thresholdType": 0, - "alertThreshold": 75, - "resolveThreshold": 80.0, - "dateCreated": "2023-09-25T22:01:28.673305Z", - "actions": [ - { - "id": "281887", - "alertRuleTriggerId": "293990", - "type": "email", - "targetType": "team", - "targetIdentifier": "2378589792734981", - "inputChannelId": None, - "integrationId": None, - "sentryAppId": None, - "dateCreated": "2023-09-25T22:01:28.680793Z", - } - ], - }, - { - "id": "492849", - "alertRuleId": "482923", - "label": "warning", - "thresholdType": 1, - "alertThreshold": 50, - "resolveThreshold": 80, - "dateCreated": "2023-09-25T22:01:28.673305Z", - "actions": [], - }, - ], - "projects": ["our-project"], - "owner": "team:4505676595200000", - "originalAlertRuleId": None, - "comparisonDelta": 10080, - "dateModified": "2023-09-25T22:01:28.637506Z", - "dateCreated": "2023-09-25T22:01:28.637514Z", - "createdBy": { - "id": 2837708, - "name": "Jane Doe", - "email": "jane.doe@sentry.io", - }, - }, - status_codes=["201"], - response_only=True, - ) - ] - - GET_METRIC_ALERT_RULE = [ - OpenApiExample( - "Get detailed view about a metric alert rule", - value={ - "id": "177412243058", - "name": "My Metric Alert Rule", - "organizationId": "4505676595200000", - "queryType": 0, - "dataset": "events", - "query": "", - "aggregate": "count_unique(user)", - "thresholdType": 0, - "resolveThreshold": None, - "timeWindow": 60, - "environment": None, - "triggers": [ - { - "id": "294385908", - "alertRuleId": "177412243058", - "label": "critical", - "thresholdType": 0, - "alertThreshold": 31.0, - "resolveThreshold": None, - "dateCreated": "2023-09-26T22:14:17.557579Z", - "actions": [], - } - ], - "projects": ["my-coolest-project"], - "owner": "team:29508397892374892", - "dateModified": "2023-09-26T22:14:17.522166Z", - "dateCreated": "2023-09-26T22:14:17.522196Z", - "createdBy": { - "id": 2834985497897, - "name": "Somebody That I Used to Know", - "email": "anon@sentry.io", - }, - "eventTypes": ["default", "error"], - }, - status_codes=["200"], - response_only=True, - ) - ] - - UPDATE_METRIC_ALERT_RULE = [ - OpenApiExample( - "Update a metric alert rule", - value={ - "id": "345989573", - "name": "P30 Transaction Duration", - "organizationId": "02403489017", - "queryType": 1, - "dataset": "transactions", - "query": "", - "aggregate": "percentile(transaction.duration,0.3)", - "thresholdType": 1, - "resolveThreshold": None, - "timeWindow": 60, - "environment": None, - "triggers": [ - { - "id": "0543809890", - "alertRuleId": "345989573", - "label": "critical", - "thresholdType": 1, - "alertThreshold": 70.0, - "resolveThreshold": None, - "dateCreated": "2023-09-25T23:35:31.832084Z", - "actions": [], - } - ], - "projects": ["backend"], - "owner": "team:9390258908", - "originalAlertRuleId": None, - "comparisonDelta": 10080.0, - "dateModified": "2023-09-25T23:35:31.787866Z", - "dateCreated": "2023-09-25T23:35:31.787875Z", - "createdBy": { - "id": 902843590658, - "name": "Spongebob Squarepants", - "email": "spongebob.s@example.com", - }, - }, - status_codes=["200"], - response_only=True, - ) - ] - - GET_METRIC_ALERT_ANOMALIES = [ - OpenApiExample( - "Fetch a list of anomalies for a metric alert rule", - value=[ - { - "timestamp": 0.1, - "value": 100.0, - "anomaly": { - "anomaly_type": "anomaly_higher_confidence", - "anomaly_value": 100, - }, - } - ], - ) - ] diff --git a/src/sentry/apidocs/parameters.py b/src/sentry/apidocs/parameters.py index d43b25b6ae8232..6ac9d7735490d9 100644 --- a/src/sentry/apidocs/parameters.py +++ b/src/sentry/apidocs/parameters.py @@ -545,16 +545,6 @@ class IssueAlertParams: ) -class MetricAlertParams: - METRIC_RULE_ID = OpenApiParameter( - name="alert_rule_id", - location="path", - required=True, - type=int, - description="The ID of the rule you'd like to query.", - ) - - class DataForwarderParams: DATA_FORWARDER_ID = OpenApiParameter( name="data_forwarder_id", diff --git a/src/sentry/incidents/endpoints/organization_alert_rule_details.py b/src/sentry/incidents/endpoints/organization_alert_rule_details.py index 698c27e8cc01ef..0c6c0350625846 100644 --- a/src/sentry/incidents/endpoints/organization_alert_rule_details.py +++ b/src/sentry/incidents/endpoints/organization_alert_rule_details.py @@ -15,17 +15,8 @@ from sentry.api.helpers.deprecation import deprecated from sentry.api.serializers import serialize from sentry.api.serializers.rest_framework.project import ProjectField -from sentry.apidocs.constants import ( - RESPONSE_ACCEPTED, - RESPONSE_FORBIDDEN, - RESPONSE_NOT_FOUND, - RESPONSE_UNAUTHORIZED, -) -from sentry.apidocs.examples.metric_alert_examples import MetricAlertExamples -from sentry.apidocs.parameters import GlobalParams, MetricAlertParams from sentry.constants import ALERTS_API_DEPRECATION_DATE from sentry.incidents.endpoints.bases import WorkflowEngineOrganizationAlertRuleEndpoint -from sentry.incidents.endpoints.serializers.alert_rule import AlertRuleSerializer from sentry.incidents.endpoints.serializers.workflow_engine_detector import ( DetailedWorkflowEngineDetectorSerializer, WorkflowEngineDetectorSerializer, @@ -293,14 +284,6 @@ class OrganizationAlertRuleDetailsEndpoint(WorkflowEngineOrganizationAlertRuleEn @extend_schema( operation_id="(DEPRECATED) Retrieve a Metric Alert Rule for an Organization", - parameters=[GlobalParams.ORG_ID_OR_SLUG, MetricAlertParams.METRIC_RULE_ID], - responses={ - 200: AlertRuleSerializer, - 401: RESPONSE_UNAUTHORIZED, - 403: RESPONSE_FORBIDDEN, - 404: RESPONSE_NOT_FOUND, - }, - examples=MetricAlertExamples.GET_METRIC_ALERT_RULE, ) @track_alert_endpoint_execution("GET", "sentry-api-0-organization-alert-rule-details") @deprecated( @@ -329,15 +312,6 @@ def get( @extend_schema( operation_id="(DEPRECATED) Update a Metric Alert Rule", - parameters=[GlobalParams.ORG_ID_OR_SLUG, MetricAlertParams.METRIC_RULE_ID], - request=OrganizationAlertRuleDetailsPutSerializer, - responses={ - 200: AlertRuleSerializer, - 401: RESPONSE_UNAUTHORIZED, - 403: RESPONSE_FORBIDDEN, - 404: RESPONSE_NOT_FOUND, - }, - examples=MetricAlertExamples.UPDATE_METRIC_ALERT_RULE, ) @track_alert_endpoint_execution("PUT", "sentry-api-0-organization-alert-rule-details") @deprecated( @@ -371,13 +345,6 @@ def put( @extend_schema( operation_id="(DEPRECATED) Delete a Metric Alert Rule", - parameters=[GlobalParams.ORG_ID_OR_SLUG, MetricAlertParams.METRIC_RULE_ID], - responses={ - 202: RESPONSE_ACCEPTED, - 401: RESPONSE_UNAUTHORIZED, - 403: RESPONSE_FORBIDDEN, - 404: RESPONSE_NOT_FOUND, - }, ) @track_alert_endpoint_execution("DELETE", "sentry-api-0-organization-alert-rule-details") @deprecated( diff --git a/src/sentry/incidents/endpoints/organization_alert_rule_index.py b/src/sentry/incidents/endpoints/organization_alert_rule_index.py index 954a28754672a5..c309bda611dd89 100644 --- a/src/sentry/incidents/endpoints/organization_alert_rule_index.py +++ b/src/sentry/incidents/endpoints/organization_alert_rule_index.py @@ -41,19 +41,11 @@ from sentry.api.serializers import serialize from sentry.api.serializers.rest_framework.project import ProjectField from sentry.api.utils import to_valid_int_id -from sentry.apidocs.constants import RESPONSE_FORBIDDEN, RESPONSE_NOT_FOUND, RESPONSE_UNAUTHORIZED -from sentry.apidocs.examples.metric_alert_examples import MetricAlertExamples -from sentry.apidocs.parameters import GlobalParams -from sentry.apidocs.utils import inline_sentry_response_serializer from sentry.constants import ALERTS_API_DEPRECATION_DATE, ObjectStatus from sentry.db.models.manager.base_query_set import BaseQuerySet from sentry.db.postgres.transactions import in_test_hide_transaction_boundary from sentry.exceptions import InvalidParams from sentry.incidents.endpoints.bases import OrganizationAlertRuleBaseEndpoint -from sentry.incidents.endpoints.serializers.alert_rule import ( - AlertRuleSerializer, - AlertRuleSerializerResponse, -) from sentry.incidents.endpoints.serializers.workflow_engine_combined import ( WorkflowEngineCombinedRuleSerializer, ) @@ -725,17 +717,6 @@ class OrganizationAlertRuleIndexEndpoint(OrganizationAlertRuleBaseEndpoint, Aler @extend_schema( operation_id="(DEPRECATED) List an Organization's Metric Alert Rules", - parameters=[GlobalParams.ORG_ID_OR_SLUG], - request=None, - responses={ - 200: inline_sentry_response_serializer( - "ListMetricAlertRules", list[AlertRuleSerializerResponse] - ), - 401: RESPONSE_UNAUTHORIZED, - 403: RESPONSE_FORBIDDEN, - 404: RESPONSE_NOT_FOUND, - }, - examples=MetricAlertExamples.LIST_METRIC_ALERT_RULES, # TODO: make ) @track_alert_endpoint_execution("GET", "sentry-api-0-organization-alert-rules") @deprecated( @@ -761,15 +742,6 @@ def get(self, request: Request, organization: Organization) -> HttpResponseBase: @extend_schema( operation_id="(DEPRECATED) Create a Metric Alert Rule for an Organization", - parameters=[GlobalParams.ORG_ID_OR_SLUG], - request=OrganizationAlertRuleIndexPostSerializer, - responses={ - 201: AlertRuleSerializer, - 401: RESPONSE_UNAUTHORIZED, - 403: RESPONSE_FORBIDDEN, - 404: RESPONSE_NOT_FOUND, - }, - examples=MetricAlertExamples.CREATE_METRIC_ALERT_RULE, ) @track_alert_endpoint_execution("POST", "sentry-api-0-organization-alert-rules") @deprecated( diff --git a/src/sentry/incidents/endpoints/serializers/alert_rule.py b/src/sentry/incidents/endpoints/serializers/alert_rule.py index f8afafe801e318..58eaf1282f6cfb 100644 --- a/src/sentry/incidents/endpoints/serializers/alert_rule.py +++ b/src/sentry/incidents/endpoints/serializers/alert_rule.py @@ -1,43 +1,10 @@ from __future__ import annotations -import logging -from collections import defaultdict -from collections.abc import Mapping, MutableMapping, Sequence from datetime import datetime -from typing import Any, TypedDict, cast +from typing import Any, TypedDict -from django.contrib.auth.models import AnonymousUser -from django.db.models import Max, Q, prefetch_related_objects from drf_spectacular.utils import extend_schema_serializer -from sentry import features -from sentry.api.serializers import Serializer, register, serialize -from sentry.incidents.models.alert_rule import ( - AlertRule, - AlertRuleActivity, - AlertRuleActivityType, - AlertRuleTrigger, - AlertRuleTriggerAction, -) -from sentry.incidents.models.incident import Incident -from sentry.models.rulesnooze import RuleSnooze -from sentry.sentry_apps.models.sentry_app_installation import prepare_ui_component -from sentry.sentry_apps.services.app import app_service -from sentry.sentry_apps.services.app.model import RpcSentryAppComponentContext -from sentry.snuba.dataset import Dataset -from sentry.snuba.models import ExtrapolationMode, SnubaQueryEventType -from sentry.users.models.user import User -from sentry.users.services.user import RpcUser -from sentry.users.services.user.service import user_service -from sentry.workflow_engine.utils.legacy_metric_tracking import report_used_legacy_models - -__all__ = [ - "AlertRuleSerializer", - "DetailedAlertRuleSerializer", -] - -logger = logging.getLogger(__name__) - class AlertRuleSerializerResponseOptional(TypedDict, total=False): environment: str | None @@ -98,283 +65,9 @@ class AlertRuleSerializerResponse(AlertRuleSerializerResponseOptional): class DetailedAlertRuleSerializerResponse(AlertRuleSerializerResponse, total=False): """ - Response type for DetailedAlertRuleSerializer, which includes additional - snooze-related fields beyond the base AlertRuleSerializerResponse. + Response type that includes additional snooze-related fields beyond the base + AlertRuleSerializerResponse. """ snoozeForEveryone: bool | None snoozeCreatedBy: str | None - - -@register(AlertRule) -class AlertRuleSerializer(Serializer): - """ - Serializer for returning an alert rule to the client - """ - - def __init__(self, expand: list[str] | None = None, prepare_component_fields: bool = False): - self.expand = expand or [] - self.prepare_component_fields = prepare_component_fields - - def get_attrs( - self, item_list: Sequence[Any], user: User | RpcUser | AnonymousUser, **kwargs: Any - ) -> defaultdict[AlertRule, Any]: - alert_rules = {item.id: item for item in item_list} - prefetch_related_objects(item_list, "snuba_query__environment") - - result: defaultdict[AlertRule, dict[str, Any]] = defaultdict(dict) - triggers = AlertRuleTrigger.objects.filter(alert_rule__in=item_list).order_by("label") - serialized_triggers = serialize(list(triggers), **kwargs) - - trigger_actions = AlertRuleTriggerAction.objects.filter( - alert_rule_trigger__alert_rule_id__in=alert_rules.keys() - ).exclude(Q(sentry_app_config__isnull=True) | Q(sentry_app_id__isnull=True)) - - sentry_app_installations_by_sentry_app_id: Mapping[str, RpcSentryAppComponentContext] = {} - organization_ids = list({alert_rule.organization_id for alert_rule in alert_rules.values()}) - if self.prepare_component_fields: - sentry_app_ids = list(trigger_actions.values_list("sentry_app_id", flat=True)) - install_contexts = app_service.get_component_contexts( - filter={"app_ids": sentry_app_ids, "organization_id": organization_ids[0]}, - component_type="alert-rule-action", - ) - sentry_app_installations_by_sentry_app_id = { - str(context.installation.sentry_app.id): context - for context in install_contexts - if context.installation.sentry_app - } - - for trigger, serialized in zip(triggers, serialized_triggers): - errors = [] - alert_rule = alert_rules[trigger.alert_rule_id] - alert_rule_triggers = result[alert_rule].setdefault("triggers", []) - for action in serialized.get("actions", []): - if action is None: - continue - - # Prepare AlertRuleTriggerActions that are SentryApp components - install_context = None - sentry_app_id = str(action.get("sentryAppId")) - if sentry_app_id: - install_context = sentry_app_installations_by_sentry_app_id.get(sentry_app_id) - if install_context: - rpc_install = install_context.installation - rpc_component = install_context.component - rpc_app = rpc_install.sentry_app - assert rpc_app - - action["sentryAppInstallationUuid"] = rpc_install.uuid - - component = ( - prepare_ui_component( - rpc_install, - rpc_component, - None, - action.get("settings"), - ) - if rpc_component - else None - ) - if component is None: - errors.append({"detail": f"Could not fetch details from {rpc_app.name}"}) - action["disabled"] = True - continue - - action["formFields"] = component.app_schema.get("settings", {}) - - if errors: - result[alert_rule]["errors"] = errors - alert_rule_triggers.append(serialized) - - alert_rule_projects = set() - for alert_rule in alert_rules.values(): - if alert_rule.projects.exists(): - for project in alert_rule.projects.all(): - alert_rule_projects.add((alert_rule.id, project.slug)) - - snuba_alert_rule_projects = AlertRule.objects.filter( - id__in=[item.id for item in item_list] - ).values_list("id", "projects__slug") - - alert_rule_projects.update( - [(id, project_slug) for id, project_slug in snuba_alert_rule_projects if project_slug] - ) - - for alert_rule_id, project_slug in alert_rule_projects: - rule_result = result[alert_rules[alert_rule_id]].setdefault("projects", []) - rule_result.append(project_slug) - - rule_activities = list( - AlertRuleActivity.objects.filter( - alert_rule__in=item_list, type=AlertRuleActivityType.CREATED.value - ) - ) - - user_by_user_id: MutableMapping[int, RpcUser] = { - user.id: user - for user in user_service.get_many_by_id( - ids=[r.user_id for r in rule_activities if r.user_id is not None] - ) - } - for rule_activity in rule_activities: - if rule_activity.user_id is not None: - rpc_user = user_by_user_id.get(rule_activity.user_id) - else: - rpc_user = None - if rpc_user: - created_by = dict( - id=rpc_user.id, name=rpc_user.get_display_name(), email=rpc_user.email - ) - else: - created_by = None - result[alert_rules[rule_activity.alert_rule_id]]["created_by"] = created_by - - for item in item_list: - if item.user_id or item.team_id: - actor = item.owner - if actor: - result[item]["owner"] = actor.identifier - - if "original_alert_rule" in self.expand: - snapshot_activities = AlertRuleActivity.objects.filter( - alert_rule__in=item_list, - type=AlertRuleActivityType.SNAPSHOT.value, - ) - for activity in snapshot_activities: - result[alert_rules[activity.alert_rule_id]]["originalAlertRuleId"] = ( - activity.previous_alert_rule_id - ) - - if "latestIncident" in self.expand: - incident_map = {} - for incident in Incident.objects.filter( - id__in=Incident.objects.filter(alert_rule__in=alert_rules) - .values("alert_rule_id") - .annotate(incident_id=Max("id")) - .values("incident_id") - ): - incident_map[incident.alert_rule_id] = serialize(incident, user=user) - for alert_rule in alert_rules.values(): - result[alert_rule]["latestIncident"] = incident_map.get(alert_rule.id, None) - return result - - def serialize( - self, - obj: AlertRule, - attrs: Mapping[Any, Any], - user: User | RpcUser | AnonymousUser, - **kwargs: Any, - ) -> AlertRuleSerializerResponse: - # Mark that we're using legacy AlertRule models - report_used_legacy_models() - - from sentry.incidents.endpoints.utils import translate_threshold - from sentry.incidents.logic import translate_aggregate_field - - env = obj.snuba_query.environment - allow_mri = features.has( - "organizations:insights-alerts", - obj.organization, - actor=user, - ) - - # Trace metrics have complicated aggregated validation that require EAP SearchResolver and do NOT need translation as they do not have tags in the old format (eg. tags[sentry:user)) - is_trace_metric = ( - obj.snuba_query.dataset == Dataset.EventsAnalyticsPlatform.value - and obj.snuba_query.event_types - and SnubaQueryEventType.EventType.TRACE_ITEM_METRIC in obj.snuba_query.event_types - ) - - # Temporary: Translate aggregate back here from `tags[sentry:user]` to `user` for the frontend. - if not is_trace_metric: - aggregate = translate_aggregate_field( - obj.snuba_query.aggregate, - reverse=True, - allow_mri=allow_mri, - allow_eap=obj.snuba_query.dataset == Dataset.EventsAnalyticsPlatform.value, - ) - else: - aggregate = obj.snuba_query.aggregate - - # Apply transparency: Convert upsampled_count() back to count() for user-facing responses - # This hides the internal upsampling implementation from users - if aggregate == "upsampled_count()": - aggregate = "count()" - - extrapolation_mode = obj.snuba_query.extrapolation_mode - - data: AlertRuleSerializerResponse = { - "id": str(obj.id), - "name": obj.name, - "organizationId": str(obj.organization_id), - "status": obj.status, - "queryType": obj.snuba_query.type, - "dataset": obj.snuba_query.dataset, - "query": obj.snuba_query.query, - "aggregate": aggregate, - "thresholdType": obj.threshold_type, - "resolveThreshold": translate_threshold(obj, obj.resolve_threshold), - # TODO: Start having the frontend expect seconds - "timeWindow": obj.snuba_query.time_window / 60, - "environment": env.name if env else None, - # TODO: Start having the frontend expect seconds - "resolution": obj.snuba_query.resolution / 60, - "thresholdPeriod": obj.threshold_period, - "triggers": attrs.get("triggers", []), - "projects": sorted(attrs.get("projects", [])), - "owner": attrs.get("owner", None), - "originalAlertRuleId": attrs.get("originalAlertRuleId", None), - "comparisonDelta": obj.comparison_delta / 60 if obj.comparison_delta else None, - "dateModified": obj.date_modified, - "dateCreated": obj.date_added, - "createdBy": attrs.get("created_by", None), - "description": obj.description if obj.description is not None else "", - "sensitivity": obj.sensitivity, - "seasonality": obj.seasonality, - "detectionType": obj.detection_type, - } - rule_snooze = RuleSnooze.objects.filter( - Q(user_id=user.id) | Q(user_id=None), alert_rule=obj - ) - if rule_snooze.exists(): - data["snooze"] = True - - if "latestIncident" in self.expand: - data["latestIncident"] = attrs.get("latestIncident", None) - if "errors" in attrs: - data["errors"] = attrs["errors"] - - if extrapolation_mode is not None: - data["extrapolationMode"] = ExtrapolationMode(extrapolation_mode).name.lower() - - return data - - -class DetailedAlertRuleSerializer(AlertRuleSerializer): - def get_attrs( - self, item_list: Sequence[Any], user: User | RpcUser | AnonymousUser, **kwargs: Any - ) -> defaultdict[AlertRule, Any]: - result = super().get_attrs(item_list, user, **kwargs) - query_to_alert_rule = {ar.snuba_query_id: ar for ar in item_list} - - for event_type in SnubaQueryEventType.objects.filter( - snuba_query_id__in=[item.snuba_query_id for item in item_list] - ): - event_types = result[query_to_alert_rule[event_type.snuba_query_id]].setdefault( - "event_types", [] - ) - event_types.append(SnubaQueryEventType.EventType(event_type.type).name.lower()) - - return result - - def serialize( - self, - obj: AlertRule, - attrs: Mapping[Any, Any], - user: User | RpcUser | AnonymousUser, - **kwargs: Any, - ) -> DetailedAlertRuleSerializerResponse: - data = cast(DetailedAlertRuleSerializerResponse, super().serialize(obj, attrs, user)) - data["eventTypes"] = sorted(attrs.get("event_types", [])) - data["snooze"] = False - return data diff --git a/src/sentry/incidents/endpoints/serializers/incident.py b/src/sentry/incidents/endpoints/serializers/incident.py index 80c5e236fc90ac..1f0bd5df9de49f 100644 --- a/src/sentry/incidents/endpoints/serializers/incident.py +++ b/src/sentry/incidents/endpoints/serializers/incident.py @@ -1,23 +1,8 @@ -from collections import defaultdict -from collections.abc import Mapping, Sequence from datetime import datetime -from typing import Any, TypedDict +from typing import TypedDict -from django.contrib.auth.models import AnonymousUser -from django.db.models import prefetch_related_objects - -from sentry.api.serializers import Serializer, register, serialize from sentry.api.serializers.models.incidentactivity import IncidentActivitySerializerResponse -from sentry.incidents.endpoints.serializers.alert_rule import ( - AlertRuleSerializerResponse, - DetailedAlertRuleSerializer, -) -from sentry.incidents.models.incident import Incident, IncidentActivity, IncidentProject -from sentry.snuba.entity_subscription import apply_dataset_query_conditions -from sentry.snuba.models import SnubaQuery -from sentry.users.models.user import User -from sentry.users.services.user import RpcUser -from sentry.workflow_engine.utils.legacy_metric_tracking import report_used_legacy_models +from sentry.incidents.endpoints.serializers.alert_rule import AlertRuleSerializerResponse class IncidentSerializerResponse(TypedDict): @@ -39,105 +24,3 @@ class IncidentSerializerResponse(TypedDict): class DetailedIncidentSerializerResponse(IncidentSerializerResponse): discoverQuery: str - - -@register(Incident) -class IncidentSerializer(Serializer): - def __init__(self, expand: list[str] | None = None) -> None: - self.expand = expand or [] - - def get_attrs( - self, item_list: Sequence[Incident], user: User | RpcUser | AnonymousUser, **kwargs: Any - ) -> dict[Incident, Any]: - prefetch_related_objects(item_list, "alert_rule__snuba_query") - incident_projects = defaultdict(list) - for incident_project in IncidentProject.objects.filter( - incident__in=item_list - ).select_related("project"): - incident_projects[incident_project.incident_id].append(incident_project.project.slug) - - alert_rules = { - d["id"]: d - for d in serialize( - {i.alert_rule for i in item_list if i.alert_rule.id}, - user, - DetailedAlertRuleSerializer(expand=self.expand), - ) - } - - results = {} - for incident in item_list: - results[incident] = {"projects": incident_projects.get(incident.id, [])} - results[incident]["alert_rule"] = alert_rules.get(str(incident.alert_rule.id)) # type: ignore[assignment] - - if "activities" in self.expand: - # There could be many activities. An incident could seesaw between error/warning for a long period. - # e.g - every 1 minute for 10 months - activities = list(IncidentActivity.objects.filter(incident__in=item_list)[:1000]) - incident_activities = defaultdict(list) - for activity, serialized_activity in zip(activities, serialize(activities, user=user)): - incident_activities[activity.incident_id].append(serialized_activity) - for incident in item_list: - results[incident]["activities"] = incident_activities[incident.id] - - return results - - def serialize( - self, - obj: Incident, - attrs: Mapping[str, Any], - user: User | RpcUser | AnonymousUser, - **kwargs: Any, - ) -> IncidentSerializerResponse: - # Mark that we're using legacy Incident models (which depend on AlertRule) - report_used_legacy_models() - - date_closed = obj.date_closed.replace(second=0, microsecond=0) if obj.date_closed else None - return { - "id": str(obj.id), - "identifier": str(obj.identifier), - "organizationId": str(obj.organization_id), - "projects": attrs["projects"], - "alertRule": attrs["alert_rule"], - "activities": attrs["activities"] if "activities" in self.expand else None, - "status": obj.status, - "statusMethod": obj.status_method, - "type": obj.type, - "title": obj.title, - "dateStarted": obj.date_started, - "dateDetected": obj.date_detected, - "dateCreated": obj.date_added, - "dateClosed": date_closed, - } - - -class DetailedIncidentSerializer(IncidentSerializer): - def __init__(self, expand: list[str] | None = None) -> None: - if expand is None: - expand = [] - if "original_alert_rule" not in expand: - expand.append("original_alert_rule") - super().__init__(expand=expand) - - def serialize( - self, - obj: Incident, - attrs: Mapping[str, Any], - user: User | RpcUser | AnonymousUser, - **kwargs: Any, - ) -> DetailedIncidentSerializerResponse: - base_context = super().serialize(obj, attrs, user) - # The query we should use to get accurate results in Discover. - context = DetailedIncidentSerializerResponse( - **base_context, discoverQuery=self._build_discover_query(obj) - ) - - return context - - def _build_discover_query(self, incident: Incident) -> str: - return apply_dataset_query_conditions( - SnubaQuery.Type(incident.alert_rule.snuba_query.type), - incident.alert_rule.snuba_query.query, - incident.alert_rule.snuba_query.event_types, - discover=True, - ) diff --git a/src/sentry/templates/sentry/debug/mail/preview.html b/src/sentry/templates/sentry/debug/mail/preview.html index 605471bc6507fc..884fcaa4bc95e8 100644 --- a/src/sentry/templates/sentry/debug/mail/preview.html +++ b/src/sentry/templates/sentry/debug/mail/preview.html @@ -28,7 +28,6 @@ - diff --git a/src/sentry/web/debug_urls.py b/src/sentry/web/debug_urls.py index ab6a85628948b5..1922ea05c5a514 100644 --- a/src/sentry/web/debug_urls.py +++ b/src/sentry/web/debug_urls.py @@ -20,7 +20,6 @@ from sentry.web.frontend.debug.debug_error_embed import DebugErrorPageEmbedView from sentry.web.frontend.debug.debug_feedback_issue import DebugFeedbackIssueEmailView from sentry.web.frontend.debug.debug_generic_issue import DebugGenericIssueEmailView -from sentry.web.frontend.debug.debug_incident_trigger_email import DebugIncidentTriggerEmailView from sentry.web.frontend.debug.debug_invalid_identity_email import DebugInvalidIdentityEmailView from sentry.web.frontend.debug.debug_mfa_added_email import DebugMfaAddedEmailView from sentry.web.frontend.debug.debug_mfa_removed_email import DebugMfaRemovedEmailView @@ -139,7 +138,6 @@ re_path( r"^debug/mail/sso-unlinked/no-password/$", DebugSsoUnlinkedNoPasswordEmailView.as_view() ), - re_path(r"^debug/mail/incident-trigger/$", DebugIncidentTriggerEmailView.as_view()), re_path(r"^debug/mail/setup-2fa/$", DebugSetup2faEmailView.as_view()), re_path(r"^debug/embed/error-page/$", DebugErrorPageEmbedView.as_view()), re_path(r"^debug/trigger-error/$", DebugTriggerErrorView.as_view()), diff --git a/src/sentry/web/frontend/debug/debug_incident_trigger_email.py b/src/sentry/web/frontend/debug/debug_incident_trigger_email.py deleted file mode 100644 index 3ebca877b60b34..00000000000000 --- a/src/sentry/web/frontend/debug/debug_incident_trigger_email.py +++ /dev/null @@ -1,74 +0,0 @@ -from unittest import mock -from uuid import uuid4 - -from sentry.api.serializers import serialize -from sentry.incidents.action_handlers import generate_incident_trigger_email_context -from sentry.incidents.endpoints.serializers.alert_rule import AlertRuleSerializer -from sentry.incidents.endpoints.serializers.incident import DetailedIncidentSerializer -from sentry.incidents.models.alert_rule import AlertRule, AlertRuleTrigger -from sentry.incidents.models.incident import Incident, IncidentStatus, TriggerStatus -from sentry.incidents.typings.metric_detector import ( - AlertContext, - MetricIssueContext, - OpenPeriodContext, -) -from sentry.models.organization import Organization -from sentry.models.project import Project -from sentry.snuba.models import SnubaQuery -from sentry.users.models.user import User -from sentry.web.frontend.base import internal_cell_silo_view - -from .mail import MailPreviewView - - -@internal_cell_silo_view -class DebugIncidentTriggerEmailView(MailPreviewView): - @mock.patch( - "sentry.users.models.user_option.UserOption.objects.get_value", return_value="US/Pacific" - ) - def get_context(self, request, user_option_mock): - organization = Organization(slug="myorg") - project = Project(slug="myproject", organization=organization) - user = User() - - query = SnubaQuery( - time_window=60, query="transaction:/some/transaction", aggregate="count()" - ) - alert_rule = AlertRule(id=1, organization=organization, name="My Alert", snuba_query=query) - incident = Incident( - id=2, - identifier=123, - organization=organization, - title="Something broke", - alert_rule=alert_rule, - status=IncidentStatus.CRITICAL.value, - ) - trigger = AlertRuleTrigger(alert_rule=alert_rule) - - alert_rule_serialized_response = serialize(alert_rule, None, AlertRuleSerializer()) - incident_serialized_response = serialize(incident, None, DetailedIncidentSerializer()) - - return generate_incident_trigger_email_context( - project=project, - organization=organization, - alert_rule_serialized_response=alert_rule_serialized_response, - incident_serialized_response=incident_serialized_response, - metric_issue_context=MetricIssueContext.from_legacy_models( - incident=incident, - new_status=IncidentStatus(incident.status), - ), - alert_context=AlertContext.from_alert_rule_incident(alert_rule), - open_period_context=OpenPeriodContext.from_incident(incident), - trigger_status=TriggerStatus.ACTIVE, - trigger_threshold=trigger.alert_threshold, - user=user, - notification_uuid=str(uuid4()), - ) - - @property - def html_template(self) -> str: - return "sentry/emails/incidents/trigger.html" - - @property - def text_template(self) -> str: - return "sentry/emails/incidents/trigger.txt" diff --git a/tests/sentry/incidents/endpoints/serializers/test_alert_rule.py b/tests/sentry/incidents/endpoints/serializers/test_alert_rule.py index 42e289ca5f40ca..418e849f52429b 100644 --- a/tests/sentry/incidents/endpoints/serializers/test_alert_rule.py +++ b/tests/sentry/incidents/endpoints/serializers/test_alert_rule.py @@ -1,24 +1,8 @@ from typing import Any -from unittest.mock import MagicMock, patch -from sentry.api.serializers import serialize -from sentry.incidents.endpoints.serializers.alert_rule import DetailedAlertRuleSerializer -from sentry.incidents.logic import ( - AlertTarget, - create_alert_rule_trigger, - create_alert_rule_trigger_action, -) -from sentry.incidents.models.alert_rule import ( - AlertRule, - AlertRuleDetectionType, - AlertRuleProjects, - AlertRuleThresholdType, - AlertRuleTriggerAction, -) +from sentry.incidents.models.alert_rule import AlertRule from sentry.models.rule import Rule -from sentry.snuba.dataset import Dataset -from sentry.snuba.models import ExtrapolationMode, SnubaQueryEventType -from sentry.testutils.cases import TestCase +from sentry.snuba.models import ExtrapolationMode from sentry.types.actor import Actor from sentry.users.services.user.service import user_service @@ -127,139 +111,3 @@ def create_issue_alert_rule(self, data: dict[str, Any]) -> Rule: rule.save() return rule - - -class AlertRuleSerializerTest(BaseAlertRuleSerializerTest, TestCase): - def test_simple(self) -> None: - alert_rule = self.create_alert_rule() - result = serialize(alert_rule) - self.assert_alert_rule_serialized(alert_rule, result) - - def test_threshold_type_resolve_threshold(self) -> None: - alert_rule = self.create_alert_rule( - threshold_type=AlertRuleThresholdType.BELOW, resolve_threshold=500 - ) - result = serialize(alert_rule) - self.assert_alert_rule_serialized(alert_rule, result) - - def test_triggers(self) -> None: - alert_rule = self.create_alert_rule() - other_alert_rule = self.create_alert_rule() - trigger = create_alert_rule_trigger(alert_rule, "test", 1000) - result = serialize([alert_rule, other_alert_rule]) - assert result[0]["triggers"] == [serialize(trigger)] - assert result[1]["triggers"] == [] - - def test_projects(self) -> None: - regular_alert_rule = self.create_alert_rule() - alert_rule_no_projects = self.create_alert_rule() - - AlertRuleProjects.objects.filter(alert_rule_id=alert_rule_no_projects.id).delete() - - assert regular_alert_rule.projects - assert not alert_rule_no_projects.projects.exists() - result = serialize([regular_alert_rule, alert_rule_no_projects]) - - assert result[0]["projects"] == [ - project.slug for project in regular_alert_rule.projects.all() - ] - # NOTE: we are now _only_ referencing alert_rule.projects fk (AlertRuleProjects) - assert result[1]["projects"] == [ - project.slug for project in alert_rule_no_projects.projects.all() - ] - - def test_environment(self) -> None: - alert_rule = self.create_alert_rule(environment=self.environment) - result = serialize(alert_rule) - self.assert_alert_rule_serialized(alert_rule, result) - - def test_eap_user_misery_aggregate(self) -> None: - alert_rule = self.create_alert_rule( - aggregate="user_misery(span.duration,300)", - dataset=Dataset.EventsAnalyticsPlatform, - ) - result = serialize(alert_rule) - self.assert_alert_rule_serialized(alert_rule, result) - assert result["aggregate"] == "user_misery(span.duration,300)" - - def test_created_by(self) -> None: - user = self.create_user("foo@example.com") - alert_rule = self.create_alert_rule(environment=self.environment, user=user) - result = serialize(alert_rule) - self.assert_alert_rule_serialized(alert_rule, result) - assert alert_rule.created_by_id == user.id - - def test_owner(self) -> None: - user = self.create_user("foo@example.com") - alert_rule = self.create_alert_rule( - environment=self.environment, - user=user, - owner=Actor.from_id(team_id=self.team.id, user_id=None), - ) - result = serialize(alert_rule) - self.assert_alert_rule_serialized(alert_rule, result) - assert alert_rule.team_id == self.team.id - assert alert_rule.user_id is None - - def test_comparison_delta_above(self) -> None: - alert_rule = self.create_alert_rule( - comparison_delta=60, - resolve_threshold=110, - detection_type=AlertRuleDetectionType.PERCENT, - ) - result = serialize(alert_rule) - self.assert_alert_rule_serialized(alert_rule, result, resolve_threshold=10) - - def test_comparison_delta_below(self) -> None: - alert_rule = self.create_alert_rule( - comparison_delta=60, - resolve_threshold=90, - threshold_type=AlertRuleThresholdType.BELOW, - detection_type=AlertRuleDetectionType.PERCENT, - ) - result = serialize(alert_rule) - self.assert_alert_rule_serialized(alert_rule, result, resolve_threshold=10) - - def test_extrapolation_mode(self) -> None: - alert_rule = self.create_alert_rule() - result = serialize(alert_rule) - self.assert_alert_rule_serialized(alert_rule, result) - - -class DetailedAlertRuleSerializerTest(BaseAlertRuleSerializerTest, TestCase): - def test_simple(self) -> None: - projects = [self.project, self.create_project()] - alert_rule = self.create_alert_rule(projects=projects) - result = serialize(alert_rule, serializer=DetailedAlertRuleSerializer()) - self.assert_alert_rule_serialized(alert_rule, result) - assert sorted(result["projects"]) == sorted(p.slug for p in projects) - assert result["eventTypes"] == [SnubaQueryEventType.EventType.ERROR.name.lower()] - - def test_triggers(self) -> None: - alert_rule = self.create_alert_rule() - other_alert_rule = self.create_alert_rule() - trigger = create_alert_rule_trigger(alert_rule, "test", 1000) - result = serialize([alert_rule, other_alert_rule], serializer=DetailedAlertRuleSerializer()) - assert result[0]["triggers"] == [serialize(trigger)] - assert result[1]["triggers"] == [] - - @patch( - "sentry.incidents.logic.get_target_identifier_display_for_integration", - return_value=AlertTarget(123, "test"), - ) - def test_trigger_actions(self, mock_get: MagicMock) -> None: - alert_rule = self.create_alert_rule() - other_alert_rule = self.create_alert_rule() - trigger = create_alert_rule_trigger(alert_rule, "test", 1000) - trigger_action = create_alert_rule_trigger_action( - trigger, - AlertRuleTriggerAction.Type.PAGERDUTY, - AlertRuleTriggerAction.TargetType.SPECIFIC, - target_identifier="123", - integration_id=self.integration.id, - priority="error", - ) - result = serialize([alert_rule, other_alert_rule], serializer=DetailedAlertRuleSerializer()) - assert result[0]["triggers"] == [serialize(trigger)] - assert result[0]["triggers"][0]["actions"] == [serialize(trigger_action)] - assert result[1]["triggers"] == [] diff --git a/tests/sentry/incidents/endpoints/serializers/test_incident.py b/tests/sentry/incidents/endpoints/serializers/test_incident.py deleted file mode 100644 index 5a36a20c00fb4b..00000000000000 --- a/tests/sentry/incidents/endpoints/serializers/test_incident.py +++ /dev/null @@ -1,98 +0,0 @@ -from datetime import timedelta - -from django.utils import timezone - -from sentry.api.serializers import serialize -from sentry.incidents.endpoints.serializers.alert_rule import DetailedAlertRuleSerializer -from sentry.incidents.endpoints.serializers.incident import ( - DetailedIncidentSerializer, - IncidentSerializer, -) -from sentry.incidents.models.incident import IncidentActivityType, IncidentStatus -from sentry.snuba.dataset import Dataset -from sentry.testutils.cases import TestCase -from sentry.testutils.helpers.datetime import freeze_time - - -class IncidentSerializerTest(TestCase): - @freeze_time() - def test_simple(self) -> None: - incident = self.create_incident(date_started=timezone.now() - timedelta(minutes=5)) - result = serialize(incident) - - assert result["id"] == str(incident.id) - assert result["identifier"] == str(incident.identifier) - assert result["organizationId"] == str(incident.organization_id) - assert result["projects"] == [p.slug for p in incident.projects.all()] - assert result["status"] == incident.status - assert result["statusMethod"] == incident.status_method - assert result["type"] == incident.type - assert result["title"] == incident.title - assert result["dateStarted"] == incident.date_started - assert result["dateDetected"] == incident.date_detected - assert result["dateCreated"] == incident.date_added - assert result["dateClosed"] == incident.date_closed - - def test_with_activities(self) -> None: - incident = self.create_incident() - activity = self.create_incident_activity( - incident=incident, - type=IncidentActivityType.STATUS_CHANGE.value, - value=str(IncidentStatus.CRITICAL.value), - previous_value=str(IncidentStatus.WARNING.value), - ) - - result = serialize(incident, serializer=IncidentSerializer(expand=["activities"])) - - assert result["activities"] is not None - assert len(result["activities"]) == 1 - assert result["activities"][0] == { - "id": str(activity.id), - "incidentIdentifier": str(incident.identifier), - "user": None, - "type": IncidentActivityType.STATUS_CHANGE.value, - "value": str(IncidentStatus.CRITICAL.value), - "previousValue": str(IncidentStatus.WARNING.value), - "comment": None, - "dateCreated": activity.date_added, - } - - -class DetailedIncidentSerializerTest(TestCase): - def test_error_alert_rule(self) -> None: - query = "test query" - incident = self.create_incident(query=query) - - serializer = DetailedIncidentSerializer() - result = serialize(incident, serializer=serializer) - alert_rule_serializer = DetailedAlertRuleSerializer() - assert result["alertRule"] == serialize( - incident.alert_rule, serializer=alert_rule_serializer - ) - assert result["discoverQuery"] == f"(event.type:error) AND ({query})" - - def test_error_alert_rule_unicode(self) -> None: - query = "统一码" - incident = self.create_incident(query=query) - - serializer = DetailedIncidentSerializer() - result = serialize(incident, serializer=serializer) - - alert_rule_serializer = DetailedAlertRuleSerializer() - assert result["alertRule"] == serialize( - incident.alert_rule, serializer=alert_rule_serializer - ) - assert result["discoverQuery"] == f"(event.type:error) AND ({query})" - - def test_transaction_alert_rule(self) -> None: - query = "test query" - alert_rule = self.create_alert_rule(dataset=Dataset.Transactions, query=query) - incident = self.create_incident(alert_rule=alert_rule) - - serializer = DetailedIncidentSerializer() - result = serialize(incident, serializer=serializer) - alert_rule_serializer = DetailedAlertRuleSerializer() - assert result["alertRule"] == serialize( - incident.alert_rule, serializer=alert_rule_serializer - ) - assert result["discoverQuery"] == f"(event.type:transaction) AND ({query})" diff --git a/tests/sentry/incidents/endpoints/test_project_alert_rule_index.py b/tests/sentry/incidents/endpoints/test_project_alert_rule_index.py index 4113e83b1f331f..a6849f7ca25b52 100644 --- a/tests/sentry/incidents/endpoints/test_project_alert_rule_index.py +++ b/tests/sentry/incidents/endpoints/test_project_alert_rule_index.py @@ -1,15 +1,5 @@ -from copy import deepcopy - -from sentry import audit_log -from sentry.api.serializers import serialize -from sentry.incidents.models.alert_rule import AlertRule -from sentry.models.auditlogentry import AuditLogEntry -from sentry.silo.base import SiloMode from sentry.snuba.dataset import Dataset from sentry.testutils.cases import APITestCase -from sentry.testutils.helpers.datetime import freeze_time -from sentry.testutils.outbox import outbox_runner -from sentry.testutils.silo import assume_test_silo_mode from sentry.testutils.skips import requires_snuba from sentry.workflow_engine.migration_helpers.alert_rule import migrate_alert_rule @@ -57,122 +47,3 @@ def test_no_feature(self) -> None: self.login_as(self.user) resp = self.get_response(self.organization.slug, self.project.slug) assert resp.status_code == 404 - - -@freeze_time() -class AlertRuleCreateEndpointTest(APITestCase): - endpoint = "sentry-api-0-project-alert-rules" - method = "post" - - def setUp(self) -> None: - super().setUp() - self.organization = self.create_organization() - self.project = self.create_project(organization=self.organization) - self.user = self.create_user() - self.valid_alert_rule = { - "aggregate": "count()", - "query": "", - "timeWindow": "300", - "resolveThreshold": 100, - "thresholdType": 0, - "triggers": [ - { - "label": "critical", - "alertThreshold": 200, - "actions": [ - {"type": "email", "targetType": "team", "targetIdentifier": self.team.id} - ], - }, - { - "label": "warning", - "alertThreshold": 150, - "actions": [ - {"type": "email", "targetType": "team", "targetIdentifier": self.team.id}, - {"type": "email", "targetType": "user", "targetIdentifier": self.user.id}, - ], - }, - ], - "projects": [self.project.slug], - "owner": self.user.id, - "name": "JustAValidTestRule", - } - self.create_member( - user=self.user, organization=self.organization, role="owner", teams=[self.team] - ) - self.login_as(self.user) - - def test_simple(self) -> None: - with ( - outbox_runner(), - self.feature(["organizations:incidents", "organizations:performance-view"]), - ): - resp = self.get_success_response( - self.organization.slug, - self.project.slug, - status_code=201, - **self.valid_alert_rule, - ) - assert "id" in resp.data - alert_rule = AlertRule.objects.get(id=resp.data["id"]) - assert resp.data == serialize(alert_rule, self.user) - - with assume_test_silo_mode(SiloMode.CONTROL): - audit_log_entry = AuditLogEntry.objects.filter( - event=audit_log.get_event_id("ALERT_RULE_ADD"), target_object=alert_rule.id - ) - assert len(audit_log_entry) == 1 - assert ( - resp.renderer_context["request"].META["REMOTE_ADDR"] - == list(audit_log_entry)[0].ip_address - ) - - def test_status_filter(self) -> None: - with ( - outbox_runner(), - self.feature( - [ - "organizations:incidents", - "organizations:performance-view", - ] - ), - ): - data = deepcopy(self.valid_alert_rule) - data["query"] = "is:unresolved" - resp = self.get_success_response( - self.organization.slug, - self.project.slug, - status_code=201, - **data, - ) - assert "id" in resp.data - alert_rule = AlertRule.objects.get(id=resp.data["id"]) - assert resp.data == serialize(alert_rule, self.user) - assert alert_rule.snuba_query.query == "is:unresolved" - - def test_project_not_in_request(self) -> None: - """Test that if you don't provide the project data in the request, we grab it from the URL""" - data = deepcopy(self.valid_alert_rule) - del data["projects"] - with ( - outbox_runner(), - self.feature(["organizations:incidents", "organizations:performance-view"]), - ): - resp = self.get_success_response( - self.organization.slug, - self.project.slug, - status_code=201, - **data, - ) - assert "id" in resp.data - alert_rule = AlertRule.objects.get(id=resp.data["id"]) - assert resp.data == serialize(alert_rule, self.user) - - with assume_test_silo_mode(SiloMode.CONTROL): - audit_log_entry = AuditLogEntry.objects.filter( - event=audit_log.get_event_id("ALERT_RULE_ADD"), target_object=alert_rule.id - ) - assert len(audit_log_entry) == 1 - assert ( - resp.renderer_context["request"].META["REMOTE_ADDR"] - == list(audit_log_entry)[0].ip_address - ) diff --git a/tests/sentry/incidents/test_charts.py b/tests/sentry/incidents/test_charts.py index 7d8262c4089612..e03da1c1769dff 100644 --- a/tests/sentry/incidents/test_charts.py +++ b/tests/sentry/incidents/test_charts.py @@ -1,24 +1,18 @@ import datetime +from typing import cast from unittest.mock import MagicMock, patch from django.utils import timezone from django.utils.dateparse import parse_datetime -from sentry.api.serializers import serialize from sentry.constants import ObjectStatus from sentry.incidents.charts import ( build_metric_alert_chart, fetch_metric_issue_open_periods, incident_date_range, ) -from sentry.incidents.endpoints.serializers.alert_rule import ( - AlertRuleSerializer, - AlertRuleSerializerResponse, -) -from sentry.incidents.endpoints.serializers.incident import ( - DetailedIncidentSerializer, - DetailedIncidentSerializerResponse, -) +from sentry.incidents.endpoints.serializers.alert_rule import AlertRuleSerializerResponse +from sentry.incidents.endpoints.serializers.incident import DetailedIncidentSerializerResponse from sentry.incidents.grouptype import MetricIssue from sentry.incidents.logic import CRITICAL_TRIGGER_LABEL from sentry.incidents.models.incident import Incident, IncidentActivityType, IncidentStatus @@ -111,12 +105,8 @@ def test_eap_alert(self, mock_client_get: MagicMock, mock_generate_chart: MagicM trigger = self.create_alert_rule_trigger(alert_rule, CRITICAL_TRIGGER_LABEL, 100) self.create_alert_rule_trigger_action(alert_rule_trigger=trigger) - alert_rule_serialized_response: AlertRuleSerializerResponse = serialize( - alert_rule, None, AlertRuleSerializer() - ) - incident_serialized_response: DetailedIncidentSerializerResponse = serialize( - incident, None, DetailedIncidentSerializer() - ) + alert_rule_serialized_response = cast(AlertRuleSerializerResponse, {}) + incident_serialized_response = cast(DetailedIncidentSerializerResponse, {}) url = build_metric_alert_chart( self.organization, @@ -135,9 +125,13 @@ def test_eap_alert(self, mock_client_get: MagicMock, mock_generate_chart: MagicM @patch("sentry.charts.backend.generate_chart", return_value="chart-url") @patch("sentry.incidents.charts.client.get") + @with_feature("organizations:workflow-engine-ui") def test_eap_log_alert( self, mock_client_get: MagicMock, mock_generate_chart: MagicMock ) -> None: + from sentry.notifications.notification_action.metric_alert_registry.handlers.utils import ( + get_detector_serializer, + ) from sentry.snuba.models import SnubaQueryEventType mock_client_get.return_value.data = {"data": []} @@ -157,20 +151,16 @@ def test_eap_log_alert( trigger = self.create_alert_rule_trigger(alert_rule, CRITICAL_TRIGGER_LABEL, 100) self.create_alert_rule_trigger_action(alert_rule_trigger=trigger) - alert_rule_serialized_response: AlertRuleSerializerResponse = serialize( - alert_rule, None, AlertRuleSerializer() - ) - incident_serialized_response: DetailedIncidentSerializerResponse = serialize( - incident, None, DetailedIncidentSerializer() - ) + detector = self.create_detector(project=self.project) + self.create_alert_rule_detector(detector=detector, alert_rule_id=alert_rule.id) url = build_metric_alert_chart( self.organization, - alert_rule_serialized_response=alert_rule_serialized_response, + alert_rule_serialized_response=cast(AlertRuleSerializerResponse, {}), alert_context=AlertContext.from_alert_rule_incident(alert_rule), snuba_query=alert_rule.snuba_query, open_period_context=OpenPeriodContext.from_incident(incident), - selected_incident_serialized=incident_serialized_response, + detector_serialized_response=get_detector_serializer(detector), ) assert url == "chart-url" @@ -202,12 +192,8 @@ def test_eap_trace_metric_alert( trigger = self.create_alert_rule_trigger(alert_rule, CRITICAL_TRIGGER_LABEL, 100) self.create_alert_rule_trigger_action(alert_rule_trigger=trigger) - alert_rule_serialized_response: AlertRuleSerializerResponse = serialize( - alert_rule, None, AlertRuleSerializer() - ) - incident_serialized_response: DetailedIncidentSerializerResponse = serialize( - incident, None, DetailedIncidentSerializer() - ) + alert_rule_serialized_response = cast(AlertRuleSerializerResponse, {}) + incident_serialized_response = cast(DetailedIncidentSerializerResponse, {}) url = build_metric_alert_chart( self.organization, diff --git a/tests/sentry/rules/actions/test_notify_event_service.py b/tests/sentry/rules/actions/test_notify_event_service.py index dbc93b321d0c9e..aa1f36914fc724 100644 --- a/tests/sentry/rules/actions/test_notify_event_service.py +++ b/tests/sentry/rules/actions/test_notify_event_service.py @@ -1,3 +1,4 @@ +from typing import cast from unittest.mock import MagicMock, patch from uuid import uuid4 @@ -6,10 +7,9 @@ from django.utils import timezone from requests.exceptions import HTTPError -from sentry.api.serializers import serialize from sentry.eventstream.types import EventStreamEventType from sentry.grouping.grouptype import ErrorGroupType -from sentry.incidents.endpoints.serializers.incident import IncidentSerializer +from sentry.incidents.endpoints.serializers.incident import IncidentSerializerResponse from sentry.incidents.models.incident import IncidentStatus from sentry.incidents.typings.metric_detector import ( AlertContext, @@ -272,9 +272,7 @@ def setUp(self) -> None: self.metric_issue_context = MetricIssueContext.from_legacy_models( self.incident, IncidentStatus.CRITICAL, metric_value=100.0 ) - self.incident_serialized_response = serialize( - self.incident, serializer=IncidentSerializer() - ) + self.incident_serialized_response = cast(IncidentSerializerResponse, {}) self.notification_context = NotificationContext( id=1, sentry_app_id=self.sentry_app.id, From eb6c25afd86f55c733a87e5e7fda5c8ceb97e027 Mon Sep 17 00:00:00 2001 From: Malachi Willey Date: Tue, 26 May 2026 10:48:01 -0700 Subject: [PATCH 18/61] feat(aci): Add sort param to workflow group history endpoint (#116031) This endpoint change will address the feedback we've gotten that alert triggers are more useful when sorted by recent triggers rather than largest count. Adds a `sort` query parameter to the group-history endpoint so that the frontend can modify the sorts. This also changes the default sort to `('-last_triggered', '-count')` rather than the current `('-count', '-last_triggered')`. This supports multiple sorts and ascending/descending, so the frontend can implement sortable table headers. --- .../organization_workflow_group_history.py | 3 +- .../workflow_group_history_serializer.py | 32 ++++++- .../test_workflow_group_history_serializer.py | 4 +- ...est_organization_workflow_group_history.py | 93 +++++++++++++++++++ 4 files changed, 129 insertions(+), 3 deletions(-) diff --git a/src/sentry/workflow_engine/endpoints/organization_workflow_group_history.py b/src/sentry/workflow_engine/endpoints/organization_workflow_group_history.py index a1d43d3713deeb..e2726ab358cd28 100644 --- a/src/sentry/workflow_engine/endpoints/organization_workflow_group_history.py +++ b/src/sentry/workflow_engine/endpoints/organization_workflow_group_history.py @@ -47,12 +47,13 @@ class OrganizationWorkflowGroupHistoryEndpoint(OrganizationWorkflowEndpoint): def get(self, request: Request, organization: Organization, workflow: Workflow) -> Response: per_page = self.get_per_page(request) cursor = self.get_cursor_from_request(request) + sort = request.GET.getlist("sort") try: start, end = get_date_range_from_params(request.GET) except InvalidParams: raise ParseError(detail="Invalid start and end dates") - results = fetch_workflow_groups_paginated(workflow, start, end, cursor, per_page) + results = fetch_workflow_groups_paginated(workflow, start, end, cursor, per_page, sort) response = Response( serialize(results.results, request.user, WorkflowGroupHistorySerializer()) diff --git a/src/sentry/workflow_engine/endpoints/serializers/workflow_group_history_serializer.py b/src/sentry/workflow_engine/endpoints/serializers/workflow_group_history_serializer.py index b429d1e8b84c99..e424abfd3ef23a 100644 --- a/src/sentry/workflow_engine/endpoints/serializers/workflow_group_history_serializer.py +++ b/src/sentry/workflow_engine/endpoints/serializers/workflow_group_history_serializer.py @@ -4,6 +4,7 @@ from typing import Any, NotRequired, TypedDict, cast from django.db.models import Count, Max, OuterRef, Subquery +from rest_framework.exceptions import ParseError from sentry.api.paginator import OffsetPaginator from sentry.api.serializers import Serializer, serialize @@ -105,12 +106,40 @@ def serialize( return result +_SORT_FIELD_MAP = { + "lastTriggered": "last_triggered", + "count": "count", +} +# `group` is appended as a stable tiebreaker so OffsetPaginator pages don't +# skip or duplicate rows when the user-provided sort fields tie. +_TIEBREAKER = "group" +_DEFAULT_ORDER_BY = ("-last_triggered", "-count", _TIEBREAKER) + + +def _parse_sort(sort: Sequence[str]) -> list[str]: + if not sort: + return list(_DEFAULT_ORDER_BY) + + order_by: list[str] = [] + for s in sort: + if s.startswith("-"): + prefix, field = "-", s[1:] + else: + prefix, field = "", s + if field not in _SORT_FIELD_MAP: + raise ParseError(detail=f"Invalid sort field: {field}") + order_by.append(f"{prefix}{_SORT_FIELD_MAP[field]}") + order_by.append(_TIEBREAKER) + return order_by + + def fetch_workflow_groups_paginated( workflow: Workflow, start: datetime, end: datetime, cursor: Cursor | None = None, per_page: int = 25, + sort: Sequence[str] = (), ) -> CursorResult[WorkflowGroupHistory]: filtered_history = WorkflowFireHistory.objects.filter( workflow=workflow, @@ -138,12 +167,13 @@ def fetch_workflow_groups_paginated( # Count distinct groups for pagination group_count = qs.count() + order_by = _parse_sort(sort) return cast( CursorResult[WorkflowGroupHistory], OffsetPaginator( qs, - order_by=("-count", "-last_triggered"), + order_by=order_by, on_results=convert_results, ).get_result(per_page, cursor, known_hits=group_count), ) diff --git a/tests/sentry/workflow_engine/endpoints/serializers/test_workflow_group_history_serializer.py b/tests/sentry/workflow_engine/endpoints/serializers/test_workflow_group_history_serializer.py index eba7941ecf73d8..f1b7e6e74c4f31 100644 --- a/tests/sentry/workflow_engine/endpoints/serializers/test_workflow_group_history_serializer.py +++ b/tests/sentry/workflow_engine/endpoints/serializers/test_workflow_group_history_serializer.py @@ -1,3 +1,4 @@ +from collections.abc import Sequence from datetime import datetime, timedelta from uuid import uuid4 @@ -116,8 +117,9 @@ def assert_expected_results( expected_results: list[WorkflowGroupHistory], cursor: Cursor | None = None, per_page: int = 25, + sort: Sequence[str] = (), ) -> CursorResult[WorkflowGroupHistory]: - result = fetch_workflow_groups_paginated(workflow, start, end, cursor, per_page) + result = fetch_workflow_groups_paginated(workflow, start, end, cursor, per_page, sort) assert result.results == expected_results, (result.results, expected_results) return result diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_group_history.py b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_group_history.py index 46a937c282a8e4..9b4bdd087eeb08 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_workflow_group_history.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_workflow_group_history.py @@ -146,6 +146,99 @@ def test_pagination(self) -> None: ) assert resp["X-Hits"] == "2" # 2 unique groups, not 4 total history records + def test_sort_by_last_triggered(self) -> None: + """Default sort is lastTriggered DESC. Explicit sort=-count orders by count DESC.""" + workflow = self.create_workflow(organization=self.organization) + + group_old = self.create_group() + group_new = self.create_group() + + # group_old: 3 triggers, last triggered 3 days ago + for i in range(3): + wfh = WorkflowFireHistory.objects.create( + workflow=workflow, group=group_old, event_id=uuid4().hex + ) + wfh.update(date_added=before_now(days=3 + i)) + + # group_new: 1 trigger, last triggered 1 day ago + wfh = WorkflowFireHistory.objects.create( + workflow=workflow, group=group_new, event_id=uuid4().hex + ) + wfh.update(date_added=before_now(days=1)) + + # Default sort (lastTriggered DESC): group_new first (more recent) + resp = self.get_success_response( + self.organization.slug, + workflow.id, + start=before_now(days=10), + end=before_now(days=0), + ) + assert resp.data[0]["count"] == 1 + assert resp.data[1]["count"] == 3 + + # Explicit sort by count DESC: group_old first (more triggers) + resp = self.get_success_response( + self.organization.slug, + workflow.id, + start=before_now(days=10), + end=before_now(days=0), + sort="-count", + ) + assert resp.data[0]["count"] == 3 + assert resp.data[1]["count"] == 1 + + def test_multiple_sorts(self) -> None: + """Multiple sort params are applied in order.""" + workflow = self.create_workflow(organization=self.organization) + + group_a = self.create_group() + group_b = self.create_group() + + # Both groups have 2 triggers, but group_a was triggered more recently + for i in range(2): + wfh = WorkflowFireHistory.objects.create( + workflow=workflow, group=group_a, event_id=uuid4().hex + ) + wfh.update(date_added=before_now(days=1 + i)) + + for i in range(2): + wfh = WorkflowFireHistory.objects.create( + workflow=workflow, group=group_b, event_id=uuid4().hex + ) + wfh.update(date_added=before_now(days=3 + i)) + + # sort=-count&sort=-lastTriggered: tied on count, group_a first (more recent) + resp = self.get_success_response( + self.organization.slug, + workflow.id, + start=before_now(days=10), + end=before_now(days=0), + sort=["-count", "-lastTriggered"], + ) + assert resp.data[0]["group"]["id"] == str(group_a.id) + assert resp.data[1]["group"]["id"] == str(group_b.id) + + # sort=-count&sort=lastTriggered: tied on count, group_b first (oldest) + resp = self.get_success_response( + self.organization.slug, + workflow.id, + start=before_now(days=10), + end=before_now(days=0), + sort=["-count", "lastTriggered"], + ) + assert resp.data[0]["group"]["id"] == str(group_b.id) + assert resp.data[1]["group"]["id"] == str(group_a.id) + + def test_invalid_sort_field(self) -> None: + self.get_error_response( + self.organization.slug, + self.workflow.id, + start=before_now(days=6), + end=before_now(days=0), + sort="invalid", + status_code=400, + ) + def test_invalid_dates_error(self) -> None: self.get_error_response( self.organization.slug, From b006f90918b6593dd4b39bb71f114d26a02605da Mon Sep 17 00:00:00 2001 From: Shashank Jarmale Date: Tue, 26 May 2026 10:48:56 -0700 Subject: [PATCH 19/61] chore(typing): Remove `tests.sentry.api.helpers.test_group_index` from mypy ignore list (#116199) Resolves [ENG-6460](https://linear.app/getsentry/issue/ENG-6460/remove-testssentryapihelperstest-group-index-from-ignore-list). Fixes the `mypy` errors in `tests.sentry.api.helpers.test_group_index`. --- pyproject.toml | 1 - src/sentry/api/helpers/group_index/update.py | 4 +- tests/sentry/api/helpers/test_group_index.py | 168 ++++++++++--------- 3 files changed, 95 insertions(+), 78 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index d0ef17c0135ae5..f29ee132614a8c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -397,7 +397,6 @@ module = [ "sentry.services.eventstore.models", "sentry.snuba.metrics.query_builder", "sentry.testutils.cases", - "tests.sentry.api.helpers.test_group_index", ] disable_error_code = [ "arg-type", diff --git a/src/sentry/api/helpers/group_index/update.py b/src/sentry/api/helpers/group_index/update.py index 2a114d781426ef..9d559a150da8bd 100644 --- a/src/sentry/api/helpers/group_index/update.py +++ b/src/sentry/api/helpers/group_index/update.py @@ -1052,7 +1052,7 @@ def handle_is_public( def handle_assigned_to( - assigned_actor: Actor, + assigned_actor: Actor | None, assigned_by: str | None, integration: str | None, group_list: Sequence[Group], @@ -1073,7 +1073,7 @@ def handle_assigned_to( if integration in [ActivityIntegration.SLACK.value, ActivityIntegration.MSTEAMS.value] else dict() ) - if assigned_actor: + if assigned_actor is not None: resolved_actor = assigned_actor.resolve() for group in group_list: assignment = GroupAssignee.objects.assign( diff --git a/tests/sentry/api/helpers/test_group_index.py b/tests/sentry/api/helpers/test_group_index.py index 327d98130e7307..35cfc1ed13a867 100644 --- a/tests/sentry/api/helpers/test_group_index.py +++ b/tests/sentry/api/helpers/test_group_index.py @@ -1,9 +1,11 @@ from datetime import UTC, datetime, timedelta from time import time +from typing import Any from unittest.mock import MagicMock, Mock, patch import pytest from django.http import QueryDict +from rest_framework.request import Request from sentry.analytics.events.advanced_search_feature_gated import AdvancedSearchFeatureGateEvent from sentry.analytics.events.manual_issue_assignment import ManualIssueAssignment @@ -106,6 +108,14 @@ def test_wildcard(self, mock_record: Mock) -> None: self.assert_analytics_recorded(mock_record) +def _wrap_request(http_request: Any, data: dict[str, Any] | None = None) -> Request: + drf_request = Request(http_request) + drf_request.user = http_request.user + if data is not None: + setattr(drf_request, "_full_data", data) + return drf_request + + class UpdateGroupsTest(TestCase): @patch("sentry.signals.issue_unresolved.send_robust") @patch("sentry.signals.issue_ignored.send_robust") @@ -113,10 +123,9 @@ def test_unresolving_resolved_group(self, send_robust: Mock, send_unresolved: Mo resolved_group = self.create_group(status=GroupStatus.RESOLVED) assert resolved_group.status == GroupStatus.RESOLVED - request = self.make_request(user=self.user, method="GET") - request.user = self.user - request.data = {"status": "unresolved", "substatus": "ongoing"} - request.GET = QueryDict(query_string=f"id={resolved_group.id}") + http_request = self.make_request(user=self.user, method="GET") + http_request.GET = QueryDict(query_string=f"id={resolved_group.id}") + request = _wrap_request(http_request, data={"status": "unresolved", "substatus": "ongoing"}) group_list = get_group_list(self.organization.id, [self.project], request.GET.getlist("id")) update_groups(request, group_list) @@ -134,10 +143,9 @@ def test_resolving_unresolved_group(self, send_robust: Mock) -> None: add_group_to_inbox(unresolved_group, GroupInboxReason.NEW) assert unresolved_group.status == GroupStatus.UNRESOLVED - request = self.make_request(user=self.user, method="GET") - request.user = self.user - request.data = {"status": "resolved", "substatus": None} - request.GET = QueryDict(query_string=f"id={unresolved_group.id}") + http_request = self.make_request(user=self.user, method="GET") + http_request.GET = QueryDict(query_string=f"id={unresolved_group.id}") + request = _wrap_request(http_request, data={"status": "resolved", "substatus": None}) group_list = get_group_list(self.organization.id, [self.project], request.GET.getlist("id")) update_groups(request, group_list) @@ -154,10 +162,11 @@ def test_ignoring_group_archived_forever(self, post_save: Mock, send_robust: Moc group = self.create_group() add_group_to_inbox(group, GroupInboxReason.NEW) - request = self.make_request(user=self.user, method="GET") - request.user = self.user - request.data = {"status": "ignored", "substatus": "archived_forever"} - request.GET = QueryDict(query_string=f"id={group.id}") + http_request = self.make_request(user=self.user, method="GET") + http_request.GET = QueryDict(query_string=f"id={group.id}") + request = _wrap_request( + http_request, data={"status": "ignored", "substatus": "archived_forever"} + ) group_list = get_group_list(self.organization.id, [self.project], request.GET.getlist("id")) update_groups(request, group_list) @@ -180,14 +189,16 @@ def test_ignoring_group_archived_until_condition_met(self, send_robust: Mock) -> group = self.create_group() add_group_to_inbox(group, GroupInboxReason.NEW) - request = self.make_request(user=self.user, method="GET") - request.user = self.user - request.data = { - "status": "ignored", - "substatus": "archived_until_condition_met", - "statusDetails": {"ignoreDuration": 1}, - } - request.GET = QueryDict(query_string=f"id={group.id}") + http_request = self.make_request(user=self.user, method="GET") + http_request.GET = QueryDict(query_string=f"id={group.id}") + request = _wrap_request( + http_request, + data={ + "status": "ignored", + "substatus": "archived_until_condition_met", + "statusDetails": {"ignoreDuration": 1}, + }, + ) group_list = get_group_list(self.organization.id, [self.project], request.GET.getlist("id")) update_groups(request, group_list) @@ -226,10 +237,9 @@ def test_unignoring_group(self, send_robust: Mock) -> None: }, ]: group = data["group"] - request = self.make_request(user=self.user, method="GET") - request.user = self.user - request.data = data["request_data"] - request.GET = QueryDict(query_string=f"id={group.id}") + http_request = self.make_request(user=self.user, method="GET") + http_request.GET = QueryDict(query_string=f"id={group.id}") + request = _wrap_request(http_request, data=data["request_data"]) group_list = get_group_list( self.organization.id, [self.project], request.GET.getlist("id") @@ -247,10 +257,9 @@ def test_mark_reviewed_group(self, send_robust: Mock) -> None: group = self.create_group() add_group_to_inbox(group, GroupInboxReason.NEW) - request = self.make_request(user=self.user, method="GET") - request.user = self.user - request.data = {"inbox": False} - request.GET = QueryDict(query_string=f"id={group.id}") + http_request = self.make_request(user=self.user, method="GET") + http_request.GET = QueryDict(query_string=f"id={group.id}") + request = _wrap_request(http_request, data={"inbox": False}) group_list = get_group_list(self.organization.id, [self.project], request.GET.getlist("id")) update_groups(request, group_list) @@ -265,10 +274,11 @@ def test_ignore_with_substatus_archived_until_escalating(self, send_robust: Mock group = self.create_group() add_group_to_inbox(group, GroupInboxReason.NEW) - request = self.make_request(user=self.user, method="GET") - request.user = self.user - request.data = {"status": "ignored", "substatus": "archived_until_escalating"} - request.GET = QueryDict(query_string=f"id={group.id}") + http_request = self.make_request(user=self.user, method="GET") + http_request.GET = QueryDict(query_string=f"id={group.id}") + request = _wrap_request( + http_request, data={"status": "ignored", "substatus": "archived_until_escalating"} + ) group_list = get_group_list(self.organization.id, [self.project], request.GET.getlist("id")) update_groups(request, group_list) @@ -284,13 +294,13 @@ def test_ignore_with_substatus_archived_until_escalating(self, send_robust: Mock def test_resolving_group_with_short_id(self, send_robust: Mock) -> None: group = self.create_group(status=GroupStatus.UNRESOLVED) - request = self.make_request( + http_request = self.make_request( user=self.user, method="GET", # The UI calls the endpoint with the short ID, not the group ID GET={"id": group.qualified_short_id}, ) - request.data = {"status": "resolved", "substatus": None} + request = _wrap_request(http_request, data={"status": "resolved", "substatus": None}) assert request.GET.getlist("id")[0] == group.qualified_short_id assert request.GET.getlist("id")[0].isdigit() is False @@ -372,10 +382,9 @@ def test_unresolve_clears_commit_resolution_links(self) -> None: assert serialized["statusDetails"]["inCommit"] is not None # Step 2: Mark as unresolved - request = self.make_request(user=self.user, method="GET") - request.user = self.user - request.data = {"status": "unresolved"} - request.GET = QueryDict(query_string=f"id={group.id}") + http_request = self.make_request(user=self.user, method="GET") + http_request.GET = QueryDict(query_string=f"id={group.id}") + request = _wrap_request(http_request, data={"status": "unresolved"}) group_list = get_group_list(self.organization.id, [self.project], request.GET.getlist("id")) update_groups(request, group_list) @@ -389,7 +398,7 @@ def test_unresolve_clears_commit_resolution_links(self) -> None: group.refresh_from_db() assert group.status == GroupStatus.UNRESOLVED - request.data = {"status": "resolved"} + request = _wrap_request(http_request, data={"status": "resolved"}) update_groups(request, group_list) group.refresh_from_db() @@ -405,10 +414,9 @@ def test_resolve_in_next_release(self) -> None: self.create_release(project=self.project, version="test@1.0.0.0") group = self.create_group(status=GroupStatus.UNRESOLVED) - request = self.make_request(user=self.user, method="GET") - request.user = self.user - request.data = {"status": "resolvedInNextRelease"} - request.GET = QueryDict(query_string=f"id={group.id}") + http_request = self.make_request(user=self.user, method="GET") + http_request.GET = QueryDict(query_string=f"id={group.id}") + request = _wrap_request(http_request, data={"status": "resolvedInNextRelease"}) group_list = get_group_list(self.organization.id, [self.project], request.GET.getlist("id")) update_groups(request, group_list) @@ -430,10 +438,9 @@ def test_resolve_in_next_release_ignores_archived_releases(self) -> None: ) group = self.create_group(status=GroupStatus.UNRESOLVED) - request = self.make_request(user=self.user, method="GET") - request.user = self.user - request.data = {"status": "resolvedInNextRelease"} - request.GET = QueryDict(query_string=f"id={group.id}") + http_request = self.make_request(user=self.user, method="GET") + http_request.GET = QueryDict(query_string=f"id={group.id}") + request = _wrap_request(http_request, data={"status": "resolvedInNextRelease"}) group_list = get_group_list(self.organization.id, [self.project], request.GET.getlist("id")) update_groups(request, group_list) @@ -450,10 +457,12 @@ def test_simple(self, mock_handle_merge: MagicMock) -> None: group_ids = [self.create_group().id, self.create_group().id] project = self.project - request = self.make_request(method="PUT") - request.user = self.user - request.data = {"merge": 1} - request.GET = {"id": group_ids, "project": [project.id]} + http_request = self.make_request(user=self.user, method="PUT") + qd = QueryDict(mutable=True) + qd.setlist("id", [str(i) for i in group_ids]) + qd.setlist("project", [str(project.id)]) + setattr(http_request, "GET", qd) + request = _wrap_request(http_request, data={"merge": 1}) group_list = get_group_list(self.organization.id, [project], group_ids) update_groups(request, group_list) @@ -478,10 +487,12 @@ def test_multiple_projects(self, mock_handle_merge: MagicMock) -> None: self.create_group(project2).id, ] - request = self.make_request(method="PUT") - request.user = self.user - request.data = {"merge": 1} - request.GET = {"id": group_ids, "project": project_ids} + http_request = self.make_request(user=self.user, method="PUT") + qd = QueryDict(mutable=True) + qd.setlist("id", [str(i) for i in group_ids]) + qd.setlist("project", [str(i) for i in project_ids]) + setattr(http_request, "GET", qd) + request = _wrap_request(http_request, data={"merge": 1}) group_list = get_group_list(self.organization.id, projects, group_ids) response = update_groups(request, group_list) @@ -499,12 +510,14 @@ def test_multiple_groups_same_project(self, mock_handle_merge: MagicMock) -> Non group_ids = [g.id for g in groups] project_ids = [p.id for p in projects] - request = self.make_request(method="PUT") - request.user = self.user - request.data = {"merge": 1} + http_request = self.make_request(user=self.user, method="PUT") + qd = QueryDict(mutable=True) + qd.setlist("id", [str(i) for i in group_ids]) # The two groups belong to the same project, so we should be able to merge them, even though # we're passing multiple project ids - request.GET = {"id": group_ids, "project": project_ids} + qd.setlist("project", [str(i) for i in project_ids]) + setattr(http_request, "GET", qd) + request = _wrap_request(http_request, data={"merge": 1}) group_list = get_group_list(self.organization.id, projects, group_ids) update_groups(request, group_list) @@ -524,10 +537,11 @@ def test_no_project_ids_passed(self, mock_handle_merge: MagicMock) -> None: group_ids = [self.create_group().id, self.create_group().id] project = self.project - request = self.make_request(method="PUT") - request.user = self.user - request.data = {"merge": 1} - request.GET = {"id": group_ids} + http_request = self.make_request(user=self.user, method="PUT") + qd = QueryDict(mutable=True) + qd.setlist("id", [str(i) for i in group_ids]) + setattr(http_request, "GET", qd) + request = _wrap_request(http_request, data={"merge": 1}) group_list = get_group_list(self.organization.id, [project], group_ids) update_groups(request, group_list) @@ -577,11 +591,13 @@ def test_metrics(self) -> None: ] project = self.project - request = self.make_request(method="PUT") - request.user = self.user - request.data = {"merge": 1} - request.GET = {"id": group_ids, "project": [project.id]} - request.META = {"HTTP_REFERER": referer} + http_request = self.make_request(user=self.user, method="PUT") + http_request.META = {"HTTP_REFERER": referer} + qd = QueryDict(mutable=True) + qd.setlist("id", [str(i) for i in group_ids]) + qd.setlist("project", [str(project.id)]) + setattr(http_request, "GET", qd) + request = _wrap_request(http_request, data={"merge": 1}) with patch("sentry.api.helpers.group_index.update.metrics.incr") as mock_metrics_incr: group_list = get_group_list(self.organization.id, [project], group_ids) @@ -619,7 +635,9 @@ def test_is_subscribed_updates(self) -> None: subscription = GroupSubscription.objects.filter(group=self.group, user_id=self.user.id) assert subscription.exists() - assert subscription.first().is_active + sub = subscription.first() + assert sub is not None + assert sub.is_active assert resp["reason"] == "unknown" @@ -1170,9 +1188,9 @@ class DeleteGroupsTest(TestCase): def test_delete_groups_simple(self, send_robust: Mock) -> None: groups = [self.create_group(), self.create_group()] group_ids = [group.id for group in groups] - request = self.make_request(user=self.user, method="GET") - request.user = self.user - request.GET = QueryDict(f"id={group_ids[0]}&id={group_ids[1]}") + http_request = self.make_request(user=self.user, method="GET") + http_request.GET = QueryDict(f"id={group_ids[0]}&id={group_ids[1]}") + request = _wrap_request(http_request) hashes = ["0" * 32, "1" * 32] for i, group in enumerate(groups): GroupHash.objects.create(project=self.project, group=group, hash=hashes[i]) @@ -1202,9 +1220,9 @@ def test_delete_groups_deletes_seer_records_by_hash( groups = [self.create_group(), self.create_group()] group_ids = [group.id for group in groups] - request = self.make_request(user=self.user, method="GET") - request.user = self.user - request.GET = QueryDict(f"id={group_ids[0]}&id={group_ids[1]}") + http_request = self.make_request(user=self.user, method="GET") + http_request.GET = QueryDict(f"id={group_ids[0]}&id={group_ids[1]}") + request = _wrap_request(http_request) hashes = ["0" * 32, "1" * 32] for i, group in enumerate(groups): GroupHash.objects.create(project=self.project, group=group, hash=hashes[i]) From 97880b24c78e9176edfeb29fb60f9e8d0781d55b Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Tue, 26 May 2026 10:51:32 -0700 Subject: [PATCH 20/61] ref(flags): Remove organizations:dashboards-drilldown-flow (#115670) Dev-only flag registered Oct 2025, never rolled out to customers. Remove the flag registration and the unreleased gated code paths. --- src/sentry/features/temporary.py | 3 +- static/app/actionCreators/modal.tsx | 11 - .../widgetBuilder/linkToDashboardModal.tsx | 256 ------------------ .../utils/discover/fieldRenderers.spec.tsx | 2 +- .../dashboards/hooks/useHasDrillDownFlows.tsx | 11 - .../groupByStep/groupBySelector.tsx | 53 ---- .../components/groupBySelector.tsx | 3 - .../components/visualize/index.tsx | 79 +----- .../dashboards/widgetBuilder/settings.ts | 3 - 9 files changed, 4 insertions(+), 417 deletions(-) delete mode 100644 static/app/components/modals/widgetBuilder/linkToDashboardModal.tsx delete mode 100644 static/app/views/dashboards/hooks/useHasDrillDownFlows.tsx delete mode 100644 static/app/views/dashboards/widgetBuilder/settings.ts diff --git a/src/sentry/features/temporary.py b/src/sentry/features/temporary.py index 7c50bb15b625c5..d1bdbda2fab254 100644 --- a/src/sentry/features/temporary.py +++ b/src/sentry/features/temporary.py @@ -68,8 +68,7 @@ def register_temporary_features(manager: FeatureManager) -> None: manager.add("organizations:dashboards-edit", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True, default=True) # Enable metrics enhanced performance for AM2+ customers as they transition from AM2 to AM3 manager.add("organizations:dashboards-metrics-transition", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) - # Enable drilldown flow for dashboards - manager.add("organizations:dashboards-drilldown-flow", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) + # Enable prebuilt dashboards for insights modules manager.add("organizations:dashboards-prebuilt-insights-dashboards", OrganizationFeature, FeatureHandlerStrategy.FLAGPOLE, api_expose=True) # Enable the details widget for dashboards diff --git a/static/app/actionCreators/modal.tsx b/static/app/actionCreators/modal.tsx index f208411930f11c..67ab641b8fdde1 100644 --- a/static/app/actionCreators/modal.tsx +++ b/static/app/actionCreators/modal.tsx @@ -14,7 +14,6 @@ import type {InviteRow} from 'sentry/components/modals/inviteMembersModal/types' import type {PrivateGamingSdkAccessModalProps} from 'sentry/components/modals/privateGamingSdkAccessModal'; import type {ReprocessEventModalOptions} from 'sentry/components/modals/reprocessEventModal'; import type {AddToDashboardModalProps} from 'sentry/components/modals/widgetBuilder/addToDashboardModal'; -import type {LinkToDashboardModalProps} from 'sentry/components/modals/widgetBuilder/linkToDashboardModal'; import type {ConsoleModalProps} from 'sentry/components/onboarding/consoleModal'; import type {Category} from 'sentry/components/platformPicker'; import {ModalStore} from 'sentry/stores/modalStore'; @@ -261,16 +260,6 @@ export async function openAddToDashboardModal(options: AddToDashboardModalProps) }); } -export async function openLinkToDashboardModal(options: LinkToDashboardModalProps) { - const {LinkToDashboardModal, modalCss} = - await import('sentry/components/modals/widgetBuilder/linkToDashboardModal'); - - openModal(deps => , { - closeEvents: 'escape-key', - modalCss, - }); -} - export async function openImportDashboardFromFileModal( options: ImportDashboardFromFileModalProps ) { diff --git a/static/app/components/modals/widgetBuilder/linkToDashboardModal.tsx b/static/app/components/modals/widgetBuilder/linkToDashboardModal.tsx deleted file mode 100644 index a079286c0a633d..00000000000000 --- a/static/app/components/modals/widgetBuilder/linkToDashboardModal.tsx +++ /dev/null @@ -1,256 +0,0 @@ -import {Fragment, useCallback, useEffect, useState, type ReactNode} from 'react'; -import {css} from '@emotion/react'; -import styled from '@emotion/styled'; - -import {Button} from '@sentry/scraps/button'; -import {Flex, Grid, type GridProps, Container} from '@sentry/scraps/layout'; -import {Select} from '@sentry/scraps/select'; -import {Tooltip} from '@sentry/scraps/tooltip'; - -import {fetchDashboard, fetchDashboards} from 'sentry/actionCreators/dashboards'; -import type {ModalRenderProps} from 'sentry/actionCreators/modal'; -import {Spinner} from 'sentry/components/forms/spinner'; -import {IconInfo} from 'sentry/icons'; -import {t, tct} from 'sentry/locale'; -import type {SelectValue} from 'sentry/types/core'; -import {useApi} from 'sentry/utils/useApi'; -import {useOrganization} from 'sentry/utils/useOrganization'; -import {useParams} from 'sentry/utils/useParams'; -import {DashboardCreateLimitWrapper} from 'sentry/views/dashboards/createLimitWrapper'; -import { - DashboardFilter, - MAX_WIDGETS, - type DashboardDetails, - type DashboardListItem, - type LinkedDashboard, -} from 'sentry/views/dashboards/types'; -import {NEW_DASHBOARD_ID} from 'sentry/views/dashboards/widgetBuilder/utils'; - -export type LinkToDashboardModalProps = { - currentLinkedDashboard?: LinkedDashboard; - onLink?: (dashboardId: string) => void; - // TODO: perhpas make this an enum - source?: string; -}; - -type Props = ModalRenderProps & LinkToDashboardModalProps; - -const SELECT_DASHBOARD_MESSAGE = t('Select a dashboard'); - -export function LinkToDashboardModal({ - Header, - Body, - Footer, - closeModal, - onLink, - currentLinkedDashboard, -}: Props) { - const api = useApi(); - const organization = useOrganization(); - const [dashboards, setDashboards] = useState(null); - const [_, setSelectedDashboard] = useState(null); - const [isDashboardListLoading, setIsDashboardListLoading] = useState(false); - const [selectedDashboardId, setSelectedDashboardId] = useState(null); - - const {dashboardId: currentDashboardId} = useParams<{dashboardId: string}>(); - - useEffect(() => { - // Track mounted state so we dont call setState on unmounted components - let unmounted = false; - - setIsDashboardListLoading(true); - - const dashboardListPromise = fetchDashboards(api, organization.slug, { - filter: DashboardFilter.SHOW_HIDDEN, - }); - const linkedDashboardPromise = currentLinkedDashboard?.dashboardId - ? fetchDashboard(api, organization.slug, currentLinkedDashboard.dashboardId).catch( - () => null - ) - : Promise.resolve(null); - - Promise.all([dashboardListPromise, linkedDashboardPromise]) - .then(([dashboardList, linkedDashboard]) => { - if (unmounted) { - return; - } - - if (linkedDashboard) { - const alreadyIncluded = dashboardList.some(d => d.id === linkedDashboard.id); - if (!alreadyIncluded) { - // Converts dashboard details to dashboard list item - dashboardList.push({ - id: linkedDashboard.id, - title: linkedDashboard.title, - filters: linkedDashboard.filters, - projects: linkedDashboard.projects ?? [], - environment: linkedDashboard.environment ?? [], - widgetDisplay: linkedDashboard.widgets.map(w => w.displayType), - widgetPreview: linkedDashboard.widgets.map(w => ({ - displayType: w.displayType, - layout: w.layout ?? null, - })), - dateCreated: linkedDashboard.dateCreated, - createdBy: linkedDashboard.createdBy, - }); - } - setSelectedDashboardId(linkedDashboard.id); - } - - setDashboards(dashboardList); - }) - .finally(() => { - setIsDashboardListLoading(false); - }); - - return () => { - unmounted = true; - }; - }, [api, organization.slug, currentLinkedDashboard?.dashboardId]); - - useEffect(() => { - // Track mounted state so we dont call setState on unmounted components - let unmounted = false; - - if (selectedDashboardId === NEW_DASHBOARD_ID || selectedDashboardId === null) { - setSelectedDashboard(null); - } else { - fetchDashboard(api, organization.slug, selectedDashboardId).then(response => { - // If component has unmounted, dont set state - if (unmounted) { - return; - } - - setSelectedDashboard(response); - }); - } - - return () => { - unmounted = true; - }; - }, [api, organization.slug, selectedDashboardId]); - - const canSubmit = selectedDashboardId !== null; - - const getOptions = useCallback( - ( - hasReachedDashboardLimit: boolean, - isLoading: boolean, - limitMessage: ReactNode | null - ) => { - if (dashboards === null) { - return []; - } - - return [ - { - label: t('+ Create New Dashboard'), - value: 'new', - disabled: hasReachedDashboardLimit || isLoading, - tooltip: hasReachedDashboardLimit ? limitMessage : undefined, - tooltipOptions: {position: 'right', isHoverable: true}, - }, - ...dashboards - .filter(dashboard => - // if adding from a dashboard, currentDashboardId will be set and we'll remove it from the list of options - currentDashboardId ? dashboard.id !== currentDashboardId : true - ) - .map(({title, id, widgetDisplay}) => ({ - label: title, - value: id, - disabled: widgetDisplay.length >= MAX_WIDGETS, - tooltip: - widgetDisplay.length >= MAX_WIDGETS && - tct('Max widgets ([maxWidgets]) per dashboard reached.', { - maxWidgets: MAX_WIDGETS, - }), - tooltipOptions: {position: 'right'}, - })), - ].filter(Boolean) as Array>; - }, - [currentDashboardId, dashboards] - ); - - function linkToDashboard() { - // TODO: Update the local state of the widget to include the links - // When the user clicks `save widget` we should update the dashboard widget link on the backend - if (selectedDashboardId) { - onLink?.(selectedDashboardId); - } - closeModal(); - } - - return ( - -
- - {t('Link to Dashboard')} - - - - -
- - - - {({hasReachedDashboardLimit, isLoading, limitMessage}) => { - if (isDashboardListLoading) { - return ; - } - return ( -