-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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'; | ||||||
|
@@ -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 = { | ||||||
|
@@ -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; | ||||||
|
||||||
|
@@ -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; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||||||
|
@@ -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() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
|
||||||
<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{' '} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
|
||||||
<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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
<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> | ||||||
); | ||||||
|
@@ -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>[] = [ | ||||||
{ | ||||||
|
@@ -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}> | ||||||
|
@@ -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)); |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yessir they do indeed