Browse Source

feat(metrics-migration): Show only relevant content on alert edit (#58750)

This PR introduces a migration mode to the alert edit page that shows
only that information to the user which is relevant for migrating the
alert thresholds. This mode is enabled by the query param `migration=1`
and only users that are affected by the migration will receive a link to
it.
Note that those changes are temporary and we will remove them once the
migration is complete.

- closes https://github.com/getsentry/sentry/issues/58614
ArthurKnaus 1 year ago
parent
commit
368efc241d

+ 5 - 2
static/app/views/alerts/edit.tsx

@@ -59,9 +59,12 @@ class ProjectAlertsEditor extends Component<Props, State> {
   }
   }
 
 
   render() {
   render() {
-    const {hasMetricAlerts, organization, project, members} = this.props;
+    const {hasMetricAlerts, organization, project, members, location} = this.props;
     const alertType = this.getAlertType();
     const alertType = this.getAlertType();
 
 
+    // TODO(telemetry-experience): Remove once the migration is complete
+    const isMigration = location?.query?.migration === '1';
+
     return (
     return (
       <Fragment>
       <Fragment>
         <SentryDocumentTitle
         <SentryDocumentTitle
@@ -73,7 +76,7 @@ class ProjectAlertsEditor extends Component<Props, State> {
           <Layout.HeaderContent>
           <Layout.HeaderContent>
             <BuilderBreadCrumbs
             <BuilderBreadCrumbs
               organization={organization}
               organization={organization}
-              title={t('Edit Alert Rule')}
+              title={isMigration ? t('Review Thresholds') : t('Edit Alert Rule')}
               projectSlug={project.slug}
               projectSlug={project.slug}
             />
             />
             <Layout.Title>{this.getTitle()}</Layout.Title>
             <Layout.Title>{this.getTitle()}</Layout.Title>

+ 5 - 5
static/app/views/alerts/rules/metric/details/body.tsx

@@ -159,11 +159,11 @@ export default function MetricDetailsBody({
   const showMigrationWarning =
   const showMigrationWarning =
     hasMigrationFeatureFlag(organization) && ruleNeedsMigration(rule);
     hasMigrationFeatureFlag(organization) && ruleNeedsMigration(rule);
 
 
-  const editThresholdLink =
+  const migrationFormLink =
     rule &&
     rule &&
     `/organizations/${organization.slug}/alerts/metric-rules/${
     `/organizations/${organization.slug}/alerts/metric-rules/${
       project?.slug ?? rule?.projects?.[0]
       project?.slug ?? rule?.projects?.[0]
-    }/${rule.id}/`;
+    }/${rule.id}/?migration=1`;
 
 
   return (
   return (
     <Fragment>
     <Fragment>
@@ -209,15 +209,15 @@ export default function MetricDetailsBody({
               showIcon
               showIcon
               trailingItems={
               trailingItems={
                 <LinkButton
                 <LinkButton
-                  to={editThresholdLink}
+                  to={migrationFormLink}
                   size="xs"
                   size="xs"
                   icon={<IconEdit size="xs" />}
                   icon={<IconEdit size="xs" />}
                 >
                 >
-                  {t('Edit')}
+                  {t('Review Thresholds')}
                 </LinkButton>
                 </LinkButton>
               }
               }
             >
             >
-              {t('The current thresholds for this alert could use some review')}
+              {t('The current thresholds for this alert could use some review.')}
             </Alert>
             </Alert>
           ) : null}
           ) : null}
 
 

+ 160 - 132
static/app/views/alerts/rules/metric/ruleConditionsForm.tsx

@@ -74,6 +74,7 @@ type Props = {
   comparisonDelta?: number;
   comparisonDelta?: number;
   disableProjectSelector?: boolean;
   disableProjectSelector?: boolean;
   isExtrapolatedChartData?: boolean;
   isExtrapolatedChartData?: boolean;
+  isMigration?: boolean;
   loadingProjects?: boolean;
   loadingProjects?: boolean;
 };
 };
 
 
@@ -375,6 +376,7 @@ class RuleConditionsForm extends PureComponent<Props, State> {
       allowChangeEventTypes,
       allowChangeEventTypes,
       dataset,
       dataset,
       isExtrapolatedChartData,
       isExtrapolatedChartData,
+      isMigration,
     } = this.props;
     } = this.props;
     const {environments} = this.state;
     const {environments} = this.state;
 
 
@@ -392,144 +394,156 @@ class RuleConditionsForm extends PureComponent<Props, State> {
         <ChartPanel>
         <ChartPanel>
           <StyledPanelBody>{this.props.thresholdChart}</StyledPanelBody>
           <StyledPanelBody>{this.props.thresholdChart}</StyledPanelBody>
         </ChartPanel>
         </ChartPanel>
-        {isExtrapolatedChartData && (
-          <OnDemandMetricAlert
-            message={t(
-              'The chart data above is an estimate based on the stored transactions that match the filters specified.'
+        {isMigration ? (
+          <Fragment>
+            <Spacer />
+            <HiddenListItem />
+            <HiddenListItem />
+          </Fragment>
+        ) : (
+          <Fragment>
+            {isExtrapolatedChartData && (
+              <OnDemandMetricAlert
+                message={t(
+                  'The chart data above is an estimate based on the stored transactions that match the filters specified.'
+                )}
+              />
             )}
             )}
-          />
-        )}
-        {this.renderInterval()}
-        <StyledListItem>{t('Filter events')}</StyledListItem>
-        <FormRow noMargin columns={1 + (allowChangeEventTypes ? 1 : 0) + 1}>
-          {this.renderProjectSelector()}
-          <SelectField
-            name="environment"
-            placeholder={t('All Environments')}
-            style={{
-              ...this.formElemBaseStyle,
-              minWidth: 230,
-              flex: 1,
-            }}
-            styles={{
-              singleValue: (base: any) => ({
-                ...base,
-              }),
-              option: (base: any) => ({
-                ...base,
-              }),
-            }}
-            options={environmentOptions}
-            isDisabled={disabled || this.state.environments === null}
-            isClearable
-            inline={false}
-            flexibleControlStateSize
-          />
-          {allowChangeEventTypes && this.renderEventTypeFilter()}
-        </FormRow>
-        <FormRow>
-          <FormField
-            name="query"
-            inline={false}
-            style={{
-              ...this.formElemBaseStyle,
-              flex: '6 0 500px',
-            }}
-            flexibleControlStateSize
-          >
-            {({onChange, onBlur, onKeyDown, initialData, value}) => {
-              return (
-                <SearchContainer>
-                  <StyledSearchBar
-                    disallowWildcard={dataset === Dataset.SESSIONS}
-                    invalidMessages={{
-                      [InvalidReason.WILDCARD_NOT_ALLOWED]: t(
-                        'The wildcard operator is not supported here.'
-                      ),
-                    }}
-                    customInvalidTagMessage={item => {
-                      if (
-                        ![Dataset.GENERIC_METRICS, Dataset.TRANSACTIONS].includes(dataset)
-                      ) {
-                        return null;
-                      }
-                      return (
-                        <SearchInvalidTag
-                          message={tct(
-                            "The field [field] isn't supported for performance alerts.",
+            {this.renderInterval()}
+            <StyledListItem>{t('Filter events')}</StyledListItem>
+            <FormRow noMargin columns={1 + (allowChangeEventTypes ? 1 : 0) + 1}>
+              {this.renderProjectSelector()}
+              <SelectField
+                name="environment"
+                placeholder={t('All Environments')}
+                style={{
+                  ...this.formElemBaseStyle,
+                  minWidth: 230,
+                  flex: 1,
+                }}
+                styles={{
+                  singleValue: (base: any) => ({
+                    ...base,
+                  }),
+                  option: (base: any) => ({
+                    ...base,
+                  }),
+                }}
+                options={environmentOptions}
+                isDisabled={disabled || this.state.environments === null}
+                isClearable
+                inline={false}
+                flexibleControlStateSize
+              />
+              {allowChangeEventTypes && this.renderEventTypeFilter()}
+            </FormRow>
+            <FormRow>
+              <FormField
+                name="query"
+                inline={false}
+                style={{
+                  ...this.formElemBaseStyle,
+                  flex: '6 0 500px',
+                }}
+                flexibleControlStateSize
+              >
+                {({onChange, onBlur, onKeyDown, initialData, value}) => {
+                  return (
+                    <SearchContainer>
+                      <StyledSearchBar
+                        disallowWildcard={dataset === Dataset.SESSIONS}
+                        invalidMessages={{
+                          [InvalidReason.WILDCARD_NOT_ALLOWED]: t(
+                            'The wildcard operator is not supported here.'
+                          ),
+                        }}
+                        customInvalidTagMessage={item => {
+                          if (
+                            ![Dataset.GENERIC_METRICS, Dataset.TRANSACTIONS].includes(
+                              dataset
+                            )
+                          ) {
+                            return null;
+                          }
+                          return (
+                            <SearchInvalidTag
+                              message={tct(
+                                "The field [field] isn't supported for performance alerts.",
+                                {
+                                  field: <code>{item.desc}</code>,
+                                }
+                              )}
+                              docLink="https://docs.sentry.io/product/alerts/create-alerts/metric-alert-config/#tags--properties"
+                            />
+                          );
+                        }}
+                        searchSource="alert_builder"
+                        defaultQuery={initialData?.query ?? ''}
+                        {...getSupportedAndOmittedTags(dataset, organization)}
+                        includeSessionTagsValues={dataset === Dataset.SESSIONS}
+                        disabled={disabled}
+                        useFormWrapper={false}
+                        organization={organization}
+                        placeholder={this.searchPlaceholder}
+                        onChange={onChange}
+                        query={initialData.query}
+                        // We only need strict validation for Transaction queries, everything else is fine
+                        highlightUnsupportedTags={
+                          organization.features.includes('alert-allow-indexed') ||
+                          (hasOnDemandMetricAlertFeature(organization) &&
+                            isOnDemandQueryString(initialData.query))
+                            ? false
+                            : [Dataset.GENERIC_METRICS, Dataset.TRANSACTIONS].includes(
+                                dataset
+                              )
+                        }
+                        onKeyDown={e => {
+                          /**
+                           * Do not allow enter key to submit the alerts form since it is unlikely
+                           * users will be ready to create the rule as this sits above required fields.
+                           */
+                          if (e.key === 'Enter') {
+                            e.preventDefault();
+                            e.stopPropagation();
+                          }
+
+                          onKeyDown?.(e);
+                        }}
+                        onClose={(query, {validSearch}) => {
+                          onFilterSearch(query, validSearch);
+                          onBlur(query);
+                        }}
+                        onSearch={query => {
+                          onFilterSearch(query, true);
+                          onChange(query, {});
+                        }}
+                        hasRecentSearches={dataset !== Dataset.SESSIONS}
+                      />
+                      {isExtrapolatedChartData && isOnDemandQueryString(value) && (
+                        <OnDemandWarningIcon
+                          color="gray500"
+                          msg={tct(
+                            `We don’t routinely collect metrics from [fields]. However, we’ll do so [strong:once this alert has been saved.]`,
                             {
                             {
-                              field: <code>{item.desc}</code>,
+                              fields: (
+                                <strong>
+                                  {getOnDemandKeys(value)
+                                    .map(key => `"${key}"`)
+                                    .join(', ')}
+                                </strong>
+                              ),
+                              strong: <strong />,
                             }
                             }
                           )}
                           )}
-                          docLink="https://docs.sentry.io/product/alerts/create-alerts/metric-alert-config/#tags--properties"
                         />
                         />
-                      );
-                    }}
-                    searchSource="alert_builder"
-                    defaultQuery={initialData?.query ?? ''}
-                    {...getSupportedAndOmittedTags(dataset, organization)}
-                    includeSessionTagsValues={dataset === Dataset.SESSIONS}
-                    disabled={disabled}
-                    useFormWrapper={false}
-                    organization={organization}
-                    placeholder={this.searchPlaceholder}
-                    onChange={onChange}
-                    query={initialData.query}
-                    // We only need strict validation for Transaction queries, everything else is fine
-                    highlightUnsupportedTags={
-                      organization.features.includes('alert-allow-indexed') ||
-                      (hasOnDemandMetricAlertFeature(organization) &&
-                        isOnDemandQueryString(initialData.query))
-                        ? false
-                        : [Dataset.GENERIC_METRICS, Dataset.TRANSACTIONS].includes(
-                            dataset
-                          )
-                    }
-                    onKeyDown={e => {
-                      /**
-                       * Do not allow enter key to submit the alerts form since it is unlikely
-                       * users will be ready to create the rule as this sits above required fields.
-                       */
-                      if (e.key === 'Enter') {
-                        e.preventDefault();
-                        e.stopPropagation();
-                      }
-
-                      onKeyDown?.(e);
-                    }}
-                    onClose={(query, {validSearch}) => {
-                      onFilterSearch(query, validSearch);
-                      onBlur(query);
-                    }}
-                    onSearch={query => {
-                      onFilterSearch(query, true);
-                      onChange(query, {});
-                    }}
-                    hasRecentSearches={dataset !== Dataset.SESSIONS}
-                  />
-                  {isExtrapolatedChartData && isOnDemandQueryString(value) && (
-                    <OnDemandWarningIcon
-                      color="gray500"
-                      msg={tct(
-                        `We don’t routinely collect metrics from [fields]. However, we’ll do so [strong:once this alert has been saved.]`,
-                        {
-                          fields: (
-                            <strong>
-                              {getOnDemandKeys(value)
-                                .map(key => `"${key}"`)
-                                .join(', ')}
-                            </strong>
-                          ),
-                          strong: <strong />,
-                        }
                       )}
                       )}
-                    />
-                  )}
-                </SearchContainer>
-              );
-            }}
-          </FormField>
-        </FormRow>
+                    </SearchContainer>
+                  );
+                }}
+              </FormField>
+            </FormRow>
+          </Fragment>
+        )}
       </Fragment>
       </Fragment>
     );
     );
   }
   }
@@ -542,6 +556,20 @@ const StyledListTitle = styled('div')`
   }
   }
 `;
 `;
 
 
+// This is a temporary hacky solution to hide list items without changing the numbering of the rest of the list
+// TODO(telemetry-experience): Remove this once the migration is complete
+const HiddenListItem = styled(ListItem)`
+  position: absolute;
+  width: 0px;
+  height: 0px;
+  opacity: 0;
+  pointer-events: none;
+`;
+
+const Spacer = styled('div')`
+  margin-bottom: ${space(2)};
+`;
+
 const ChartPanel = styled(Panel)`
 const ChartPanel = styled(Panel)`
   margin-bottom: ${space(1)};
   margin-bottom: ${space(1)};
 `;
 `;

+ 3 - 1
static/app/views/alerts/rules/metric/ruleForm.spec.tsx

@@ -24,12 +24,13 @@ jest.mock('sentry/utils/analytics', () => ({
 }));
 }));
 
 
 describe('Incident Rules Form', () => {
 describe('Incident Rules Form', () => {
-  let organization, project, routerContext;
+  let organization, project, routerContext, location;
   const createWrapper = props =>
   const createWrapper = props =>
     render(
     render(
       <RuleFormContainer
       <RuleFormContainer
         params={{orgId: organization.slug, projectId: project.slug}}
         params={{orgId: organization.slug, projectId: project.slug}}
         organization={organization}
         organization={organization}
+        location={location}
         project={project}
         project={project}
         {...props}
         {...props}
       />,
       />,
@@ -42,6 +43,7 @@ describe('Incident Rules Form', () => {
     });
     });
     organization = initialData.organization;
     organization = initialData.organization;
     project = initialData.project;
     project = initialData.project;
+    location = initialData.router.location;
     ProjectsStore.loadInitialData([project]);
     ProjectsStore.loadInitialData([project]);
     routerContext = initialData.routerContext;
     routerContext = initialData.routerContext;
     MockApiClient.addMockResponse({
     MockApiClient.addMockResponse({

+ 15 - 12
static/app/views/alerts/rules/metric/ruleForm.tsx

@@ -129,6 +129,8 @@ type State = {
   triggers: Trigger[];
   triggers: Trigger[];
   comparisonDelta?: number;
   comparisonDelta?: number;
   isExtrapolatedChartData?: boolean;
   isExtrapolatedChartData?: boolean;
+  // TODO(telemetry-experiment): remove this state once the migration is complete
+  triggersHaveChanged?: boolean;
   uuid?: string;
   uuid?: string;
 } & DeprecatedAsyncComponent['state'];
 } & DeprecatedAsyncComponent['state'];
 
 
@@ -472,6 +474,7 @@ class RuleFormContainer extends DeprecatedAsyncComponent<Props, State> {
 
 
   handleFieldChange = (name: string, value: unknown) => {
   handleFieldChange = (name: string, value: unknown) => {
     const {projects} = this.props;
     const {projects} = this.props;
+
     const dataset = this.checkOnDemandMetricsDataset(
     const dataset = this.checkOnDemandMetricsDataset(
       this.state.dataset,
       this.state.dataset,
       this.state.query
       this.state.query
@@ -703,7 +706,7 @@ class RuleFormContainer extends DeprecatedAsyncComponent<Props, State> {
         clearIndicators();
         clearIndicators();
       }
       }
 
 
-      return {triggers, triggerErrors};
+      return {triggers, triggerErrors, triggersHaveChanged: true};
     });
     });
   };
   };
 
 
@@ -912,6 +915,7 @@ class RuleFormContainer extends DeprecatedAsyncComponent<Props, State> {
       router,
       router,
       disableProjectSelector,
       disableProjectSelector,
       eventView,
       eventView,
+      location,
     } = this.props;
     } = this.props;
     const {
     const {
       name,
       name,
@@ -930,9 +934,12 @@ class RuleFormContainer extends DeprecatedAsyncComponent<Props, State> {
       dataset,
       dataset,
       alertType,
       alertType,
       isExtrapolatedChartData,
       isExtrapolatedChartData,
+      triggersHaveChanged,
     } = this.state;
     } = this.state;
 
 
     const wizardBuilderChart = this.renderTriggerChart();
     const wizardBuilderChart = this.renderTriggerChart();
+    // TODO(telemetry-experience): Remove this and all connected logic once the migration is complete
+    const isMigration = location?.query?.migration === '1';
 
 
     const triggerForm = (disabled: boolean) => (
     const triggerForm = (disabled: boolean) => (
       <Triggers
       <Triggers
@@ -941,6 +948,7 @@ class RuleFormContainer extends DeprecatedAsyncComponent<Props, State> {
         errors={this.state.triggerErrors}
         errors={this.state.triggerErrors}
         triggers={triggers}
         triggers={triggers}
         aggregate={aggregate}
         aggregate={aggregate}
+        isMigration={isMigration}
         resolveThreshold={resolveThreshold}
         resolveThreshold={resolveThreshold}
         thresholdPeriod={thresholdPeriod}
         thresholdPeriod={thresholdPeriod}
         thresholdType={thresholdType}
         thresholdType={thresholdType}
@@ -1028,12 +1036,15 @@ class RuleFormContainer extends DeprecatedAsyncComponent<Props, State> {
               </Confirm>
               </Confirm>
             ) : null
             ) : null
           }
           }
-          submitLabel={t('Save Rule')}
+          submitLabel={
+            isMigration && !triggersHaveChanged ? t('Looks good to me!') : t('Save Rule')
+          }
         >
         >
           <List symbol="colored-numeric">
           <List symbol="colored-numeric">
             <RuleConditionsForm
             <RuleConditionsForm
               project={project}
               project={project}
               organization={organization}
               organization={organization}
+              isMigration={isMigration}
               router={router}
               router={router}
               disabled={formDisabled}
               disabled={formDisabled}
               thresholdChart={wizardBuilderChart}
               thresholdChart={wizardBuilderChart}
@@ -1054,15 +1065,7 @@ class RuleFormContainer extends DeprecatedAsyncComponent<Props, State> {
             <AlertListItem>{t('Set thresholds')}</AlertListItem>
             <AlertListItem>{t('Set thresholds')}</AlertListItem>
             {thresholdTypeForm(formDisabled)}
             {thresholdTypeForm(formDisabled)}
             {showMigrationWarning && (
             {showMigrationWarning && (
-              <Alert
-                type="warning"
-                showIcon
-                trailingItems={
-                  <Button size="xs" type="submit">
-                    {t('Looks good to me!')}
-                  </Button>
-                }
-              >
+              <Alert type="warning" showIcon>
                 {tct(
                 {tct(
                   'Check the chart above and make sure the current thresholds are still valid, given that this alert is now based on [tooltip:total events].',
                   'Check the chart above and make sure the current thresholds are still valid, given that this alert is now based on [tooltip:total events].',
                   {
                   {
@@ -1088,7 +1091,7 @@ class RuleFormContainer extends DeprecatedAsyncComponent<Props, State> {
               </Alert>
               </Alert>
             )}
             )}
             {triggerForm(formDisabled)}
             {triggerForm(formDisabled)}
-            {ruleNameOwnerForm(formDisabled)}
+            {isMigration ? <Fragment /> : ruleNameOwnerForm(formDisabled)}
           </List>
           </List>
         </Form>
         </Form>
       </Main>
       </Main>

+ 17 - 13
static/app/views/alerts/rules/metric/triggers/index.tsx

@@ -36,12 +36,13 @@ type Props = {
   onThresholdTypeChange: (thresholdType: AlertRuleThresholdType) => void;
   onThresholdTypeChange: (thresholdType: AlertRuleThresholdType) => void;
   organization: Organization;
   organization: Organization;
   projects: Project[];
   projects: Project[];
-
   resolveThreshold: UnsavedMetricRule['resolveThreshold'];
   resolveThreshold: UnsavedMetricRule['resolveThreshold'];
 
 
   thresholdPeriod: UnsavedMetricRule['thresholdPeriod'];
   thresholdPeriod: UnsavedMetricRule['thresholdPeriod'];
+
   thresholdType: UnsavedMetricRule['thresholdType'];
   thresholdType: UnsavedMetricRule['thresholdType'];
   triggers: Trigger[];
   triggers: Trigger[];
+  isMigration?: boolean;
 };
 };
 
 
 /**
 /**
@@ -104,6 +105,7 @@ class Triggers extends Component<Props> {
       thresholdPeriod,
       thresholdPeriod,
       comparisonType,
       comparisonType,
       resolveThreshold,
       resolveThreshold,
+      isMigration,
       onThresholdTypeChange,
       onThresholdTypeChange,
       onResolveThresholdChange,
       onResolveThresholdChange,
       onThresholdPeriodChange,
       onThresholdPeriodChange,
@@ -133,18 +135,20 @@ class Triggers extends Component<Props> {
           </PanelBody>
           </PanelBody>
         </Panel>
         </Panel>
 
 
-        <ActionsPanel
-          disabled={disabled}
-          loading={availableActions === null}
-          error={false}
-          availableActions={availableActions}
-          currentProject={currentProject}
-          organization={organization}
-          projects={projects}
-          triggers={triggers}
-          onChange={this.handleChangeActions}
-          onAdd={this.handleAddAction}
-        />
+        {isMigration ? null : (
+          <ActionsPanel
+            disabled={disabled}
+            loading={availableActions === null}
+            error={false}
+            availableActions={availableActions}
+            currentProject={currentProject}
+            organization={organization}
+            projects={projects}
+            triggers={triggers}
+            onChange={this.handleChangeActions}
+            onAdd={this.handleAddAction}
+          />
+        )}
       </Fragment>
       </Fragment>
     );
     );
   }
   }