Skip to content
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

Add filter field to index threshold rule type #142255

Merged
merged 24 commits into from
Oct 13, 2022

Conversation

ersin-erdal
Copy link
Contributor

@ersin-erdal ersin-erdal commented Sep 29, 2022

Resolves: #66046

Adds a new filter field to index-threshold rule type.

To validate:
Create an index-threshold rule with and without filter and observe the number of documents in the visualisation.

index-th

@ersin-erdal ersin-erdal added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) release_note:feature Makes this part of the condensed release notes Feature:Alerting/RuleTypes Issues related to specific Alerting Rules Types v8.6.0 labels Sep 29, 2022
@@ -208,7 +209,6 @@ export const EsQueryExpression: React.FC<
languageId="xjson"
width="100%"
height="200px"
data-test-subj="queryJsonEditor"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CodeEditor UI Component doesn't pass the data-test-subj attribute to the final rendered html, so i moved it to the parent row element

@@ -55,9 +55,6 @@ export default function ruleTests({ getService }: FtrProviderContext) {
const endDateMillis = Date.now() + (RULE_INTERVALS_TO_WRITE - 1) * RULE_INTERVAL_MILLIS;
endDate = new Date(endDateMillis).toISOString();

// write documents from now to the future end date in 3 groups
await createEsDocumentsInGroups(3);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function creates documents with a timespan field that has values between now and the future.
But the rule looks for last "x" seconds, or in other words: between past and now.
it works, since we use patterns to test the messages. but for the new filterKuery filed i needed an exact number, so i moved this function from beforeEach to each test case. And used a fixed date for the new test case.

@ersin-erdal ersin-erdal marked this pull request as ready for review October 5, 2022 17:01
@ersin-erdal ersin-erdal requested a review from a team as a code owner October 5, 2022 17:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

# Conflicts:
#	x-pack/plugins/triggers_actions_ui/server/data/lib/time_series_query.test.ts
#	x-pack/plugins/triggers_actions_ui/server/data/lib/time_series_query.ts
@@ -294,20 +303,21 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => {
await discardNewRuleCreation();
});

it('should show error when es_query is invalid', async () => {
// Failing: See https://github.com/elastic/kibana/issues/126873
it.skip('should show error when es_query is invalid', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the flaky test case, therefore moved skip from describe block to here.

@ersin-erdal ersin-erdal requested a review from a team as a code owner October 6, 2022 16:58
@@ -110,7 +110,7 @@ pageLoadAssetSize:
share: 71239
snapshotRestore: 79032
spaces: 57868
stackAlerts: 29684
stackAlerts: 58316
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a follow-up issue to reduce bundle size: #142902

Copy link
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.

Looks great! Verified that adding a filter works as expected. Left one comment about adding a debounce to the filter validation.

@@ -133,6 +142,13 @@ export const IndexThresholdAlertTypeExpression: React.FunctionComponent<
setEsFields(currentEsFields);
};

const handleFilterChange = useCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any way to add a debounce here so not every keystroke is validated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debouncing here just prevents the value to be set.
Current validation is triggered on every field change of a rule. We need to change the whole validation implementation on framework level in order to tackle validations for each field separately.

Copy link
Contributor

@doakalexi doakalexi left a comment

Choose a reason for hiding this comment

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

LGTM!

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
stackAlerts 101.1KB 73.9KB -27.3KB

Page load bundle

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

id before after diff
stackAlerts 13.9KB 42.3KB +28.4KB
Unknown metric groups

API count

id before after diff
triggersActionsUi 517 518 +1

History

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

@ersin-erdal ersin-erdal merged commit 660a24e into elastic:main Oct 13, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 13, 2022
@ersin-erdal ersin-erdal deleted the 66046-index-threshold-filter branch October 13, 2022 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Alerting/RuleTypes Issues related to specific Alerting Rules Types release_note:feature Makes this part of the condensed release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filtering with alert creation (index threshold)
7 participants