Skip to content

Commit c62601a

Browse files
authored
feat(alerts): Disable editing alert query for txn (#93440)
Lock changes to the alert if transaction deprecation is disabled.
1 parent a464390 commit c62601a

File tree

4 files changed

+193
-64
lines changed

4 files changed

+193
-64
lines changed

static/app/views/alerts/rules/metric/ruleConditionsForm.tsx

Lines changed: 113 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
} from 'sentry/components/alerts/onDemandMetricAlert';
1313
import {Alert} from 'sentry/components/core/alert';
1414
import {Select} from 'sentry/components/core/select';
15+
import {Tooltip} from 'sentry/components/core/tooltip';
1516
import {getHasTag} from 'sentry/components/events/searchBar';
1617
import {
1718
STATIC_FIELD_TAGS,
@@ -66,11 +67,17 @@ import {
6667
DATA_SOURCE_TO_SET_AND_EVENT_TYPES,
6768
} from 'sentry/views/alerts/utils';
6869
import type {AlertType} from 'sentry/views/alerts/wizard/options';
69-
import {getSupportedAndOmittedTags} from 'sentry/views/alerts/wizard/options';
70+
import {
71+
DEPRECATED_TRANSACTION_ALERTS,
72+
getSupportedAndOmittedTags,
73+
} from 'sentry/views/alerts/wizard/options';
7074
import {useSpanTags} from 'sentry/views/explore/contexts/spanTagsContext';
7175
import {TraceItemAttributeProvider} from 'sentry/views/explore/contexts/traceItemAttributeContext';
7276
import {TraceItemDataset} from 'sentry/views/explore/types';
73-
import {hasEAPAlerts} from 'sentry/views/insights/common/utils/hasEAPAlerts';
77+
import {
78+
deprecateTransactionAlerts,
79+
hasEAPAlerts,
80+
} from 'sentry/views/insights/common/utils/hasEAPAlerts';
7481

7582
import {
7683
DEFAULT_AGGREGATE,
@@ -263,6 +270,24 @@ class RuleConditionsForm extends PureComponent<Props, State> {
263270
};
264271
}
265272

273+
get disableTransactionAlertType() {
274+
const {organization, alertType, isEditing} = this.props;
275+
276+
return (
277+
isEditing &&
278+
alertType &&
279+
deprecateTransactionAlerts(organization) &&
280+
DEPRECATED_TRANSACTION_ALERTS.includes(alertType) &&
281+
hasEAPAlerts(organization)
282+
);
283+
}
284+
285+
get transactionAlertDisabledMessage() {
286+
return t(
287+
'Transaction based alerts are no longer supported. Create span alerts instead.'
288+
);
289+
}
290+
266291
renderEventTypeFilter() {
267292
const {organization, disabled, alertType, isErrorMigration} = this.props;
268293

@@ -387,7 +412,9 @@ class RuleConditionsForm extends PureComponent<Props, State> {
387412

388413
return (
389414
<Select
390-
isDisabled={disabled || disableProjectSelector}
415+
isDisabled={
416+
disabled || disableProjectSelector || this.disableTransactionAlertType
417+
}
391418
value={selectedProject.id}
392419
options={projectOptions}
393420
onChange={({value}: {value: Project['id']}) => {
@@ -436,6 +463,7 @@ class RuleConditionsForm extends PureComponent<Props, State> {
436463
dataset,
437464
comparisonType,
438465
onTimeWindowChange,
466+
isEditing,
439467
} = this.props;
440468

441469
return (
@@ -445,34 +473,46 @@ class RuleConditionsForm extends PureComponent<Props, State> {
445473
<div>{t('Define your metric')}</div>
446474
</StyledListTitle>
447475
</StyledListItem>
448-
<FormRow>
449-
<WizardField
450-
name="aggregate"
451-
help={null}
452-
organization={organization}
453-
disabled={disabled}
454-
project={project}
455-
style={{
456-
...this.formElemBaseStyle,
457-
flex: 1,
458-
}}
459-
inline={false}
460-
flexibleControlStateSize
461-
columnWidth={200}
462-
alertType={alertType}
463-
required
464-
/>
465-
<Select
466-
name="timeWindow"
467-
styles={this.selectControlStyles}
468-
options={getTimeWindowOptions(dataset, comparisonType)}
469-
isDisabled={disabled}
470-
value={timeWindow}
471-
onChange={({value}: any) => onTimeWindowChange(value)}
472-
inline={false}
473-
flexibleControlStateSize
474-
/>
475-
</FormRow>
476+
<Tooltip
477+
title={this.transactionAlertDisabledMessage}
478+
disabled={!this.disableTransactionAlertType}
479+
isHoverable
480+
>
481+
<FormRow>
482+
<WizardField
483+
name="aggregate"
484+
help={null}
485+
organization={organization}
486+
disabled={disabled}
487+
project={project}
488+
style={{
489+
...this.formElemBaseStyle,
490+
flex: 1,
491+
}}
492+
inline={false}
493+
flexibleControlStateSize
494+
columnWidth={200}
495+
alertType={alertType}
496+
required
497+
isEditing={isEditing}
498+
disabledReason={
499+
this.disableTransactionAlertType
500+
? this.transactionAlertDisabledMessage
501+
: undefined
502+
}
503+
/>
504+
<Select
505+
name="timeWindow"
506+
styles={this.selectControlStyles}
507+
options={getTimeWindowOptions(dataset, comparisonType)}
508+
isDisabled={disabled || this.disableTransactionAlertType}
509+
value={timeWindow}
510+
onChange={({value}: any) => onTimeWindowChange(value)}
511+
inline={false}
512+
flexibleControlStateSize
513+
/>
514+
</FormRow>
515+
</Tooltip>
476516
</Fragment>
477517
);
478518
}
@@ -545,34 +585,43 @@ class RuleConditionsForm extends PureComponent<Props, State> {
545585
)}
546586
{!isErrorMigration && this.renderInterval()}
547587
<StyledListItem>{t('Filter events')}</StyledListItem>
548-
<FormRow noMargin columns={1 + (allowChangeEventTypes ? 1 : 0) + 1}>
549-
{this.renderProjectSelector()}
550-
<SelectField
551-
name="environment"
552-
placeholder={t('All Environments')}
553-
style={{
554-
...this.formElemBaseStyle,
555-
minWidth: 230,
556-
flex: 1,
557-
}}
558-
styles={{
559-
singleValue: (base: any) => ({
560-
...base,
561-
}),
562-
option: (base: any) => ({
563-
...base,
564-
}),
565-
}}
566-
options={environmentOptions}
567-
isDisabled={
568-
disabled || this.state.environments === null || isErrorMigration
569-
}
570-
isClearable
571-
inline={false}
572-
flexibleControlStateSize
573-
/>
574-
{allowChangeEventTypes && this.renderEventTypeFilter()}
575-
</FormRow>
588+
<Tooltip
589+
title={this.transactionAlertDisabledMessage}
590+
disabled={!this.disableTransactionAlertType}
591+
isHoverable
592+
>
593+
<FormRow noMargin columns={1 + (allowChangeEventTypes ? 1 : 0) + 1}>
594+
{this.renderProjectSelector()}
595+
<SelectField
596+
name="environment"
597+
placeholder={t('All Environments')}
598+
style={{
599+
...this.formElemBaseStyle,
600+
minWidth: 230,
601+
flex: 1,
602+
}}
603+
styles={{
604+
singleValue: (base: any) => ({
605+
...base,
606+
}),
607+
option: (base: any) => ({
608+
...base,
609+
}),
610+
}}
611+
options={environmentOptions}
612+
isDisabled={
613+
disabled ||
614+
this.state.environments === null ||
615+
isErrorMigration ||
616+
this.disableTransactionAlertType
617+
}
618+
isClearable
619+
inline={false}
620+
flexibleControlStateSize
621+
/>
622+
{allowChangeEventTypes && this.renderEventTypeFilter()}
623+
</FormRow>
624+
</Tooltip>
576625
<FormRow noMargin>
577626
<FormField
578627
name="query"
@@ -604,7 +653,11 @@ class RuleConditionsForm extends PureComponent<Props, State> {
604653
placeholder={this.searchPlaceholder}
605654
searchSource="alert_builder"
606655
filterKeys={filterKeys}
607-
disabled={disabled || isErrorMigration}
656+
disabled={
657+
disabled ||
658+
isErrorMigration ||
659+
this.disableTransactionAlertType
660+
}
608661
onChange={onChange}
609662
invalidMessages={{
610663
[InvalidReason.WILDCARD_NOT_ALLOWED]: t(

static/app/views/alerts/rules/metric/ruleForm.spec.tsx

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -540,6 +540,52 @@ describe('Incident Rules Form', () => {
540540
);
541541
}, 10000);
542542

543+
it('disables editing transaction alert type if deprecated flag is enabled', async () => {
544+
organization.features = [
545+
...organization.features,
546+
'performance-view',
547+
'visibility-explore-view',
548+
'performance-transaction-deprecation-alerts',
549+
];
550+
const metricRule = MetricRuleFixture();
551+
createWrapper({
552+
rule: {
553+
...metricRule,
554+
aggregate: 'count()',
555+
eventTypes: ['transaction'],
556+
dataset: 'transactions',
557+
},
558+
ruleId: rule.id,
559+
});
560+
561+
await userEvent.hover(screen.getAllByText('Throughput')[1]!);
562+
expect(
563+
await screen.findByText(
564+
'Transaction based alerts are no longer supported. Create span alerts instead.'
565+
)
566+
).toBeInTheDocument();
567+
568+
await userEvent.hover(screen.getByText('project-slug'));
569+
expect(
570+
await screen.findByText(
571+
'Transaction based alerts are no longer supported. Create span alerts instead.'
572+
)
573+
).toBeInTheDocument();
574+
575+
const radio = screen.getByRole('radio', {
576+
name: 'Percent Change: {x%} higher or lower compared to previous period',
577+
});
578+
expect(radio).not.toBeChecked();
579+
580+
await userEvent.click(
581+
screen.getByText(
582+
'Percent Change: {x%} higher or lower compared to previous period'
583+
)
584+
);
585+
586+
await waitFor(() => expect(radio).toBeChecked());
587+
});
588+
543589
it('switches from percent change to count', async () => {
544590
createWrapper({
545591
ruleId: rule.id,

static/app/views/alerts/rules/metric/wizardField.tsx

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import type {AlertType} from 'sentry/views/alerts/wizard/options';
1717
import {
1818
AlertWizardAlertNames,
1919
AlertWizardRuleTemplates,
20+
DEPRECATED_TRANSACTION_ALERTS,
2021
} from 'sentry/views/alerts/wizard/options';
2122
import {QueryField} from 'sentry/views/discover/table/queryField';
2223
import {FieldValueKind} from 'sentry/views/discover/table/types';
@@ -50,6 +51,12 @@ export default function WizardField({
5051
alertType,
5152
...fieldProps
5253
}: Props) {
54+
const isDeprecatedTransactionAlertType =
55+
alertType &&
56+
deprecateTransactionAlerts(organization) &&
57+
DEPRECATED_TRANSACTION_ALERTS.includes(alertType) &&
58+
hasEAPAlerts(organization);
59+
5360
const deprecatedTransactionAggregationOptions: MenuOption[] = [
5461
{
5562
label: AlertWizardAlertNames.throughput,
@@ -158,6 +165,15 @@ export default function WizardField({
158165
},
159166
]
160167
: []),
168+
169+
...(fieldProps.isEditing && isDeprecatedTransactionAlertType
170+
? [
171+
{
172+
label: AlertWizardAlertNames[alertType],
173+
value: alertType,
174+
},
175+
]
176+
: []),
161177
],
162178
},
163179
{
@@ -173,7 +189,7 @@ export default function WizardField({
173189

174190
return (
175191
<FormField {...fieldProps}>
176-
{({onChange, model, disabled}: any) => {
192+
{({onChange, model, disabled, isEditing, disabledReason}: any) => {
177193
const aggregate = model.getValue('aggregate');
178194
const dataset: Dataset = model.getValue('dataset');
179195
const selectedTemplate: AlertType = alertType || 'eap_metrics';
@@ -183,7 +199,10 @@ export default function WizardField({
183199
dataset,
184200
alertType,
185201
});
186-
const fieldOptions = generateFieldOptions({organization, ...fieldOptionsConfig});
202+
const fieldOptions = generateFieldOptions({
203+
organization,
204+
...fieldOptionsConfig,
205+
});
187206
const fieldValue = getFieldValue(aggregate ?? '', model);
188207

189208
const fieldKey =
@@ -208,7 +227,8 @@ export default function WizardField({
208227
<Select
209228
value={selectedTemplate}
210229
options={menuOptions}
211-
disabled={disabled}
230+
disabled={disabled || (isEditing && isDeprecatedTransactionAlertType)}
231+
disabledReason={disabledReason}
212232
onChange={(option: MenuOption) => {
213233
// @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message
214234
const template = AlertWizardRuleTemplates[option.value];
@@ -239,7 +259,7 @@ export default function WizardField({
239259
gridColumns={gridColumns}
240260
inFieldLabels={inFieldLabels}
241261
shouldRenderTag={false}
242-
disabled={disabled}
262+
disabled={disabled || (isEditing && isDeprecatedTransactionAlertType)}
243263
hideParameterSelector={hideParameterSelector}
244264
hidePrimarySelector={hidePrimarySelector}
245265
/>

static/app/views/alerts/wizard/options.tsx

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,16 @@ export type MetricAlertType = Exclude<
6464
'issues' | 'uptime_monitor' | 'crons_monitor'
6565
>;
6666

67+
export const DEPRECATED_TRANSACTION_ALERTS: AlertType[] = [
68+
'throughput',
69+
'trans_duration',
70+
'apdex',
71+
'failure_rate',
72+
'lcp',
73+
'fid',
74+
'cls',
75+
];
76+
6777
export const DatasetMEPAlertQueryTypes: Record<
6878
Exclude<Dataset, Dataset.ISSUE_PLATFORM | Dataset.SESSIONS | Dataset.REPLAYS>, // IssuePlatform (search_issues) is not used in alerts, so we can exclude it here
6979
MEPAlertsQueryType

0 commit comments

Comments
 (0)