feat(aci): Create detector on submit #93380
Conversation
github-actions
bot
added
the
Scope: Frontend
scttcper
changed the title
❌ 4 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
There was a problem hiding this comment.
😅 sorry for so many comments, in future i think it might be easier if the PR is a bit smaller scope. things like introducing the priorityControl then using it in the detector form would make it a bit easier to review.
biggest pieces of feedback would be if we can match the enums a bit better to the server, then you might have an easier time with typing etc and wanting to understand why we are re-mapping the priority level things to a new structure.
|
|
||
| return ( | ||
| <NewDetectorLayout> | ||
| <MetricDetectorForm model={model} /> | ||
| <FullHeightForm |
There was a problem hiding this comment.
is any of this stuff something we should make shared components from?
It seems like we could have a lot of little split out components to me,
<StyledLayoutHeader>
<Layout.HeaderContent>
<NewDetectorBreadcrumbs />
<Layout.Title>
<EditableDetectorName />
</Layout.Title>
</Layout.HeaderContent>
</StyledLayoutHeader>
might be a shareable DetectorHeader -- need to find those common components because we'll have so many different detector types. (the other trick is to make them composable to allow for overrides)
There was a problem hiding this comment.
the thirds layout we're using is the shared layout component for now
| /** | ||
| * Creates the data source configuration for the detector | ||
| */ | ||
| function createDataSource(data: MetricDetectorFormData): NewDataSource { |
There was a problem hiding this comment.
should the NewDataSource type be a generic type that we pass in stuff like SnubaDataSource to? that way we could have a generic for anything that's common in the data sources and have a strict typing to snuba for the metric case.
There was a problem hiding this comment.
yeah i've not figured out what that looks like yet. That part of the form isn't built
| const eventTypes = ['error']; | ||
|
|
||
| return { | ||
| queryType: 0, |
There was a problem hiding this comment.
| queryType: 0, | |
| queryType: DetectorType.METRIC, |
seems like we should drive this through an enum, and something we can access from the form itself.
There was a problem hiding this comment.
i'll figure out query type and the enum when we add that part of the form soon
| : lowThresholdDirection === 'higher' | ||
| ? t('higher') | ||
| : t('lower')} | ||
| {conditionType === 'gt' ? t('higher') : t('lower')} |
There was a problem hiding this comment.
should we cache consistent translations like higher and lower in a constants file?
There was a problem hiding this comment.
usually ends up not being worth it because of casing and stuff or you want 'higher than' in another file
| @@ -1,22 +0,0 @@ | |||
| import SentryDocumentTitle from 'sentry/components/sentryDocumentTitle'; | |||
There was a problem hiding this comment.
Curious why we're removing this component? it seems like a helpful wrapper component to build more monitors quickly.
There was a problem hiding this comment.
The layouts were preemptively split out a bit early. Right now they're getting in the way since they're trying to share code between multiple pages that are changing. We'll likely have a single page for edit/new and another for step 1 similar to metric alerts. Right now they don't have a ton in common.
Allows creating new detectors via the ui. Not everything is working yet like resolution threshold.