Conversation
|
@elasticmachine merge upstream |
maximpn
left a comment
There was a problem hiding this comment.
@nkhristinin Thanks for implementing this feature!
Local testing didn't reveal any issues.
I left some comments but these are minor and should be really straightforward to address.
Is there a ticket/epic describing the functionality? It'd be nice to have a link in the PR description.
| let color = 'subdued'; | ||
| if (status === 'pending') { | ||
| color = 'warning'; | ||
| } else if (status === 'running') { | ||
| color = 'success'; | ||
| } |
There was a problem hiding this comment.
nit: A bit cleaner approach is to move this logic into a separate function named like getBackfillStatusColor() implemented via switch like
function getBackfillStatusColor(status: BackfillStatus): IconColor {
switch (status) {
case 'pending': return 'warning';
case 'running': return 'success';
default: return 'subdued';
}
}|
|
||
| export const RuleBackfillsInfo = React.memo<{ ruleId: string }>(({ ruleId }) => { | ||
| const isManualRuleRunEnabled = useIsExperimentalFeatureEnabled('manualRuleRunEnabled'); | ||
| const [autoRefreshInterval, setAutoRefreshInterval] = useState(3000); |
There was a problem hiding this comment.
Auto refresh interval is set to a magic number 3000. It's worth to add a constant.
I'm curious though why 3000 has been chosen.
nit: It looks like a good idea to have one or more shared constants for refresh intervals and reuse them. For example rules management table has auto refresh functionality with a longer interval.
There was a problem hiding this comment.
It's kind of random chosen, I try make balance between to fast, as group can be disappear, but also not make it to long that user wait long time
| const [autoRefreshInterval, setAutoRefreshInterval] = useState(3000); | ||
| const [isAutoRefresh, setIsAutoRefresh] = useState(false); | ||
| const [pageIndex, setPageIndex] = useState(0); | ||
| const [pageSize, setPageSize] = useState(10); |
There was a problem hiding this comment.
nit: 10 items per page is also a magic constant.
| width: '10%', | ||
| }; | ||
|
|
||
| const columns: Array<EuiBasicTableColumn<BackfillRow>> = [ |
There was a problem hiding this comment.
It looks like columns configuration could be moved outside component's body. For example defined below the component as const BACKFILLS_TABLE_COLUMNS = .... Though taking into account it can be extended with stopAction it's even better to create a separate function named like getBackfillsTableColumns(hasUserCRUDPermissions: boolean): Array<EuiBasicTableColumn<BackfillRow>>.
| pageIndex, | ||
| pageSize, | ||
| totalItemCount: data?.total ?? 0, | ||
| pageSizeOptions: [5, 10, 25], |
There was a problem hiding this comment.
nit: Should it reuse already existing page sizes used for the other tables? If not it's still worth to have a constant for page sizes used here.
| created_at: '2023-01-01T00:00:00Z', | ||
| duration: '2h', | ||
| enabled: true, | ||
| rule: { |
There was a problem hiding this comment.
I'm curious if all that data is required for the test. It looks like it could be shrieked to bare minimum like id and using of @ts-expect-error with a comment will help to solve typing issues.
There was a problem hiding this comment.
By some reason I like to have full data structure here, as it can help to easily see what can be in response
| ): { | ||
| total: number; | ||
| complete: number; | ||
| running: number; | ||
| pending: number; | ||
| error: number; | ||
| timeout: number; | ||
| } => { |
There was a problem hiding this comment.
This type has too many fields for an inlined type. There is a type BackfillRow which has the same fields plus some extra. I should be much better to have a separate type containing inly these fields and defined BackfillRow as a union of this new type and Backfill.
| error: number; | ||
| timeout: number; | ||
| } => { | ||
| return backfill?.schedule?.reduce( |
There was a problem hiding this comment.
nit: I encounter reduce quite often in Kibana's codebase. IMHO it has worse readability than for of. WDYT?
There was a problem hiding this comment.
It can be more readable sometimes, but for this use case It think it's fine
| createRule({ | ||
| ...getNewRule(), | ||
| }).then((rule) => { |
There was a problem hiding this comment.
| createRule({ | |
| ...getNewRule(), | |
| }).then((rule) => { | |
| createRule(getNewRule()).then((rule) => { |
| cy.get(EXECUTION_RUN_TYPE_FILTER_ITEM).contains(ruleType).click(); | ||
| }; | ||
|
|
||
| export const getBackfillsTable = () => cy.get(RULE_BACKFILLS_TABLE).find('tbody tr'); |
There was a problem hiding this comment.
Just to be precise this task returns table rows instead of the table itself. Either this function should be renamed to getBackfillsTableRows or it should return the table DOM element.
|
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
maximpn
left a comment
There was a problem hiding this comment.
@nkhristinin Thanks for addressing my comments 🙏
I seems you missed one and I additionally left a couple of minor comments but approve the PR in advance.
| }); | ||
|
|
||
| expect(output[1]).toMatchObject({ | ||
| ...input[1], |
There was a problem hiding this comment.
This line isn't necessary with toMatchObject.
| const AUTO_REFRESH_INTERVAL = 3000; | ||
| const DEFAULT_PAGE_SIZE = 10; | ||
|
|
||
| const getBackfillsTableColumns = (hasCRUDPermissions: boolean) => { |
There was a problem hiding this comment.
Ideally this function should be placed below RuleBackfillsInfo. It will improve readability since a reader first sees exported component and then implementation details.
Show backfill group for manual runs
UX - copy is not final, there will be an additional ticket for that, which we address later
After user execute manual rule API we will present backfill group. Backfill group contains 1 to N scheduled entries. Each of the entry - is associated with rule run (potential, in progress, completed)
Backfill group is removed after all task completed, so this why they disappear from UI in the video
Screen.Recording.2024-05-28.at.14.48.48.mov
How to test
1 . Enable feature flag -
manualRuleRunEnabled2. For you rule call schedule api
/internal/alerting/rules/backfill/_schedulePOSTWith this body (put your values for rule id and date range):