Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Enable activated alert creation via alert rule form #68959

Merged
merged 4 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions static/app/routes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1212,6 +1212,7 @@ function buildRoutes() {
);

const alertChildRoutes = ({forCustomerDomain}: {forCustomerDomain: boolean}) => {
// ALERT CHILD ROUTES
return (
<Fragment>
<IndexRoute
Expand Down Expand Up @@ -1316,6 +1317,7 @@ function buildRoutes() {
</Fragment>
);
};
// ALERT ROUTES
const alertRoutes = (
<Fragment>
{USING_CUSTOMER_DOMAIN && (
Expand Down
5 changes: 5 additions & 0 deletions static/app/types/alerts.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -291,3 +291,8 @@ export enum MonitorType {
CONTINUOUS = 0,
ACTIVATED = 1,
}

export enum ActivationConditionType {
RELEASE_CREATION = 0,
DEPLOY_CREATION = 1,
Comment on lines +296 to +297
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the ints map to the database storage level? if so 👍 if not, maybe we could use strings to define these so 0 won't evaluate as falsy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
4 changes: 2 additions & 2 deletions static/app/views/alerts/rules/metric/actions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ function isSavedRule(rule: MetricRule): rule is SavedMetricRule {
}

/**
* Add a new rule or update an existing rule
* Add a new alert rule or update an existing alert rule
*
* @param api API Client
* @param orgId Organization slug
Expand All @@ -16,7 +16,7 @@ function isSavedRule(rule: MetricRule): rule is SavedMetricRule {
*/
export function addOrUpdateRule(
api: Client,
orgId: string,
orgId: string, // organization slug
rule: MetricRule,
query?: object | any
) {
Expand Down
200 changes: 177 additions & 23 deletions static/app/views/alerts/rules/metric/ruleConditionsForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {SearchInvalidTag} from 'sentry/components/smartSearchBar/searchInvalidTa
import {t, tct} from 'sentry/locale';
import {space} from 'sentry/styles/space';
import type {Environment, Organization, Project, SelectValue} from 'sentry/types';
import {ActivationConditionType, MonitorType} from 'sentry/types/alerts';
import {getDisplayName} from 'sentry/utils/environment';
import {hasCustomMetrics} from 'sentry/utils/metrics/features';
import {getMRI} from 'sentry/utils/metrics/mri';
Expand Down Expand Up @@ -69,20 +70,28 @@ type Props = {
disabled: boolean;
onComparisonDeltaChange: (value: number) => void;
onFilterSearch: (query: string, isQueryValid) => void;
onMonitorTypeSelect: (activatedAlertFields: {
activationCondition?: ActivationConditionType | undefined;
monitorType?: MonitorType;
monitorWindowSuffix?: string | undefined;
monitorWindowValue?: number | undefined;
}) => void;
onTimeWindowChange: (value: number) => void;
organization: Organization;
project: Project;
projects: Project[];
router: InjectedRouter;
thresholdChart: React.ReactNode;
timeWindow: number;
activationCondition?: ActivationConditionType;
allowChangeEventTypes?: boolean;
comparisonDelta?: number;
disableProjectSelector?: boolean;
isErrorMigration?: boolean;
isExtrapolatedChartData?: boolean;
isTransactionMigration?: boolean;
loadingProjects?: boolean;
monitorType?: number;
};

type State = {
Expand Down Expand Up @@ -162,6 +171,20 @@ class RuleConditionsForm extends PureComponent<Props, State> {
}
}

get selectControlStyles() {
return {
control: (provided: {[x: string]: string | number | boolean}) => ({
...provided,
minWidth: 200,
maxWidth: 300,
}),
container: (provided: {[x: string]: string | number | boolean}) => ({
...provided,
margin: `${space(0.5)}`,
}),
};
}

renderEventTypeFilter() {
const {organization, disabled, alertType, isErrorMigration} = this.props;

Expand Down Expand Up @@ -326,8 +349,15 @@ class RuleConditionsForm extends PureComponent<Props, State> {
}

renderInterval() {
const {organization, disabled, alertType, timeWindow, onTimeWindowChange, project} =
this.props;
const {
organization,
disabled,
alertType,
timeWindow,
onTimeWindowChange,
project,
monitorType,
} = this.props;
Copy link
Contributor

Choose a reason for hiding this comment

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

we're using a lot of props here - i wonder if we could use hooks or reduce the prop drilling 🤔


return (
<Fragment>
Expand All @@ -353,27 +383,107 @@ class RuleConditionsForm extends PureComponent<Props, State> {
alertType={alertType}
required
/>
<SelectControl
name="timeWindow"
styles={{
control: (provided: {[x: string]: string | number | boolean}) => ({
...provided,
minWidth: 200,
maxWidth: 300,
}),
container: (provided: {[x: string]: string | number | boolean}) => ({
...provided,
margin: `${space(0.5)}`,
}),
}}
options={this.timeWindowOptions}
required
isDisabled={disabled}
value={timeWindow}
onChange={({value}) => onTimeWindowChange(value)}
inline={false}
flexibleControlStateSize
/>
{monitorType === MonitorType.CONTINUOUS && (
<SelectControl
name="timeWindow"
styles={this.selectControlStyles}
options={this.timeWindowOptions}
required={monitorType === MonitorType.CONTINUOUS}
isDisabled={disabled}
value={timeWindow}
onChange={({value}) => onTimeWindowChange(value)}
inline={false}
flexibleControlStateSize
/>
)}
</FormRow>
</Fragment>
);
}

renderMonitorTypeSelect() {
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than following the render* pattern here, could we just create a new functional component instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😢 was considering it...

but rather than having 2 render patterns, I figured we should keep it consistent.

hopefully we'll eventually rip this all out anyways

const {
onMonitorTypeSelect,
monitorType,
activationCondition,
timeWindow,
onTimeWindowChange,
} = this.props;

return (
<Fragment>
<StyledListItem>
<StyledListTitle>
<div>{t('Select Monitor Type')}</div>
</StyledListTitle>
</StyledListItem>
<FormRow>
<MonitorSelect>
<MonitorCard
position="left"
isSelected={monitorType === MonitorType.CONTINUOUS}
onClick={() =>
onMonitorTypeSelect({
monitorType: MonitorType.CONTINUOUS,
activationCondition,
})
}
>
<strong>Continuous</strong>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
<strong>Continuous</strong>
<strong>{t('Continuous')</strong>

<div>{t('Continuously monitor trends for the metrics outlined below')}</div>
</MonitorCard>
<MonitorCard
position="right"
isSelected={monitorType === MonitorType.ACTIVATED}
onClick={() =>
onMonitorTypeSelect({
monitorType: MonitorType.ACTIVATED,
})
}
>
<strong>Conditional</strong>
{monitorType === MonitorType.ACTIVATED ? (
<ActivatedAlertFields>
Monitor{' '}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
Monitor{' '}
{`${t('Monitor')} '}

<SelectControl
name="activationCondition"
styles={this.selectControlStyles}
options={[
{
value: ActivationConditionType.RELEASE_CREATION,
label: t('New Release'),
},
{
value: ActivationConditionType.DEPLOY_CREATION,
label: t('New Deploy'),
},
Comment on lines +452 to +459
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move these to constants

]}
required
value={activationCondition}
onChange={({value}) =>
onMonitorTypeSelect({activationCondition: value})
}
Comment on lines +463 to +465
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: prefer creating named function for these as it will redefine the function each render. if you give it a name then v8 can optimize.

Copy link
Member

Choose a reason for hiding this comment

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

Is this true @saponifi3d? Do you have a reference that describes what V8 does here?

We've almost never use named functions for callbacks, not sure if we want to start unless this really is a thing (but I'm not so sure it is)

inline={false}
flexibleControlStateSize
/>{' '}
for{' '}
saponifi3d marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for{' '}
{`${t('for')} `}

<SelectControl
name="timeWindow"
styles={this.selectControlStyles}
options={this.timeWindowOptions}
value={timeWindow}
onChange={({value}) => onTimeWindowChange(value)}
inline={false}
flexibleControlStateSize
/>
</ActivatedAlertFields>
) : (
<div>
{t('Temporarily monitor specified query given activation condition')}
</div>
)}
</MonitorCard>
</MonitorSelect>
</FormRow>
</Fragment>
);
Expand All @@ -395,6 +505,7 @@ class RuleConditionsForm extends PureComponent<Props, State> {
} = this.props;

const {environments} = this.state;
const hasActivatedAlerts = organization.features.includes('activated-alert-rules');

const environmentOptions: SelectValue<string | null>[] = [
{
Expand Down Expand Up @@ -425,6 +536,7 @@ class RuleConditionsForm extends PureComponent<Props, State> {
)}
/>
)}
{hasActivatedAlerts && this.renderMonitorTypeSelect()}
{!isErrorMigration && this.renderInterval()}
<StyledListItem>{t('Filter events')}</StyledListItem>
<FormRow noMargin columns={1 + (allowChangeEventTypes ? 1 : 0) + 1}>
Expand Down Expand Up @@ -653,4 +765,46 @@ const FormRow = styled('div')<{columns?: number; noMargin?: boolean}>`
`}
`;

const MonitorSelect = styled('div')`
border-radius: ${p => p.theme.borderRadius};
border: 1px solid ${p => p.theme.border};
width: 100%;
display: grid;
grid-template-columns: 1fr 1fr;
`;

type MonitorCardProps = {
isSelected: boolean;
/**
* Adds hover and focus states to the card
*/
position: 'left' | 'right';
};

const MonitorCard = styled('div')<MonitorCardProps>`
padding: ${space(1)};
display: flex;
flex-grow: 1;
flex-direction: column;
cursor: pointer;

&:focus,
&:hover {
outline: 1px solid ${p => p.theme.purple200};
background-color: ${p => p.theme.backgroundSecondary};
}

border-top-left-radius: ${p => (p.position === 'left' ? p.theme.borderRadius : 0)};
border-bottom-left-radius: ${p => (p.position === 'left' ? p.theme.borderRadius : 0)};
border-top-right-radius: ${p => (p.position !== 'left' ? p.theme.borderRadius : 0)};
border-bottom-right-radius: ${p => (p.position !== 'left' ? p.theme.borderRadius : 0)};
saponifi3d marked this conversation as resolved.
Show resolved Hide resolved
outline: ${p => (p.isSelected ? `1px solid ${p.theme.purple400}` : 'none')};
`;

const ActivatedAlertFields = styled('div')`
display: flex;
align-items: center;
justify-content: space-between;
`;

export default withApi(withProjects(RuleConditionsForm));
40 changes: 40 additions & 0 deletions static/app/views/alerts/rules/metric/ruleForm.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import selectEvent from 'sentry-test/selectEvent';
import {addErrorMessage} from 'sentry/actionCreators/indicator';
import type FormModel from 'sentry/components/forms/model';
import ProjectsStore from 'sentry/stores/projectsStore';
import {ActivationConditionType, MonitorType} from 'sentry/types/alerts';
import {metric} from 'sentry/utils/analytics';
import RuleFormContainer from 'sentry/views/alerts/rules/metric/ruleForm';
import {Dataset} from 'sentry/views/alerts/rules/metric/types';
Expand Down Expand Up @@ -254,6 +255,45 @@ describe('Incident Rules Form', () => {
);
});

it('creates a rule with an activation condition', async () => {
organization.features = [
...organization.features,
'mep-rollout-flag',
'activated-alert-rules',
];
const rule = MetricRuleFixture({
monitorType: MonitorType.ACTIVATED,
activationCondition: ActivationConditionType.RELEASE_CREATION,
});
createWrapper({
rule: {
...rule,
id: undefined,
aggregate: 'count()',
eventTypes: ['transaction'],
dataset: 'transactions',
},
});

expect(await screen.findByTestId('alert-total-events')).toHaveTextContent('Total5');

await userEvent.click(screen.getByLabelText('Save Rule'));

expect(createRule).toHaveBeenCalledWith(
expect.anything(),
expect.objectContaining({
data: expect.objectContaining({
name: 'My Incident Rule',
projects: ['project-slug'],
aggregate: 'count()',
eventTypes: ['transaction'],
dataset: 'generic_metrics',
thresholdPeriod: 1,
}),
})
);
});

it('switches to custom metric and selects event.type:error', async () => {
organization.features = [...organization.features, 'performance-view'];
const rule = MetricRuleFixture();
Expand Down
Loading
Loading