-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(aci): redirect alert rules to detectors #103682
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
base: master
Are you sure you want to change the base?
Conversation
|
saponifi3d
left a comment
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.
i just generally have nits, but not super familiar with the routing layer here -- would recommend getting a second set of eyes for that stuff.
| const {ruleId, detectorId} = useParams(); | ||
|
|
||
| const shouldRedirect = | ||
| !user.isStaff && organization.features.includes('workflow-engine-ui'); |
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.
curious about the !user.isStaff piece here, is that so we can continue with everything as is today w/o redirecting? might be nice to just have staff use the same experience; and this could be a nice forcing function for everyone to dog food this before we flip the LA switch.
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.
Adding the isStaff check makes it easier for us to compare the old vs. new experience while developing, so I'd like to keep this check for now
| alertType === 'crons' | ||
| ? 'monitor_check_in_failure' | ||
| : alertType === 'uptime' | ||
| ? 'uptime_domain_failure' | ||
| : null; |
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.
nit: recommend against having nested ternaries, makes readability even with these indentations a bit tricky.
Maybe something like:
const getDetectionType = (alertType: string): string => {
switch(alertType) {
case 'crons':
return 'monitor_check_in_failure';
case 'uptime':
return 'uptime_domain_failure';
default:
return null;
}
}
// in use
const detectorType = getDetectionType(alertType);this makes it a little easier to quickly read, but also makes it a lot easier to add a 3rd type if we need to or anything.
static/app/router/routes.tsx
Outdated
| { | ||
| path: 'details/:ruleId/', | ||
| component: make(() => import('sentry/views/alerts/rules/metric/details')), | ||
| component: WorkflowEngineRedirectToDetectorDetails, |
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.
import should still be lazy
| * Base component for workflow engine redirects that require fetching | ||
| * workflow data from a metric alert rule before redirecting. | ||
| */ | ||
| function WorkflowEngineRedirectWithAlertRuleData({ |
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.
can we make this a hook (or small HoCish wrapper for class components) and wrap the areas we want to redirect instead. Adding these to the route tree doesn't totally make sense if they aren't views
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.
This can't be a hook because it needs to render the loading state while the requests are in flight, but it could be an HoC. That would feel cleaner and wouldn't require changing the route tree
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.
i think that's where i'm at yeah this shouldn't need to be in the route tree
| { | ||
| path: 'details/:ruleId/', | ||
| component: make(() => import('sentry/views/alerts/rules/metric/details')), | ||
| component: withDetectorDetailsRedirect( |
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.
Unfortunately this isn't completely lazy, you need to make a new file with a default export that is export default withDetectorDetailsRedirect(MetricDetails)
redirects metric alert rules and uptime monitors to the new monitors views.
note that links to cron monitors do not get redirected at the moment since that would require a lookup from the monitorSlug to the detectorId