Skip to content

Initial backfull group#184144

Merged
nkhristinin merged 16 commits intoelastic:mainfrom
nkhristinin:manual-run-group
May 29, 2024
Merged

Initial backfull group#184144
nkhristinin merged 16 commits intoelastic:mainfrom
nkhristinin:manual-run-group

Conversation

@nkhristinin
Copy link
Copy Markdown
Contributor

@nkhristinin nkhristinin commented May 23, 2024

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

  • Show amount of tasks
  • Ability to cancel run
  • There auto refresh - which is disabled by default, as backfills group remove after completion
Screen.Recording.2024-05-28.at.14.48.48.mov

How to test

1 . Enable feature flag - manualRuleRunEnabled
2. For you rule call schedule api /internal/alerting/rules/backfill/_schedule POST
With this body (put your values for rule id and date range):

[{"rule_id":"58b4b926-6348-4c23-be1f-870a461fa342","start":"2024-05-21T13:00:00.000Z","end":"2024-05-21T14:05:00.000Z"}]

@nkhristinin nkhristinin marked this pull request as ready for review May 28, 2024 13:02
@nkhristinin nkhristinin requested review from a team as code owners May 28, 2024 13:02
@nkhristinin nkhristinin requested a review from maximpn May 28, 2024 13:02
@nkhristinin nkhristinin added the release_note:skip Skip the PR/issue when compiling release notes label May 28, 2024
@nkhristinin
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@elastic elastic locked and limited conversation to collaborators May 28, 2024
Copy link
Copy Markdown
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

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

@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.

Comment on lines +14 to +19
let color = 'subdued';
if (status === 'pending') {
color = 'warning';
} else if (status === 'running') {
color = 'success';
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: 10 items per page is also a magic constant.

width: '10%',
};

const columns: Array<EuiBasicTableColumn<BackfillRow>> = [
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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: {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

By some reason I like to have full data structure here, as it can help to easily see what can be in response

Comment on lines +11 to +18
): {
total: number;
complete: number;
running: number;
pending: number;
error: number;
timeout: number;
} => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I encounter reduce quite often in Kibana's codebase. IMHO it has worse readability than for of. WDYT?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It can be more readable sometimes, but for this use case It think it's fine

Comment on lines +50 to +52
createRule({
...getNewRule(),
}).then((rule) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
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');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

response ops changes LGTM

@nkhristinin nkhristinin requested a review from maximpn May 29, 2024 14:29
@nkhristinin
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 5488 5496 +8

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
alerting 834 836 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 15.2MB 15.2MB +6.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
alerting 24.8KB 24.9KB +186.0B
Unknown metric groups

API count

id before after diff
alerting 866 868 +2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Copy Markdown
Contributor

@maximpn maximpn left a comment

Choose a reason for hiding this comment

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

@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],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This line isn't necessary with toMatchObject.

const AUTO_REFRESH_INTERVAL = 3000;
const DEFAULT_PAGE_SIZE = 10;

const getBackfillsTableColumns = (hasCRUDPermissions: boolean) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally this function should be placed below RuleBackfillsInfo. It will improve readability since a reader first sees exported component and then implementation details.

@nkhristinin nkhristinin merged commit aa7ffc4 into elastic:main May 29, 2024
@kibanamachine kibanamachine added v8.15.0 backport:skip This PR does not require backporting labels May 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants